From 1525bb4d8e99b1b948fe079f15e400a8262c1f5e Mon Sep 17 00:00:00 2001 From: John Griffith Date: Fri, 11 Sep 2015 06:21:40 +0000 Subject: [PATCH] Don't rely on provider_id for resource deletion Provider_id info isn't necessarily reliable when dealing with failure cases. There are a number of cases where the info in the Cinder database may not be updated. This change uses the old delete routines instead of trying to use the provider_id info. There are ways to do both, however it's probably not that beneficial for now. Change-Id: Idbfd98b19f3c7f1b231ed26a2cc6c98aa2899e2a Closes-Bug: #1494597 --- cinder/tests/unit/test_solidfire.py | 42 ++++++++++--- cinder/volume/drivers/solidfire.py | 94 ++++++++++++++--------------- 2 files changed, 79 insertions(+), 57 deletions(-) diff --git a/cinder/tests/unit/test_solidfire.py b/cinder/tests/unit/test_solidfire.py index 7ef891035..c3cda4d28 100644 --- a/cinder/tests/unit/test_solidfire.py +++ b/cinder/tests/unit/test_solidfire.py @@ -528,10 +528,12 @@ class SolidFireVolumeTestCase(test.TestCase): sfv.delete_volume(testvol) - def test_delete_volume_fails_no_volume(self): - self.stubs.Set(solidfire.SolidFireDriver, - '_issue_api_request', - self.fake_issue_api_request) + def test_delete_volume_no_volume_on_backend(self): + fake_sfaccounts = [{'accountID': 5, + 'name': 'testprjid', + 'targetSecret': 'shhhh', + 'username': 'john-wayne'}] + fake_no_volumes = [] testvol = {'project_id': 'testprjid', 'name': 'no-name', 'size': 1, @@ -539,11 +541,35 @@ class SolidFireVolumeTestCase(test.TestCase): 'created_at': timeutils.utcnow()} sfv = solidfire.SolidFireDriver(configuration=self.configuration) - try: + with mock.patch.object(sfv, + '_get_sfaccounts_for_tenant', + return_value=fake_sfaccounts), \ + mock.patch.object(sfv, + '_get_volumes_for_account', + return_value=fake_no_volumes): sfv.delete_volume(testvol) - self.fail("Should have thrown Error") - except Exception: - pass + + def test_delete_snapshot_no_snapshot_on_backend(self): + fake_sfaccounts = [{'accountID': 5, + 'name': 'testprjid', + 'targetSecret': 'shhhh', + 'username': 'john-wayne'}] + fake_no_volumes = [] + testsnap = {'project_id': 'testprjid', + 'name': 'no-name', + 'size': 1, + 'id': 'a720b3c0-d1f0-11e1-9b23-0800200c9a66', + 'volume_id': 'b831c4d1-d1f0-11e1-9b23-0800200c9a66', + 'created_at': timeutils.utcnow()} + + sfv = solidfire.SolidFireDriver(configuration=self.configuration) + with mock.patch.object(sfv, + '_get_sfaccounts_for_tenant', + return_value=fake_sfaccounts), \ + mock.patch.object(sfv, + '_get_volumes_for_account', + return_value=fake_no_volumes): + sfv.delete_snapshot(testsnap) def test_get_cluster_info(self): self.stubs.Set(solidfire.SolidFireDriver, diff --git a/cinder/volume/drivers/solidfire.py b/cinder/volume/drivers/solidfire.py index 2bec3bddd..97b8dd56b 100644 --- a/cinder/volume/drivers/solidfire.py +++ b/cinder/volume/drivers/solidfire.py @@ -909,63 +909,59 @@ class SolidFireDriver(san.SanISCSIDriver): return model - def delete_volume(self, vref): + def delete_volume(self, volume): """Delete SolidFire Volume from device. - SolidFire allows multiple volumes with same name, - volumeID is what's guaranteed unique. + SolidFire allows multiple volumes with same name, + volumeID is what's guaranteed unique. """ - sf_vid, sf_acctid, sf_clusterid = ( - self._parse_provider_id_string(vref['provider_id'])) - try: - params = {'volumeID': sf_vid} - self._issue_api_request('DeleteVolume', params) - except exception.SolidFireAPIException as ex: - if 'xVolumeIDDoesNotExist' in ex.mesg: - LOG.error(_LE("Volume ID %s was not found on " - "the SolidFire Cluster while attempting " - "delete_volume operation!"), sf_vid) - else: - raise + sf_vol = None + accounts = self._get_sfaccounts_for_tenant(volume['project_id']) + if accounts is None: + LOG.error(_LE("Account for Volume ID %s was not found on " + "the SolidFire Cluster while attempting " + "delete_volume operation!"), volume['id']) + LOG.error(_LE("This usually means the volume was never " + "successfully created.")) + return - def _legacy_snap_delete(self, snapshot): - vols = self._issue_api_request( - 'ListActiveVolumes', {})['result']['volumes'] - for v in vols: - if v['volumeID'].replace("UUID-", "") == snapshot['id']: - try: - self._issue_api_request('DeleteVolume', - {'volumeID': v['volumeID']}) - except exception.SolidFireAPIException as ex: - if 'xVolumeIDDoesNotExist' in ex.mesg: - LOG.error(_LE("Snapshot ID %s was not found on " - "the SolidFire Cluster while attempting " - "delete_snapshot operation!"), - snapshot['id']) - else: - raise + for acc in accounts: + vols = self._get_volumes_for_account(acc['accountID'], + volume['id']) + if vols: + sf_vol = vols[0] + break + + if sf_vol is not None: + params = {'volumeID': sf_vol['volumeID']} + self._issue_api_request('DeleteVolume', params) + else: + LOG.error(_LE("Volume ID %s was not found on " + "the SolidFire Cluster while attempting " + "delete_volume operation!"), volume['id']) def delete_snapshot(self, snapshot): """Delete the specified snapshot from the SolidFire cluster.""" - # NOTE(jdg): For snapshots, they may be clones if they're "old" - # that means the provider_id won't update on init, so it will - # be empty here and we'll have to inspect volumes - if snapshot['provider_id']: - sf_sid, sf_vid, sf_clusterid = ( - self._parse_provider_id_string(snapshot['provider_id'])) - try: - params = {'snapshotID': sf_sid} - self._issue_api_request('DeleteSnapshot', - params, - version='6.0') - except exception.SolidFireAPIException as ex: - if 'xSnapshotIDDoesNotExist' in ex.mesg: - pass - else: - LOG.debug("Snapshot provider_id not found, " - "checking old style clones.") - self._legacy_snap_delete(snapshot) + sf_snap_name = 'UUID-%s' % snapshot['id'] + accounts = self._get_sfaccounts_for_tenant(snapshot['project_id']) + snap = None + for acct in accounts: + params = {'accountID': acct['accountID']} + sf_vol = self._get_sf_volume(snapshot['volume_id'], params) + if sf_vol: + sf_snaps = self._get_sf_snapshots(sf_vol['volumeID']) + snap = next((s for s in sf_snaps if s["name"] == sf_snap_name), + None) + if snap: + params = {'snapshotID': snap['snapshotID']} + 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.") + self.delete_volume(snapshot) def create_snapshot(self, snapshot): sfaccount = self._get_sfaccount(snapshot['project_id']) -- 2.45.2