From 1706e3989157f912bce99beb99c75216a064eb2d Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Tue, 12 Dec 2017 21:27:28 -0500 Subject: [PATCH] Raise MarkerNotFound if BuildRequestList.get_by_filters doesn't find marker For some reason, probably because build requests are meant to be short lived and we don't get a lot of bugs about paging misbehavior, when paging instances with a marker, we didn't raise MarkerNotFound if we didn't find the marker in the list of build requests. Doing so would match what we do when paging over cells and listing instances using a marker. Once we find the marker, be that in build_requests, or one of the cells, we need to set the marker to None to stop looking for it elsewhere if we have more space to fill our limit. For example, see change I8a957bebfcecd6ac712103c346e028d80f1ecd7c. This patch fixes the issue by raising MarkerNotFound from BuildRequestList get_by_filters if there is a marker and we didn't find a build request for it. The compute API get_all() method handles that as normal and continues looking for the marker in one of the cells. Change-Id: I1aa3ca6cc70cef65d24dec1e7db9491c9b73f7ab Closes-Bug: #1737856 --- nova/compute/api.py | 14 +++++++++++--- nova/objects/build_request.py | 5 ++++- .../tests/functional/db/test_build_request.py | 7 +++++++ nova/tests/unit/compute/test_compute_api.py | 19 ++++++++++--------- 4 files changed, 32 insertions(+), 13 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 073c841ec4db..a23c66a59c9b 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -2353,9 +2353,17 @@ class API(base.Base): # [sorted instances with no host] + [sorted instances with host]. # This means BuildRequest and cell0 instances first, then cell # instances - build_requests = objects.BuildRequestList.get_by_filters( - context, filters, limit=limit, marker=marker, sort_keys=sort_keys, - sort_dirs=sort_dirs) + try: + build_requests = objects.BuildRequestList.get_by_filters( + context, filters, limit=limit, marker=marker, + sort_keys=sort_keys, sort_dirs=sort_dirs) + # If we found the marker in we need to set it to None + # so we don't expect to find it in the cells below. + marker = None + except exception.MarkerNotFound: + # If we didn't find the marker in the build requests then keep + # looking for it in the cells. + build_requests = objects.BuildRequestList() build_req_instances = objects.InstanceList( objects=[build_req.instance for build_req in build_requests]) # Only subtract from limit if it is not None diff --git a/nova/objects/build_request.py b/nova/objects/build_request.py index 5dc7ddf96b2b..38006c50f63b 100644 --- a/nova/objects/build_request.py +++ b/nova/objects/build_request.py @@ -433,7 +433,8 @@ class BuildRequestList(base.ObjectListBase, base.NovaObject): filtered_build_reqs.append(build_req) - if (len(filtered_build_reqs) < 2) or (not sort_keys): + if (((len(filtered_build_reqs) < 2) or (not sort_keys)) + and not marker): # No need to sort return cls(context, objects=filtered_build_reqs) @@ -446,6 +447,8 @@ class BuildRequestList(base.ObjectListBase, base.NovaObject): if build_req.instance.uuid == marker: marker_index = i break + else: + raise exception.MarkerNotFound(marker=marker) len_build_reqs = len(sorted_build_reqs) limit_index = len_build_reqs if limit: diff --git a/nova/tests/functional/db/test_build_request.py b/nova/tests/functional/db/test_build_request.py index b097f59bcd00..e34109cd8931 100644 --- a/nova/tests/functional/db/test_build_request.py +++ b/nova/tests/functional/db/test_build_request.py @@ -526,6 +526,13 @@ class BuildRequestListTestCase(test.NoDBTestCase): objects.base.obj_equal_prims(req.instance, req_list[i].instance) + def test_get_by_filters_marker_not_found(self): + self._create_req() + self.assertRaises(exception.MarkerNotFound, + build_request.BuildRequestList.get_by_filters, + self.context, {}, marker=uuidutils.generate_uuid(), + sort_keys=['id'], sort_dirs=['asc']) + def test_get_by_filters_limit(self): reqs = [self._create_req(), self._create_req(), diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 59bc87a17a34..0b8ae9edb667 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -5141,7 +5141,7 @@ class _ComputeAPIUnitTestMixIn(object): sort_keys=['baz'], sort_dirs=['desc']) fields = ['metadata', 'info_cache', 'security_groups'] mock_inst_get.assert_called_once_with( - self.context, {'foo': 'bar'}, None, 'fake-marker', + self.context, {'foo': 'bar'}, None, None, fields, ['baz'], ['desc']) for i, instance in enumerate(build_req_instances + cell_instances): self.assertEqual(instance, instances[i]) @@ -5176,7 +5176,7 @@ class _ComputeAPIUnitTestMixIn(object): sort_keys=['baz'], sort_dirs=['desc']) fields = ['metadata', 'info_cache', 'security_groups'] mock_inst_get.assert_called_once_with( - self.context, {'foo': 'bar'}, None, 'fake-marker', + self.context, {'foo': 'bar'}, None, None, fields, ['baz'], ['desc']) for i, instance in enumerate(build_req_instances + cell_instances): self.assertEqual(instance, instances[i]) @@ -5211,7 +5211,7 @@ class _ComputeAPIUnitTestMixIn(object): sort_keys=['baz'], sort_dirs=['desc']) fields = ['metadata', 'info_cache', 'security_groups'] mock_inst_get.assert_called_once_with( - self.context, {'foo': 'bar'}, 8, 'fake-marker', + self.context, {'foo': 'bar'}, 8, None, fields, ['baz'], ['desc']) for i, instance in enumerate(build_req_instances + cell_instances): self.assertEqual(instance, instances[i]) @@ -5249,7 +5249,7 @@ class _ComputeAPIUnitTestMixIn(object): fields = ['metadata', 'info_cache', 'security_groups'] mock_inst_get.assert_called_once_with( mock.ANY, {'foo': 'bar'}, - 8, 'fake-marker', + 8, None, fields, ['baz'], ['desc']) for i, instance in enumerate(build_req_instances + cell_instances): @@ -5549,7 +5549,7 @@ class Cellsv1DeprecatedTestMixIn(object): sort_keys=['baz'], sort_dirs=['desc']) fields = ['metadata', 'info_cache', 'security_groups'] mock_inst_get.assert_called_once_with( - self.context, {'foo': 'bar'}, limit=None, marker='fake-marker', + self.context, {'foo': 'bar'}, limit=None, marker=None, fields=fields, sort_keys=['baz'], sort_dirs=['desc']) for i, instance in enumerate(build_req_instances + cell_instances): self.assertEqual(instance, instances[i]) @@ -5584,7 +5584,7 @@ class Cellsv1DeprecatedTestMixIn(object): sort_keys=['baz'], sort_dirs=['desc']) fields = ['metadata', 'info_cache', 'security_groups'] mock_inst_get.assert_called_once_with( - self.context, {'foo': 'bar'}, limit=None, marker='fake-marker', + self.context, {'foo': 'bar'}, limit=None, marker=None, fields=fields, sort_keys=['baz'], sort_dirs=['desc']) for i, instance in enumerate(build_req_instances + cell_instances): self.assertEqual(instance, instances[i]) @@ -5619,7 +5619,7 @@ class Cellsv1DeprecatedTestMixIn(object): sort_keys=['baz'], sort_dirs=['desc']) fields = ['metadata', 'info_cache', 'security_groups'] mock_inst_get.assert_called_once_with( - self.context, {'foo': 'bar'}, limit=8, marker='fake-marker', + self.context, {'foo': 'bar'}, limit=8, marker=None, fields=fields, sort_keys=['baz'], sort_dirs=['desc']) for i, instance in enumerate(build_req_instances + cell_instances): self.assertEqual(instance, instances[i]) @@ -5669,7 +5669,7 @@ class Cellsv1DeprecatedTestMixIn(object): mock_target_cell.assert_any_call(self.context, cm) fields = ['metadata', 'info_cache', 'security_groups'] inst_get_calls = [mock.call(cctxt, {'foo': 'bar'}, - limit=8, marker='fake-marker', + limit=8, marker=None, fields=fields, sort_keys=['baz'], sort_dirs=['desc']), mock.call(mock.ANY, {'foo': 'bar'}, @@ -5699,7 +5699,8 @@ class Cellsv1DeprecatedTestMixIn(object): @mock.patch.object(context, 'target_cell') @mock.patch.object(objects.BuildRequestList, 'get_by_filters', - return_value=objects.BuildRequestList(objects=[])) + side_effect=exception.MarkerNotFound( + marker=uuids.marker)) @mock.patch.object(objects.CellMapping, 'get_by_uuid') @mock.patch.object(objects.CellMappingList, 'get_all') def test_get_all_cell0_marker_not_found(self, mock_cm_get_all,