From 3b90fc7e9728c7eda45cf50259e6c5d249b5d4c9 Mon Sep 17 00:00:00 2001 From: John Griffith Date: Wed, 2 Sep 2015 15:56:15 +0000 Subject: [PATCH] Remove useless response checks in SolidFire driver The main issue_api_request method in the SolidFire driver has had a mechanism to check validity of response data for some time now. It checks the response and if there isn't a valid result in the response it raises. The problem is that we still have some cruft from the old code that does local checking/verfication. These checks won't ever actually be called because of the exception, and they create some confusion. This patch just removes those useless checks and cleans up the api_request calls a bit. Change-Id: Ifbcda0f943bb0b9dc47aff25bbb9245ce361fc48 Closes-Bug: #1491485 --- cinder/tests/unit/test_solidfire.py | 21 ++-- cinder/volume/drivers/solidfire.py | 167 +++++++++------------------- 2 files changed, 66 insertions(+), 122 deletions(-) diff --git a/cinder/tests/unit/test_solidfire.py b/cinder/tests/unit/test_solidfire.py index 9422fcf138e..a3a4b2d7af4 100644 --- a/cinder/tests/unit/test_solidfire.py +++ b/cinder/tests/unit/test_solidfire.py @@ -191,10 +191,13 @@ class SolidFireVolumeTestCase(test.TestCase): def fake_issue_api_request_fails(obj, method, params, version='1.0', endpoint=None): - return {'error': {'code': 000, - 'name': 'DummyError', - 'message': 'This is a fake error response'}, - 'id': 1} + response = {'error': {'code': 000, + 'name': 'DummyError', + 'message': 'This is a fake error response'}, + 'id': 1} + msg = ('Error (%s) encountered during ' + 'SolidFire API call.' % response['error']['name']) + raise exception.SolidFireAPIException(message=msg) def fake_set_qos_by_volume_type(self, type_id, ctxt): return {'minIOPS': 500, @@ -459,8 +462,8 @@ class SolidFireVolumeTestCase(test.TestCase): self.stubs.Set(solidfire.SolidFireDriver, '_issue_api_request', self.fake_issue_api_request_fails) - account = sfv._create_sfaccount('project-id') - self.assertIsNone(account) + self.assertRaises(exception.SolidFireAPIException, + sfv._create_sfaccount, 'project-id') def test_get_sfaccount_by_name(self): sfv = solidfire.SolidFireDriver(configuration=self.configuration) @@ -475,8 +478,8 @@ class SolidFireVolumeTestCase(test.TestCase): self.stubs.Set(solidfire.SolidFireDriver, '_issue_api_request', self.fake_issue_api_request_fails) - account = sfv._get_sfaccount_by_name('some-name') - self.assertIsNone(account) + self.assertRaises(exception.SolidFireAPIException, + sfv._get_sfaccount_by_name, 'some-name') @mock.patch.object(solidfire.SolidFireDriver, '_issue_api_request') @mock.patch.object(solidfire.SolidFireDriver, '_create_template_account') @@ -593,7 +596,7 @@ class SolidFireVolumeTestCase(test.TestCase): 'created_at': timeutils.utcnow()} sfv = solidfire.SolidFireDriver(configuration=self.configuration) - self.assertRaises(exception.SolidFireAccountNotFound, + self.assertRaises(exception.SolidFireAPIException, sfv.extend_volume, testvol, 2) diff --git a/cinder/volume/drivers/solidfire.py b/cinder/volume/drivers/solidfire.py index f877347cf43..48cdfb89d66 100644 --- a/cinder/volume/drivers/solidfire.py +++ b/cinder/volume/drivers/solidfire.py @@ -281,9 +281,8 @@ class SolidFireDriver(san.SanISCSIDriver): def _get_volumes_by_sfaccount(self, account_id): """Get all volumes on cluster for specified account.""" params = {'accountID': account_id} - data = self._issue_api_request('ListVolumesForAccount', params) - if 'result' in data: - return data['result']['volumes'] + return self._issue_api_request( + 'ListVolumesForAccount', params)['result']['volumes'] def _get_sfaccount_by_name(self, sf_account_name): """Get SolidFire account object by name.""" @@ -335,21 +334,15 @@ class SolidFireDriver(san.SanISCSIDriver): 'initiatorSecret': chap_secret, 'targetSecret': chap_secret, 'attributes': {}} - data = self._issue_api_request('AddAccount', params) - if 'result' in data: - sfaccount = self._get_sfaccount_by_name(sf_account_name) + self._issue_api_request('AddAccount', params) + sfaccount = self._get_sfaccount_by_name(sf_account_name) return sfaccount def _get_cluster_info(self): """Query the SolidFire cluster for some property info.""" params = {} - data = self._issue_api_request('GetClusterInfo', params) - if 'result' not in data: - msg = _("API response: %s") % data - raise exception.SolidFireAPIException(msg) - - return data['result'] + return self._issue_api_request('GetClusterInfo', params)['result'] def _generate_random_string(self, length): """Generates random_string to use for CHAP password.""" @@ -496,20 +489,13 @@ class SolidFireDriver(san.SanISCSIDriver): return self._issue_api_request('ModifyVolume', params) def _do_volume_create(self, sf_account, params): - data = self._issue_api_request('CreateVolume', params) - if (('result' not in data) or ('volumeID' not in data['result'])): - msg = _("Failed volume create: %s") % data - raise exception.SolidFireAPIException(msg) - - sf_volume_id = data['result']['volumeID'] - return self._get_model_info(sf_account, sf_volume_id) + sf_volid = self._issue_api_request( + 'CreateVolume', params)['result']['volumeID'] + return self._get_model_info(sf_account, sf_volid) def _do_snapshot_create(self, params): - data = self._issue_api_request('CreateSnapshot', params, version='6.0') - if (('result' not in data) or ('snapshotID' not in data['result'])): - msg = _("Failed snapshot create: %s") % data - raise exception.SolidFireAPIException(msg) - return data['result']['snapshotID'] + return self._issue_api_request( + 'CreateSnapshot', params, version='6.0')['result']['snapshotID'] def _set_qos_presets(self, volume): qos = {} @@ -553,14 +539,12 @@ class SolidFireDriver(san.SanISCSIDriver): return qos def _get_sf_volume(self, uuid, params): - data = self._issue_api_request('ListVolumesForAccount', params) - if 'result' not in data: - msg = _("Failed to get SolidFire Volume: %s") % data - raise exception.SolidFireAPIException(msg) + vols = self._issue_api_request( + 'ListVolumesForAccount', params)['result']['volumes'] found_count = 0 sf_volref = None - for v in data['result']['volumes']: + for v in vols: # NOTE(jdg): In the case of "name" we can't # update that on manage/import, so we use # the uuid attribute @@ -592,11 +576,8 @@ class SolidFireDriver(san.SanISCSIDriver): params = {} if sf_volid: params = {'volumeID': sf_volid} - data = self._issue_api_request('ListSnapshots', params, version='6.0') - if 'result' not in data: - msg = _("Failed to get SolidFire Snapshot: %s") % data - raise exception.SolidFireAPIException(msg) - return data['result']['snapshots'] + return self._issue_api_request( + 'ListSnapshots', params, version='6.0')['result']['snapshots'] def _create_image_volume(self, context, image_meta, image_service, @@ -686,11 +667,7 @@ class SolidFireDriver(san.SanISCSIDriver): # Bummer, it's been updated, delete it params = {'accountID': self.template_account_id} params['volumeID'] = sf_vol['volumeID'] - data = self._issue_api_request('DeleteVolume', params) - if 'result' not in data: - msg = _("Failed to delete SolidFire Image-Volume: %s") % data - raise exception.SolidFireAPIException(msg) - + self._issue_api_request('DeleteVolume', params) if not self._create_image_volume(context, image_meta, image_service, @@ -699,44 +676,37 @@ class SolidFireDriver(san.SanISCSIDriver): raise exception.SolidFireAPIException(msg) def _get_sfaccounts_for_tenant(self, cinder_project_id): - data = self._issue_api_request('ListAccounts', {}) - if 'result' not in data: - msg = _("API response: %s") % data - raise exception.SolidFireAPIException(msg) + accounts = self._issue_api_request( + 'ListAccounts', {})['result']['accounts'] # Note(jdg): On SF we map account-name to OpenStack's tenant ID # we use tenantID in here to get secondaries that might exist # Also: we expect this to be sorted, so we get the primary first # in the list - return sorted([acc for acc in data['result']['accounts'] if + return sorted([acc for acc in accounts if cinder_project_id in acc['username']]) def _get_all_active_volumes(self, cinder_uuid=None): params = {} - data = self._issue_api_request('ListActiveVolumes', - params) - if 'result' not in data: - msg = _("Failed get active SolidFire volumes: %s") % data - raise exception.SolidFireAPIException(msg) + volumes = self._issue_api_request('ListActiveVolumes', + params)['result']['volumes'] if cinder_uuid: - deleted_vols = ([v for v in data['result']['volumes'] if - cinder_uuid in v.name]) + vols = ([v for v in volumes if + cinder_uuid in v.name]) else: - deleted_vols = [v for v in data['result']['volumes']] - return deleted_vols + vols = [v for v in volumes] + + return vols def _get_all_deleted_volumes(self, cinder_uuid=None): params = {} - data = self._issue_api_request('ListDeletedVolumes', - params) - if 'result' not in data: - msg = _("Failed get Deleted SolidFire volumes: %s") % data - raise exception.SolidFireAPIException(msg) + vols = self._issue_api_request('ListDeletedVolumes', + params)['result']['volumes'] if cinder_uuid: - deleted_vols = ([v for v in data['result']['volumes'] if + deleted_vols = ([v for v in vols if cinder_uuid in v['name']]) else: - deleted_vols = [v for v in data['result']['volumes']] + deleted_vols = [v for v in vols] return deleted_vols def _get_account_create_availability(self, accounts): @@ -757,13 +727,13 @@ class SolidFireDriver(san.SanISCSIDriver): # we require the solidfire accountID, uuid of volume # is optional params = {'accountID': sf_account_id} - response = self._issue_api_request('ListVolumesForAccount', - params) + vols = self._issue_api_request('ListVolumesForAccount', + params)['result']['volumes'] if cinder_uuid: - vlist = [v for v in response['result']['volumes'] if + vlist = [v for v in vols if cinder_uuid in v['name']] else: - vlist = [v for v in response['result']['volumes']] + vlist = [v for v in vols] vlist = sorted(vlist, key=lambda k: k['volumeID']) return vlist @@ -910,11 +880,7 @@ class SolidFireDriver(san.SanISCSIDriver): if sf_vol is not None: params = {'volumeID': sf_vol['volumeID']} - data = self._issue_api_request('DeleteVolume', params) - - if 'result' not in data: - msg = _("Failed to delete SolidFire Volume: %s") % data - raise exception.SolidFireAPIException(msg) + self._issue_api_request('DeleteVolume', params) else: LOG.error(_LE("Volume ID %s was not found on " "the SolidFire Cluster while attempting " @@ -933,13 +899,9 @@ class SolidFireDriver(san.SanISCSIDriver): None) if snap: params = {'snapshotID': snap['snapshotID']} - data = self._issue_api_request('DeleteSnapshot', - params, - version='6.0') - if 'result' not in data: - msg = (_("Failed to delete SolidFire Snapshot: %s") % - data) - raise exception.SolidFireAPIException(msg) + self._issue_api_request('DeleteSnapshot', + params, + version='6.0') return # Make sure it's not "old style" using clones as snaps LOG.debug("Snapshot not found, checking old style clones.") @@ -1004,11 +966,8 @@ class SolidFireDriver(san.SanISCSIDriver): 'volumeID': sf_vol['volumeID'], 'totalSize': int(new_size * units.Gi) } - data = self._issue_api_request('ModifyVolume', - params, version='5.0') - - if 'result' not in data: - raise exception.SolidFireAPIDataException(data=data) + self._issue_api_request('ModifyVolume', + params, version='5.0') def _update_cluster_status(self): """Retrieve status info for the Cluster.""" @@ -1017,8 +976,6 @@ class SolidFireDriver(san.SanISCSIDriver): # NOTE(jdg): The SF api provides an UNBELIEVABLE amount # of stats data, this is just one of the calls results = self._issue_api_request('GetClusterCapacity', params) - if 'result' not in results: - LOG.error(_LE('Failed to get updated stats')) results = results['result']['clusterCapacity'] free_capacity = ( @@ -1067,10 +1024,7 @@ class SolidFireDriver(san.SanISCSIDriver): 'attributes': attributes } - data = self._issue_api_request('ModifyVolume', params) - - if 'result' not in data: - raise exception.SolidFireAPIDataException(data=data) + self._issue_api_request('ModifyVolume', params) def detach_volume(self, context, volume, attachment=None): sfaccount = self._get_sfaccount(volume['project_id']) @@ -1091,10 +1045,7 @@ class SolidFireDriver(san.SanISCSIDriver): 'attributes': attributes } - data = self._issue_api_request('ModifyVolume', params) - - if 'result' not in data: - raise exception.SolidFireAPIDataException(data=data) + self._issue_api_request('ModifyVolume', params) def accept_transfer(self, context, volume, new_user, new_project): @@ -1116,11 +1067,8 @@ class SolidFireDriver(san.SanISCSIDriver): 'volumeID': sf_vol['volumeID'], 'accountID': sfaccount['accountID'] } - data = self._issue_api_request('ModifyVolume', - params, version='5.0') - - if 'result' not in data: - raise exception.SolidFireAPIDataException(data=data) + self._issue_api_request('ModifyVolume', + params, version='5.0') volume['project_id'] = new_project volume['user_id'] = new_user @@ -1180,11 +1128,10 @@ class SolidFireDriver(san.SanISCSIDriver): # First get the volume on the SF cluster (MUST be active) params = {'startVolumeID': sfid, 'limit': 1} - data = self._issue_api_request('ListActiveVolumes', params) - if 'result' not in data: - raise exception.SolidFireAPIDataException(data=data) - sf_ref = data['result']['volumes'][0] + vols = self._issue_api_request( + 'ListActiveVolumes', params)['result']['volumes'] + sf_ref = vols[0] sfaccount = self._create_sfaccount(volume['project_id']) attributes = {} @@ -1214,10 +1161,8 @@ class SolidFireDriver(san.SanISCSIDriver): 'attributes': attributes, 'qos': qos} - data = self._issue_api_request('ModifyVolume', - params, version='5.0') - if 'result' not in data: - raise exception.SolidFireAPIDataException(data=data) + self._issue_api_request('ModifyVolume', + params, version='5.0') return self._get_model_info(sfaccount, sf_ref['volumeID']) @@ -1235,11 +1180,9 @@ class SolidFireDriver(san.SanISCSIDriver): params = {'startVolumeID': int(sfid), 'limit': 1} - data = self._issue_api_request('ListActiveVolumes', params) - if 'result' not in data: - raise exception.SolidFireAPIDataException(data=data) - sf_ref = data['result']['volumes'][0] - return int(sf_ref['totalSize']) / int(units.Gi) + vols = self._issue_api_request( + 'ListActiveVolumes', params)['result']['volumes'] + return int(vols[0]['totalSize']) / int(units.Gi) def unmanage(self, volume): """Mark SolidFire Volume as unmanaged (export from Cinder).""" @@ -1263,10 +1206,8 @@ class SolidFireDriver(san.SanISCSIDriver): params = {'volumeID': int(sf_vol['volumeID']), 'attributes': attributes} - data = self._issue_api_request('ModifyVolume', - params, version='5.0') - if 'result' not in data: - raise exception.SolidFireAPIDataException(data=data) + self._issue_api_request('ModifyVolume', + params, version='5.0') # #### Interface methods for transport layer #### #