From b8a1f08bc828b25871d5db2071e103330e248f6c Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Mon, 15 Apr 2019 23:31:02 +0100 Subject: [PATCH] compute: Use source_bdms to reset attachment_ids during LM rollback As we now provide the original source bdms during rollback we can also use them to reset any volume attachment ids to the ids used by the source compute host. Change-Id: Ibe9215c07a1ee00e0e121c69bcf7ee1b1b80fae0 (cherry picked from commit 0eea5041c4047bf3ab8bf71973926d80592e5e5c) (cherry picked from commit 8b865378b8d238abaac620d31b98f0ff2c355941) --- nova/compute/manager.py | 25 +++++++------------- nova/objects/migrate_data.py | 2 +- nova/tests/unit/compute/test_compute.py | 31 +++++++++++++++++++------ 3 files changed, 34 insertions(+), 24 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index e7243d4a1211..342a46130323 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -6492,7 +6492,7 @@ class ComputeManager(manager.Manager): # save current attachment so we can detach it on success, # or restore it on a rollback. # NOTE(mdbooth): This data is no longer used by the source - # host since change I0390c9ff. We can't remove it until we + # host since change Ibe9215c0. We can't remove it until we # are sure the source host has been upgraded. migrate_data.old_vol_attachment_ids[bdm.volume_id] = \ bdm.attachment_id @@ -7284,24 +7284,17 @@ class ComputeManager(manager.Manager): self.compute_rpcapi.remove_volume_connection( context, instance, bdm.volume_id, dest) - if bdm.attachment_id: - # 3.44 cinder api flow. Set the bdm's - # attachment_id to the old attachment of the source - # host. If old_attachments is not there, then - # there was an error before the new attachment was made. - # TODO(lyarwood): migrate_data.old_vol_attachment_ids can - # be removed now as we can lookup the original - # attachment_ids from the source_bdms list here. - old_attachments = migrate_data.old_vol_attachment_ids \ - if 'old_vol_attachment_ids' in migrate_data else None - if old_attachments and bdm.volume_id in old_attachments: - self.volume_api.attachment_delete(context, - bdm.attachment_id) - bdm.attachment_id = old_attachments[bdm.volume_id] + source_bdm = source_bdms_by_volid[bdm.volume_id] + if bdm.attachment_id and source_bdm.attachment_id: + # NOTE(lyarwood): 3.44 cinder api flow. Delete the + # attachment used by the destination and reset the bdm + # attachment_id to the old attachment of the source host. + self.volume_api.attachment_delete(context, + bdm.attachment_id) + bdm.attachment_id = source_bdm.attachment_id # NOTE(lyarwood): Rollback the connection_info stored within # the BDM to that used by the source and not the destination. - source_bdm = source_bdms_by_volid[bdm.volume_id] bdm.connection_info = source_bdm.connection_info bdm.save() diff --git a/nova/objects/migrate_data.py b/nova/objects/migrate_data.py index 71901af9e9a4..12b23c36f150 100644 --- a/nova/objects/migrate_data.py +++ b/nova/objects/migrate_data.py @@ -109,7 +109,7 @@ class LiveMigrateData(obj_base.NovaObject): # old_vol_attachment_ids is a dict used to store the old attachment_ids # for each volume so they can be restored on a migration rollback. The # key is the volume_id, and the value is the attachment_id. - # TODO(mdbooth): This field was made redundant by change I0390c9ff. We + # TODO(mdbooth): This field was made redundant by change Ibe9215c0. We # should eventually remove it. 'old_vol_attachment_ids': fields.DictOfStringsField(), # wait_for_vif_plugged is set in pre_live_migration on the destination diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 6039ae9c9699..f857c5826b3e 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -6277,6 +6277,7 @@ class ComputeTestCase(BaseTestCase, # cleanup db.instance_destroy(c, instance['uuid']) + @mock.patch.object(cinder.API, 'attachment_delete') @mock.patch.object(fake.FakeDriver, 'get_instance_disk_info') @mock.patch.object(compute_rpcapi.ComputeAPI, 'pre_live_migration') @mock.patch.object(objects.ComputeNode, @@ -6289,7 +6290,8 @@ class ComputeTestCase(BaseTestCase, def test_live_migration_exception_rolls_back(self, mock_save, mock_rollback, mock_remove, mock_get_bdms, - mock_get_node, mock_pre, mock_get_disk): + mock_get_node, mock_pre, mock_get_disk, + mock_attachment_delete): # Confirm exception when pre_live_migration fails. c = context.get_admin_context() @@ -6307,20 +6309,26 @@ class ComputeTestCase(BaseTestCase, # All the fake BDMs we've generated, in order fake_bdms = [] + # A list of the attachment_ids returned by gen_fake_bdms + fake_attachment_ids = [] + def gen_fake_bdms(obj, instance): - # generate a unique fake connection_info every time we're called, - # simulating connection_info being mutated elsewhere. + # generate a unique fake connection_info and attachment_id every + # time we're called, simulating attachment_id and connection_info + # being mutated elsewhere. bdms = objects.BlockDeviceMappingList(objects=[ objects.BlockDeviceMapping( - **fake_block_device.FakeDbBlockDeviceDict( + **fake_block_device.AnonFakeDbBlockDeviceDict( {'volume_id': uuids.volume_id_1, + 'attachment_id': uuidutils.generate_uuid(), 'source_type': 'volume', 'connection_info': jsonutils.dumps(uuidutils.generate_uuid()), 'destination_type': 'volume'})), objects.BlockDeviceMapping( - **fake_block_device.FakeDbBlockDeviceDict( + **fake_block_device.AnonFakeDbBlockDeviceDict( {'volume_id': uuids.volume_id_2, + 'attachment_id': uuidutils.generate_uuid(), 'source_type': 'volume', 'connection_info': jsonutils.dumps(uuidutils.generate_uuid()), @@ -6328,6 +6336,7 @@ class ComputeTestCase(BaseTestCase, ]) for bdm in bdms: bdm.save = mock.Mock() + fake_attachment_ids.append(bdm.attachment_id) fake_bdms.append(bdms) return bdms @@ -6379,10 +6388,13 @@ class ComputeTestCase(BaseTestCase, # BDMs with unique connection_info every time it's called. These are # stored in fake_bdms in the order they were generated. We assert here # that the last BDMs generated (in _rollback_live_migration) now have - # the same connection_info as the first BDMs generated (before calling - # pre_live_migration), and that we saved them. + # the same connection_info and attachment_id as the first BDMs + # generated (before calling pre_live_migration), and that we saved + # them. self.assertGreater(len(fake_bdms), 1) for source_bdm, final_bdm in zip(fake_bdms[0], fake_bdms[-1]): + self.assertEqual(source_bdm.attachment_id, + final_bdm.attachment_id) self.assertEqual(source_bdm.connection_info, final_bdm.connection_info) final_bdm.save.assert_called() @@ -6395,6 +6407,11 @@ class ComputeTestCase(BaseTestCase, destroy_disks=True, migrate_data=test.MatchType( migrate_data_obj.XenapiLiveMigrateData)) + # Assert that the final attachment_ids returned by + # BlockDeviceMappingList.get_by_instance_uuid are then deleted. + mock_attachment_delete.assert_has_calls([ + mock.call(c, fake_attachment_ids.pop()), + mock.call(c, fake_attachment_ids.pop())], any_order=True) @mock.patch.object(compute_rpcapi.ComputeAPI, 'pre_live_migration') @mock.patch.object(compute_rpcapi.ComputeAPI,