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
(cherry picked from commit 1706e39)
(cherry picked from commit 344029b)
(cherry picked from commit b00b2fe)

Author: mriedem

nova/compute/api.py

@@ -2315,9 +2315,17 @@ def _remap_system_metadata_filter(metadata):
         # [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

nova/objects/build_request.py

@@ -370,7 +370,8 @@ def get_by_filters(cls, context, filters, limit=None, marker=None,
 
             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)
 
@@ -383,6 +384,8 @@ def get_by_filters(cls, context, filters, limit=None, marker=None,
                 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:

nova/tests/functional/db/test_build_request.py

@@ -526,6 +526,13 @@ def test_get_by_filters_marker(self):
             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(),

nova/tests/unit/compute/test_compute_api.py

@@ -4345,7 +4345,7 @@ def test_get_all_includes_build_requests(self, mock_cell_mapping_get,
                 self.context, {'foo': 'bar'}, limit=None, marker='fake-marker',
                 sort_keys=['baz'], sort_dirs=['desc'])
             mock_inst_get.assert_called_once_with(
-                self.context, {'foo': 'bar'}, limit=None, marker='fake-marker',
+                self.context, {'foo': 'bar'}, limit=None, marker=None,
                 expected_attrs=None, sort_keys=['baz'], sort_dirs=['desc'])
             for i, instance in enumerate(build_req_instances + cell_instances):
                 self.assertEqual(instance, instances[i])
@@ -4379,7 +4379,7 @@ def test_get_all_includes_build_requests_filter_dupes(self,
                 self.context, {'foo': 'bar'}, limit=None, marker='fake-marker',
                 sort_keys=['baz'], sort_dirs=['desc'])
             mock_inst_get.assert_called_once_with(
-                self.context, {'foo': 'bar'}, limit=None, marker='fake-marker',
+                self.context, {'foo': 'bar'}, limit=None, marker=None,
                 expected_attrs=None, sort_keys=['baz'], sort_dirs=['desc'])
             for i, instance in enumerate(build_req_instances + cell_instances):
                 self.assertEqual(instance, instances[i])
@@ -4413,14 +4413,15 @@ def test_get_all_build_requests_decrement_limit(self,
                 self.context, {'foo': 'bar'}, limit=10, marker='fake-marker',
                 sort_keys=['baz'], sort_dirs=['desc'])
             mock_inst_get.assert_called_once_with(
-                self.context, {'foo': 'bar'}, limit=8, marker='fake-marker',
+                self.context, {'foo': 'bar'}, limit=8, marker=None,
                 expected_attrs=None, sort_keys=['baz'], sort_dirs=['desc'])
             for i, instance in enumerate(build_req_instances + cell_instances):
                 self.assertEqual(instance, instances[i])
 
     @mock.patch.object(context, 'target_cell')
     @mock.patch.object(objects.BuildRequestList, 'get_by_filters',
-                       return_value=objects.BuildRequestList(objects=[]))
+                       side_effect=exception.MarkerNotFound(
+                           marker='fake-marker'))
     @mock.patch.object(objects.CellMapping, 'get_by_uuid')
     def test_get_all_includes_cell0(self, mock_cell_mapping_get,
                                     mock_buildreq_get, mock_target_cell):
@@ -4496,11 +4497,11 @@ def test_get_all_includes_build_request_cell0(self, mock_cell_mapping_get,
             mock_target_cell.assert_called_once_with(self.context,
                                                      cell_mapping)
             inst_get_calls = [mock.call(self.context, {'foo': 'bar'},
-                                        limit=8, marker='fake-marker',
+                                        limit=8, marker=None,
                                         expected_attrs=None, sort_keys=['baz'],
                                         sort_dirs=['desc']),
                               mock.call(self.context, {'foo': 'bar'},
-                                        limit=6, marker='fake-marker',
+                                        limit=6, marker=None,
                                         expected_attrs=None, sort_keys=['baz'],
                                         sort_dirs=['desc'])
                               ]
@@ -4513,7 +4514,8 @@ def test_get_all_includes_build_request_cell0(self, mock_cell_mapping_get,
 
     @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')
     def test_get_all_cell0_marker_not_found(self, mock_cell_mapping_get,
                                             mock_buildreq_get,