diff --git a/nova/tests/unit/virt/ironic/test_driver.py b/nova/tests/unit/virt/ironic/test_driver.py index 52aa37ac134d..ea40d73f5999 100644 --- a/nova/tests/unit/virt/ironic/test_driver.py +++ b/nova/tests/unit/virt/ironic/test_driver.py @@ -582,71 +582,58 @@ class IronicDriverTestCase(test.NoDBTestCase): @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() - uuids = self.driver.list_instance_uuids() + self.assertEqual(sorted(expected), sorted(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)) + @mock.patch.object(ironic_driver.IronicDriver, '_refresh_cache') + def test_list_instance_uuids_fail(self, mock_cache): + mock_cache.side_effect = exception.VirtDriverNotReady - # 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') diff --git a/nova/virt/ironic/driver.py b/nova/virt/ironic/driver.py index 77fefb81ea48..43401097497a 100644 --- a/nova/virt/ironic/driver.py +++ b/nova/virt/ironic/driver.py @@ -647,13 +647,21 @@ class IronicDriver(virt_driver.ComputeDriver): :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 @@ class IronicDriver(virt_driver.ComputeDriver): :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. diff --git a/releasenotes/notes/ironic-list-instance-respect-partition-key-339ff653eaa00753.yaml b/releasenotes/notes/ironic-list-instance-respect-partition-key-339ff653eaa00753.yaml new file mode 100644 index 000000000000..96f2a12b8ef9 --- /dev/null +++ b/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.