From d5a8c72032afcafe0b72c671a87272973af291cc Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Thu, 21 Oct 2021 13:02:23 +0200 Subject: [PATCH] Remove attach and detach volume driver methods As part of the effort agreed in the last PTG to simplify and properly define the driver interface this patch removes "attach_volume" and "detach_volume" methods from the driver interface and from the SolidFire driver, the only driver that is using it. Change-Id: Ieb08a6870da92e438b3e7d6f48c1bdeb4d560e22 --- cinder/interface/volume_driver.py | 19 ------- .../attachments/test_attachments_manager.py | 15 +----- .../drivers/solidfire/test_solidfire.py | 29 ----------- .../unit/volume/test_volume_migration.py | 3 +- cinder/volume/driver.py | 9 ---- cinder/volume/drivers/solidfire.py | 52 ------------------- cinder/volume/manager.py | 12 ----- ...e-no-attach-metadata-b17729ebd34703db.yaml | 6 +++ 8 files changed, 9 insertions(+), 136 deletions(-) create mode 100644 releasenotes/notes/solidfire-no-attach-metadata-b17729ebd34703db.yaml diff --git a/cinder/interface/volume_driver.py b/cinder/interface/volume_driver.py index e3a7640c5e9..d8c4d1759e5 100644 --- a/cinder/interface/volume_driver.py +++ b/cinder/interface/volume_driver.py @@ -200,17 +200,6 @@ class VolumeDriverCore(base.CinderInterface): volume has already been attached. """ - def attach_volume(self, context, volume, instance_uuid, host_name, - mountpoint): - """Lets the driver know Nova has attached the volume to an instance. - - :param context: Security/policy info for the request. - :param volume: Volume being attached. - :param instance_uuid: ID of the instance being attached to. - :param host_name: The host name. - :param mountpoint: Device mount point on the instance. - """ - def terminate_connection(self, volume, connector): """Remove access to a volume. @@ -223,14 +212,6 @@ class VolumeDriverCore(base.CinderInterface): force-detach and can be None. """ - def detach_volume(self, context, volume, attachment=None): - """Detach volume from an instance. - - :param context: Security/policy info for the request. - :param volume: Volume being detached. - :param attachment: (Optional) Attachment information. - """ - def clone_image(self, volume, image_location, image_id, image_metadata, image_service): """Clone an image to a volume. diff --git a/cinder/tests/unit/attachments/test_attachments_manager.py b/cinder/tests/unit/attachments/test_attachments_manager.py index 201ebccc66b..887486370db 100644 --- a/cinder/tests/unit/attachments/test_attachments_manager.py +++ b/cinder/tests/unit/attachments/test_attachments_manager.py @@ -69,9 +69,7 @@ class AttachmentManagerTestCase(test.TestCase): 'attach_mode': 'rw'} attachment_ref = db.volume_attach(self.context, values) with mock.patch.object( - self.manager, '_notify_about_volume_usage'),\ - mock.patch.object( - self.manager.driver, 'attach_volume') as mock_attach: + self.manager, '_notify_about_volume_usage'): expected = { 'encrypted': False, 'qos_specs': None, @@ -87,11 +85,6 @@ class AttachmentManagerTestCase(test.TestCase): vref, connector, attachment_ref.id)) - mock_attach.assert_called_once_with(self.context, - vref, - attachment_ref.instance_uuid, - connector['host'], - "na") expected = { 'encrypted': False, 'qos_specs': None, @@ -196,8 +189,7 @@ class AttachmentManagerTestCase(test.TestCase): @mock.patch.object(self.context, 'elevated') @mock.patch.object(self.manager, '_connection_terminate') @mock.patch.object(self.manager.driver, 'remove_export') - @mock.patch.object(self.manager.driver, 'detach_volume') - def _test(mock_detach, mock_rm_export, mock_con_term, mock_elevated, + def _test(mock_rm_export, mock_con_term, mock_elevated, mock_db_detached, mock_db_meta_delete, mock_get_attachment): mock_elevated.return_value = self.context mock_con_term.return_value = False @@ -214,8 +206,6 @@ class AttachmentManagerTestCase(test.TestCase): self.manager.attachment_delete(self.context, attachment1.id, vref) - mock_detach.assert_called_once_with(self.context, vref, - attachment1) mock_db_detached.called_once_with(self.context, vref, attachment1.id) mock_db_meta_delete.called_once_with(self.context, vref.id, @@ -227,7 +217,6 @@ class AttachmentManagerTestCase(test.TestCase): mock_con_term.return_value = True vref.volume_attachment.objects.append(attachment2) - mock_detach.reset_mock() mock_rm_export.reset_mock() mock_db_detached.reset_mock() mock_db_meta_delete.reset_mock() diff --git a/cinder/tests/unit/volume/drivers/solidfire/test_solidfire.py b/cinder/tests/unit/volume/drivers/solidfire/test_solidfire.py index 4eacec41e0d..3bf27379a6c 100644 --- a/cinder/tests/unit/volume/drivers/solidfire/test_solidfire.py +++ b/cinder/tests/unit/volume/drivers/solidfire/test_solidfire.py @@ -1373,35 +1373,6 @@ class SolidFireVolumeTestCase(test.TestCase): self.assertTrue(migrated) self.assertEqual({}, updates) - @data(None, 'Success', 'Error', f'target:{f_uuid[0]}') - @mock.patch.object(solidfire.SolidFireDriver, '_get_sf_volume') - @mock.patch.object(solidfire.SolidFireDriver, '_get_sfaccount') - def test_attach_volume(self, mig_status, mock_get_sfaccount, - mock_get_sf_volume): - mock_get_sfaccount.return_value = self.fake_sfaccount - i_uuid = 'fake_instance_uuid' - ctx = context.get_admin_context() - type_fields = {} - vol_type = fake_volume.fake_volume_type_obj(ctx, **type_fields) - utc_now = timeutils.utcnow().isoformat() - vol_fields = { - 'id': f_uuid[0], - 'created_at': utc_now, - 'volume_type': vol_type, - 'volume_type_id': vol_type.id, - 'migration_status': mig_status, - } - vol = fake_volume.fake_volume_obj(ctx, **vol_fields) - sf_vol = self.fake_sfvol - mock_get_sf_volume.return_value = sf_vol - - sfv = solidfire.SolidFireDriver(configuration=self.configuration) - sfv.attach_volume(ctx, vol, i_uuid, 'fake_host', '/dev/sdf') - self.assertEqual(sf_vol['attributes']['attached_to'], - i_uuid) - mock_get_sfaccount.assert_called() - mock_get_sf_volume.assert_called() - def test_retype_with_qos_spec(self): test_type = {'name': 'sf-1', 'qos_specs_id': 'fb0576d7-b4b5-4cad-85dc-ca92e6a497d1', diff --git a/cinder/tests/unit/volume/test_volume_migration.py b/cinder/tests/unit/volume/test_volume_migration.py index 739ffdfbb4e..965ee09792a 100644 --- a/cinder/tests/unit/volume/test_volume_migration.py +++ b/cinder/tests/unit/volume/test_volume_migration.py @@ -675,8 +675,7 @@ class VolumeMigrationTestCase(base.BaseVolumeTestCase): mock.patch.object(volume_rpcapi.VolumeAPI, 'attach_volume') as mock_attach_volume,\ mock.patch.object(volume_rpcapi.VolumeAPI, - 'update_migrated_volume'),\ - mock.patch.object(self.volume.driver, 'attach_volume'): + 'update_migrated_volume'): mock_attach_volume.side_effect = self.fake_attach_volume old_volume_host = old_volume.host new_volume_host = new_volume.host diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py index 683803019ff..dbebc0a69ec 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -1383,15 +1383,6 @@ class BaseVD(object, metaclass=abc.ABCMeta): """Clean up after an interrupted image copy.""" pass - def attach_volume(self, context, volume, instance_uuid, host_name, - mountpoint): - """Callback for volume attached to instance or host.""" - pass - - def detach_volume(self, context, volume, attachment=None): - """Callback for volume detached.""" - pass - def do_setup(self, context): """Any initialization the volume driver does while starting.""" pass diff --git a/cinder/volume/drivers/solidfire.py b/cinder/volume/drivers/solidfire.py index a5e50e9cca6..b998822f572 100644 --- a/cinder/volume/drivers/solidfire.py +++ b/cinder/volume/drivers/solidfire.py @@ -2291,63 +2291,11 @@ class SolidFireDriver(san.SanISCSIDriver): properties['data']['discard'] = True return properties - def attach_volume(self, context, volume, - instance_uuid, host_name, - mountpoint): - - sfaccount = self._get_sfaccount(volume['project_id']) - params = {'accountID': sfaccount['accountID']} - - # In a retype of an attached volume scenario, the volume id will be - # as a target on 'migration_status', otherwise it'd be None. - migration_status = volume.get('migration_status') - if migration_status and 'target' in migration_status: - __, vol_id = migration_status.split(':') - else: - vol_id = volume['id'] - sf_vol = self._get_sf_volume(vol_id, params) - if sf_vol is None: - LOG.error("Volume ID %s was not found on " - "the SolidFire Cluster while attempting " - "attach_volume operation!", volume['id']) - raise exception.VolumeNotFound(volume_id=volume['id']) - - attributes = sf_vol['attributes'] - attributes['attach_time'] = volume.get('attach_time', None) - attributes['attached_to'] = instance_uuid - params = { - 'volumeID': sf_vol['volumeID'], - 'attributes': attributes - } - - self._issue_api_request('ModifyVolume', params) - def terminate_connection(self, volume, properties, force): return self._sf_terminate_connection(volume, properties, force) - def detach_volume(self, context, volume, attachment=None): - sfaccount = self._get_sfaccount(volume['project_id']) - params = {'accountID': sfaccount['accountID']} - - sf_vol = self._get_sf_volume(volume['id'], params) - if sf_vol is None: - LOG.error("Volume ID %s was not found on " - "the SolidFire Cluster while attempting " - "detach_volume operation!", volume['id']) - raise exception.VolumeNotFound(volume_id=volume['id']) - - attributes = sf_vol['attributes'] - attributes['attach_time'] = None - attributes['attached_to'] = None - params = { - 'volumeID': sf_vol['volumeID'], - 'attributes': attributes - } - - self._issue_api_request('ModifyVolume', params) - def accept_transfer(self, context, volume, new_user, new_project): diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 35e34ef01cd..88ea785ea19 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -1424,11 +1424,6 @@ class VolumeManager(manager.CleanableManager, {'volume_id': volume_id, 'instance': instance_uuid, 'mount': mountpoint, 'host': host_name_sanitized}, resource=volume) - self.driver.attach_volume(context, - volume, - instance_uuid, - host_name_sanitized, - mountpoint) except Exception as excep: with excutils.save_and_reraise_exception(): self.message_api.create( @@ -1507,7 +1502,6 @@ class VolumeManager(manager.CleanableManager, {'volume_id': volume_id, 'instance': attachment.get('instance_uuid')}, resource=volume) - self.driver.detach_volume(context, volume, attachment) except Exception: with excutils.save_and_reraise_exception(): self.db.volume_attachment_update( @@ -4851,11 +4845,6 @@ class VolumeManager(manager.CleanableManager, try: volume_utils.require_driver_initialized(self.driver) - self.driver.attach_volume(context, - vref, - attachment_ref.instance_uuid, - connector.get('host', ''), - connector.get('mountpoint', 'na')) except Exception as err: self.message_api.create( context, message_field.Action.UPDATE_ATTACHMENT, @@ -4947,7 +4936,6 @@ class VolumeManager(manager.CleanableManager, LOG.debug('Deleting attachment %(attachment_id)s.', {'attachment_id': attachment.id}, resource=vref) - self.driver.detach_volume(context, vref, attachment) if has_shared_connection is not None and not has_shared_connection: self.driver.remove_export(context.elevated(), vref) except Exception: diff --git a/releasenotes/notes/solidfire-no-attach-metadata-b17729ebd34703db.yaml b/releasenotes/notes/solidfire-no-attach-metadata-b17729ebd34703db.yaml new file mode 100644 index 00000000000..e5950889547 --- /dev/null +++ b/releasenotes/notes/solidfire-no-attach-metadata-b17729ebd34703db.yaml @@ -0,0 +1,6 @@ +--- +other: + - | + SolidFire driver: Driver no longer stores attach timestamp and instance as + metadata on the storage array. Any metadata remaining in the array must be + considered outdated and incorrect.