From cf586f599e7a5f5fe32f04d642e13a1509dc970e Mon Sep 17 00:00:00 2001 From: Chris Dent Date: Sun, 9 Sep 2018 06:57:24 -0600 Subject: [PATCH] Rationalize and clarify database configuration Discussion in IRC, hangouts and summarized in email [1] led to a decision that in placement the use of a default data connection string was dangerous for people migrating from placement-in-nova to placement by itself. Instead what we want are explicit but small required actions for operators/deployers and a hard fail if they do not do them. A critical first step to make that work is removing the default connection string and requiring an explicit [placement_database]/connection value. To whit, this patch: * removes api_database configuration settings * removes the default connection value * make it the value required (which causes config parsing to fail) * updates test fixtures to remove knowledge of other databases besides 'placement_database' * adds a unit tests to confirm cfg.RequiredOptError is raised when expected (when the setting is not set) * Comments in the wsgi app that a hard fail will happen when config and required settings are missing In local testing I have confirmed two different hard failures that we want: * If there is no configuration file at all (either in the default /etc/placement/placement.conf location or the same file set by OS_PLACEMENT_CONFIG_DIR), fail * If there is a config file and it has no [placement_database]/connection, fail. Note that all these changes are not fully meaningful until the change in Ibed4c64fa4e74c830d0d2fa040089bf9dc7b8f29, which changes the placement/wsgi.py app. That's currently the only external entry point to using placement, and will not work until those changes are in place. [1] http://lists.openstack.org/pipermail/openstack-dev/2018-September/134444.html Change-Id: Iecf5006b32ca7bc2d46b7a59e78b4941a2e98122 --- placement/conf/database.py | 98 +++++----------------------- placement/db_api.py | 13 ++-- placement/tests/functional/base.py | 11 ++-- placement/tests/unit/test_db_conf.py | 32 +++++++++ placement/tests/unit/test_policy.py | 3 + placement/wsgi.py | 11 +++- 6 files changed, 68 insertions(+), 100 deletions(-) create mode 100644 placement/tests/unit/test_db_conf.py diff --git a/placement/conf/database.py b/placement/conf/database.py index 3df1299d4..47843b0d2 100644 --- a/placement/conf/database.py +++ b/placement/conf/database.py @@ -17,81 +17,9 @@ from __future__ import absolute_import from oslo_config import cfg from oslo_db import options as oslo_db_options -from placement.conf import paths - -_DEFAULT_SQL_CONNECTION = 'sqlite:///' + paths.state_path_def( - 'placement.sqlite') _ENRICHED = False -# NOTE(markus_z): We cannot simply do: -# conf.register_opts(oslo_db_options.database_opts, 'api_database') -# If we reuse a db config option for two different groups ("api_database" -# and "database") and deprecate or rename a config option in one of these -# groups, "oslo.config" cannot correctly determine which one to update. -# That's why we copied & pasted these config options for the "api_database" -# group here. See commit ba407e3 ("Add support for multiple database engines") -# for more details. -api_db_group = cfg.OptGroup('api_database', - title='API Database Options', - help=""" -The *Nova API Database* is a separate database which is used for information -which is used across *cells*. This database is mandatory since the Mitaka -release (13.0.0). -""") - -api_db_opts = [ - # TODO(markus_z): This should probably have a required=True attribute - cfg.StrOpt('connection', - secret=True, - help=''), - cfg.StrOpt('connection_parameters', - default='', - help=''), - cfg.BoolOpt('sqlite_synchronous', - default=True, - help=''), - cfg.StrOpt('slave_connection', - secret=True, - help=''), - cfg.StrOpt('mysql_sql_mode', - default='TRADITIONAL', - help=''), - cfg.IntOpt('connection_recycle_time', - default=3600, - deprecated_name='idle_timeout', - help=''), - # TODO(markus_z): We should probably default this to 5 to not rely on the - # SQLAlchemy default. Otherwise we wouldn't provide a stable default. - cfg.IntOpt('max_pool_size', - help=''), - cfg.IntOpt('max_retries', - default=10, - help=''), - # TODO(markus_z): This should have a minimum attribute of 0 - cfg.IntOpt('retry_interval', - default=10, - help=''), - # TODO(markus_z): We should probably default this to 10 to not rely on the - # SQLAlchemy default. Otherwise we wouldn't provide a stable default. - cfg.IntOpt('max_overflow', - help=''), - # TODO(markus_z): This should probably make use of the "choices" attribute. - # "oslo.db" uses only the values [<0, 0, 50, 100] see module - # /oslo_db/sqlalchemy/engines.py method "_setup_logging" - cfg.IntOpt('connection_debug', - default=0, - help=''), - cfg.BoolOpt('connection_trace', - default=False, - help=''), - # TODO(markus_z): We should probably default this to 30 to not rely on the - # SQLAlchemy default. Otherwise we wouldn't provide a stable default. - cfg.IntOpt('pool_timeout', - help='') -] # noqa - - def enrich_help_text(alt_db_opts): def get_db_opts(): @@ -107,20 +35,30 @@ def enrich_help_text(alt_db_opts): # texts here if needed. alt_db_opt.help = db_opt.help + alt_db_opt.help -# NOTE(cdent): See the note above on api_db_group. The same issues -# apply here. - +# NOTE(markus_z): We cannot simply do: +# conf.register_opts(oslo_db_options.database_opts, 'placement_database') +# If we reuse a db config option for two different groups ("placement_database" +# and "database") and deprecate or rename a config option in one of these +# groups, "oslo.config" cannot correctly determine which one to update. +# That's why we copied & pasted these config options for the +# "placement_database" group here. See nova commit ba407e3 ("Add support +# for multiple database engines") for more details. +# TODO(cdent): Consider our future options of using 'database' instead of +# 'placement_database' for the group. This is already loose in the wild, +# explicit, and safe if there will ever be more than one database, so may +# be good to leave it. placement_db_group = cfg.OptGroup('placement_database', title='Placement API database options', help=""" -The *Placement API Database* is a separate database which can be used with the -placement service. This database is optional: if the connection option is not -set, the nova api database will be used instead. +The *Placement API Database* is a the database used with the placement +service. If the connection option is not set, the placement service will +not start. """) placement_db_opts = [ cfg.StrOpt('connection', help='', + required=True, secret=True), cfg.StrOpt('connection_parameters', default='', @@ -159,8 +97,6 @@ placement_db_opts = [ def register_opts(conf): - oslo_db_options.set_defaults(conf, connection=_DEFAULT_SQL_CONNECTION) - conf.register_opts(api_db_opts, group=api_db_group) conf.register_opts(placement_db_opts, group=placement_db_group) @@ -174,10 +110,8 @@ def list_opts(): # here. global _ENRICHED if not _ENRICHED: - enrich_help_text(api_db_opts) enrich_help_text(placement_db_opts) _ENRICHED = True return { - api_db_group: api_db_opts, placement_db_group: placement_db_opts, } diff --git a/placement/db_api.py b/placement/db_api.py index 792c168a3..83105e00c 100644 --- a/placement/db_api.py +++ b/placement/db_api.py @@ -13,10 +13,11 @@ own file so the nova db_api (which has cascading imports) is not imported. """ - from oslo_db.sqlalchemy import enginefacade +from oslo_log import log as logging +LOG = logging.getLogger(__name__) placement_context_manager = enginefacade.transaction_context() @@ -25,14 +26,8 @@ def _get_db_conf(conf_group): def configure(conf): - # If [placement_database]/connection is not set in conf, then placement - # data will be stored in the nova_api database. - if conf.placement_database.connection is None: - placement_context_manager.configure( - **_get_db_conf(conf.api_database)) - else: - placement_context_manager.configure( - **_get_db_conf(conf.placement_database)) + placement_context_manager.configure( + **_get_db_conf(conf.placement_database)) def get_placement_engine(): diff --git a/placement/tests/functional/base.py b/placement/tests/functional/base.py index 03a9b3caf..504215a98 100644 --- a/placement/tests/functional/base.py +++ b/placement/tests/functional/base.py @@ -39,13 +39,10 @@ class TestCase(testtools.TestCase): # Manage required configuration conf_fixture = self.useFixture(config_fixture.Config(CONF)) - # The Database fixture will get confused if only one of the databases - # is configured. - for group in ('placement_database', 'api_database', 'database'): - conf_fixture.config( - group=group, - connection='sqlite://', - sqlite_synchronous=False) + conf_fixture.config( + group='placement_database', + connection='sqlite://', + sqlite_synchronous=False) CONF([], default_config_files=[]) self.useFixture(policy_fixture.PolicyFixture()) diff --git a/placement/tests/unit/test_db_conf.py b/placement/tests/unit/test_db_conf.py new file mode 100644 index 000000000..357c28f2b --- /dev/null +++ b/placement/tests/unit/test_db_conf.py @@ -0,0 +1,32 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import testtools + +from oslo_config import cfg +from oslo_config import fixture as config_fixture + + +CONF = cfg.CONF + + +class TestPlacementDBConf(testtools.TestCase): + """Test cases for Placement DB Setup.""" + + def setUp(self): + super(TestPlacementDBConf, self).setUp() + self.conf_fixture = self.useFixture(config_fixture.Config(CONF)) + + def test_missing_config_raises(self): + self.assertRaises( + cfg.RequiredOptError, self.conf_fixture.conf, + [], default_config_files=[]) diff --git a/placement/tests/unit/test_policy.py b/placement/tests/unit/test_policy.py index 5a98c6f00..047222c9e 100644 --- a/placement/tests/unit/test_policy.py +++ b/placement/tests/unit/test_policy.py @@ -34,6 +34,9 @@ class PlacementPolicyTestCase(testtools.TestCase): self.conf = self.useFixture(config_fixture.Config(CONF)).conf self.ctxt = context.RequestContext(user_id='fake', project_id='fake') self.target = {'user_id': 'fake', 'project_id': 'fake'} + # A value is required in the database connection opt for CONF to + # parse. + CONF.set_default('connection', 'stub', group='placement_database') CONF([], default_config_files=[]) def test_modified_policy_reloads(self): diff --git a/placement/wsgi.py b/placement/wsgi.py index bd6730753..ade858bff 100644 --- a/placement/wsgi.py +++ b/placement/wsgi.py @@ -99,12 +99,19 @@ def _set_middleware_defaults(): def init_application(): # initialize the config system conffile = _get_config_file() + # This will raise cfg.ConfigFilesNotFoundError and cfg.RequiredOptError + # when either conffile is not there or some required option is not set + # (notably the database connection string). We want both of these to + # be a hard fail and prevent the application from starting so we hard + # fail here. The error will show up in the wsgi server's logs and the + # app will not start. _parse_args([], default_config_files=[conffile]) - db_api.configure(conf.CONF) - # initialize the logging system setup_logging(conf.CONF) + # configure database + db_api.configure(conf.CONF) + # dump conf at debug if log_options if conf.CONF.log_options: conf.CONF.log_opt_values(