From c4820305d2f9ee8d62bcc708baf3fa6dfe7ca960 Mon Sep 17 00:00:00 2001 From: Kevin_Zheng Date: Fri, 14 Apr 2017 11:57:59 +0800 Subject: [PATCH] Use deepcopy when process filters in db api In db API when we process filters, we didn't use deepcopy. In cases of "tags" and "not-tags" we used pop to get the first tag, filtered out results, and then joined with other tags for later filtering. When we did pop(), the original value was deleted, the key "tags"/"not-tags" remains. In the cell scenario, both single cell(we will query cell0 and the other cell) and multicell, as we have to query all the cells in a loop and the tags list in the filter will keep popping, this will lead to either a HTTP 500 error(popping from an empty list) or incorrect result(when number of tags in the list is larger than cell number, no HTTP 500 will show, but the filter results for each cell will be different as each loop will pop one tag). closes-bug: #1682693 Change-Id: Ia2738dd0c7d1842b68c83d0a9e75e26b2f8d492a --- nova/db/sqlalchemy/api.py | 2 +- .../regressions/test_bug_1682693.py | 9 ++----- nova/tests/unit/db/test_db_api.py | 26 +++++++++++++++++++ 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 7d688987f542..754fac7eba55 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -2147,7 +2147,7 @@ def instance_get_all_by_filters_sort(context, filters, limit=None, marker=None, # Make a copy of the filters dictionary to use going forward, as we'll # be modifying it and we shouldn't affect the caller's use of it. - filters = filters.copy() + filters = copy.deepcopy(filters) if 'changes-since' in filters: changes_since = timeutils.normalize_time(filters['changes-since']) diff --git a/nova/tests/functional/regressions/test_bug_1682693.py b/nova/tests/functional/regressions/test_bug_1682693.py index 03249a9a123f..c16229ead530 100644 --- a/nova/tests/functional/regressions/test_bug_1682693.py +++ b/nova/tests/functional/regressions/test_bug_1682693.py @@ -14,7 +14,6 @@ from nova import test from nova.tests import fixtures as nova_fixtures -from nova.tests.functional.api import client as api_client from nova.tests.functional import integrated_helpers from nova.tests.unit.image import fake as image_fake from nova.tests.unit import policy_fixture @@ -90,9 +89,5 @@ class ServerTagsFilteringTest(test.TestCase, self.assertEqual(['bar', 'foo'], sorted(server['tags'])) # query for the shared tag and we should get two servers back - # FIXME(mriedem): This causes a 500 error until bug 1682693 is fixed. - ex = self.assertRaises(api_client.OpenStackApiException, - self.api.get_servers, - search_opts=dict(tags='foo')) - self.assertEqual(500, ex.response.status_code) - self.assertIn('IndexError', ex.message) + servers = self.api.get_servers(search_opts=dict(tags='foo')) + self.assertEqual(2, len(servers)) diff --git a/nova/tests/unit/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py index 91968dc27f0a..259f730d5e36 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/nova/tests/unit/db/test_db_api.py @@ -10348,6 +10348,32 @@ class TestInstanceTagsFiltering(test.TestCase): self._assertEqualInstanceUUIDs([uuids[0], uuids[1], uuids[3], uuids[4], uuids[6], uuids[7]], result) + def test_instance_get_all_by_filters_not_tags_multiple_cells(self): + """Test added for bug 1682693. + + In cells v2 scenario, db.instance_get_all_by_filters() will + be called multiple times to search across all cells. This + test tests that filters for all cells remain the same in the + loop. + """ + uuids = self._create_instances(8) + + db.instance_tag_set(self.ctxt, uuids[0], [u't1']) + db.instance_tag_set(self.ctxt, uuids[1], [u't2']) + db.instance_tag_set(self.ctxt, uuids[2], [u't1', u't2']) + db.instance_tag_set(self.ctxt, uuids[3], [u't2', u't3']) + db.instance_tag_set(self.ctxt, uuids[4], [u't3']) + db.instance_tag_set(self.ctxt, uuids[5], [u't1', u't2', u't3']) + db.instance_tag_set(self.ctxt, uuids[6], [u't3', u't4']) + db.instance_tag_set(self.ctxt, uuids[7], []) + + filters = {'not-tags': [u't1', u't2']} + + result = db.instance_get_all_by_filters(self.ctxt, filters) + + self._assertEqualInstanceUUIDs([uuids[0], uuids[1], uuids[3], uuids[4], + uuids[6], uuids[7]], result) + def test_instance_get_all_by_filters_not_tags_any(self): uuids = self._create_instances(8)