openstack
/
nova
Code Issues Proposed changes

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
This commit is contained in:
EdLeafe 2017-07-26 23:07:38 +00:00 committed by Matt Riedemann
parent 4579d2e557
commit c3118b91db
2 changed files with 269 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

264
nova/tests/unit/virt/ironic/test_driver.py
View File

@ -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')

50
nova/virt/ironic/driver.py
View File

@ -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.