openstack/cinder
Code Issues Proposed changes

Remove useless response checks in SolidFire driver

Browse Source 
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
This commit is contained in:
John Griffith 2015-09-02 15:56:15 +00:00
parent 0773a4130d
commit 3b90fc7e97
2 changed files with 66 additions and 122 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
cinder/tests/unit/test_solidfire.py
View File

@ -191,10 +191,13 @@ class SolidFireVolumeTestCase(test.TestCase):
def fake_issue_api_request_fails(obj, method, def fake_issue_api_request_fails(obj, method,
params, version='1.0', params, version='1.0',
endpoint=None): endpoint=None):
return {'error': {'code': 000, response = {'error': {'code': 000,
'name': 'DummyError', 'name': 'DummyError',
'message': 'This is a fake error response'}, 'message': 'This is a fake error response'},
'id': 1} '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): def fake_set_qos_by_volume_type(self, type_id, ctxt):
return {'minIOPS': 500, return {'minIOPS': 500,
@ -459,8 +462,8 @@ class SolidFireVolumeTestCase(test.TestCase):
self.stubs.Set(solidfire.SolidFireDriver, self.stubs.Set(solidfire.SolidFireDriver,
'_issue_api_request', '_issue_api_request',
self.fake_issue_api_request_fails) self.fake_issue_api_request_fails)
account = sfv._create_sfaccount('project-id') self.assertRaises(exception.SolidFireAPIException,
self.assertIsNone(account) sfv._create_sfaccount, 'project-id')
def test_get_sfaccount_by_name(self): def test_get_sfaccount_by_name(self):
sfv = solidfire.SolidFireDriver(configuration=self.configuration) sfv = solidfire.SolidFireDriver(configuration=self.configuration)
@ -475,8 +478,8 @@ class SolidFireVolumeTestCase(test.TestCase):
self.stubs.Set(solidfire.SolidFireDriver, self.stubs.Set(solidfire.SolidFireDriver,
'_issue_api_request', '_issue_api_request',
self.fake_issue_api_request_fails) self.fake_issue_api_request_fails)
account = sfv._get_sfaccount_by_name('some-name') self.assertRaises(exception.SolidFireAPIException,
self.assertIsNone(account) sfv._get_sfaccount_by_name, 'some-name')
@mock.patch.object(solidfire.SolidFireDriver, '_issue_api_request') @mock.patch.object(solidfire.SolidFireDriver, '_issue_api_request')
@mock.patch.object(solidfire.SolidFireDriver, '_create_template_account') @mock.patch.object(solidfire.SolidFireDriver, '_create_template_account')
@ -593,7 +596,7 @@ class SolidFireVolumeTestCase(test.TestCase):
'created_at': timeutils.utcnow()} 'created_at': timeutils.utcnow()}
sfv = solidfire.SolidFireDriver(configuration=self.configuration) sfv = solidfire.SolidFireDriver(configuration=self.configuration)
self.assertRaises(exception.SolidFireAccountNotFound, self.assertRaises(exception.SolidFireAPIException,
sfv.extend_volume, sfv.extend_volume,
testvol, 2) testvol, 2)

167
cinder/volume/drivers/solidfire.py
View File

@ -281,9 +281,8 @@ class SolidFireDriver(san.SanISCSIDriver):
def _get_volumes_by_sfaccount(self, account_id): def _get_volumes_by_sfaccount(self, account_id):
"""Get all volumes on cluster for specified account.""" """Get all volumes on cluster for specified account."""
params = {'accountID': account_id} params = {'accountID': account_id}
data = self._issue_api_request('ListVolumesForAccount', params) return self._issue_api_request(
if 'result' in data: 'ListVolumesForAccount', params)['result']['volumes']
return data['result']['volumes']
def _get_sfaccount_by_name(self, sf_account_name): def _get_sfaccount_by_name(self, sf_account_name):
"""Get SolidFire account object by name.""" """Get SolidFire account object by name."""
@ -335,21 +334,15 @@ class SolidFireDriver(san.SanISCSIDriver):
'initiatorSecret': chap_secret, 'initiatorSecret': chap_secret,
'targetSecret': chap_secret, 'targetSecret': chap_secret,
'attributes': {}} 'attributes': {}}
data = self._issue_api_request('AddAccount', params) self._issue_api_request('AddAccount', params)
if 'result' in data: sfaccount = self._get_sfaccount_by_name(sf_account_name)
sfaccount = self._get_sfaccount_by_name(sf_account_name)
return sfaccount return sfaccount
def _get_cluster_info(self): def _get_cluster_info(self):
"""Query the SolidFire cluster for some property info.""" """Query the SolidFire cluster for some property info."""
params = {} params = {}
data = self._issue_api_request('GetClusterInfo', params) return self._issue_api_request('GetClusterInfo', params)['result']
if 'result' not in data:
msg = _("API response: %s") % data
raise exception.SolidFireAPIException(msg)
return data['result']
def _generate_random_string(self, length): def _generate_random_string(self, length):
"""Generates random_string to use for CHAP password.""" """Generates random_string to use for CHAP password."""
@ -496,20 +489,13 @@ class SolidFireDriver(san.SanISCSIDriver):
return self._issue_api_request('ModifyVolume', params) return self._issue_api_request('ModifyVolume', params)
def _do_volume_create(self, sf_account, params): def _do_volume_create(self, sf_account, params):
data = self._issue_api_request('CreateVolume', params) sf_volid = self._issue_api_request(
if (('result' not in data) or ('volumeID' not in data['result'])): 'CreateVolume', params)['result']['volumeID']
msg = _("Failed volume create: %s") % data return self._get_model_info(sf_account, sf_volid)
raise exception.SolidFireAPIException(msg)
sf_volume_id = data['result']['volumeID']
return self._get_model_info(sf_account, sf_volume_id)
def _do_snapshot_create(self, params): def _do_snapshot_create(self, params):
data = self._issue_api_request('CreateSnapshot', params, version='6.0') return self._issue_api_request(
if (('result' not in data) or ('snapshotID' not in data['result'])): 'CreateSnapshot', params, version='6.0')['result']['snapshotID']
msg = _("Failed snapshot create: %s") % data
raise exception.SolidFireAPIException(msg)
return data['result']['snapshotID']
def _set_qos_presets(self, volume): def _set_qos_presets(self, volume):
qos = {} qos = {}
@ -553,14 +539,12 @@ class SolidFireDriver(san.SanISCSIDriver):
return qos return qos
def _get_sf_volume(self, uuid, params): def _get_sf_volume(self, uuid, params):
data = self._issue_api_request('ListVolumesForAccount', params) vols = self._issue_api_request(
if 'result' not in data: 'ListVolumesForAccount', params)['result']['volumes']
msg = _("Failed to get SolidFire Volume: %s") % data
raise exception.SolidFireAPIException(msg)
found_count = 0 found_count = 0
sf_volref = None sf_volref = None
for v in data['result']['volumes']: for v in vols:
# NOTE(jdg): In the case of "name" we can't # NOTE(jdg): In the case of "name" we can't
# update that on manage/import, so we use # update that on manage/import, so we use
# the uuid attribute # the uuid attribute
@ -592,11 +576,8 @@ class SolidFireDriver(san.SanISCSIDriver):
params = {} params = {}
if sf_volid: if sf_volid:
params = {'volumeID': sf_volid} params = {'volumeID': sf_volid}
data = self._issue_api_request('ListSnapshots', params, version='6.0') return self._issue_api_request(
if 'result' not in data: 'ListSnapshots', params, version='6.0')['result']['snapshots']
msg = _("Failed to get SolidFire Snapshot: %s") % data
raise exception.SolidFireAPIException(msg)
return data['result']['snapshots']
def _create_image_volume(self, context, def _create_image_volume(self, context,
image_meta, image_service, image_meta, image_service,
@ -686,11 +667,7 @@ class SolidFireDriver(san.SanISCSIDriver):
# Bummer, it's been updated, delete it # Bummer, it's been updated, delete it
params = {'accountID': self.template_account_id} params = {'accountID': self.template_account_id}
params['volumeID'] = sf_vol['volumeID'] params['volumeID'] = sf_vol['volumeID']
data = self._issue_api_request('DeleteVolume', params) self._issue_api_request('DeleteVolume', params)
if 'result' not in data:
msg = _("Failed to delete SolidFire Image-Volume: %s") % data
raise exception.SolidFireAPIException(msg)
if not self._create_image_volume(context, if not self._create_image_volume(context,
image_meta, image_meta,
image_service, image_service,
@ -699,44 +676,37 @@ class SolidFireDriver(san.SanISCSIDriver):
raise exception.SolidFireAPIException(msg) raise exception.SolidFireAPIException(msg)
def _get_sfaccounts_for_tenant(self, cinder_project_id): def _get_sfaccounts_for_tenant(self, cinder_project_id):
data = self._issue_api_request('ListAccounts', {}) accounts = self._issue_api_request(
if 'result' not in data: 'ListAccounts', {})['result']['accounts']
msg = _("API response: %s") % data
raise exception.SolidFireAPIException(msg)
# Note(jdg): On SF we map account-name to OpenStack's tenant ID # Note(jdg): On SF we map account-name to OpenStack's tenant ID
# we use tenantID in here to get secondaries that might exist # we use tenantID in here to get secondaries that might exist
# Also: we expect this to be sorted, so we get the primary first # Also: we expect this to be sorted, so we get the primary first
# in the list # 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']]) cinder_project_id in acc['username']])
def _get_all_active_volumes(self, cinder_uuid=None): def _get_all_active_volumes(self, cinder_uuid=None):
params = {} params = {}
data = self._issue_api_request('ListActiveVolumes', volumes = self._issue_api_request('ListActiveVolumes',
params) params)['result']['volumes']
if 'result' not in data:
msg = _("Failed get active SolidFire volumes: %s") % data
raise exception.SolidFireAPIException(msg)
if cinder_uuid: if cinder_uuid:
deleted_vols = ([v for v in data['result']['volumes'] if vols = ([v for v in volumes if
cinder_uuid in v.name]) cinder_uuid in v.name])
else: else:
deleted_vols = [v for v in data['result']['volumes']] vols = [v for v in volumes]
return deleted_vols
return vols
def _get_all_deleted_volumes(self, cinder_uuid=None): def _get_all_deleted_volumes(self, cinder_uuid=None):
params = {} params = {}
data = self._issue_api_request('ListDeletedVolumes', vols = self._issue_api_request('ListDeletedVolumes',
params) params)['result']['volumes']
if 'result' not in data:
msg = _("Failed get Deleted SolidFire volumes: %s") % data
raise exception.SolidFireAPIException(msg)
if cinder_uuid: 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']]) cinder_uuid in v['name']])
else: else:
deleted_vols = [v for v in data['result']['volumes']] deleted_vols = [v for v in vols]
return deleted_vols return deleted_vols
def _get_account_create_availability(self, accounts): def _get_account_create_availability(self, accounts):
@ -757,13 +727,13 @@ class SolidFireDriver(san.SanISCSIDriver):
# we require the solidfire accountID, uuid of volume # we require the solidfire accountID, uuid of volume
# is optional # is optional
params = {'accountID': sf_account_id} params = {'accountID': sf_account_id}
response = self._issue_api_request('ListVolumesForAccount', vols = self._issue_api_request('ListVolumesForAccount',
params) params)['result']['volumes']
if cinder_uuid: if cinder_uuid:
vlist = [v for v in response['result']['volumes'] if vlist = [v for v in vols if
cinder_uuid in v['name']] cinder_uuid in v['name']]
else: else:
vlist = [v for v in response['result']['volumes']] vlist = [v for v in vols]
vlist = sorted(vlist, key=lambda k: k['volumeID']) vlist = sorted(vlist, key=lambda k: k['volumeID'])
return vlist return vlist
@ -910,11 +880,7 @@ class SolidFireDriver(san.SanISCSIDriver):
if sf_vol is not None: if sf_vol is not None:
params = {'volumeID': sf_vol['volumeID']} params = {'volumeID': sf_vol['volumeID']}
data = self._issue_api_request('DeleteVolume', params) self._issue_api_request('DeleteVolume', params)
if 'result' not in data:
msg = _("Failed to delete SolidFire Volume: %s") % data
raise exception.SolidFireAPIException(msg)
else: else:
LOG.error(_LE("Volume ID %s was not found on " LOG.error(_LE("Volume ID %s was not found on "
"the SolidFire Cluster while attempting " "the SolidFire Cluster while attempting "
@ -933,13 +899,9 @@ class SolidFireDriver(san.SanISCSIDriver):
None) None)
if snap: if snap:
params = {'snapshotID': snap['snapshotID']} params = {'snapshotID': snap['snapshotID']}
data = self._issue_api_request('DeleteSnapshot', self._issue_api_request('DeleteSnapshot',
params, params,
version='6.0') version='6.0')
if 'result' not in data:
msg = (_("Failed to delete SolidFire Snapshot: %s") %
data)
raise exception.SolidFireAPIException(msg)
return return
# Make sure it's not "old style" using clones as snaps # Make sure it's not "old style" using clones as snaps
LOG.debug("Snapshot not found, checking old style clones.") LOG.debug("Snapshot not found, checking old style clones.")
@ -1004,11 +966,8 @@ class SolidFireDriver(san.SanISCSIDriver):
'volumeID': sf_vol['volumeID'], 'volumeID': sf_vol['volumeID'],
'totalSize': int(new_size * units.Gi) 'totalSize': int(new_size * units.Gi)
} }
data = self._issue_api_request('ModifyVolume', self._issue_api_request('ModifyVolume',
params, version='5.0') params, version='5.0')
if 'result' not in data:
raise exception.SolidFireAPIDataException(data=data)
def _update_cluster_status(self): def _update_cluster_status(self):
"""Retrieve status info for the Cluster.""" """Retrieve status info for the Cluster."""
@ -1017,8 +976,6 @@ class SolidFireDriver(san.SanISCSIDriver):
# NOTE(jdg): The SF api provides an UNBELIEVABLE amount # NOTE(jdg): The SF api provides an UNBELIEVABLE amount
# of stats data, this is just one of the calls # of stats data, this is just one of the calls
results = self._issue_api_request('GetClusterCapacity', params) results = self._issue_api_request('GetClusterCapacity', params)
if 'result' not in results:
LOG.error(_LE('Failed to get updated stats'))
results = results['result']['clusterCapacity'] results = results['result']['clusterCapacity']
free_capacity = ( free_capacity = (
@ -1067,10 +1024,7 @@ class SolidFireDriver(san.SanISCSIDriver):
'attributes': attributes 'attributes': attributes
} }
data = self._issue_api_request('ModifyVolume', params) self._issue_api_request('ModifyVolume', params)
if 'result' not in data:
raise exception.SolidFireAPIDataException(data=data)
def detach_volume(self, context, volume, attachment=None): def detach_volume(self, context, volume, attachment=None):
sfaccount = self._get_sfaccount(volume['project_id']) sfaccount = self._get_sfaccount(volume['project_id'])
@ -1091,10 +1045,7 @@ class SolidFireDriver(san.SanISCSIDriver):
'attributes': attributes 'attributes': attributes
} }
data = self._issue_api_request('ModifyVolume', params) self._issue_api_request('ModifyVolume', params)
if 'result' not in data:
raise exception.SolidFireAPIDataException(data=data)
def accept_transfer(self, context, volume, def accept_transfer(self, context, volume,
new_user, new_project): new_user, new_project):
@ -1116,11 +1067,8 @@ class SolidFireDriver(san.SanISCSIDriver):
'volumeID': sf_vol['volumeID'], 'volumeID': sf_vol['volumeID'],
'accountID': sfaccount['accountID'] 'accountID': sfaccount['accountID']
} }
data = self._issue_api_request('ModifyVolume', self._issue_api_request('ModifyVolume',
params, version='5.0') params, version='5.0')
if 'result' not in data:
raise exception.SolidFireAPIDataException(data=data)
volume['project_id'] = new_project volume['project_id'] = new_project
volume['user_id'] = new_user volume['user_id'] = new_user
@ -1180,11 +1128,10 @@ class SolidFireDriver(san.SanISCSIDriver):
# First get the volume on the SF cluster (MUST be active) # First get the volume on the SF cluster (MUST be active)
params = {'startVolumeID': sfid, params = {'startVolumeID': sfid,
'limit': 1} 'limit': 1}
data = self._issue_api_request('ListActiveVolumes', params) vols = self._issue_api_request(
if 'result' not in data: 'ListActiveVolumes', params)['result']['volumes']
raise exception.SolidFireAPIDataException(data=data)
sf_ref = data['result']['volumes'][0]
sf_ref = vols[0]
sfaccount = self._create_sfaccount(volume['project_id']) sfaccount = self._create_sfaccount(volume['project_id'])
attributes = {} attributes = {}
@ -1214,10 +1161,8 @@ class SolidFireDriver(san.SanISCSIDriver):
'attributes': attributes, 'attributes': attributes,
'qos': qos} 'qos': qos}
data = self._issue_api_request('ModifyVolume', self._issue_api_request('ModifyVolume',
params, version='5.0') params, version='5.0')
if 'result' not in data:
raise exception.SolidFireAPIDataException(data=data)
return self._get_model_info(sfaccount, sf_ref['volumeID']) return self._get_model_info(sfaccount, sf_ref['volumeID'])
@ -1235,11 +1180,9 @@ class SolidFireDriver(san.SanISCSIDriver):
params = {'startVolumeID': int(sfid), params = {'startVolumeID': int(sfid),
'limit': 1} 'limit': 1}
data = self._issue_api_request('ListActiveVolumes', params) vols = self._issue_api_request(
if 'result' not in data: 'ListActiveVolumes', params)['result']['volumes']
raise exception.SolidFireAPIDataException(data=data) return int(vols[0]['totalSize']) / int(units.Gi)
sf_ref = data['result']['volumes'][0]
return int(sf_ref['totalSize']) / int(units.Gi)
def unmanage(self, volume): def unmanage(self, volume):
"""Mark SolidFire Volume as unmanaged (export from Cinder).""" """Mark SolidFire Volume as unmanaged (export from Cinder)."""
@ -1263,10 +1206,8 @@ class SolidFireDriver(san.SanISCSIDriver):
params = {'volumeID': int(sf_vol['volumeID']), params = {'volumeID': int(sf_vol['volumeID']),
'attributes': attributes} 'attributes': attributes}
data = self._issue_api_request('ModifyVolume', self._issue_api_request('ModifyVolume',
params, version='5.0') params, version='5.0')
if 'result' not in data:
raise exception.SolidFireAPIDataException(data=data)
# #### Interface methods for transport layer #### # # #### Interface methods for transport layer #### #