[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
This commit is contained in:
Jay Faulkner
parent
5e914c27a0
commit
fa3cf7d50c
@ -572,71 +572,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')
|
||||
|
||||
@ -632,15 +632,21 @@ class IronicDriver(virt_driver.ComputeDriver):
|
||||
:raises: VirtDriverNotReady
|
||||
|
||||
"""
|
||||
# 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_id',),
|
||||
)
|
||||
]
|
||||
|
||||
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.
|
||||
@ -649,13 +655,15 @@ class IronicDriver(virt_driver.ComputeDriver):
|
||||
:raises: VirtDriverNotReady
|
||||
|
||||
"""
|
||||
return [
|
||||
node.instance_id for node in self._get_node_list(
|
||||
return_generator=True,
|
||||
associated=True,
|
||||
fields=('instance_id',),
|
||||
)
|
||||
]
|
||||
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.
|
||||
|
||||
@ -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.
|
||||
Loading…
Reference in New Issue
Block a user