Merge "compute: Use source_bdms to reset attachment_ids during LM rollback"

Author: Zuul

nova/compute/manager.py

@@ -6541,7 +6541,7 @@ def pre_live_migration(self, context, instance, block_migration, disk,
                     # 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
@@ -7342,24 +7342,17 @@ def _rollback_live_migration(self, context, instance,
                 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()
 

nova/objects/migrate_data.py

@@ -124,7 +124,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

nova/tests/unit/compute/test_compute.py

@@ -6211,6 +6211,7 @@ def stupid(*args, **kwargs):
         # 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,
@@ -6223,7 +6224,8 @@ def stupid(*args, **kwargs):
     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()
 
@@ -6241,27 +6243,34 @@ def test_live_migration_exception_rolls_back(self, mock_save,
         # 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()),
                          'destination_type': 'volume'}))
             ])
             for bdm in bdms:
                 bdm.save = mock.Mock()
+                fake_attachment_ids.append(bdm.attachment_id)
             fake_bdms.append(bdms)
             return bdms
 
@@ -6312,10 +6321,13 @@ def do_it(mock_client, mock_setup):
         # 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()
@@ -6328,6 +6340,11 @@ def do_it(mock_client, mock_setup):
             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,