From c3118b91db4b97a689c9a2f25a9554a0ccb29252 Mon Sep 17 00:00:00 2001 From: EdLeafe Date: Wed, 26 Jul 2017 23:07:38 +0000 Subject: [PATCH] Handle addition of new nodes/instances in ironic flavor migration The original idea for ironic flavor migration was to handle it in init_host(), so that the code wouldn't run often. But in a situation like an upgrade with multiple Nova computes handling ironic nodes, this may not cover all the nodes, so we are moving it to be run whenever the cache is refreshed, and new nodes for the compute service that have instances are detected. We also need to cover the cases where an existing node has a new instance created on it to ensure that that new instance's flavor is also migrated. Closes-Bug: #1707021 Change-Id: If0a7af3a958cf3aba95b89b96df5ff6bf252b110 --- nova/tests/unit/virt/ironic/test_driver.py | 264 ++++++++++++++++++--- nova/virt/ironic/driver.py | 50 ++-- 2 files changed, 269 insertions(+), 45 deletions(-) diff --git a/nova/tests/unit/virt/ironic/test_driver.py b/nova/tests/unit/virt/ironic/test_driver.py index 3330ddabf598..701c85828422 100644 --- a/nova/tests/unit/virt/ironic/test_driver.py +++ b/nova/tests/unit/virt/ironic/test_driver.py @@ -2093,15 +2093,18 @@ class IronicDriverSyncTestCase(IronicDriverTestCase): def setUp(self): super(IronicDriverSyncTestCase, self).setUp() - # Since the init_host code we're testing runs in a spawn_n green - # thread, ensure that the thread completes. + self.driver.node_cache = {} + # Since the code we're testing runs in a spawn_n green thread, ensure + # that the thread completes. self.useFixture(fixtures.SpawnIsSynchronousFixture()) - @mock.patch.object(ironic_driver.IronicDriver, '_refresh_cache') + @mock.patch.object(ironic_driver.IronicDriver, '_get_node_list') + @mock.patch.object(objects.ServiceList, 'get_all_computes_by_hv_type') + @mock.patch.object(objects.InstanceList, 'get_uuids_by_host') @mock.patch.object(objects.Instance, 'get_by_uuid') @mock.patch.object(objects.Instance, 'save') def test_pike_flavor_migration(self, mock_save, mock_get_by_uuid, - mock_refresh_cache): + mock_get_uuids_by_host, mock_svc_by_hv, mock_get_node_list): node1_uuid = uuidutils.generate_uuid() node2_uuid = uuidutils.generate_uuid() hostname = "ironic-compute" @@ -2128,32 +2131,34 @@ class IronicDriverSyncTestCase(IronicDriverTestCase): resource_class="second", network_interface="flat") inst_dict = {inst1.uuid: inst1, inst2.uuid: inst2} + mock_get_uuids_by_host.return_value = [inst1.uuid, inst2.uuid] + mock_svc_by_hv.return_value = [] + self.driver.node_cache = {} + mock_get_node_list.return_value = [node1, node2] def fake_inst_by_uuid(ctx, uuid, expected_attrs=None): return inst_dict.get(uuid) mock_get_by_uuid.side_effect = fake_inst_by_uuid - def fake_refresh(): - self.driver.node_cache = {node1_uuid: node1, node2_uuid: node2} - - mock_refresh_cache.side_effect = fake_refresh - self.assertEqual({}, inst1.flavor.extra_specs) self.assertEqual({}, inst2.flavor.extra_specs) - self.driver.init_host(hostname) + self.driver._refresh_cache() self.assertEqual(2, mock_save.call_count) expected_specs = {"resources:CUSTOM_FIRST": "1"} self.assertEqual(expected_specs, inst1.flavor.extra_specs) expected_specs = {"resources:CUSTOM_SECOND": "1"} self.assertEqual(expected_specs, inst2.flavor.extra_specs) - @mock.patch.object(ironic_driver.IronicDriver, '_refresh_cache') + @mock.patch.object(ironic_driver.IronicDriver, '_get_node_list') + @mock.patch.object(objects.ServiceList, 'get_all_computes_by_hv_type') + @mock.patch.object(objects.InstanceList, 'get_uuids_by_host') @mock.patch.object(objects.Instance, 'get_by_uuid') @mock.patch.object(objects.Instance, 'save') - def test_pike_flavor_migration_dupe(self, mock_save, mock_get_by_uuid, - mock_refresh_cache): + def test_pike_flavor_migration_instance_migrated(self, mock_save, + mock_get_by_uuid, mock_get_uuids_by_host, mock_svc_by_hv, + mock_get_node_list): node1_uuid = uuidutils.generate_uuid() node2_uuid = uuidutils.generate_uuid() hostname = "ironic-compute" @@ -2180,18 +2185,17 @@ class IronicDriverSyncTestCase(IronicDriverTestCase): resource_class="second", network_interface="flat") inst_dict = {inst1.uuid: inst1, inst2.uuid: inst2} + mock_get_uuids_by_host.return_value = [inst1.uuid, inst2.uuid] + self.driver.node_cache = {} + mock_get_node_list.return_value = [node1, node2] + mock_svc_by_hv.return_value = [] def fake_inst_by_uuid(ctx, uuid, expected_attrs=None): return inst_dict.get(uuid) mock_get_by_uuid.side_effect = fake_inst_by_uuid - def fake_refresh(): - self.driver.node_cache = {node1_uuid: node1, node2_uuid: node2} - - mock_refresh_cache.side_effect = fake_refresh - - self.driver.init_host(hostname) + self.driver._refresh_cache() # Since one instance already had its extra_specs updated with the # custom resource_class, only the other one should be updated and # saved. @@ -2202,11 +2206,14 @@ class IronicDriverSyncTestCase(IronicDriverTestCase): self.assertEqual(expected_specs, inst2.flavor.extra_specs) @mock.patch.object(ironic_driver.LOG, 'warning') - @mock.patch.object(ironic_driver.IronicDriver, '_refresh_cache') + @mock.patch.object(ironic_driver.IronicDriver, '_get_node_list') + @mock.patch.object(objects.ServiceList, 'get_all_computes_by_hv_type') + @mock.patch.object(objects.InstanceList, 'get_uuids_by_host') @mock.patch.object(objects.Instance, 'get_by_uuid') @mock.patch.object(objects.Instance, 'save') def test_pike_flavor_migration_missing_rc(self, mock_save, - mock_get_by_uuid, mock_refresh_cache, mock_warning): + mock_get_by_uuid, mock_get_uuids_by_host, mock_svc_by_hv, + mock_get_node_list, mock_warning): node1_uuid = uuidutils.generate_uuid() node2_uuid = uuidutils.generate_uuid() hostname = "ironic-compute" @@ -2233,18 +2240,17 @@ class IronicDriverSyncTestCase(IronicDriverTestCase): resource_class="second", network_interface="flat") inst_dict = {inst1.uuid: inst1, inst2.uuid: inst2} + mock_get_uuids_by_host.return_value = [inst1.uuid, inst2.uuid] + mock_svc_by_hv.return_value = [] + self.driver.node_cache = {} + mock_get_node_list.return_value = [node1, node2] def fake_inst_by_uuid(ctx, uuid, expected_attrs=None): return inst_dict.get(uuid) mock_get_by_uuid.side_effect = fake_inst_by_uuid - def fake_refresh(): - self.driver.node_cache = {node1_uuid: node1, node2_uuid: node2} - - mock_refresh_cache.side_effect = fake_refresh - - self.driver.init_host(hostname) + self.driver._refresh_cache() # Since one instance was on a node with no resource class set, # only the other one should be updated and saved. self.assertEqual(1, mock_save.call_count) @@ -2252,11 +2258,215 @@ class IronicDriverSyncTestCase(IronicDriverTestCase): self.assertEqual(expected_specs, inst1.flavor.extra_specs) expected_specs = {"resources:CUSTOM_SECOND": "1"} self.assertEqual(expected_specs, inst2.flavor.extra_specs) + # Verify that the LOG.warning was called correctly self.assertEqual(1, mock_warning.call_count) self.assertIn("does not have its resource_class set.", mock_warning.call_args[0][0]) self.assertEqual({"node": node1.uuid}, mock_warning.call_args[0][1]) + @mock.patch.object(ironic_driver.IronicDriver, '_get_node_list') + @mock.patch.object(objects.ServiceList, 'get_all_computes_by_hv_type') + @mock.patch.object(objects.InstanceList, 'get_uuids_by_host') + @mock.patch.object(objects.Instance, 'get_by_uuid') + @mock.patch.object(objects.Instance, 'save') + def test_pike_flavor_migration_refresh_called_again(self, mock_save, + mock_get_by_uuid, mock_get_uuids_by_host, mock_svc_by_hv, + mock_get_node_list): + node1_uuid = uuidutils.generate_uuid() + node2_uuid = uuidutils.generate_uuid() + hostname = "ironic-compute" + fake_flavor1 = objects.Flavor() + fake_flavor1.extra_specs = {} + fake_flavor2 = objects.Flavor() + fake_flavor2.extra_specs = {} + inst1 = fake_instance.fake_instance_obj(self.ctx, + node=node1_uuid, + host=hostname, + flavor=fake_flavor1) + inst2 = fake_instance.fake_instance_obj(self.ctx, + node=node2_uuid, + host=hostname, + flavor=fake_flavor2) + node1 = ironic_utils.get_test_node(uuid=node1_uuid, + instance_uuid=inst1.uuid, + instance_type_id=1, + resource_class="first", + network_interface="flat") + node2 = ironic_utils.get_test_node(uuid=node2_uuid, + instance_uuid=inst2.uuid, + instance_type_id=2, + resource_class="second", + network_interface="flat") + inst_dict = {inst1.uuid: inst1, inst2.uuid: inst2} + mock_get_uuids_by_host.return_value = [inst1.uuid, inst2.uuid] + mock_svc_by_hv.return_value = [] + self.driver.node_cache = {} + mock_get_node_list.return_value = [node1, node2] + + def fake_inst_by_uuid(ctx, uuid, expected_attrs=None): + return inst_dict.get(uuid) + + mock_get_by_uuid.side_effect = fake_inst_by_uuid + + self.driver._refresh_cache() + self.assertEqual(2, mock_get_by_uuid.call_count) + # Refresh the cache again. The mock for getting an instance by uuid + # should not be called again. + mock_get_by_uuid.reset_mock() + self.driver._refresh_cache() + mock_get_by_uuid.assert_not_called() + + @mock.patch.object(ironic_driver.IronicDriver, '_get_node_list') + @mock.patch.object(objects.ServiceList, 'get_all_computes_by_hv_type') + @mock.patch.object(objects.InstanceList, 'get_uuids_by_host') + @mock.patch.object(objects.Instance, 'get_by_uuid') + @mock.patch.object(objects.Instance, 'save') + def test_pike_flavor_migration_no_node_change(self, mock_save, + mock_get_by_uuid, mock_get_uuids_by_host, mock_svc_by_hv, + mock_get_node_list): + node1_uuid = uuidutils.generate_uuid() + node2_uuid = uuidutils.generate_uuid() + hostname = "ironic-compute" + fake_flavor1 = objects.Flavor() + fake_flavor1.extra_specs = {"resources:CUSTOM_FIRST": "1"} + fake_flavor2 = objects.Flavor() + fake_flavor2.extra_specs = {"resources:CUSTOM_SECOND": "1"} + inst1 = fake_instance.fake_instance_obj(self.ctx, + node=node1_uuid, + host=hostname, + flavor=fake_flavor1) + inst2 = fake_instance.fake_instance_obj(self.ctx, + node=node2_uuid, + host=hostname, + flavor=fake_flavor2) + node1 = ironic_utils.get_test_node(uuid=node1_uuid, + instance_uuid=inst1.uuid, + instance_type_id=1, + resource_class="first", + network_interface="flat") + node2 = ironic_utils.get_test_node(uuid=node2_uuid, + instance_uuid=inst2.uuid, + instance_type_id=2, + resource_class="second", + network_interface="flat") + inst_dict = {inst1.uuid: inst1, inst2.uuid: inst2} + mock_get_uuids_by_host.return_value = [inst1.uuid, inst2.uuid] + self.driver.node_cache = {node1_uuid: node1, node2_uuid: node2} + self.driver._migrated_instance_uuids = set([inst1.uuid, inst2.uuid]) + mock_get_node_list.return_value = [node1, node2] + mock_svc_by_hv.return_value = [] + + def fake_inst_by_uuid(ctx, uuid, expected_attrs=None): + return inst_dict.get(uuid) + + mock_get_by_uuid.side_effect = fake_inst_by_uuid + + self.driver._refresh_cache() + # Since the nodes did not change in the call to _refresh_cache(), and + # their instance_uuids were in the cache, none of the mocks in the + # migration script should have been called. + self.assertFalse(mock_get_by_uuid.called) + self.assertFalse(mock_save.called) + + @mock.patch.object(ironic_driver.IronicDriver, '_get_node_list') + @mock.patch.object(objects.ServiceList, 'get_all_computes_by_hv_type') + @mock.patch.object(objects.InstanceList, 'get_uuids_by_host') + @mock.patch.object(objects.Instance, 'get_by_uuid') + @mock.patch.object(objects.Instance, 'save') + def test_pike_flavor_migration_just_instance_change(self, mock_save, + mock_get_by_uuid, mock_get_uuids_by_host, mock_svc_by_hv, + mock_get_node_list): + node1_uuid = uuidutils.generate_uuid() + node2_uuid = uuidutils.generate_uuid() + node3_uuid = uuidutils.generate_uuid() + hostname = "ironic-compute" + fake_flavor1 = objects.Flavor() + fake_flavor1.extra_specs = {} + fake_flavor2 = objects.Flavor() + fake_flavor2.extra_specs = {} + fake_flavor3 = objects.Flavor() + fake_flavor3.extra_specs = {} + inst1 = fake_instance.fake_instance_obj(self.ctx, + node=node1_uuid, + host=hostname, + flavor=fake_flavor1) + inst2 = fake_instance.fake_instance_obj(self.ctx, + node=node2_uuid, + host=hostname, + flavor=fake_flavor2) + inst3 = fake_instance.fake_instance_obj(self.ctx, + node=node3_uuid, + host=hostname, + flavor=fake_flavor3) + node1 = ironic_utils.get_test_node(uuid=node1_uuid, + instance_uuid=inst1.uuid, + instance_type_id=1, + resource_class="first", + network_interface="flat") + node2 = ironic_utils.get_test_node(uuid=node2_uuid, + instance_uuid=inst2.uuid, + instance_type_id=2, + resource_class="second", + network_interface="flat") + inst_dict = {inst1.uuid: inst1, inst2.uuid: inst2, inst3.uuid: inst3} + mock_get_uuids_by_host.return_value = [inst1.uuid, inst2.uuid] + self.driver.node_cache = {node1_uuid: node1, node2_uuid: node2} + mock_get_node_list.return_value = [node1, node2] + mock_svc_by_hv.return_value = [] + + def fake_inst_by_uuid(ctx, uuid, expected_attrs=None): + return inst_dict.get(uuid) + + mock_get_by_uuid.side_effect = fake_inst_by_uuid + + self.driver._refresh_cache() + # Since this is a fresh driver, neither will be in the migration cache, + # so the migration mocks should have been called. + self.assertTrue(mock_get_by_uuid.called) + self.assertTrue(mock_save.called) + + # Now call _refresh_cache() again. Since neither the nodes nor their + # instances change, none of the mocks in the migration script should + # have been called. + mock_get_by_uuid.reset_mock() + mock_save.reset_mock() + self.driver._refresh_cache() + self.assertFalse(mock_get_by_uuid.called) + self.assertFalse(mock_save.called) + + # Now change the node on node2 to inst3 + node2.instance_uuid = inst3.uuid + mock_get_uuids_by_host.return_value = [inst1.uuid, inst3.uuid] + # Call _refresh_cache() again. Since the instance on node2 changed, the + # migration mocks should have been called. + mock_get_by_uuid.reset_mock() + mock_save.reset_mock() + self.driver._refresh_cache() + self.assertTrue(mock_get_by_uuid.called) + self.assertTrue(mock_save.called) + + @mock.patch.object(fields.ResourceClass, 'normalize_name') + @mock.patch.object(ironic_driver.IronicDriver, '_node_from_cache') + def test_pike_flavor_migration_empty_node(self, mock_node_from_cache, + mock_normalize): + mock_node_from_cache.return_value = None + self.driver._pike_flavor_migration([uuids.node]) + mock_normalize.assert_not_called() + + @mock.patch.object(fields.ResourceClass, 'normalize_name') + @mock.patch.object(ironic_driver.IronicDriver, '_node_from_cache') + def test_pike_flavor_migration_already_migrated(self, mock_node_from_cache, + mock_normalize): + node1 = ironic_utils.get_test_node(uuid=uuids.node1, + instance_uuid=uuids.instance, + instance_type_id=1, + resource_class="first", + network_interface="flat") + mock_node_from_cache.return_value = node1 + self.driver._migrated_instance_uuids = set([uuids.instance]) + self.driver._pike_flavor_migration([uuids.node1]) + mock_normalize.assert_not_called() + @mock.patch.object(instance_metadata, 'InstanceMetadata') @mock.patch.object(configdrive, 'ConfigDriveBuilder') diff --git a/nova/virt/ironic/driver.py b/nova/virt/ironic/driver.py index 940652e59426..c09e216e8ee7 100644 --- a/nova/virt/ironic/driver.py +++ b/nova/virt/ironic/driver.py @@ -156,6 +156,11 @@ class IronicDriver(virt_driver.ComputeDriver): self.ironicclient = client_wrapper.IronicClientWrapper() + # This is needed for the instance flavor migration in Pike, and should + # be removed in Queens. Since this will run several times in the life + # of the driver, track the instances that have already been migrated. + self._migrated_instance_uuids = set() + def _get_node(self, node_uuid): """Get a node by its UUID.""" return self.ironicclient.call('node.get', node_uuid, @@ -511,14 +516,9 @@ class IronicDriver(virt_driver.ComputeDriver): :param host: the hostname of the compute host. """ - if not self.node_cache: - self._refresh_cache() - # Since there could be many, many instances controlled by this host, - # spawn this asynchronously so as not to stall the startup of the - # compute service. - utils.spawn_n(self._pike_flavor_migration, host) + return - def _pike_flavor_migration(self, host): + def _pike_flavor_migration(self, node_uuids): """This code is needed in Pike to prevent problems where an operator has already adjusted their flavors to add the custom resource class to extra_specs. Since existing ironic instances will not have this in @@ -533,28 +533,32 @@ class IronicDriver(virt_driver.ComputeDriver): """ ctx = nova_context.get_admin_context() - # Using itervalues here as the number of nodes can be very high - for node in six.itervalues(self.node_cache): - if not node.instance_uuid: + for node_uuid in node_uuids: + node = self._node_from_cache(node_uuid) + if not node: continue - - if not node.resource_class: + node_rc = node.resource_class + if not node_rc: LOG.warning("Node %(node)s does not have its resource_class " "set.", {"node": node.uuid}) continue - rc = obj_fields.ResourceClass.normalize_name(node.resource_class) + if node.instance_uuid in self._migrated_instance_uuids: + continue + normalized_rc = obj_fields.ResourceClass.normalize_name(node_rc) instance = objects.Instance.get_by_uuid(ctx, node.instance_uuid, expected_attrs=["flavor"]) specs = instance.flavor.extra_specs - res_key = "resources:%s" % rc - if res_key in specs: - # Flavor has already been updated + resource_key = "resources:%s" % normalized_rc + self._migrated_instance_uuids.add(node.instance_uuid) + if resource_key in specs: + # The compute must have been restarted, and the instance.flavor + # has already been migrated continue - specs[res_key] = 1 + specs[resource_key] = "1" instance.save() LOG.debug("The flavor extra_specs for Ironic instance %(inst)s " "have been updated for custom resource class '%(rc)s'.", - {"inst": instance.uuid, "rc": rc}) + {"inst": instance.uuid, "rc": node_rc}) return def _get_hypervisor_type(self): @@ -694,6 +698,16 @@ class IronicDriver(virt_driver.ComputeDriver): self.node_cache = node_cache self.node_cache_time = time.time() + # For Pike, we need to ensure that all instances have their flavor + # migrated to include the resource_class. Since there could be many, + # many instances controlled by this host, spawn this asynchronously so + # as not to block this service. + node_uuids = [node.uuid for node in self.node_cache.values() + if node.instance_uuid and + node.instance_uuid not in self._migrated_instance_uuids] + if node_uuids: + # No need to run unless something has changed + utils.spawn_n(self._pike_flavor_migration, node_uuids) def get_available_nodes(self, refresh=False): """Returns the UUIDs of Ironic nodes managed by this compute service.