From 2d405a8bfd280b541be892c36fda09c60d921a84 Mon Sep 17 00:00:00 2001 From: Takashi Kajinami Date: Fri, 12 Apr 2024 12:32:35 +0900 Subject: [PATCH] Use oslo.db to generate db engine Berbican has been historically using own implementation with sqlalchemy to connect to database but this causes some feature gaps with the other services using oslo.db to generate database engine. This replaces the own implementation by oslo.db's one so that barbican can also leverage the features implemented in the shared library. With this change the deprecated database options are removed, because the deprecated options were already removed from oslo.db. Change-Id: I10fe4ab04996885e8aff7fab8ace78a6fe7eb6e7 --- barbican/common/config.py | 69 +------------------ barbican/model/repositories.py | 69 ++----------------- .../model/repositories/test_repositories.py | 38 +--------- devstack/lib/barbican | 2 +- etc/oslo-config-generator/barbican.conf | 1 + .../notes/oslo-db-9381293cc7bedc7d.yaml | 17 +++++ 6 files changed, 31 insertions(+), 165 deletions(-) create mode 100644 releasenotes/notes/oslo-db-9381293cc7bedc7d.yaml diff --git a/barbican/common/config.py b/barbican/common/config.py index eed2cc4f3..30c6a2b35 100644 --- a/barbican/common/config.py +++ b/barbican/common/config.py @@ -21,6 +21,7 @@ import logging import os from oslo_config import cfg +from oslo_db import options as db_options from oslo_log import log from oslo_middleware import cors from oslo_policy import opts as policy_opts @@ -63,68 +64,6 @@ host_opts = [ "'http://localhost:9311'")), ] -core_db_opts = [ - cfg.StrOpt('connection', - default="sqlite:///barbican.sqlite", - secret=True, - deprecated_opts=[cfg.DeprecatedOpt('sql_connection', - group='DEFAULT')], - help=u._("SQLAlchemy connection string for the reference " - "implementation registry server. Any valid " - "SQLAlchemy connection string is fine. See: " - "http://www.sqlalchemy.org/docs/05/reference/" - "sqlalchemy/connections.html#sqlalchemy." - "create_engine. Note: For absolute addresses, use " - "'////' slashes after 'sqlite:'.")), - cfg.IntOpt('connection_recycle_time', default=3600, - deprecated_opts=[cfg.DeprecatedOpt('sql_idle_timeout', - group='DEFAULT')], - help=u._("Period in seconds after which SQLAlchemy should " - "reestablish its connection to the database. MySQL " - "uses a default `wait_timeout` of 8 hours, after " - "which it will drop idle connections. This can result " - "in 'MySQL Gone Away' exceptions. If you notice this, " - "you can lower this value to ensure that SQLAlchemy " - "reconnects before MySQL can drop the connection.")), - cfg.IntOpt('max_retries', default=60, - deprecated_opts=[cfg.DeprecatedOpt('sql_max_retries', - group='DEFAULT')], - help=u._("Maximum number of database connection retries " - "during startup. Set to -1 to specify an infinite " - "retry count.")), - cfg.IntOpt('retry_interval', default=1, - deprecated_opts=[cfg.DeprecatedOpt('sql_retry_interval', - group='DEFAULT')], - help=u._("Interval between retries of opening a SQL " - "connection.")), - cfg.IntOpt('max_pool_size', default=5, - deprecated_opts=[cfg.DeprecatedOpt('sql_pool_size', - group='DEFAULT')], - help=u._("Size of pool used by SQLAlchemy. This is the largest " - "number of connections that will be kept persistently " - "in the pool. Can be set to 0 to indicate no size " - "limit. To disable pooling, use a NullPool with " - "sql_pool_class instead. Comment out to allow " - "SQLAlchemy to select the default.")), - cfg.IntOpt('max_overflow', default=10, - deprecated_opts=[cfg.DeprecatedOpt('sql_pool_max_overflow', - group='DEFAULT')], - help=u._("# The maximum overflow size of the pool used by " - "SQLAlchemy. When the number of checked-out " - "connections reaches the size set in max_pool_size, " - "additional connections will be returned up to this " - "limit. It follows then that the total number of " - "simultaneous connections the pool will allow is " - "max_pool_size + max_overflow. Can be set to -1 to " - "indicate no overflow limit, so no limit will be " - "placed on the total number of concurrent " - "connections. Comment out to allow SQLAlchemy to " - "select the default.")), -] - -db_opt_group = cfg.OptGroup(name='database', - title='Database Options') - db_opts = [ cfg.BoolOpt('db_auto_create', default=False, help=u._("Create the Barbican database on service startup.")), @@ -255,7 +194,6 @@ def list_opts(): yield None, common_opts yield None, host_opts yield None, db_opts - yield db_opt_group, core_db_opts yield retry_opt_group, retry_opts yield queue_opt_group, queue_opts yield ks_queue_opt_group, ks_queue_opts @@ -288,14 +226,13 @@ def parse_args(conf, args=None, usage=None, default_config_files=None): def new_config(): conf = cfg.ConfigOpts() log.register_options(conf) + db_options.set_defaults(conf, connection='sqlite:///barbican.sqlite') + conf.register_opts(context_opts) conf.register_opts(common_opts) conf.register_opts(host_opts) conf.register_opts(db_opts) - conf.register_group(db_opt_group) - conf.register_opts(core_db_opts, group=db_opt_group) - conf.register_group(retry_opt_group) conf.register_opts(retry_opts, group=retry_opt_group) diff --git a/barbican/model/repositories.py b/barbican/model/repositories.py index 435a862ae..8fa95e34b 100644 --- a/barbican/model/repositories.py +++ b/barbican/model/repositories.py @@ -26,7 +26,7 @@ import sys import time from oslo_db import exception as db_exc -from oslo_db.sqlalchemy import session +from oslo_db.sqlalchemy import enginefacade from oslo_utils import timeutils from oslo_utils import uuidutils import sqlalchemy @@ -167,25 +167,9 @@ def get_session(): def _get_engine(engine): if not engine: - connection = CONF.database.connection - if not connection: - raise exception.BarbicanException( - u._('No SQL connection configured')) - - # TODO(jfwood): - # connection_dict = sqlalchemy.engine.url.make_url(_CONNECTION) - - engine_args = { - 'connection_recycle_time': CONF.database.connection_recycle_time - } - if CONF.database.max_pool_size: - engine_args['max_pool_size'] = CONF.database.max_pool_size - if CONF.database.max_overflow: - engine_args['max_overflow'] = CONF.database.max_overflow - db_connection = None try: - engine = _create_engine(connection, **engine_args) + engine = _create_engine() db_connection = engine.connect() except Exception as err: msg = u._( @@ -209,6 +193,10 @@ def _get_engine(engine): return engine +def _create_engine(): + return enginefacade.writer.get_engine() + + def model_query(model, *args, **kwargs): """Query helper for simpler session usage.""" session = kwargs.get('session') @@ -239,22 +227,6 @@ def is_db_connection_error(args): return False -def _create_engine(connection, **engine_args): - LOG.debug('Sql connection: please check "sql_connection" property in ' - 'barbican configuration file; Args: %s', engine_args) - - engine = session.create_engine(connection, **engine_args) - - # TODO(jfwood): if 'mysql' in connection_dict.drivername: - # TODO(jfwood): sqlalchemy.event.listen(_ENGINE, 'checkout', - # TODO(jfwood): ping_listener) - - # Wrap the engine's connect method with a retry decorator. - engine.connect = wrap_db_error(engine.connect) - - return engine - - def _auto_generate_tables(engine, tables): if tables and 'alembic_version' in tables: # Upgrade the database to the latest version. @@ -269,35 +241,6 @@ def _auto_generate_tables(engine, tables): commands.stamp() -def wrap_db_error(f): - """Retry DB connection. Copied from nova and modified.""" - def _wrap(*args, **kwargs): - try: - return f(*args, **kwargs) - except sqlalchemy.exc.OperationalError as e: - if not is_db_connection_error(e.args[0]): - raise - - remaining_attempts = CONF.database.max_retries - while True: - LOG.warning('SQL connection failed. %d attempts left.', - remaining_attempts) - remaining_attempts -= 1 - time.sleep(CONF.database.retry_interval) - try: - return f(*args, **kwargs) - except sqlalchemy.exc.OperationalError as e: - if (remaining_attempts <= 0 or not - is_db_connection_error(e.args[0])): - raise - except sqlalchemy.exc.DBAPIError: - raise - except sqlalchemy.exc.DBAPIError: - raise - _wrap.__name__ = f.__name__ - return _wrap - - def clean_paging_values(offset_arg=0, limit_arg=CONF.default_limit_paging): """Cleans and safely limits raw paging offset/limit values.""" offset_arg = offset_arg or 0 diff --git a/barbican/tests/model/repositories/test_repositories.py b/barbican/tests/model/repositories/test_repositories.py index 3f9bba744..fd823549c 100644 --- a/barbican/tests/model/repositories/test_repositories.py +++ b/barbican/tests/model/repositories/test_repositories.py @@ -14,7 +14,6 @@ from unittest import mock from alembic import script as alembic_script from oslo_config import cfg -import sqlalchemy from barbican.common import config from barbican.common import exception @@ -194,28 +193,6 @@ class WhenTestingBaseRepository(database_utils.RepositoryTestCase): str(exception_result)) -class WhenTestingWrapDbError(utils.BaseTestCase): - - def setUp(self): - super(WhenTestingWrapDbError, self).setUp() - repositories.CONF.set_override("max_retries", 0, "database") - repositories.CONF.set_override("retry_interval", 0, "database") - - @mock.patch('barbican.model.repositories.is_db_connection_error') - def test_should_raise_operational_error_is_connection_error( - self, mock_is_db_error): - mock_is_db_error.return_value = True - - @repositories.wrap_db_error - def test_function(): - raise sqlalchemy.exc.OperationalError( - 'statement', 'params', 'orig') - - self.assertRaises( - sqlalchemy.exc.OperationalError, - test_function) - - class WhenTestingGetEnginePrivate(utils.BaseTestCase): def setUp(self): @@ -240,6 +217,7 @@ class WhenTestingGetEnginePrivate(utils.BaseTestCase): 'Error configuring registry database with supplied ' 'database connection. Got error: Abort!', str(exception_result)) + engine.connect.assert_called_once_with() @mock.patch('barbican.model.repositories._create_engine') def test_should_complete_with_no_alembic_create_default_configs( @@ -253,12 +231,7 @@ class WhenTestingGetEnginePrivate(utils.BaseTestCase): repositories._get_engine(None) engine.connect.assert_called_once_with() - mock_create_engine.assert_called_once_with( - 'connection', - connection_recycle_time=3600, - max_pool_size=repositories.CONF.database.max_pool_size, - max_overflow=repositories.CONF.database.max_overflow - ) + mock_create_engine.assert_called_once_with() @mock.patch('barbican.model.repositories._create_engine') def test_should_complete_with_no_alembic_create_pool_configs( @@ -277,12 +250,7 @@ class WhenTestingGetEnginePrivate(utils.BaseTestCase): repositories._get_engine(None) engine.connect.assert_called_once_with() - mock_create_engine.assert_called_once_with( - 'connection', - connection_recycle_time=3600, - max_pool_size=22, - max_overflow=11 - ) + mock_create_engine.assert_called_once_with() class WhenTestingAutoGenerateTables(utils.BaseTestCase): diff --git a/devstack/lib/barbican b/devstack/lib/barbican index 8d190aa5f..3672f67c9 100644 --- a/devstack/lib/barbican +++ b/devstack/lib/barbican @@ -154,7 +154,7 @@ function configure_barbican { fi # Set the database connection url - iniset $BARBICAN_CONF DEFAULT sql_connection `database_connection_url barbican` + iniset $BARBICAN_CONF database connection `database_connection_url barbican` # Disable auto-migration when deploying Barbican iniset $BARBICAN_CONF DEFAULT db_auto_create False diff --git a/etc/oslo-config-generator/barbican.conf b/etc/oslo-config-generator/barbican.conf index 8cb13ae2e..b6dbd2dbb 100644 --- a/etc/oslo-config-generator/barbican.conf +++ b/etc/oslo-config-generator/barbican.conf @@ -10,6 +10,7 @@ namespace = barbican.plugin.secret_store.kmip namespace = barbican.plugin.secret_store.vault namespace = keystonemiddleware.audit namespace = keystonemiddleware.auth_token +namespace = oslo.db namespace = oslo.log namespace = oslo.messaging namespace = oslo.middleware.cors diff --git a/releasenotes/notes/oslo-db-9381293cc7bedc7d.yaml b/releasenotes/notes/oslo-db-9381293cc7bedc7d.yaml new file mode 100644 index 000000000..f30b49b48 --- /dev/null +++ b/releasenotes/notes/oslo-db-9381293cc7bedc7d.yaml @@ -0,0 +1,17 @@ +--- +features: + - | + Now Barbican uses oslo.db for database connection. The features implemented + in oslo.db can be now leveraged in Barbican. + +upgrade: + - | + The following deprecated database options were effectively removed. Use + the equivalent oslo.db library options instead. + + - ``[DEFAULT] sql_connection`` + - ``[DEFAULT] sql_idle_timeout`` + - ``[DEFAULT] sql_max_retries`` + - ``[DEFAULT] sql_retry_interval`` + - ``[DEFAULT] sql_pool_size`` + - ``[DEFAULT] sql_pool_max_overflow``