[ironic] Partition & use cache for list_instance*

list_instances and list_instance_uuids, as written in the Ironic driver,
do not currently respect conductor_group paritioning. Given a nova
compute is intended to limit it's scope of work to the conductor group
it is configured to work with; this is a bug.

Additionally, this should be a significant performance boost for a
couple of reasons; firstly, instead of calling the Ironic API and
getting all nodes, instead of the subset (when using conductor group),
we're now properly getting the subset of nodes -- this is the optimized
path in the Ironic DB and API code. Secondly, we're now using the
driver's node cache to respond to these requests. Since list_instances
and list_instance_uuids is used by periodic tasks, these operating with
data that may be slightly stale should have minimal impact compared to
the performance benefits.

Closes-bug: #2043036
Change-Id: If31158e3269e5e06848c29294fdaa147beedb5a5
(cherry picked from commit fa3cf7d)
(cherry picked from commit 555d7d0)

Author: jayofdoom

nova/tests/unit/virt/ironic/test_driver.py

@@ -582,71 +582,58 @@ def test__get_node_list_fail(self):
 
     @mock.patch.object(objects.Instance, 'get_by_uuid')
     def test_list_instances(self, mock_inst_by_uuid):
-        nodes = []
+        nodes = {}
         instances = []
         for i in range(2):
             uuid = uuidutils.generate_uuid()
+            node_uuid = uuidutils.generate_uuid()
             instances.append(fake_instance.fake_instance_obj(self.ctx,
                                                              id=i,
                                                              uuid=uuid))
-            nodes.append(ironic_utils.get_test_node(instance_id=uuid,
-                                                    fields=['instance_id']))
+            nodes[node_uuid] = ironic_utils.get_test_node(
+                id=node_uuid, instance_id=uuid, fields=('instance_id',))
         mock_inst_by_uuid.side_effect = instances
-        self.mock_conn.nodes.return_value = iter(nodes)
+        self.driver.node_cache = nodes
 
         response = self.driver.list_instances()
 
-        self.mock_conn.nodes.assert_called_with(associated=True,
-                                                fields=['instance_uuid'])
         expected_calls = [mock.call(mock.ANY, instances[0].uuid),
                           mock.call(mock.ANY, instances[1].uuid)]
         mock_inst_by_uuid.assert_has_calls(expected_calls)
         self.assertEqual(['instance-00000000', 'instance-00000001'],
                          sorted(response))
 
-    # NOTE(dustinc) This test ensures we use instance_uuid not instance_id in
-    # 'fields' when calling ironic.
+    @mock.patch.object(ironic_driver.IronicDriver, '_refresh_cache')
     @mock.patch.object(objects.Instance, 'get_by_uuid')
-    def test_list_instances_uses_instance_uuid(self, mock_inst_by_uuid):
-        self.driver.list_instances()
-
-        self.mock_conn.nodes.assert_called_with(associated=True,
-                                                fields=['instance_uuid'])
-
-    @mock.patch.object(objects.Instance, 'get_by_uuid')
-    def test_list_instances_fail(self, mock_inst_by_uuid):
-        self.mock_conn.nodes.side_effect = exception.NovaException
+    def test_list_instances_fail(self, mock_inst_by_uuid, mock_cache):
+        mock_cache.side_effect = exception.VirtDriverNotReady
 
         self.assertRaises(exception.VirtDriverNotReady,
                           self.driver.list_instances)
-        self.mock_conn.nodes.assert_called_with(associated=True,
-                                                fields=['instance_uuid'])
         self.assertFalse(mock_inst_by_uuid.called)
 
     def test_list_instance_uuids(self):
         num_nodes = 2
-        nodes = []
+        nodes = {}
         for n in range(num_nodes):
-            nodes.append(ironic_utils.get_test_node(
-                                      instance_id=uuidutils.generate_uuid(),
-                                      fields=['instance_id']))
-        self.mock_conn.nodes.return_value = iter(nodes)
+            node_uuid = uuidutils.generate_uuid()
+            instance_uuid = uuidutils.generate_uuid()
+            nodes[instance_uuid] = ironic_utils.get_test_node(
+                                    id=node_uuid,
+                                    instance_id=instance_uuid,
+                                    fields=('instance_id',))
+        self.driver.node_cache = nodes
+        instance_uuids = self.driver.list_instance_uuids()
+        expected = nodes.keys()
+
+        self.assertEqual(sorted(expected), sorted(instance_uuids))
+
+    @mock.patch.object(ironic_driver.IronicDriver, '_refresh_cache')
+    def test_list_instance_uuids_fail(self, mock_cache):
+        mock_cache.side_effect = exception.VirtDriverNotReady
 
-        uuids = self.driver.list_instance_uuids()
-
-        self.mock_conn.nodes.assert_called_with(associated=True,
-                                                fields=['instance_uuid'])
-        expected = [n.instance_id for n in nodes]
-        self.assertEqual(sorted(expected), sorted(uuids))
-
-    # NOTE(dustinc) This test ensures we use instance_uuid not instance_id in
-    # 'fields' when calling ironic.
-    @mock.patch.object(objects.Instance, 'get_by_uuid')
-    def test_list_instance_uuids_uses_instance_uuid(self, mock_inst_by_uuid):
-        self.driver.list_instance_uuids()
-
-        self.mock_conn.nodes.assert_called_with(associated=True,
-                                                fields=['instance_uuid'])
+        self.assertRaises(exception.VirtDriverNotReady,
+                          self.driver.list_instance_uuids)
 
     @mock.patch.object(objects.InstanceList, 'get_uuids_by_host')
     @mock.patch.object(objects.ServiceList, 'get_all_computes_by_hv_type')

nova/virt/ironic/driver.py

@@ -647,13 +647,21 @@ def list_instances(self):
         :raises: VirtDriverNotReady
 
         """
-        # NOTE(dustinc): The SDK returns an object with instance_id,
-        #  but the Ironic API expects instance_uuid in query.
+        # NOTE(JayF): As of this writing, November 2023, this is only called
+        #             one place; in compute/manager.py, and only if
+        #             list_instance_uuids is not implemented. This means that
+        #             this is effectively dead code in the Ironic driver.
+        if not self.node_cache:
+            # Empty cache, try to populate it. If we cannot populate it, this
+            # is OK. This information is only used to cleanup deleted nodes;
+            # if Ironic has no deleted nodes; we're good.
+            self._refresh_cache()
+
         context = nova_context.get_admin_context()
-        return [objects.Instance.get_by_uuid(context, i.instance_id).name
-                for i in self._get_node_list(return_generator=True,
-                                             associated=True,
-                                             fields=['instance_uuid'])]
+
+        return [objects.Instance.get_by_uuid(context, node.instance_id).name
+                for node in self.node_cache.values()
+                if node.instance_id is not None]
 
     def list_instance_uuids(self):
         """Return the IDs of all the instances provisioned.
@@ -662,10 +670,15 @@ def list_instance_uuids(self):
         :raises: VirtDriverNotReady
 
         """
-        # NOTE(dustinc): The SDK returns an object with instance_id,
-        #  but the Ironic API expects instance_uuid in query.
-        return [node.instance_id for node in self._get_node_list(
-            return_generator=True, associated=True, fields=['instance_uuid'])]
+        if not self.node_cache:
+            # Empty cache, try to populate it. If we cannot populate it, this
+            # is OK. This information is only used to cleanup deleted nodes;
+            # if Ironic has no deleted nodes; we're good.
+            self._refresh_cache()
+
+        return [node.instance_id
+                for node in self.node_cache.values()
+                if node.instance_id is not None]
 
     def node_is_available(self, nodename):
         """Confirms a Nova hypervisor node exists in the Ironic inventory.

releasenotes/notes/ironic-list-instance-respect-partition-key-339ff653eaa00753.yaml

@@ -0,0 +1,5 @@
+fixes:
+  - Ironic virt driver now uses the node cache and respects partition keys,
+    such as conductor group, for list_instances and list_instance_uuids calls.
+    This fix will improve performance of the periodic queries which use these
+    driver methods and reduce API and DB load on the backing Ironic service.