From 8ed9c9434fb5911f57a30a7000b30b17d9840aa0 Mon Sep 17 00:00:00 2001 From: Chris Dent Date: Mon, 2 Sep 2019 12:44:50 +0100 Subject: [PATCH] Tune up db.instance_get_all_uuids_by_hosts When instance_get_all_uuids_by_hosts was added [1] some follow up cleanups where suggested. This change provides them: * removal of redundance in docstring * moving docstring to the public method, rather than the private implementation * more clarity on the type of the default (defaultdict(list)) and the implications thereof * Using an sa.bindparam in the 'in_' call. This requires that the SQLAlchemy requirment be raised to at least 1.2.0 where the feature was added. 1.2.19, the latest bugfix release, is chosen. [1] If92fe8b75d20a738f37e2a74c52c59bfc699a74f Change-Id: Ib538ab070d73b06ddeb9fea3af149304e40952ec --- lower-constraints.txt | 2 +- nova/db/sqlalchemy/api.py | 16 ++++++++-------- nova/objects/instance.py | 3 +-- nova/tests/unit/db/test_db_api.py | 4 ++-- nova/tests/unit/objects/test_instance.py | 3 ++- requirements.txt | 2 +- 6 files changed, 15 insertions(+), 15 deletions(-) diff --git a/lower-constraints.txt b/lower-constraints.txt index 42d97ad814..718a3093bc 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -145,7 +145,7 @@ simplejson==3.13.2 six==1.10.0 smmap2==2.0.3 sortedcontainers==2.1.0 -SQLAlchemy==1.0.10 +SQLAlchemy==1.2.19 sqlalchemy-migrate==0.11.0 sqlparse==0.2.4 statsd==3.2.2 diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index a6a58e92ee..37321c1e82 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -2659,28 +2659,28 @@ def instance_get_all_by_host(context, host, columns_to_join=None): def _instance_get_all_uuids_by_hosts(context, hosts): - """Return a dict, keyed by hostname, of a list of the instance uuids on the - host for each supplied hostname. - - Returns a dict, keyed by hostname, of a list of UUIDs, not Instance model - objects. - """ itbl = models.Instance.__table__ default_deleted_value = itbl.c.deleted.default.arg sel = sql.select([itbl.c.host, itbl.c.uuid]) sel = sel.where(sql.and_( itbl.c.deleted == default_deleted_value, - itbl.c.host.in_(hosts))) + itbl.c.host.in_(sa.bindparam('hosts', expanding=True)))) # group the instance UUIDs by hostname res = collections.defaultdict(list) - for rec in context.session.execute(sel).fetchall(): + for rec in context.session.execute(sel, {'hosts': hosts}).fetchall(): res[rec[0]].append(rec[1]) return res @pick_context_manager_reader def instance_get_all_uuids_by_hosts(context, hosts): + """Return a dict, keyed by hostname, of a list of the instance uuids on the + host for each supplied hostname, not Instance model objects. + + The dict is a defaultdict of list, thus inspecting the dict for a host not + in the dict will return an empty list not a KeyError. + """ return _instance_get_all_uuids_by_hosts(context, hosts) diff --git a/nova/objects/instance.py b/nova/objects/instance.py index abc07fe641..83a313ecbf 100644 --- a/nova/objects/instance.py +++ b/nova/objects/instance.py @@ -1430,8 +1430,7 @@ class InstanceList(base.ObjectListBase, base.NovaObject): @base.remotable_classmethod def get_uuids_by_host(cls, context, host): - return db.instance_get_all_uuids_by_hosts(context, [host]).get( - host, []) + return db.instance_get_all_uuids_by_hosts(context, [host])[host] @base.remotable_classmethod def get_uuids_by_hosts(cls, context, hosts): diff --git a/nova/tests/unit/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py index 0f8be358d5..9eb97d03a0 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/nova/tests/unit/db/test_db_api.py @@ -362,8 +362,8 @@ class UnsupportedDbRegexpTestCase(DbTestCase): test2 = self.create_instance_with_args(display_name='test2') test3 = self.create_instance_with_args(display_name='test3') uuids = [i.uuid for i in (test1, test2, test3)] - results = db.instance_get_all_uuids_by_hosts(self.context, - [test1.host]) + results = db.instance_get_all_uuids_by_hosts( + self.context, [test1.host]) self.assertEqual(1, len(results)) self.assertIn(test1.host, results) found_uuids = results[test1.host] diff --git a/nova/tests/unit/objects/test_instance.py b/nova/tests/unit/objects/test_instance.py index ff3f8fcd07..f3dff7ece2 100644 --- a/nova/tests/unit/objects/test_instance.py +++ b/nova/tests/unit/objects/test_instance.py @@ -12,6 +12,7 @@ # License for the specific language governing permissions and limitations # under the License. +import collections import datetime import six @@ -1915,7 +1916,7 @@ class _TestInstanceListObject(object): @mock.patch('nova.db.api.instance_get_all_uuids_by_hosts') def test_get_uuids_by_host_no_match(self, mock_get_all): - mock_get_all.return_value = {} + mock_get_all.return_value = collections.defaultdict(list) actual_uuids = objects.InstanceList.get_uuids_by_host( self.context, 'b') self.assertEqual([], actual_uuids) diff --git a/requirements.txt b/requirements.txt index 2849da6252..ca308300c9 100644 --- a/requirements.txt +++ b/requirements.txt @@ -3,7 +3,7 @@ # process, which may cause wedges in the gate later. pbr!=2.1.0,>=2.0.0 # Apache-2.0 -SQLAlchemy!=1.1.5,!=1.1.6,!=1.1.7,!=1.1.8,>=1.0.10 # MIT +SQLAlchemy>=1.2.19 # MIT decorator>=3.4.0 # BSD eventlet!=0.20.1,>=0.20.0 # MIT Jinja2>=2.10 # BSD License (3 clause)