From dc6dac360c8ab026897d1f4bc04f30e5bd8a9998 Mon Sep 17 00:00:00 2001 From: Amit Uniyal Date: Mon, 14 Aug 2023 10:42:06 +0000 Subject: [PATCH] Removed explicit call to delete attachment This was a TODO to remove delete attachment call from refresh after remove_volume_connection call. Remove volume connection process itself deletes attachment on passing delete_attachment flag. Bumps RPC API version. Change-Id: I03ec3ee3ee1eeb6563a1dd6876094a7f4423d860 --- nova/cmd/manage.py | 13 ++----------- nova/compute/manager.py | 9 ++++++--- nova/compute/rpcapi.py | 24 ++++++++++++++++++------ nova/objects/service.py | 6 +++++- nova/tests/unit/cmd/test_manage.py | 4 +--- nova/tests/unit/compute/test_rpcapi.py | 2 +- 6 files changed, 33 insertions(+), 25 deletions(-) diff --git a/nova/cmd/manage.py b/nova/cmd/manage.py index a9b5ade5fb5e..39e4dd05b60f 100644 --- a/nova/cmd/manage.py +++ b/nova/cmd/manage.py @@ -3132,12 +3132,10 @@ class VolumeAttachmentCommands(object): # 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. if instance.host == connector['host']: compute_rpcapi.remove_volume_connection( - cctxt, instance, volume_id, instance.host) + cctxt, instance, volume_id, instance.host, + delete_attachment=True) else: msg = ( f"The compute host '{connector['host']}' in the " @@ -3145,13 +3143,6 @@ class VolumeAttachmentCommands(object): f"'{instance.host}'.") raise exception.HostConflict(_(msg)) - # 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( diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 87c8b6ac0a91..9ca17e259c17 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -618,7 +618,7 @@ class ComputeVirtAPI(virtapi.VirtAPI): class ComputeManager(manager.Manager): """Manages the running instances from creation to destruction.""" - target = messaging.Target(version='6.2') + target = messaging.Target(version='6.3') def __init__(self, compute_driver=None, *args, **kwargs): """Load configuration options and connect to the hypervisor.""" @@ -7967,7 +7967,9 @@ class ComputeManager(manager.Manager): old_volume_id, new_volume_id) @wrap_exception() - def remove_volume_connection(self, context, volume_id, instance): + def remove_volume_connection( + self, context, volume_id, instance, + delete_attachment=False): """Remove the volume connection on this host Detach the volume from this instance on this host, and if this is @@ -7986,7 +7988,8 @@ class ComputeManager(manager.Manager): # we cannot simply delete a v3 style attachment here without # needing to do some behavior modification of that # _rollback_live_migration flow which gets messy. - self._remove_volume_connection(context, bdm, instance) + self._remove_volume_connection( + context, bdm, instance, delete_attachment) except exception.NotFound: pass diff --git a/nova/compute/rpcapi.py b/nova/compute/rpcapi.py index dfff5795f2a9..42fa2a95b922 100644 --- a/nova/compute/rpcapi.py +++ b/nova/compute/rpcapi.py @@ -404,6 +404,7 @@ class ComputeAPI(object): flavor * 6.1 - Add reimage_boot_volume parameter to rebuild_instance() * 6.2 - Add target_state parameter to rebuild_instance() + * 6.3 - Add delete_attachment parameter to remove_volume_connection ''' VERSION_ALIASES = { @@ -1133,12 +1134,23 @@ class ComputeAPI(object): cctxt.cast(ctxt, 'remove_fixed_ip_from_instance', instance=instance, address=address) - def remove_volume_connection(self, ctxt, instance, volume_id, host): - version = self._ver(ctxt, '5.0') - cctxt = self.router.client(ctxt).prepare( - server=host, version=version) - return cctxt.call(ctxt, 'remove_volume_connection', - instance=instance, volume_id=volume_id) + def remove_volume_connection( + self, ctxt, instance, volume_id, host, + delete_attachment=False + ): + version = '6.3' + client = self.router.client(ctxt) + kwargs = { + 'instance': instance, + 'volume_id': volume_id, + 'delete_attachment': delete_attachment + } + if not client.can_send_version(version): + kwargs.pop('delete_attachment') + version = self._ver(ctxt, '5.0') + + cctxt = client.prepare(server=host, version=version) + return cctxt.call(ctxt, 'remove_volume_connection', **kwargs) def rescue_instance(self, ctxt, instance, rescue_password, rescue_image_ref=None, clean_shutdown=True): diff --git a/nova/objects/service.py b/nova/objects/service.py index 215d21b96dbb..c895d56c46fd 100644 --- a/nova/objects/service.py +++ b/nova/objects/service.py @@ -31,7 +31,7 @@ LOG = logging.getLogger(__name__) # NOTE(danms): This is the global service version counter -SERVICE_VERSION = 66 +SERVICE_VERSION = 67 # NOTE(danms): This is our SERVICE_VERSION history. The idea is that any @@ -231,6 +231,10 @@ SERVICE_VERSION_HISTORY = ( # Version 66: Compute RPC v6.2: # Add target_state parameter to rebuild_instance() {'compute_rpc': '6.2'}, + # Version 67: Compute RPC v6.3: + # Add delete_attachment parameter to remove_volume_connection() + {'compute_rpc': '6.3'}, + ) # This is the version after which we can rely on having a persistent diff --git a/nova/tests/unit/cmd/test_manage.py b/nova/tests/unit/cmd/test_manage.py index 0756624f6e63..285506b72194 100644 --- a/nova/tests/unit/cmd/test_manage.py +++ b/nova/tests/unit/cmd/test_manage.py @@ -3732,9 +3732,7 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase): mock.ANY, uuidsentinel.volume, uuidsentinel.instance) fake_compute_rpcapi.remove_volume_connection.assert_called_once_with( mock.ANY, mock_get_instance.return_value, uuidsentinel.volume, - mock_get_instance.return_value.host) - fake_volume_api.attachment_delete.assert_called_once_with( - mock.ANY, uuidsentinel.instance) + mock_get_instance.return_value.host, delete_attachment=True) fake_volume_api.attachment_update.assert_called_once_with( mock.ANY, uuidsentinel.new_attachment, mock.ANY, device_name) fake_volume_api.attachment_complete.assert_called_once_with( diff --git a/nova/tests/unit/compute/test_rpcapi.py b/nova/tests/unit/compute/test_rpcapi.py index b2cbbe2b5ae1..227db0541b0c 100644 --- a/nova/tests/unit/compute/test_rpcapi.py +++ b/nova/tests/unit/compute/test_rpcapi.py @@ -965,7 +965,7 @@ class ComputeRpcAPITestCase(test.NoDBTestCase): def test_remove_volume_connection(self): self._test_compute_api('remove_volume_connection', 'call', instance=self.fake_instance_obj, volume_id='id', host='host', - version='6.0') + delete_attachment=True, version='6.3') def test_rescue_instance(self): self._test_compute_api('rescue_instance', 'cast',