From 9993efdeef4606f78182d084e4120a158a4e64b8 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Mon, 29 Jan 2018 17:22:30 -0500 Subject: [PATCH] Allow connection query string to be passed separately. The Nova project has made the decision to store the entire contents of the "sql_connection" field in the database, with alterations to the host value, such that various "cells" database URLs can be located using Nova's database itself as a central registry of database URLs. This architecture has produced several problems. The first is that it is impossible to apply parameters to the URL that are local to the client machine; the leading example of this is the MySQL "bind_host" variable, which must match the hostname of the connecting host. Because cells puts these URLs into the database and shares them with all controllers, we have to use a workaround with the read_default_file parameter to specify a controller-local file of options; this is not a standard thing for other database drivers, and these parameters only apply to the MySQL driver and not the SQLAlchemy engine. The next issue is that it is inconvenient to add parameters to the URL at all, once Nova has already been running, as one must manually use Nova's command line tools to alter all the URLs that have already been copied into the database and alter the query parameters with each of those individually, and then restart *all* services who will all receive the parameter (no way to add params to just one controller). Nova's "store the URL in the database" feature only needs to be able to locate the host / database name of the alternate database, and not change the URL tuning parameters. This patch adds a new oslo.db parameter connection_parameters which allows the params to be applied separately from the sql_connection parameter, so that Nova can continue persisting sql_connection but the parameters remain local to the nova.conf file. A URL parameter that truly had to remain persisted in Nova's database (there aren't any) could still be applied at the sql_connection level. This feature is essential not just so that we can again place simple parameters into controller-local files like "bind_host", but also to allow for configuration of SQLAlchemy features such as plugins that do connection pool monitoring. Change-Id: Id4de4b09ec4719cbf8b372629fcf58cf368a33d4 --- oslo_db/options.py | 5 ++ oslo_db/sqlalchemy/enginefacade.py | 6 +- oslo_db/sqlalchemy/engines.py | 20 ++++- oslo_db/tests/sqlalchemy/test_sqlalchemy.py | 74 +++++++++++++++++++ ...onnection_parameters-231aa7d8b7d2d416.yaml | 7 ++ 5 files changed, 110 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/add_connection_parameters-231aa7d8b7d2d416.yaml diff --git a/oslo_db/options.py b/oslo_db/options.py index 8bd70a25..9a08552c 100644 --- a/oslo_db/options.py +++ b/oslo_db/options.py @@ -146,6 +146,11 @@ database_opts = [ 'error before error is ' 'raised. Set to -1 to specify an infinite retry ' 'count.'), + cfg.StrOpt('connection_parameters', + default='', + help='Optional URL parameters to append onto the connection ' + 'URL at connect time; specify as ' + 'param1=value1¶m2=value2&...'), ] diff --git a/oslo_db/sqlalchemy/enginefacade.py b/oslo_db/sqlalchemy/enginefacade.py index 80c320ba..fe26b9ad 100644 --- a/oslo_db/sqlalchemy/enginefacade.py +++ b/oslo_db/sqlalchemy/enginefacade.py @@ -149,7 +149,8 @@ class _TransactionFactory(object): 'thread_checkin': _Default(True), 'json_serializer': _Default(None), 'json_deserializer': _Default(None), - 'logging_name': _Default(None) + 'logging_name': _Default(None), + 'connection_parameters': _Default(None) } self._maker_cfg = { 'expire_on_commit': _Default(False), @@ -219,6 +220,9 @@ class _TransactionFactory(object): :param connection_debug: engine logging level, defaults to 0. set to 50 for INFO, 100 for DEBUG. + :param connection_parameters: additional parameters to append onto the + database URL query string, pass as "param1=value1¶m2=value2&..." + :param max_pool_size: max size of connection pool, uses CONF for default diff --git a/oslo_db/sqlalchemy/engines.py b/oslo_db/sqlalchemy/engines.py index 2808ef48..05045ca8 100644 --- a/oslo_db/sqlalchemy/engines.py +++ b/oslo_db/sqlalchemy/engines.py @@ -104,6 +104,21 @@ def _setup_logging(connection_debug=0): logger.setLevel(logging.WARNING) +def _extend_url_parameters(url, connection_parameters): + for key, value in six.moves.urllib.parse.parse_qs( + connection_parameters).items(): + if key in url.query: + existing = url.query[key] + if not isinstance(existing, list): + url.query[key] = existing = utils.to_list(existing) + existing.extend(value) + value = existing + else: + url.query[key] = value + if len(value) == 1: + url.query[key] = value[0] + + def _vet_url(url): if "+" not in url.drivername and not url.drivername.startswith("sqlite"): if url.drivername.startswith("mysql"): @@ -132,11 +147,14 @@ def create_engine(sql_connection, sqlite_fk=False, mysql_sql_mode=None, connection_trace=False, max_retries=10, retry_interval=10, thread_checkin=True, logging_name=None, json_serializer=None, - json_deserializer=None): + json_deserializer=None, connection_parameters=None): """Return a new SQLAlchemy engine.""" url = sqlalchemy.engine.url.make_url(sql_connection) + if connection_parameters: + _extend_url_parameters(url, connection_parameters) + _vet_url(url) engine_args = { diff --git a/oslo_db/tests/sqlalchemy/test_sqlalchemy.py b/oslo_db/tests/sqlalchemy/test_sqlalchemy.py index 4ae25064..c078986e 100644 --- a/oslo_db/tests/sqlalchemy/test_sqlalchemy.py +++ b/oslo_db/tests/sqlalchemy/test_sqlalchemy.py @@ -213,6 +213,79 @@ class FakeDB2Engine(object): pass +class QueryParamTest(test_base.DbTestCase): + def _fixture(self): + from sqlalchemy import create_engine + + def _mock_create_engine(*arg, **kw): + return create_engine("sqlite://") + + return mock.patch( + "oslo_db.sqlalchemy.engines.sqlalchemy.create_engine", + side_effect=_mock_create_engine) + + def test_add_assorted_params(self): + with self._fixture() as ce: + engines.create_engine( + "mysql+pymysql://foo:bar@bat", + connection_parameters="foo=bar&bat=hoho&bat=param2") + + self.assertEqual( + ce.mock_calls[0][1][0].query, + {'bat': ['hoho', 'param2'], 'foo': 'bar'} + ) + + def test_add_no_params(self): + with self._fixture() as ce: + engines.create_engine( + "mysql+pymysql://foo:bar@bat") + + self.assertEqual( + ce.mock_calls[0][1][0].query, + {} + ) + + def test_combine_params(self): + with self._fixture() as ce: + engines.create_engine( + "mysql+pymysql://foo:bar@bat/" + "?charset=utf8¶m_file=tripleo.cnf", + connection_parameters="plugin=sqlalchemy_collectd&" + "collectd_host=127.0.0.1&" + "bind_host=192.168.1.5") + + self.assertEqual( + ce.mock_calls[0][1][0].query, + { + 'bind_host': '192.168.1.5', + 'charset': 'utf8', + 'collectd_host': '127.0.0.1', + 'param_file': 'tripleo.cnf', + 'plugin': 'sqlalchemy_collectd' + } + ) + + def test_combine_multi_params(self): + with self._fixture() as ce: + engines.create_engine( + "mysql+pymysql://foo:bar@bat/" + "?charset=utf8¶m_file=tripleo.cnf&plugin=connmon", + connection_parameters="plugin=sqlalchemy_collectd&" + "collectd_host=127.0.0.1&" + "bind_host=192.168.1.5") + + self.assertEqual( + ce.mock_calls[0][1][0].query, + { + 'bind_host': '192.168.1.5', + 'charset': 'utf8', + 'collectd_host': '127.0.0.1', + 'param_file': 'tripleo.cnf', + 'plugin': ['connmon', 'sqlalchemy_collectd'] + } + ) + + class MySQLDefaultModeTestCase(test_base.MySQLOpportunisticTestCase): def test_default_is_traditional(self): with self.engine.connect() as conn: @@ -357,6 +430,7 @@ class EngineFacadeTestCase(oslo_test.BaseTestCase): thread_checkin=mock.ANY, json_serializer=None, json_deserializer=None, + connection_parameters='', logging_name=mock.ANY, ) get_maker.assert_called_once_with(engine=create_engine(), diff --git a/releasenotes/notes/add_connection_parameters-231aa7d8b7d2d416.yaml b/releasenotes/notes/add_connection_parameters-231aa7d8b7d2d416.yaml new file mode 100644 index 00000000..c4fa1ab8 --- /dev/null +++ b/releasenotes/notes/add_connection_parameters-231aa7d8b7d2d416.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + Added new option connection_parameters which allows SQLAlchemy query + parameters to be stated separately from the URL itself, to allow + URL-persistence schemes like Nova cells to use controller-local + query parameters that aren't broadcast to all other servers.