SF ensure the correct volume is deleted

When two management systems are available openstack/cinder and
SolidFire-API/GUI  a volume can be deleted though either interface. Thus
if a volume is deleted through the SolidFire-interface and then again
through the openstack/cinder interface it was possible to delete another
volume.

The original code made an incorrect assumption on all the possible values returned
from a query to find the target volume. This could lead to deleting a
volume that was not the target of the original delete command from openstack/cinder.

The correction in the module _get_sfvol_by_cinder_vref  validates that the
returned object/volumes-id matchs the requested volume-id with a
comparison of svol['volumeID'] to int(sf_vid)

Closes-Bug: #1782373
Change-Id: I92a8e76c64c4e23dd185875e009132938ace5091

cinder/tests/unit/volume/drivers/solidfire/test_solidfire.py

@@ -35,6 +35,29 @@ from cinder.volume import qos_specs
 from cinder.volume import volume_types
 
 
+class mock_vref(object):
+    def __init__(self):
+        self._name_id = None
+        self.admin_metadata = {}
+        self.attach_status = 'detached'
+        self.id = '262b9ce2-a71a-4fbe-830c-c20c5596caea'
+        self.project_id = '52423d9394ad4c67b3b9034da58cedbc'
+        self.provider_id = '5 4 6ecebf5d-5521-4ce1-80f3-358ebc1b9cdc'
+        self.size = 20
+
+    def __setitem__(self, item, value):
+        self.__dict__[item] = value
+
+    def __getitem__(self, item):
+        return self.__dict__[item]
+
+    def get(self, item, arg2 = None):
+        return self.__dict__[item]
+
+f_uuid = ['262b9ce2-a71a-4fbe-830c-c20c5596caea',
+          '362b9ce2-a71a-4fbe-830c-c20c5596caea']
+
+
 @ddt.ddt
 class SolidFireVolumeTestCase(test.TestCase):
 
@@ -74,7 +97,9 @@ class SolidFireVolumeTestCase(test.TestCase):
                             'size': 1,
                             'id': 'a720b3c0-d1f0-11e1-9b23-0800200c9a66',
                             'volume_type_id': 'fast',
-                            'created_at': timeutils.utcnow()}
+                            'created_at': timeutils.utcnow(),
+                            'attributes':
+                            {'uuid': '262b9ce2-a71a-4fbe-830c-c20c5596caea'}}
         self.fake_image_meta = {'id': '17c550bb-a411-44c0-9aaf-0d96dd47f501',
                                 'updated_at': datetime.datetime(2013, 9,
                                                                 28, 15,
@@ -164,7 +189,7 @@ class SolidFireVolumeTestCase(test.TestCase):
                              'enable512e': True,
                              'access': "readWrite",
                              'status': "active",
-                             'attributes': {},
+                             'attributes': {'uuid': f_uuid[0]},
                              'qos': None,
                              'iqn': test_name}]}}
             return result
@@ -184,6 +209,35 @@ class SolidFireVolumeTestCase(test.TestCase):
                              'qos': None,
                              'iqn': test_name}]}}
             return result
+        elif method is 'ListVolumes':
+            test_name = "get_sfvol_by_cinder"
+            result = {'result': {
+                'volumes': [{'volumeID': 5,
+                             'name': test_name,
+                             'accountID': 8,
+                             'sliceCount': 1,
+                             'totalSize': int(1.75 * units.Gi),
+                             'enable512e': True,
+                             'access': "readWrite",
+                             'status': "active",
+                             'attributes': {'uuid': f_uuid[0]},
+                             'qos': None,
+                             'iqn': test_name},
+                            {'volumeID': 15,
+                             'name': test_name,
+                             'accountID': 8,
+                             'sliceCount': 1,
+                             'totalSize': int(1.75 * units.Gi),
+                             'enable512e': True,
+                             'access': "readWrite",
+                             'status': "active",
+                             'attributes': {'uuid': f_uuid[1]},
+                             'qos': None,
+                             'iqn': test_name}]}}
+            for v in result['result']['volumes']:
+                if int(v['volumeID']) == int(params['startVolumeID']):
+                    break
+            return v
         elif method is 'DeleteSnapshot':
             return {'result': {}}
         elif method is 'GetClusterVersionInfo':
@@ -500,6 +554,116 @@ class SolidFireVolumeTestCase(test.TestCase):
         self.assertRaises(exception.SolidFireAPIException,
                           sfv._get_sfaccount_by_name, 'some-name')
 
+    def test_get_sfvol_by_cinder_vref_no_provider_id(self):
+        fake_sfaccounts = [{'accountID': 25,
+                            'name': 'testprjid',
+                            'targetSecret': 'shhhh',
+                            'username': 'john-wayne',
+                            'volumes': [6, 7, 20]}]
+        self.mock_vref = mock_vref()
+
+        vol_result = {'volumeID': 5,
+                      'name': 'test_volume',
+                      'accountID': 25,
+                      'sliceCount': 1,
+                      'totalSize': 1 * units.Gi,
+                      'enable512e': True,
+                      'access': "readWrite",
+                      'status': "active",
+                      'attributes': {'uuid': f_uuid[0]},
+                      'qos': None,
+                      'iqn': 'super_fake_iqn'}
+
+        mod_conf = self.configuration
+        mod_conf.sf_enable_vag = True
+        sfv = solidfire.SolidFireDriver(configuration=mod_conf)
+        with mock.patch.object(sfv,
+                               '_get_sfaccounts_for_tenant',
+                               return_value = fake_sfaccounts), \
+                mock.patch.object(sfv, '_issue_api_request',
+                                  side_effect = self.fake_issue_api_request):
+            self.mock_vref['provider_id'] = None
+            sfvol = sfv._get_sfvol_by_cinder_vref(self.mock_vref)
+            self.assertIsNotNone(sfvol)
+            self.assertEqual(sfvol['attributes']['uuid'],
+                             vol_result['attributes']['uuid'])
+            self.assertEqual(sfvol['volumeID'], vol_result['volumeID'])
+
+    def test_get_sfvol_by_cinder_vref_no_provider_id_nomatch(self):
+        fake_sfaccounts = [{'accountID': 5,
+                            'name': 'testprjid',
+                            'targetSecret': 'shhhh',
+                            'username': 'john-wayne',
+                            'volumes': [5, 6, 7, 8]}]
+
+        self.mock_vref = mock_vref()
+        mod_conf = self.configuration
+        mod_conf.sf_enable_vag = True
+
+        sfv = solidfire.SolidFireDriver(configuration=mod_conf)
+        with mock.patch.object(sfv,
+                               '_get_sfaccounts_for_tenant',
+                               return_value = fake_sfaccounts), \
+                mock.patch.object(sfv, '_issue_api_request',
+                                  side_effect = self.fake_issue_api_request):
+            self.mock_vref['provider_id'] = None
+            self.mock_vref['id'] = '142b9c32-a71A-4fbe-830c-c20c5596caea'
+            sfvol = sfv._get_sfvol_by_cinder_vref(self.mock_vref)
+            self.assertIsNone(sfvol)
+
+    def test_get_sfvol_by_cinder_vref_nomatch(self):
+        fake_sfaccounts = [{'accountID': 5,
+                            'name': 'testprjid',
+                            'targetSecret': 'shhhh',
+                            'username': 'john-wayne',
+                            'volumes': [5, 6, 7, 8]}]
+
+        self.mock_vref = mock_vref()
+        mod_conf = self.configuration
+        mod_conf.sf_enable_vag = True
+        sfv = solidfire.SolidFireDriver(configuration=mod_conf)
+        with mock.patch.object(sfv,
+                               '_get_sfaccounts_for_tenant',
+                               return_value = fake_sfaccounts), \
+                mock.patch.object(sfv, '_issue_api_request',
+                                  side_effect = self.fake_issue_api_request):
+            p_i = '324 8 6ecebf5d-5521-4ce1-80f3-358ebc1b9cdc'
+            self.mock_vref['provider_id'] = p_i
+            self.mock_vref['id'] = '142b9c32-a71A-4fbe-830c-c20c5596caea'
+            sfvol = sfv._get_sfvol_by_cinder_vref(self.mock_vref)
+            self.assertIsNone(sfvol)
+
+    def test_get_sfvol_by_cinder_vref(self):
+        fake_sfaccounts = [{'accountID': 5,
+                            'name': 'testprjid',
+                            'targetSecret': 'shhhh',
+                            'username': 'john-wayne',
+                            'volumes': [5, 6, 7, 8]}]
+
+        self.mock_vref = mock_vref()
+
+        get_vol_result = {'volumeID': 5,
+                          'name': 'test_volume',
+                          'accountID': 25,
+                          'sliceCount': 1,
+                          'totalSize': 1 * units.Gi,
+                          'enable512e': True,
+                          'access': "readWrite",
+                          'status': "active",
+                          'attributes': {'uuid': f_uuid[0]},
+                          'qos': None,
+                          'iqn': 'super_fake_iqn'}
+
+        sfv = solidfire.SolidFireDriver(configuration=self.configuration)
+        with mock.patch.object(sfv, '_get_sfaccounts_for_tenant',
+                               return_value = fake_sfaccounts), \
+                mock.patch.object(sfv, '_issue_api_request',
+                                  side_effect = self.fake_issue_api_request):
+
+            sfvol = sfv._get_sfvol_by_cinder_vref(self.mock_vref)
+            self.assertIsNotNone(sfvol)
+            self.assertEqual(get_vol_result['volumeID'], sfvol['volumeID'])
+
     def test_delete_volume(self):
         vol_id = 'a720b3c0-d1f0-11e1-9b23-0800200c9a66'
         testvol = test_utils.create_volume(

cinder/volume/drivers/solidfire.py

@@ -555,7 +555,12 @@ class SolidFireDriver(san.SanISCSIDriver):
         return vlist
 
     def _get_sfvol_by_cinder_vref(self, vref):
+        # sfvols is one or more element objects returned from a list call
+        # sfvol is the single volume object that will be returned or it will
+        # be None
+        sfvols = None
         sfvol = None
+
         provider_id = vref.get('provider_id', None)
         if provider_id:
             try:
@@ -573,6 +578,10 @@ class SolidFireDriver(san.SanISCSIDriver):
                         {'startVolumeID': sf_vid,
                          'limit': 1},
                         version='8.0')['result']['volumes'][0]
+                    # Bug 1782373 validate the list returned has what we asked
+                    # for, check if there was no match
+                    if sfvol['volumeID'] != int(sf_vid):
+                        sfvol = None
                 except Exception:
                     pass
         if not sfvol:
@@ -582,18 +591,13 @@ class SolidFireDriver(san.SanISCSIDriver):
                 sfvols = self._issue_api_request(
                     'ListVolumesForAccount',
                     {'accountID': account['accountID']})['result']['volumes']
-                if len(sfvols) >= 1:
+                # Bug 1782373  match single vref.id encase no provider as the
-                    sfvol = sfvols[0]
+                # above call will return a list for the account
-                    break
+                for sfv in sfvols:
-        if not sfvol:
+                    if sfv['attributes'].get('uuid', None) == vref.id:
-            # Hmmm, frankly if we get here there's a problem,
+                        sfvol = sfv
-            # but try one last trick
-            LOG.info("Failed to find volume by provider_id or account, "
-                     "attempting find by attributes.")
-            for v in sfvols:
-                if v['Attributes'].get('uuid', None):
-                    sfvol = v
                         break
+
         return sfvol
 
     def _get_sfaccount_by_name(self, sf_account_name, endpoint=None):
@@ -1008,6 +1012,8 @@ class SolidFireDriver(san.SanISCSIDriver):
             tvol['provider_location'] = template_vol['provider_location']
             tvol['provider_auth'] = template_vol['provider_auth']
 
+            attach_info = None
+
             try:
                 connector = {'multipath': False}
                 conn = self.initialize_connection(tvol, connector)
@@ -1034,6 +1040,7 @@ class SolidFireDriver(san.SanISCSIDriver):
                           exc)
                 LOG.debug('Removing SolidFire Cache Volume (SF ID): %s',
                           vol['volumeID'])
+                if attach_info is not None:
                     self._detach_volume(context, attach_info, tvol, properties)
                 self._issue_api_request('DeleteVolume', params)
                 self._issue_api_request('PurgeDeletedVolume', params)

releasenotes/notes/bug-reno-69539ecb9b0b5464.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    The Solidfire cinder driver has been fixed to ensure delete happens
+    on the correct volume.