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

diff --git a/cinder/tests/unit/test_solidfire.py b/cinder/tests/unit/test_solidfire.py
index 9422fcf138eeb65429dad473db39a675697e0ccb..a3a4b2d7af474d147b3544e7ab0f887b9c2d22bb 100644 (file)
--- 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 f877347cf43a3deb13316fc8327a6316aba1b878..48cdfb89d6670c7be394f211c5509fa057fb1070 100644 (file)
--- 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 #### #