From bf2b6b9d90a59e66684a1294f536558ccafb4fd7 Mon Sep 17 00:00:00 2001 From: Eric Harney Date: Thu, 7 Jan 2016 16:25:52 -0500 Subject: [PATCH] Delete volumes with snapshots This adds the 'cascade' parameter to volume delete, which deletes snapshots along with a volume in one call. This is done in the volume manager, and not in a driver-optimized way, which will be a later improvement. Blueprint: del-vols-with-snaps Change-Id: I33d15b76d4bd0de14c635d404b2c97096c977a58 --- cinder/api/v2/volumes.py | 4 +- cinder/db/api.py | 4 ++ cinder/db/sqlalchemy/api.py | 9 ++++ .../unit/api/contrib/test_volume_unmanage.py | 2 +- cinder/tests/unit/api/v2/test_volumes.py | 3 +- cinder/tests/unit/test_volume.py | 43 +++++++++++++++++++ cinder/tests/unit/test_volume_rpcapi.py | 19 ++++++-- cinder/volume/api.py | 34 +++++++++++++-- cinder/volume/manager.py | 34 +++++++++++++-- cinder/volume/rpcapi.py | 18 ++++++-- 10 files changed, 153 insertions(+), 17 deletions(-) diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py index d7aa1c0ba..4a4d16d23 100644 --- a/cinder/api/v2/volumes.py +++ b/cinder/api/v2/volumes.py @@ -186,11 +186,13 @@ class VolumeController(wsgi.Controller): """Delete a volume.""" context = req.environ['cinder.context'] + cascade = utils.get_bool_param('cascade', req.params) + LOG.info(_LI("Delete volume with id: %s"), id, context=context) try: volume = self.volume_api.get(context, id) - self.volume_api.delete(context, volume) + self.volume_api.delete(context, volume, cascade=cascade) except exception.VolumeNotFound as error: raise exc.HTTPNotFound(explanation=error.msg) return webob.Response(status_int=202) diff --git a/cinder/db/api.py b/cinder/db/api.py index abde7c802..c0c7b1b3d 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -255,6 +255,10 @@ def volume_has_snapshots_filter(): return IMPL.volume_has_snapshots_filter() +def volume_has_undeletable_snapshots_filter(): + return IMPL.volume_has_undeletable_snapshots_filter() + + def volume_has_attachments_filter(): return IMPL.volume_has_attachments_filter() diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 407c4999c..d8bd24740 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -1831,6 +1831,15 @@ def volume_has_snapshots_filter(): ~models.Snapshot.deleted)) +def volume_has_undeletable_snapshots_filter(): + deletable_statuses = ['available', 'error'] + return sql.exists().where( + and_(models.Volume.id == models.Snapshot.volume_id, + ~models.Snapshot.deleted, + or_(models.Snapshot.cgsnapshot_id != None, # noqa: != None + models.Snapshot.status.notin_(deletable_statuses)))) + + def volume_has_attachments_filter(): return sql.exists().where( and_(models.Volume.id == models.VolumeAttachment.volume_id, diff --git a/cinder/tests/unit/api/contrib/test_volume_unmanage.py b/cinder/tests/unit/api/contrib/test_volume_unmanage.py index eb4ec9846..2fdfa29ce 100644 --- a/cinder/tests/unit/api/contrib/test_volume_unmanage.py +++ b/cinder/tests/unit/api/contrib/test_volume_unmanage.py @@ -65,7 +65,7 @@ class VolumeUnmanageTest(test.TestCase): res = self._get_resp(vol.id) self.assertEqual(202, res.status_int, res) - mock_rpcapi.assert_called_once_with(self.ctxt, mock.ANY, True) + mock_rpcapi.assert_called_once_with(self.ctxt, mock.ANY, True, False) vol = objects.volume.Volume.get_by_id(self.ctxt, vol.id) self.assertEqual('deleting', vol.status) db.volume_destroy(self.ctxt, vol.id) diff --git a/cinder/tests/unit/api/v2/test_volumes.py b/cinder/tests/unit/api/v2/test_volumes.py index 63271508c..6dafffeab 100644 --- a/cinder/tests/unit/api/v2/test_volumes.py +++ b/cinder/tests/unit/api/v2/test_volumes.py @@ -1283,7 +1283,8 @@ class VolumeApiTest(test.TestCase): self.assertEqual(202, resp.status_int) def test_volume_delete_attached(self): - def stub_volume_attached(self, context, volume, force=False): + def stub_volume_attached(self, context, volume, + force=False, cascade=False): raise exception.VolumeAttached(volume_id=volume['id']) self.stubs.Set(volume_api.API, "delete", stub_volume_attached) self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py index 2b0ca5e49..6cf65569b 100644 --- a/cinder/tests/unit/test_volume.py +++ b/cinder/tests/unit/test_volume.py @@ -4298,6 +4298,49 @@ class VolumeTestCase(BaseVolumeTestCase): None, connector) + def test_cascade_delete_volume_with_snapshots(self): + """Test volume deletion with dependent snapshots.""" + volume = tests_utils.create_volume(self.context, **self.volume_params) + self.volume.create_volume(self.context, volume['id']) + snapshot = self._create_snapshot(volume['id'], size=volume['size']) + self.volume.create_snapshot(self.context, volume['id'], snapshot) + self.assertEqual( + snapshot.id, objects.Snapshot.get_by_id(self.context, + snapshot.id).id) + + volume['status'] = 'available' + volume['host'] = 'fakehost' + + volume_api = cinder.volume.api.API() + + volume_api.delete(self.context, + volume, + cascade=True) + + def test_cascade_delete_volume_with_snapshots_error(self): + """Test volume deletion with dependent snapshots.""" + volume = tests_utils.create_volume(self.context, **self.volume_params) + self.volume.create_volume(self.context, volume['id']) + snapshot = self._create_snapshot(volume['id'], size=volume['size']) + self.volume.create_snapshot(self.context, volume['id'], snapshot) + self.assertEqual( + snapshot.id, objects.Snapshot.get_by_id(self.context, + snapshot.id).id) + + snapshot.update({'status': 'in-use'}) + snapshot.save() + + volume['status'] = 'available' + volume['host'] = 'fakehost' + + volume_api = cinder.volume.api.API() + + self.assertRaises(exception.InvalidVolume, + volume_api.delete, + self.context, + volume, + cascade=True) + @ddt.ddt class VolumeMigrationTestCase(VolumeTestCase): diff --git a/cinder/tests/unit/test_volume_rpcapi.py b/cinder/tests/unit/test_volume_rpcapi.py index aa34d7eb1..3f923cdea 100644 --- a/cinder/tests/unit/test_volume_rpcapi.py +++ b/cinder/tests/unit/test_volume_rpcapi.py @@ -291,8 +291,9 @@ class VolumeRpcAPITestCase(test.TestCase): rpc_method='cast', volume=self.fake_volume_obj, unmanage_only=False, - version='1.33') - can_send_version.assert_called_once_with('1.33') + cascade=False, + version='1.40') + can_send_version.assert_any_call('1.40') @mock.patch('oslo_messaging.RPCClient.can_send_version', return_value=False) @@ -302,7 +303,19 @@ class VolumeRpcAPITestCase(test.TestCase): volume=self.fake_volume_obj, unmanage_only=False, version='1.15') - can_send_version.assert_called_once_with('1.33') + can_send_version.assert_any_call('1.33') + + @mock.patch('oslo_messaging.RPCClient.can_send_version', + return_value=True) + def test_delete_volume_cascade(self, can_send_version): + self._test_volume_api('delete_volume', + rpc_method='cast', + volume=self.fake_volume_obj, + unmanage_only=False, + cascade=True, + version='1.40') + can_send_version.assert_any_call('1.33') + can_send_version.assert_any_call('1.40') def test_create_snapshot(self): self._test_volume_api('create_snapshot', diff --git a/cinder/volume/api.py b/cinder/volume/api.py index f6514d28c..3d3f9429c 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -329,7 +329,10 @@ class API(base.Base): return vref @wrap_check_policy - def delete(self, context, volume, force=False, unmanage_only=False): + def delete(self, context, volume, + force=False, + unmanage_only=False, + cascade=False): if context.is_admin and context.project_id != volume.project_id: project_id = volume.project_id else: @@ -374,8 +377,12 @@ class API(base.Base): expected['status'] = ('available', 'error', 'error_restoring', 'error_extending') - # Volume cannot have snapshots if we want to delete it - filters = [~db.volume_has_snapshots_filter()] + if cascade: + # Allow deletion if all snapshots are in an expected state + filters = [~db.volume_has_undeletable_snapshots_filter()] + else: + # Don't allow deletion of volume with snapshots + filters = [~db.volume_has_snapshots_filter()] values = {'status': 'deleting', 'terminated_at': timeutils.utcnow()} result = volume.conditional_update(values, expected, filters) @@ -388,6 +395,22 @@ class API(base.Base): LOG.info(msg) raise exception.InvalidVolume(reason=msg) + if cascade: + values = {'status': 'deleting'} + expected = {'status': ('available', 'error', 'deleting'), + 'cgsnapshot_id': None} + snapshots = objects.snapshot.SnapshotList.get_all_for_volume( + context, volume.id) + for s in snapshots: + result = s.conditional_update(values, expected, filters) + + if not result: + volume.update({'status': 'error_deleting'}) + volume.save() + + msg = _('Failed to update snapshot.') + raise exception.InvalidVolume(reason=msg) + cache = image_cache.ImageVolumeCache(self.db, self) entry = cache.get_by_image_volume(context, volume.id) if entry: @@ -404,7 +427,10 @@ class API(base.Base): msg = _("Unable to delete encrypted volume: %s.") % e.msg raise exception.InvalidVolume(reason=msg) - self.volume_rpcapi.delete_volume(context, volume, unmanage_only) + self.volume_rpcapi.delete_volume(context, + volume, + unmanage_only, + cascade) LOG.info(_LI("Delete volume request issued successfully."), resource=volume) diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index fdf14716e..3bb833000 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -204,7 +204,7 @@ def locked_snapshot_operation(f): class VolumeManager(manager.SchedulerDependentManager): """Manages attachable block storage devices.""" - RPC_API_VERSION = '1.39' + RPC_API_VERSION = '1.40' target = messaging.Target(version=RPC_API_VERSION) @@ -636,8 +636,10 @@ class VolumeManager(manager.SchedulerDependentManager): return vol_ref.id @locked_volume_operation - def delete_volume(self, context, volume_id, unmanage_only=False, - volume=None): + def delete_volume(self, context, volume_id, + unmanage_only=False, + volume=None, + cascade=False): """Deletes and unexports volume. 1. Delete a volume(normal case) @@ -675,6 +677,13 @@ class VolumeManager(manager.SchedulerDependentManager): raise exception.InvalidVolume( reason=_("volume is not local to this node")) + if unmanage_only and cascade: + # This could be done, but is ruled out for now just + # for simplicity. + raise exception.Invalid( + reason=_("Unmanage and cascade delete options " + "are mutually exclusive.")) + # The status 'deleting' is not included, because it only applies to # the source volume to be deleted after a migration. No quota # needs to be handled for it. @@ -693,6 +702,25 @@ class VolumeManager(manager.SchedulerDependentManager): self.driver.remove_export(context, volume) if unmanage_only: self.driver.unmanage(volume) + elif cascade: + LOG.debug('Performing cascade delete.') + snapshots = objects.SnapshotList.get_all_for_volume(context, + volume.id) + for s in snapshots: + if s.status != 'deleting': + self._clear_db(context, is_migrating_dest, volume, + 'error_deleting') + + msg = (_("Snapshot %(id)s was found in state " + "%(state)s rather than 'deleting' during " + "cascade delete.") % {'id': s.id, + 'state': s.status}) + raise exception.InvalidSnapshot(reason=msg) + + self.delete_snapshot(context, s) + + LOG.debug('Snapshots deleted, issuing volume delete') + self.driver.delete_volume(volume) else: self.driver.delete_volume(volume) except exception.VolumeIsBusy: diff --git a/cinder/volume/rpcapi.py b/cinder/volume/rpcapi.py index 2a785133c..3967f71f2 100644 --- a/cinder/volume/rpcapi.py +++ b/cinder/volume/rpcapi.py @@ -92,9 +92,10 @@ class VolumeAPI(rpc.RPCAPI): 1.38 - Scaling backup service, add get_backup_device() and secure_file_operations_enabled() 1.39 - Update replication methods to reflect new backend rep strategy + 1.40 - Add cascade option to delete_volume(). """ - RPC_API_VERSION = '1.39' + RPC_API_VERSION = '1.40' TOPIC = CONF.volume_topic BINARY = 'cinder-volume' @@ -152,13 +153,22 @@ class VolumeAPI(rpc.RPCAPI): request_spec_p = jsonutils.to_primitive(request_spec) cctxt.cast(ctxt, 'create_volume', **msg_args) - def delete_volume(self, ctxt, volume, unmanage_only=False): + def delete_volume(self, ctxt, volume, unmanage_only=False, cascade=False): msg_args = {'volume_id': volume.id, 'unmanage_only': unmanage_only} + + version = '1.15' + if self.client.can_send_version('1.33'): version = '1.33' msg_args['volume'] = volume - else: - version = '1.15' + + if self.client.can_send_version('1.40'): + version = '1.40' + if cascade: + msg_args['cascade'] = cascade + elif cascade: + msg = _('Cascade option is not supported.') + raise exception.Invalid(reason=msg) cctxt = self._get_cctxt(volume.host, version) cctxt.cast(ctxt, 'delete_volume', **msg_args)