Refactor volume connection cleanup out of _post_live_migration

The _post_live_migration method is huge and hard to review/test/
maintain in isolation so we need to break down the complexity.
To that end this change refactors the source host volume connection
cleanup code into another method which also allows us to simplify
a couple of unit tests that target that specific set of code.
The error handling in that new method is tested by the functional
test class LiveMigrationCinderFailure.

Change-Id: Id0e8b1c32600d53382e5ac938e403258c80221a0
(cherry picked from commit e6916ab11469dde8fb4a8a2936f23b3f8647e24d)

3 files changed, 53 insertions, 90 deletions

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index d29b58534b..fc0bdcb04e 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -7548,37 +7548,16 @@ class ComputeManager(manager.Manager):
 
         return (do_cleanup, destroy_disks)
 
-    @wrap_exception()
-    @wrap_instance_fault
-    def _post_live_migration(self, ctxt, instance, dest,
-                             block_migration=False, migrate_data=None,
-                             source_bdms=None):
-        """Post operations for live migration.
-
-        This method is called from live_migration
-        and mainly updating database record.
-
-        :param ctxt: security context
-        :param instance: instance dict
-        :param dest: destination host
-        :param block_migration: if true, prepare for block migration
-        :param migrate_data: if not None, it is a dict which has data
-        :param source_bdms: BDMs prior to modification by the destination
-                            compute host. Set by _do_live_migration and not
-                            part of the callback interface, so this is never
-                            None
-        required for live migration without shared storage
+    def _post_live_migration_remove_source_vol_connections(
+            self, context, instance, source_bdms):
+        """Disconnect volume connections from the source host during
+        _post_live_migration.
 
+        :param context: nova auth RequestContext
+        :param instance: Instance object being live migrated
+        :param source_bdms: BlockDeviceMappingList representing the attached
+            volumes with connection_info set for the source host
         """
-        LOG.info('_post_live_migration() is started..',
-                 instance=instance)
-
-        # Cleanup source host post live-migration
-        block_device_info = self._get_instance_block_device_info(
-                            ctxt, instance, bdms=source_bdms)
-        self.driver.post_live_migration(ctxt, instance, block_device_info,
-                                        migrate_data)
-
         # Detaching volumes.
         connector = self.driver.get_volume_connector(instance)
         for bdm in source_bdms:
@@ -7600,27 +7579,62 @@ class ComputeManager(manager.Manager):
                         # remove the volume connection without detaching from
                         # hypervisor because the instance is not running
                         # anymore on the current host
-                        self.volume_api.terminate_connection(ctxt,
+                        self.volume_api.terminate_connection(context,
                                                              bdm.volume_id,
                                                              connector)
                     else:
                         # cinder v3.44 api flow - delete the old attachment
                         # for the source host
-                        self.volume_api.attachment_delete(ctxt,
+                        self.volume_api.attachment_delete(context,
                                                           bdm.attachment_id)
 
                 except Exception as e:
                     if bdm.attachment_id is None:
                         LOG.error('Connection for volume %s not terminated on '
                                   'source host %s during post_live_migration: '
-                                   '%s', bdm.volume_id, self.host,
-                                   six.text_type(e), instance=instance)
+                                  '%s', bdm.volume_id, self.host,
+                                  six.text_type(e), instance=instance)
                     else:
                         LOG.error('Volume attachment %s not deleted on source '
                                   'host %s during post_live_migration: %s',
                                   bdm.attachment_id, self.host,
                                   six.text_type(e), instance=instance)
 
+    @wrap_exception()
+    @wrap_instance_fault
+    def _post_live_migration(self, ctxt, instance, dest,
+                             block_migration=False, migrate_data=None,
+                             source_bdms=None):
+        """Post operations for live migration.
+
+        This method is called from live_migration
+        and mainly updating database record.
+
+        :param ctxt: security context
+        :param instance: instance dict
+        :param dest: destination host
+        :param block_migration: if true, prepare for block migration
+        :param migrate_data: if not None, it is a dict which has data
+        :param source_bdms: BDMs prior to modification by the destination
+                            compute host. Set by _do_live_migration and not
+                            part of the callback interface, so this is never
+                            None
+        required for live migration without shared storage
+
+        """
+        LOG.info('_post_live_migration() is started..',
+                 instance=instance)
+
+        # Cleanup source host post live-migration
+        block_device_info = self._get_instance_block_device_info(
+                            ctxt, instance, bdms=source_bdms)
+        self.driver.post_live_migration(ctxt, instance, block_device_info,
+                                        migrate_data)
+
+        # Disconnect volumes from this (the source) host.
+        self._post_live_migration_remove_source_vol_connections(
+            ctxt, instance, source_bdms)
+
         # Releasing vlan.
         # (not necessary in current implementation?)
 
diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py
index dc3a49e5a1..fef93b470d 100644
--- a/nova/tests/unit/compute/test_compute.py
+++ b/nova/tests/unit/compute/test_compute.py
@@ -6658,9 +6658,6 @@ class ComputeTestCase(BaseTestCase,
                                         'state_description': 'migrating',
                                         'state': power_state.PAUSED},
                                         ctxt=c)
-        instance.update({'task_state': task_states.MIGRATING,
-                         'power_state': power_state.PAUSED})
-        instance.save()
 
         bdms = block_device_obj.block_device_make_list(c,
                 [fake_block_device.FakeDbBlockDeviceDict({
@@ -6672,29 +6669,16 @@ class ComputeTestCase(BaseTestCase,
                  ])
 
         with test.nested(
-            mock.patch.object(self.compute.network_api,
-                              'migrate_instance_start'),
-            mock.patch.object(self.compute.compute_rpcapi,
-                              'post_live_migration_at_destination'),
-            mock.patch.object(self.compute.network_api,
-                              'setup_networks_on_host'),
-            mock.patch.object(self.compute.instance_events,
-                              'clear_events_for_instance'),
-            mock.patch.object(self.compute,
-                              '_get_instance_block_device_info'),
             mock.patch.object(self.compute.driver, 'get_volume_connector'),
             mock.patch.object(cinder.API, 'terminate_connection'),
         ) as (
-            migrate_instance_start, post_live_migration_at_destination,
-            setup_networks_on_host, clear_events_for_instance,
-            get_instance_volume_block_device_info, get_volume_connector,
+            get_volume_connector,
             terminate_connection,
         ):
             get_volume_connector.return_value = 'fake-connector'
 
-            self.compute._post_live_migration(c, instance, 'dest_host',
-                                              migrate_data=mock.MagicMock(),
-                                              source_bdms=bdms)
+            self.compute._post_live_migration_remove_source_vol_connections(
+                c, instance, bdms)
 
             terminate_connection.assert_called_once_with(
                     c, uuids.volume_id, 'fake-connector')
diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py
index 17fcb0e99b..b41577586e 100644
--- a/nova/tests/unit/compute/test_compute_mgr.py
+++ b/nova/tests/unit/compute/test_compute_mgr.py
@@ -9385,52 +9385,17 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase,
              'volume_id': volume_id, 'device_name': '/dev/vdb',
              'instance_uuid': instance.uuid})
         vol_bdm.attachment_id = uuids.attachment
-        migrate_data = migrate_data_obj.LiveMigrateData()
-        migrate_data.migration = objects.Migration(uuid=uuids.migration,
-                                                   dest_node=instance.node,
-                                                   source_node='src')
         image_bdm.attachment_id = uuids.attachment3
 
-        @mock.patch.object(instance, 'get_network_info')
-        @mock.patch('nova.compute.utils.notify_about_instance_action')
-        @mock.patch('nova.objects.ConsoleAuthToken.'
-                    'clean_console_auths_for_instance')
-        @mock.patch.object(migrate_data.migration, 'save',
-                           new=lambda: None)
-        @mock.patch.object(self.compute.reportclient,
-                           'get_allocations_for_consumer_by_provider')
-        @mock.patch.object(vol_bdm, 'save')
-        @mock.patch.object(self.compute, 'update_available_resource')
         @mock.patch.object(self.compute.volume_api, 'attachment_delete')
-        @mock.patch.object(self.compute, '_get_instance_block_device_info')
-        @mock.patch.object(self.compute, 'compute_rpcapi')
-        @mock.patch.object(self.compute, 'driver')
-        @mock.patch.object(self.compute, '_notify_about_instance_usage')
-        @mock.patch.object(self.compute, 'network_api')
-        def _test(mock_net_api, mock_notify, mock_driver,
-                  mock_rpc, mock_get_bdm_info, mock_attach_delete,
-                  mock_update_resource, mock_bdm_save, mock_ga,
-                  mock_clean, mock_notify_action, mock_get_nw_info):
-            self._mock_rt()
-
+        def _test(mock_attach_delete):
             bdms = objects.BlockDeviceMappingList(objects=[vol_bdm, image_bdm])
 
-            self.compute._post_live_migration(self.context,
-                                              instance,
-                                              dest_host,
-                                              migrate_data=migrate_data,
-                                              source_bdms=bdms)
+            self.compute._post_live_migration_remove_source_vol_connections(
+                self.context, instance, bdms)
 
             mock_attach_delete.assert_called_once_with(
                 self.context, uuids.attachment)
-            mock_clean.assert_called_once_with(self.context, instance.uuid)
-            mock_notify_action.assert_has_calls([
-                mock.call(self.context, instance, 'fake-mini',
-                          action='live_migration_post', phase='start'),
-                mock.call(self.context, instance, 'fake-mini',
-                          action='live_migration_post', phase='end')])
-            self.assertEqual(2, mock_notify_action.call_count)
-            mock_get_nw_info.assert_called()
 
         _test()