From c84846dbf93588e5420b3565d163c0393443e6d6 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Tue, 22 Oct 2019 17:59:32 -0400 Subject: [PATCH] Fix listing deleted servers with a marker Change I1aa3ca6cc70cef65d24dec1e7db9491c9b73f7ab in Queens, which was backported through to Newton, introduced a regression when listing deleted servers with a marker because it assumes that if BuildRequestList.get_by_filters does not raise MarkerNotFound that the marker was found among the build requests and does not account for that get_by_filters method short-circuiting if filtering servers with deleted/cleaned/limit=0. The API code then nulls out the marker which means you'll continue to get the marker instance back in the results even though you shouldn't, and that can cause an infinite loop in some client-side tooling like nova's CLI: nova list --deleted --limit -1 This fixes the bug by raising MarkerNotFound from BuildRequestList.get_by_filters if we have a marker but are short-circuiting and returning early from the method based on limit or filters. Conflicts: nova/tests/functional/regressions/test_bug_1849409.py NOTE(mriedem): The conflict is due to the Rocky-specific change to the functional test in I324193129acb6ac739133c7e76920762a8987a84 due to test_concurrent_request_server_group_members_over_quota leaking deleted servers for some reason. Change-Id: Ic2b19c2aa06b3059ab0344b6ac56ffd62b3f755d Closes-Bug: #1849409 (cherry picked from commit df03499843aa7fd6089bd4d07b9d0eb5a8c14b47) (cherry picked from commit 03a2508362ecf50463d0659142312a30d0fb91f3) (cherry picked from commit 6038455e1dafeaf8ccfdd4be9e993beea3c9fb45) --- nova/objects/build_request.py | 12 +++++++----- .../tests/functional/regressions/test_bug_1849409.py | 4 +--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/nova/objects/build_request.py b/nova/objects/build_request.py index 86303c62afe0..61f5116f2e6f 100644 --- a/nova/objects/build_request.py +++ b/nova/objects/build_request.py @@ -372,14 +372,16 @@ class BuildRequestList(base.ObjectListBase, base.NovaObject): @base.remotable_classmethod def get_by_filters(cls, context, filters, limit=None, marker=None, sort_keys=None, sort_dirs=None): - if limit == 0: - return cls(context, objects=[]) + # Short-circuit on anything that will not yield results. # 'deleted' records can not be returned from here since build_requests # are not soft deleted. - if filters.get('deleted', False): - return cls(context, objects=[]) # 'cleaned' records won't exist as they would need to be deleted. - if filters.get('cleaned', False): + if (limit == 0 or + filters.get('deleted', False) or + filters.get('cleaned', False)): + # If we have a marker honor the MarkerNotFound semantics. + if marker: + raise exception.MarkerNotFound(marker=marker) return cls(context, objects=[]) # Because the build_requests table stores an instance as a serialized diff --git a/nova/tests/functional/regressions/test_bug_1849409.py b/nova/tests/functional/regressions/test_bug_1849409.py index 1b432a2b0cde..f3a4f60a74e4 100644 --- a/nova/tests/functional/regressions/test_bug_1849409.py +++ b/nova/tests/functional/regressions/test_bug_1849409.py @@ -59,7 +59,5 @@ class ListDeletedServersWithMarker(test.TestCase, servers = self.api.get_servers(detail=False, search_opts={'deleted': True, 'marker': server['id']}) - # FIXME(mriedem): This is bug 1849409 where the marker param is not - # honored correctly when using deleted=True. server_names = [serv['name'] for serv in servers] - self.assertIn(server['name'], server_names) + self.assertNotIn(server['name'], server_names)