From 89646a6cb43ac76895d097fa8c38b43800f1c5db Mon Sep 17 00:00:00 2001
From: Andrey Kurilin <andr.kurilin@gmail.com>
Date: Mon, 4 Dec 2023 16:32:25 +0100
Subject: [PATCH] Small improvements for Neutron Port and Nova Server cleanup

Change-Id: Ife293f744488ab8805b4da3187f342592eb5a54c
---
 CHANGELOG.rst                             |  5 +++
 rally_openstack/task/cleanup/resources.py | 31 +++++++++++++++-
 tests/unit/task/cleanup/test_resources.py | 45 +++++++++++++++++++----
 3 files changed, 72 insertions(+), 9 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 3fc736f5..7690ba8c 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -35,6 +35,11 @@ Fixed
   cannot be passed any longer due to more strict validation from
   python-novaclient library.
 
+* Neutron ports cleanup does not use API filtering by project_id which causes
+  redundant load to the cloud.
+
+* Nova server cleanup fails to list resources due to wrong marker.
+
 [2.3.0] - 2023-08-01
 --------------------
 
diff --git a/rally_openstack/task/cleanup/resources.py b/rally_openstack/task/cleanup/resources.py
index 9723a285..f54f0720 100644
--- a/rally_openstack/task/cleanup/resources.py
+++ b/rally_openstack/task/cleanup/resources.py
@@ -144,7 +144,33 @@ _nova_order = get_order(200)
 class NovaServer(base.ResourceManager):
     def list(self):
         """List all servers."""
-        return self._manager().list(limit=-1)
+        from novaclient import exceptions as nova_exc
+
+        nc = self._manager()
+
+        all_servers = []
+
+        marker = None
+        bad_markers = []
+        while True:
+            try:
+                servers = nc.list(marker=marker)
+            except nova_exc.BadRequest as e:
+                if f"marker [{marker}] not found" not in e.message:
+                    raise
+                LOG.error(f"Ignoring '{e.message}' error to fetch more "
+                          f"servers for cleanup.")
+                bad_markers.append(marker)
+                servers = []
+            else:
+                bad_markers = []
+
+            all_servers.extend(servers)
+            if not servers and (not bad_markers
+                                or len(bad_markers) == len(all_servers)):
+                break
+            marker = all_servers[-(len(bad_markers) + 1)].id
+        return all_servers
 
     def delete(self):
         if getattr(self.raw_resource, "OS-EXT-STS:locked", False):
@@ -431,7 +457,8 @@ class NeutronPort(NeutronMixin):
 
     def _get_resources(self, resource):
         if resource not in self._cache:
-            resources = getattr(self._neutron, "list_%s" % resource)()
+            getter = getattr(self._neutron, "list_%s" % resource)
+            resources = getter(tenant_id=self.tenant_uuid)
             self._cache[resource] = [r for r in resources
                                      if r["tenant_id"] == self.tenant_uuid]
         return self._cache[resource]
diff --git a/tests/unit/task/cleanup/test_resources.py b/tests/unit/task/cleanup/test_resources.py
index 08550903..0cb10daa 100644
--- a/tests/unit/task/cleanup/test_resources.py
+++ b/tests/unit/task/cleanup/test_resources.py
@@ -15,6 +15,7 @@
 
 import copy
 from unittest import mock
+import uuid
 
 import ddt
 from neutronclient.common import exceptions as neutron_exceptions
@@ -77,12 +78,41 @@ class MagnumMixinTestCase(test.TestCase):
 class NovaServerTestCase(test.TestCase):
 
     def test_list(self):
-        server = resources.NovaServer()
-        server._manager = mock.MagicMock()
+        class Server(object):
+            def __init__(self, id=None):
+                self.id = id or str(uuid.uuid4())
 
-        server.list()
+        manager_cls = mock.Mock()
+        manager = manager_cls.return_value
 
-        server._manager.return_value.list.assert_called_once_with(limit=-1)
+        server1 = Server(id=1)
+        server2 = Server(id=2)
+        server3 = Server(id=3)
+        server4 = Server(id=4)
+
+        manager.list.side_effect = (
+            [server1, server2, server3],
+            # simulate marker error
+            nova_exc.BadRequest(code=400, message="marker [3] not found"),
+            nova_exc.BadRequest(code=400, message="marker [2] not found"),
+            # valid response
+            [server4],
+            # fetching is completed
+            []
+        )
+
+        servers_res = resources.NovaServer()
+        servers_res._manager = manager_cls
+
+        self.assertEqual(
+            [server1, server2, server3, server4],
+            servers_res.list()
+        )
+
+        self.assertEqual(
+            [mock.call(marker=m) for m in (None, 3, 2, 1, 4)],
+            manager.list.call_args_list
+        )
 
     def test_delete(self):
         server = resources.NovaServer()
@@ -460,7 +490,8 @@ class NeutronPortTestCase(test.TestCase):
             {"tenant_id": tenant_uuid, "id": "id7",
              "device_owner": "network:dhcp",
              "device_id": "asdasdasd"},
-            # the case when port is from another tenant
+            # the case when port is from another tenant. it should not be
+            #   here as we are filtering by tenant id, but anyway.
             {"tenant_id": "wrong tenant", "id": "id8", "name": "foo"},
             # WTF port without any parent and name
             {"tenant_id": tenant_uuid, "id": "id9", "device_owner": ""},
@@ -496,8 +527,8 @@ class NeutronPortTestCase(test.TestCase):
         user = mock.Mock(neutron=neutron)
         self.assertEqual(expected_ports, resources.NeutronPort(
             user=user, tenant_uuid=tenant_uuid).list())
-        neutron.list_ports.assert_called_once_with()
-        neutron.list_routers.assert_called_once_with()
+        neutron.list_ports.assert_called_once_with(tenant_id=tenant_uuid)
+        neutron.list_routers.assert_called_once_with(tenant_id=tenant_uuid)
 
 
 @ddt.ddt