Added context manager for instance lock

Moved lock and unlock instance code to context manager.
Updated _refresh volume attachment method to use instance
lock context manager

Now there will be a single request ID for the lock, refresh, and unlock
actions. Earlier, the volume_attachment refresh operation used to have a
unique req-id for each action.

Related-Bug: #2012365
Change-Id: I6588836c3484a26d67a5995710761f0f6b6a4c18
(cherry picked from commit 211737d)
(cherry picked from commit 9eebaf3)

Author: auniyal61

nova/cmd/manage.py

@@ -22,6 +22,7 @@
 """
 
 import collections
+from contextlib import contextmanager
 import functools
 import os
 import re
@@ -144,6 +145,37 @@ def format_dict(dct, dict_property="Property", dict_value='Value',
     return encodeutils.safe_encode(pt.get_string()).decode()
 
 
+@contextmanager
+def locked_instance(cell_mapping, instance, reason):
+    """Context manager to lock and unlock instance,
+    lock state will be restored regardless of the success or failure
+    of target functionality.
+
+    :param cell_mapping: instance-cell-mapping
+    :param instance: instance to be lock and unlock
+    :param reason: reason, why lock is required
+    """
+
+    compute_api = api.API()
+
+    initial_state = 'locked' if instance.locked else 'unlocked'
+    if not instance.locked:
+        with context.target_cell(
+                    context.get_admin_context(),
+                    cell_mapping
+                ) as cctxt:
+            compute_api.lock(cctxt, instance, reason=reason)
+    try:
+        yield
+    finally:
+        if initial_state == 'unlocked':
+            with context.target_cell(
+                    context.get_admin_context(),
+                    cell_mapping
+                ) as cctxt:
+                compute_api.unlock(cctxt, instance)
+
+
 class DbCommands(object):
     """Class for managing the main database."""
 
@@ -2998,10 +3030,8 @@ def _refresh(self, instance_uuid, volume_id, connector):
         :param instance_uuid: UUID of instance
         :param volume_id: ID of volume attached to the instance
         :param connector: Connector with which to create the new attachment
+        :return status_code: volume-refresh status_code 0 on success
         """
-        volume_api = cinder.API()
-        compute_rpcapi = rpcapi.ComputeAPI()
-        compute_api = api.API()
 
         ctxt = context.get_admin_context()
         im = objects.InstanceMapping.get_by_instance_uuid(ctxt, instance_uuid)
@@ -3017,111 +3047,97 @@ def _refresh(self, instance_uuid, volume_id, connector):
                     state=instance.vm_state,
                     method='refresh connection_info (must be stopped)')
 
-            if instance.locked:
-                raise exception.InstanceInvalidState(
-                    instance_uuid=instance_uuid, attr='locked', state='True',
-                    method='refresh connection_info (must be unlocked)')
-
-            compute_api.lock(
-                cctxt, instance,
-                reason=(
-                    f'Refreshing connection_info for BDM {bdm.uuid} '
-                    f'associated with instance {instance_uuid} and volume '
-                    f'{volume_id}.'))
-
-        # NOTE(lyarwood): Yes this is weird but we need to recreate the admin
-        # context here to ensure the lock above uses a unique request-id
-        # versus the following refresh and eventual unlock.
-        ctxt = context.get_admin_context()
-        with context.target_cell(ctxt, im.cell_mapping) as cctxt:
-            instance_action = None
-            new_attachment_id = None
+            locking_reason = (
+                f'Refreshing connection_info for BDM {bdm.uuid} '
+                f'associated with instance {instance_uuid} and volume '
+                f'{volume_id}.')
+
+            with locked_instance(im.cell_mapping, instance, locking_reason):
+                return self._do_refresh(
+                    cctxt, instance, volume_id, bdm, connector)
+
+    def _do_refresh(self, cctxt, instance,
+            volume_id, bdm, connector):
+        volume_api = cinder.API()
+        compute_rpcapi = rpcapi.ComputeAPI()
+
+        new_attachment_id = None
+        try:
+            # Log this as an instance action so operators and users are
+            # aware that this has happened.
+            instance_action = objects.InstanceAction.action_start(
+                cctxt, instance.uuid,
+                instance_actions.NOVA_MANAGE_REFRESH_VOLUME_ATTACHMENT)
+
+            # Create a blank attachment to keep the volume reserved
+            new_attachment_id = volume_api.attachment_create(
+                cctxt, volume_id, instance.uuid)['id']
+
+            # RPC call to the compute to cleanup the connections, which
+            # will in turn unmap the volume from the compute host
+            # TODO(lyarwood): Add delete_attachment as a kwarg to
+            # remove_volume_connection as is available in the private
+            # method within the manager.
+            compute_rpcapi.remove_volume_connection(
+                cctxt, instance, volume_id, instance.host)
+
+            # Delete the existing volume attachment if present in the bdm.
+            # This isn't present when the original attachment was made
+            # using the legacy cinderv2 APIs before the cinderv3 attachment
+            # based APIs were present.
+            if bdm.attachment_id:
+                volume_api.attachment_delete(cctxt, bdm.attachment_id)
+
+            # Update the attachment with host connector, this regenerates
+            # the connection_info that we can now stash in the bdm.
+            new_connection_info = volume_api.attachment_update(
+                cctxt, new_attachment_id, connector,
+                bdm.device_name)['connection_info']
+
+            # Before we save it to the BDM ensure the serial is stashed as
+            # is done in various other codepaths when attaching volumes.
+            if 'serial' not in new_connection_info:
+                new_connection_info['serial'] = bdm.volume_id
+
+            # Save the new attachment id and connection_info to the DB
+            bdm.attachment_id = new_attachment_id
+            bdm.connection_info = jsonutils.dumps(new_connection_info)
+            bdm.save()
+
+            # Finally mark the attachment as complete, moving the volume
+            # status from attaching to in-use ahead of the instance
+            # restarting
+            volume_api.attachment_complete(cctxt, new_attachment_id)
+            return 0
+
+        finally:
+            # If the bdm.attachment_id wasn't updated make sure we clean
+            # up any attachments created during the run.
+            bdm = objects.BlockDeviceMapping.get_by_volume_and_instance(
+                cctxt, volume_id, instance.uuid)
+            if (
+                new_attachment_id and
+                bdm.attachment_id != new_attachment_id
+            ):
+                volume_api.attachment_delete(cctxt, new_attachment_id)
+
+            # If we failed during attachment_update the bdm.attachment_id
+            # has already been deleted so recreate it now to ensure the
+            # volume is still associated with the instance and clear the
+            # now stale connection_info.
             try:
-                # Log this as an instance action so operators and users are
-                # aware that this has happened.
-                instance_action = objects.InstanceAction.action_start(
-                    cctxt, instance_uuid,
-                    instance_actions.NOVA_MANAGE_REFRESH_VOLUME_ATTACHMENT)
-
-                # Create a blank attachment to keep the volume reserved
-                new_attachment_id = volume_api.attachment_create(
-                    cctxt, volume_id, instance_uuid)['id']
-
-                # RPC call to the compute to cleanup the connections, which
-                # will in turn unmap the volume from the compute host
-                # TODO(lyarwood): Add delete_attachment as a kwarg to
-                # remove_volume_connection as is available in the private
-                # method within the manager.
-                compute_rpcapi.remove_volume_connection(
-                    cctxt, instance, volume_id, instance.host)
-
-                # Delete the existing volume attachment if present in the bdm.
-                # This isn't present when the original attachment was made
-                # using the legacy cinderv2 APIs before the cinderv3 attachment
-                # based APIs were present.
-                if bdm.attachment_id:
-                    volume_api.attachment_delete(cctxt, bdm.attachment_id)
-
-                # Update the attachment with host connector, this regenerates
-                # the connection_info that we can now stash in the bdm.
-                new_connection_info = volume_api.attachment_update(
-                    cctxt, new_attachment_id, connector,
-                    bdm.device_name)['connection_info']
-
-                # Before we save it to the BDM ensure the serial is stashed as
-                # is done in various other codepaths when attaching volumes.
-                if 'serial' not in new_connection_info:
-                    new_connection_info['serial'] = bdm.volume_id
-
-                # Save the new attachment id and connection_info to the DB
-                bdm.attachment_id = new_attachment_id
-                bdm.connection_info = jsonutils.dumps(new_connection_info)
+                volume_api.attachment_get(cctxt, bdm.attachment_id)
+            except exception.VolumeAttachmentNotFound:
+                bdm.attachment_id = volume_api.attachment_create(
+                    cctxt, volume_id, instance.uuid)['id']
+                bdm.connection_info = None
                 bdm.save()
 
-                # Finally mark the attachment as complete, moving the volume
-                # status from attaching to in-use ahead of the instance
-                # restarting
-                volume_api.attachment_complete(cctxt, new_attachment_id)
-                return 0
-
-            finally:
-                # If the bdm.attachment_id wasn't updated make sure we clean
-                # up any attachments created during the run.
-                bdm = objects.BlockDeviceMapping.get_by_volume_and_instance(
-                    cctxt, volume_id, instance_uuid)
-                if (
-                    new_attachment_id and
-                    bdm.attachment_id != new_attachment_id
-                ):
-                    volume_api.attachment_delete(cctxt, new_attachment_id)
-
-                # If we failed during attachment_update the bdm.attachment_id
-                # has already been deleted so recreate it now to ensure the
-                # volume is still associated with the instance and clear the
-                # now stale connection_info.
-                try:
-                    volume_api.attachment_get(cctxt, bdm.attachment_id)
-                except exception.VolumeAttachmentNotFound:
-                    bdm.attachment_id = volume_api.attachment_create(
-                        cctxt, volume_id, instance_uuid)['id']
-                    bdm.connection_info = None
-                    bdm.save()
-
-                # Finish the instance action if it was created and started
-                # TODO(lyarwood): While not really required we should store
-                # the exec and traceback in here on failure.
-                if instance_action:
-                    instance_action.finish()
-
-                # NOTE(lyarwood): As above we need to unlock the instance with
-                # a fresh context and request-id to keep it unique. It's safe
-                # to assume that the instance is locked as this point as the
-                # earlier call to lock isn't part of this block.
-                with context.target_cell(
-                    context.get_admin_context(),
-                    im.cell_mapping
-                ) as u_cctxt:
-                    compute_api.unlock(u_cctxt, instance)
+            # Finish the instance action if it was created and started
+            # TODO(lyarwood): While not really required we should store
+            # the exec and traceback in here on failure.
+            if instance_action:
+                instance_action.finish()
 
     @action_description(
         _("Refresh the connection info for a given volume attachment"))

nova/tests/unit/cmd/test_manage.py

@@ -3472,6 +3472,7 @@ def test_refresh_invalid_connector_path_file(self, mock_exists):
     def _test_refresh(self, mock_exists):
         ctxt = context.get_admin_context()
         cell_ctxt = context.get_admin_context()
+        cell_ctxt.cell_uuid = '39fd7a1f-db62-45bc-a7b6-8137cef87c9d'
         fake_connector = self._get_fake_connector_info()
 
         mock_exists.return_value = True
@@ -3528,11 +3529,14 @@ def test_refresh_invalid_instance_state(
         output = self.output.getvalue().strip()
         self.assertIn('must be stopped', output)
 
+    @mock.patch.object(objects.InstanceAction, 'action_start')
+    @mock.patch.object(manage.VolumeAttachmentCommands, '_do_refresh')
     @mock.patch.object(
         objects.BlockDeviceMapping, 'get_by_volume_and_instance')
     @mock.patch.object(objects.Instance, 'get_by_uuid')
-    def test_refresh_instance_already_locked_failure(
-        self, mock_get_instance, mock_get_bdm
+    def test_refresh_instance_already_locked(
+        self, mock_get_instance, mock_get_bdm,
+        mock__do_refresh, mock_action_start
     ):
         """Test refresh with instance when instance is already locked."""
         mock_get_instance.return_value = objects.Instance(
@@ -3542,11 +3546,11 @@ def test_refresh_instance_already_locked_failure(
         mock_get_bdm.return_value = objects.BlockDeviceMapping(
             uuid=uuidsentinel.bdm, volume_id=uuidsentinel.volume,
             attachment_id=uuidsentinel.instance)
+        mock_action = mock.Mock(spec=objects.InstanceAction)
+        mock_action_start.return_value = mock_action
 
-        ret = self._test_refresh()
-        self.assertEqual(5, ret)
-        output = self.output.getvalue().strip()
-        self.assertIn('must be unlocked', output)
+        self._test_refresh()
+        mock__do_refresh.assert_called_once()
 
     @mock.patch.object(
         objects.BlockDeviceMapping, 'get_by_volume_and_instance')
@@ -3573,9 +3577,9 @@ def test_refresh_invalid_volume_id(self, mock_get_instance, mock_get_bdm):
     @mock.patch.object(objects.Instance, 'get_by_uuid')
     @mock.patch.object(objects.InstanceAction, 'action_start')
     def test_refresh_attachment_unknown_failure(
-        self, mock_action_start, mock_get_instance, mock_get_bdm, mock_lock,
-        mock_unlock, mock_attachment_create, mock_attachment_delete,
-        mock_attachment_get
+        self, mock_action_start, mock_get_instance,
+        mock_get_bdm, mock_lock, mock_unlock, mock_attachment_create,
+        mock_attachment_delete, mock_attachment_get
     ):
         """Test refresh with instance when any other error happens.
         """
@@ -3615,8 +3619,9 @@ def test_refresh_attachment_unknown_failure(
     @mock.patch.object(objects.Instance, 'get_by_uuid')
     @mock.patch.object(objects.InstanceAction, 'action_start')
     def test_refresh(
-        self, mock_action_start, mock_get_instance, mock_get_bdm,
-        mock_save_bdm, mock_compute_api, mock_volume_api, mock_compute_rpcapi
+        self, mock_action_start, mock_get_instance,
+        mock_get_bdm, mock_save_bdm, mock_compute_api, mock_volume_api,
+        mock_compute_rpcapi
     ):
         """Test refresh with a successful code path."""
         fake_compute_api = mock_compute_api.return_value
@@ -3695,6 +3700,44 @@ def test_error_post_mortem(self, mock_conf, mock_parse_args, mock_pm):
             self.assertEqual(255, manage.main())
             self.assertTrue(mock_pm.called)
 
+    def _lock_instance(self, ctxt, instance, reason):
+        instance.locked = True
+
+    def _unlock_instance(self, ctxt, instance):
+        instance.locked = False
+
+    def test_locked_instance(self):
+        cm = objects.CellMapping(name='foo', uuid=uuidsentinel.cell)
+        proj_uuid = uuidutils.generate_uuid()
+        instance = objects.Instance(
+            project_id=proj_uuid, uuid=uuidsentinel.instance)
+        instance.locked = True
+
+        with mock.patch('nova.compute.api.API') as mock_api:
+            mock_api.return_value.lock.side_effect = self._lock_instance
+            mock_api.return_value.unlock.side_effect = self._unlock_instance
+
+            with manage.locked_instance(cm, instance, 'some'):
+                self.assertTrue(instance.locked)
+
+        self.assertTrue(instance.locked)
+
+    def test_unlocked_instance(self):
+        cm = objects.CellMapping(name='foo', uuid=uuidsentinel.cell)
+        proj_uuid = uuidutils.generate_uuid()
+        instance = objects.Instance(
+            project_id=proj_uuid, uuid=uuidsentinel.instance)
+        instance.locked = False
+
+        with mock.patch('nova.compute.api.API') as mock_api:
+            mock_api.return_value.lock.side_effect = self._lock_instance
+            mock_api.return_value.unlock.side_effect = self._unlock_instance
+
+            with manage.locked_instance(cm, instance, 'some'):
+                self.assertTrue(instance.locked)
+
+        self.assertFalse(instance.locked)
+
 
 class LibvirtCommandsTestCase(test.NoDBTestCase):