From 211737d0ce23a8a9b1f3953f4816150a9a3271fa Mon Sep 17 00:00:00 2001 From: Amit Uniyal Date: Mon, 13 Feb 2023 21:06:09 +0000 Subject: [PATCH] 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 --- nova/cmd/manage.py | 222 ++++++++++++++++------------- nova/tests/unit/cmd/test_manage.py | 65 +++++++-- 2 files changed, 173 insertions(+), 114 deletions(-) diff --git a/nova/cmd/manage.py b/nova/cmd/manage.py index d9c56e729996..2f432cf3e702 100644 --- a/nova/cmd/manage.py +++ b/nova/cmd/manage.py @@ -22,6 +22,7 @@ """ import collections +from contextlib import contextmanager import functools import os import re @@ -147,6 +148,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.""" @@ -3018,10 +3050,8 @@ class VolumeAttachmentCommands(object): :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) @@ -3037,111 +3067,97 @@ class VolumeAttachmentCommands(object): 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)') + locking_reason = ( + f'Refreshing connection_info for BDM {bdm.uuid} ' + f'associated with instance {instance_uuid} and volume ' + f'{volume_id}.') - 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}.')) + with locked_instance(im.cell_mapping, instance, locking_reason): + return self._do_refresh( + cctxt, instance, volume_id, bdm, connector) - # 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 + 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")) diff --git a/nova/tests/unit/cmd/test_manage.py b/nova/tests/unit/cmd/test_manage.py index 0bd4db284e77..f9142d37f603 100644 --- a/nova/tests/unit/cmd/test_manage.py +++ b/nova/tests/unit/cmd/test_manage.py @@ -3492,6 +3492,7 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase): 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 @@ -3548,11 +3549,14 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase): 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( @@ -3562,11 +3566,11 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase): 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') @@ -3593,9 +3597,9 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase): @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. """ @@ -3635,8 +3639,9 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase): @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 @@ -3715,6 +3720,44 @@ class TestNovaManageMain(test.NoDBTestCase): 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):