From d1b3adfbe471ad99ddf0dd2b483b77435c157aea Mon Sep 17 00:00:00 2001 From: Jay Rubenstein Date: Wed, 18 Jul 2018 11:39:28 -0600 Subject: [PATCH] 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 --- .../drivers/solidfire/test_solidfire.py | 168 +++++++++++++++++- cinder/volume/drivers/solidfire.py | 33 ++-- .../notes/bug-reno-69539ecb9b0b5464.yaml | 5 + 3 files changed, 191 insertions(+), 15 deletions(-) create mode 100644 releasenotes/notes/bug-reno-69539ecb9b0b5464.yaml diff --git a/cinder/tests/unit/volume/drivers/solidfire/test_solidfire.py b/cinder/tests/unit/volume/drivers/solidfire/test_solidfire.py index c1665e0cf7f..422fc8d7dbd 100644 --- a/cinder/tests/unit/volume/drivers/solidfire/test_solidfire.py +++ b/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( diff --git a/cinder/volume/drivers/solidfire.py b/cinder/volume/drivers/solidfire.py index 71ea239a455..9ca645121e2 100644 --- a/cinder/volume/drivers/solidfire.py +++ b/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: - sfvol = sfvols[0] - break - if not sfvol: - # Hmmm, frankly if we get here there's a problem, - # 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 + # Bug 1782373 match single vref.id encase no provider as the + # above call will return a list for the account + for sfv in sfvols: + if sfv['attributes'].get('uuid', None) == vref.id: + sfvol = sfv + 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,7 +1040,8 @@ class SolidFireDriver(san.SanISCSIDriver): exc) LOG.debug('Removing SolidFire Cache Volume (SF ID): %s', vol['volumeID']) - self._detach_volume(context, attach_info, tvol, properties) + 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) return diff --git a/releasenotes/notes/bug-reno-69539ecb9b0b5464.yaml b/releasenotes/notes/bug-reno-69539ecb9b0b5464.yaml new file mode 100644 index 00000000000..e971e118828 --- /dev/null +++ b/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.