From 60c7607a91547efdd076d454d3a699bce2a0aab2 Mon Sep 17 00:00:00 2001 From: Paul Michali Date: Mon, 11 Feb 2013 09:58:41 -0500 Subject: [PATCH] Remove cfg option default value and check if missing Currently, several plugins already check config options at __init__() for validity and will exit, if the settings are incorrect. However, most (all?) config option definitions have default values, so if the option is missing, a valid, but maybe unexpected value will be used. This is what occurred in the bug. The proposed fix is to take a config option, sql_connection, which is used by many plugins, and remove the default value. Then, at init time, when the config option is used in configure_db(), a check is made for the value. If the value is not set, a warning is logged and the value is set to the default, for db/api.py. It is expected that this will be the only module to consume this config option. Added UT to check that log warning is issued. Also, changed the timing so that the test takes 0.25 secs vs 12 secs. Removed UTs in two plugin tests that checked the default value for sql_connection. Other alternatives explored in previous patches, were to either raise an exception, or mark this config option as "required". This resulted in a large number of changes to tests, and required config overrides in plugins that imported quantum.db.api, but did not use sql_connection. In order to keep this solution (of this log-hanging fruit) fix, the proposed, simpler change is being made. Some cleanup to the Cisco plugin test case was also made, so that the mock was more in line with what production code does. bug 1059923 Change-Id: I8c2a4e05231ac4e172d0dccece067e6fdb354341 --- quantum/db/api.py | 8 +++++++- quantum/tests/unit/cisco/test_nexus_plugin.py | 15 +++++---------- .../tests/unit/linuxbridge/test_defaults.py | 2 -- quantum/tests/unit/nec/test_config.py | 1 - quantum/tests/unit/nicira/test_defaults.py | 1 - .../unit/openvswitch/test_ovs_defaults.py | 1 - quantum/tests/unit/ryu/test_defaults.py | 1 - quantum/tests/unit/test_db.py | 19 ++++++++++++++++++- 8 files changed, 30 insertions(+), 18 deletions(-) diff --git a/quantum/db/api.py b/quantum/db/api.py index a1e0abf45..6dcd8f3b2 100644 --- a/quantum/db/api.py +++ b/quantum/db/api.py @@ -36,10 +36,11 @@ from quantum.openstack.common import cfg from quantum.openstack.common import log as logging LOG = logging.getLogger(__name__) +SQL_CONNECTION_DEFAULT = 'sqlite://' database_opts = [ - cfg.StrOpt('sql_connection', default='sqlite://', + cfg.StrOpt('sql_connection', help=_('The SQLAlchemy connection string used to connect to ' 'the database')), cfg.IntOpt('sql_max_retries', default=-1, @@ -111,6 +112,11 @@ def configure_db(): global _ENGINE if not _ENGINE: sql_connection = cfg.CONF.DATABASE.sql_connection + if not sql_connection: + LOG.warn(_("Option 'sql_connection' not specified " + "in any config file - using default " + "value '%s'" % SQL_CONNECTION_DEFAULT)) + sql_connection = SQL_CONNECTION_DEFAULT connection_dict = sql.engine.url.make_url(sql_connection) engine_args = { 'pool_recycle': 3600, diff --git a/quantum/tests/unit/cisco/test_nexus_plugin.py b/quantum/tests/unit/cisco/test_nexus_plugin.py index f4d9d206d..ee60fdf73 100644 --- a/quantum/tests/unit/cisco/test_nexus_plugin.py +++ b/quantum/tests/unit/cisco/test_nexus_plugin.py @@ -19,7 +19,6 @@ import unittest from quantum.db import api as db from quantum.openstack.common import importutils from quantum.plugins.cisco.common import cisco_constants as const -from quantum.plugins.cisco.db import network_db_v2 as cdb from quantum.plugins.cisco.db import network_models_v2 from quantum.plugins.cisco.nexus import cisco_nexus_plugin_v2 @@ -62,9 +61,6 @@ class TestCiscoNexusPlugin(unittest.TestCase): } self._hostname = HOSTNAME - def new_cdb_init(): - db.configure_db() - def new_nexus_init(self): self._client = importutils.import_object(NEXUS_DRIVER) self._nexus_ip = NEXUS_IP_ADDRESS @@ -78,13 +74,12 @@ class TestCiscoNexusPlugin(unittest.TestCase): 'password': self._nexus_password } } + db.configure_db() - with mock.patch.object(cdb, 'initialize', new=new_cdb_init): - cdb.initialize() - with mock.patch.object(cisco_nexus_plugin_v2.NexusPlugin, - '__init__', new=new_nexus_init): - self._cisco_nexus_plugin = cisco_nexus_plugin_v2.NexusPlugin() - self._cisco_nexus_plugin._nexus_switches = self._nexus_switches + with mock.patch.object(cisco_nexus_plugin_v2.NexusPlugin, + '__init__', new=new_nexus_init): + self._cisco_nexus_plugin = cisco_nexus_plugin_v2.NexusPlugin() + self._cisco_nexus_plugin._nexus_switches = self._nexus_switches def test_a_create_network(self): """ diff --git a/quantum/tests/unit/linuxbridge/test_defaults.py b/quantum/tests/unit/linuxbridge/test_defaults.py index f23ad5418..ac9840ffc 100644 --- a/quantum/tests/unit/linuxbridge/test_defaults.py +++ b/quantum/tests/unit/linuxbridge/test_defaults.py @@ -23,8 +23,6 @@ from quantum.plugins.linuxbridge.common import config class ConfigurationTest(unittest.TestCase): def test_defaults(self): - self.assertEqual('sqlite://', - cfg.CONF.DATABASE.sql_connection) self.assertEqual(-1, cfg.CONF.DATABASE.sql_max_retries) self.assertEqual(2, diff --git a/quantum/tests/unit/nec/test_config.py b/quantum/tests/unit/nec/test_config.py index 06508b17c..3aa82faab 100644 --- a/quantum/tests/unit/nec/test_config.py +++ b/quantum/tests/unit/nec/test_config.py @@ -23,7 +23,6 @@ from quantum.plugins.nec.common import config class ConfigurationTest(unittest.TestCase): def test_defaults(self): - self.assertEqual('sqlite://', config.CONF.DATABASE.sql_connection) self.assertEqual(-1, config.CONF.DATABASE.sql_max_retries) self.assertEqual(2, config.CONF.DATABASE.reconnect_interval) self.assertEqual('br-int', config.CONF.OVS.integration_bridge) diff --git a/quantum/tests/unit/nicira/test_defaults.py b/quantum/tests/unit/nicira/test_defaults.py index b636fdde7..fabd03071 100644 --- a/quantum/tests/unit/nicira/test_defaults.py +++ b/quantum/tests/unit/nicira/test_defaults.py @@ -22,7 +22,6 @@ from quantum.plugins.nicira.nicira_nvp_plugin.common import config class ConfigurationTest(unittest.TestCase): def test_defaults(self): - self.assertEqual('sqlite://', cfg.CONF.DATABASE.sql_connection) self.assertEqual(-1, cfg.CONF.DATABASE.sql_max_retries) self.assertEqual(2, cfg.CONF.DATABASE.reconnect_interval) self.assertEqual(64, cfg.CONF.NVP.max_lp_per_bridged_ls) diff --git a/quantum/tests/unit/openvswitch/test_ovs_defaults.py b/quantum/tests/unit/openvswitch/test_ovs_defaults.py index 8363df859..919863b92 100644 --- a/quantum/tests/unit/openvswitch/test_ovs_defaults.py +++ b/quantum/tests/unit/openvswitch/test_ovs_defaults.py @@ -26,7 +26,6 @@ class ConfigurationTest(unittest.TestCase): self.assertEqual('br-int', cfg.CONF.OVS.integration_bridge) self.assertFalse(cfg.CONF.OVS.enable_tunneling) self.assertEqual('br-tun', cfg.CONF.OVS.tunnel_bridge) - self.assertEqual('sqlite://', cfg.CONF.DATABASE.sql_connection) self.assertEqual(-1, cfg.CONF.DATABASE.sql_max_retries) self.assertEqual(2, cfg.CONF.DATABASE.reconnect_interval) self.assertEqual(2, cfg.CONF.AGENT.polling_interval) diff --git a/quantum/tests/unit/ryu/test_defaults.py b/quantum/tests/unit/ryu/test_defaults.py index 5b203eefd..7da846bcd 100644 --- a/quantum/tests/unit/ryu/test_defaults.py +++ b/quantum/tests/unit/ryu/test_defaults.py @@ -26,7 +26,6 @@ class ConfigurationTest(unittest2.TestCase): """Configuration file Tests""" def test_defaults(self): self.assertEqual('br-int', cfg.CONF.OVS.integration_bridge) - self.assertEqual('sqlite://', cfg.CONF.DATABASE.sql_connection) self.assertEqual(-1, cfg.CONF.DATABASE.sql_max_retries) self.assertEqual(2, cfg.CONF.DATABASE.reconnect_interval) self.assertEqual(2, cfg.CONF.AGENT.polling_interval) diff --git a/quantum/tests/unit/test_db.py b/quantum/tests/unit/test_db.py index 5fa660f5a..1b8cc003f 100644 --- a/quantum/tests/unit/test_db.py +++ b/quantum/tests/unit/test_db.py @@ -24,8 +24,25 @@ from quantum.openstack.common import cfg class DBTestCase(unittest.TestCase): + def setUp(self): + cfg.CONF.set_override('sql_max_retries', 1, 'DATABASE') + cfg.CONF.set_override('reconnect_interval', 0, 'DATABASE') + + def tearDown(self): + db._ENGINE = None + cfg.CONF.reset() + def test_db_reconnect(self): - cfg.CONF.set_override('sql_max_retries', 3, 'DATABASE') with mock.patch.object(db, 'register_models') as mock_register: mock_register.return_value = False db.configure_db() + + def test_warn_when_no_connection(self): + with mock.patch.object(db, 'register_models') as mock_register: + mock_register.return_value = False + with mock.patch.object(db.LOG, 'warn') as mock_log: + mock_log.return_value = False + db.configure_db() + self.assertEquals(mock_log.call_count, 1) + args = mock_log.call_args + self.assertNotEqual(args.find('sql_connection'), -1) -- 2.45.2