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

@@ -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.

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'),\
+                self.manager, '_notify_about_volume_usage'):
-                mock.patch.object(
-                self.manager.driver, 'attach_volume') as mock_attach:
             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_rm_export, mock_con_term, mock_elevated,
-        def _test(mock_detach, 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()

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',

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'),\
+                                  'update_migrated_volume'):
-                mock.patch.object(self.volume.driver, 'attach_volume'):
             mock_attach_volume.side_effect = self.fake_attach_volume
             old_volume_host = old_volume.host
             new_volume_host = new_volume.host

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

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):
 

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:

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.