manager: Reduce unnecessary calls

The call to get_volume_connector() is unnecessary when all
BDMs are not volume. It affects the performance of live
migration or shutdown when instance hasn't attached volumes.
Set the connector is none and call 'get_volume_connector'
to assign a value to connector when it is first used.

Change-Id: I46239b2a614858c435ad12cc69079a3e2b312b35

Author: FionaLi-oc

nova/compute/manager.py

@@ -2891,6 +2891,7 @@ def _shutdown_instance(self, context, instance,
             self._try_deallocate_network(context, instance, requested_networks)
 
         timer.restart()
+        connector = None
         for bdm in vol_bdms:
             try:
                 if bdm.attachment_id:
@@ -2899,7 +2900,8 @@ def _shutdown_instance(self, context, instance,
                 else:
                     # NOTE(vish): actual driver detach done in driver.destroy,
                     #             so just tell cinder that we are done with it.
-                    connector = self.driver.get_volume_connector(instance)
+                    if connector is None:
+                        connector = self.driver.get_volume_connector(instance)
                     self.volume_api.terminate_connection(context,
                                                          bdm.volume_id,
                                                          connector)
@@ -8138,7 +8140,7 @@ def pre_live_migration(self, context, instance, disk, migrate_data):
             action=fields.NotificationAction.LIVE_MIGRATION_PRE,
             phase=fields.NotificationPhase.START, bdms=bdms)
 
-        connector = self.driver.get_volume_connector(instance)
+        connector = None
         try:
             for bdm in bdms:
                 if bdm.is_volume and bdm.attachment_id is not None:
@@ -8152,6 +8154,8 @@ def pre_live_migration(self, context, instance, disk, migrate_data):
                     #
                     # Also note that attachment_update is not needed as we
                     # are providing the connector in the create call.
+                    if connector is None:
+                        connector = self.driver.get_volume_connector(instance)
                     attach_ref = self.volume_api.attachment_create(
                         context, bdm.volume_id, bdm.instance_uuid,
                         connector=connector, mountpoint=bdm.device_name)
@@ -8641,7 +8645,7 @@ def _post_live_migration_remove_source_vol_connections(
             volumes with connection_info set for the source host
         """
         # Detaching volumes.
-        connector = self.driver.get_volume_connector(instance)
+        connector = None
         for bdm in source_bdms:
             if bdm.is_volume:
                 # Detaching volumes is a call to an external API that can fail.
@@ -8661,6 +8665,9 @@ def _post_live_migration_remove_source_vol_connections(
                         # remove the volume connection without detaching from
                         # hypervisor because the instance is not running
                         # anymore on the current host
+                        if connector is None:
+                            connector = self.driver.get_volume_connector(
+                                instance)
                         self.volume_api.terminate_connection(context,
                                                              bdm.volume_id,
                                                              connector)

nova/tests/unit/compute/test_compute.py

@@ -11478,13 +11478,61 @@ def fake_volume_get(self, context, volume_id, microversion=None):
             mock_detach.assert_called_once_with(mock.ANY, uuids.volume,
                                                 instance.uuid, None)
 
+    @mock.patch.object(context.RequestContext, 'elevated')
+    @mock.patch.object(cinder.API, 'detach')
+    @mock.patch.object(cinder.API, 'terminate_connection')
+    @mock.patch.object(compute_manager.ComputeManager,
+                       '_get_instance_block_device_info')
+    @mock.patch('nova.virt.fake.FakeDriver.get_volume_connector')
+    def test_shutdown_with_legacy_volume_detach(
+        self, mock_get_connector, mock_info, mock_terminate, mock_detach,
+        mock_elevated,
+    ):
+        # test _shutdown_instance with legacy BDMs without a volume
+        # attachment ID
+        admin = context.get_admin_context()
+        mock_elevated.return_value = admin
+        instance = self._create_fake_instance_obj()
+        connector = 'fake-connector'
+        mock_get_connector.return_value = connector
+
+        vol_a_bdm = block_device_obj.BlockDeviceMapping(
+            instance_uuid=instance['uuid'],
+            source_type='volume', destination_type='volume',
+            delete_on_termination=False,
+            volume_id=uuids.volume_a_id,
+            attachment_id=None)
+        vol_b_bdm = block_device_obj.BlockDeviceMapping(
+            instance_uuid=instance['uuid'],
+            source_type='volume', destination_type='volume',
+            delete_on_termination=False,
+            volume_id=uuids.volume_b_id,
+            attachment_id=None)
+        bdms = [vol_a_bdm, vol_b_bdm]
+
+        self.compute._shutdown_instance(admin, instance, bdms)
+
+        # we should only got the connector once, regardless of the number of
+        # volumes
+        mock_get_connector.assert_called_once_with(instance)
+        # but we should have separate terminate and detach calls
+        mock_terminate.assert_has_calls([
+            mock.call(admin, uuids.volume_a_id, connector),
+            mock.call(admin, uuids.volume_b_id, connector),
+        ])
+        mock_detach.assert_has_calls([
+            mock.call(admin, uuids.volume_a_id, instance.uuid),
+            mock.call(admin, uuids.volume_b_id, instance.uuid),
+        ])
+
     @mock.patch.object(context.RequestContext, 'elevated')
     @mock.patch.object(cinder.API, 'attachment_delete')
     @mock.patch.object(compute_manager.ComputeManager,
                        '_get_instance_block_device_info')
-    def test_shutdown_with_attachment_delete(self, mock_info,
-                                             mock_attach_delete,
-                                             mock_elevated):
+    @mock.patch('nova.virt.fake.FakeDriver.get_volume_connector')
+    def test_shutdown_with_attachment_delete(
+        self, mock_get_connector, mock_info, mock_attach_delete, mock_elevated,
+    ):
         # test _shutdown_instance with volume bdm containing an
         # attachment id. This should use the v3 cinder api.
         admin = context.get_admin_context()
@@ -11504,14 +11552,18 @@ def test_shutdown_with_attachment_delete(self, mock_info,
         self.compute._shutdown_instance(admin, instance, bdms)
 
         mock_attach_delete.assert_called_once_with(admin, attachment_id)
+        # we shouldn't try to get a connector for a cinder v3-style attachment
+        mock_get_connector.assert_not_called()
 
     @mock.patch.object(compute_manager.LOG, 'debug')
     @mock.patch.object(cinder.API, 'attachment_delete')
     @mock.patch.object(compute_manager.ComputeManager,
                        '_get_instance_block_device_info')
-    def test_shutdown_with_attachment_not_found(self, mock_info,
-                                                mock_attach_delete,
-                                                mock_debug_log):
+    @mock.patch('nova.virt.fake.FakeDriver.get_volume_connector')
+    def test_shutdown_with_attachment_not_found(
+        self, mock_get_connector, mock_info, mock_attach_delete,
+        mock_debug_log,
+    ):
         # test _shutdown_instance with attachment_delete throwing
         # a VolumeAttachmentNotFound exception. This should not
         # cause _shutdown_instance to fail. Only a debug log
@@ -11537,6 +11589,8 @@ def test_shutdown_with_attachment_not_found(self, mock_info,
         # get last call to LOG.debug and verify correct exception is in there
         self.assertIsInstance(mock_debug_log.call_args[0][1],
                               exception.VolumeAttachmentNotFound)
+        # we shouldn't try to get a connector for a cinder v3-style attachment
+        mock_get_connector.assert_not_called()
 
     def test_terminate_with_volumes(self):
         # Make sure that volumes get detached during instance termination.