From 4d454f6eb1d3948e9c33563dce2f12b69a1b7392 Mon Sep 17 00:00:00 2001 From: Eric Harney Date: Tue, 5 Jul 2016 13:44:04 -0400 Subject: [PATCH] Cascade + force volume delete parameters Allow volume delete to take parameters "cascade", or "force", or both. A new policy field, "volume:force_delete" is added with the default of "rule:admin_api". Implements: blueprint volume-delete-parameters APIImpact: New parameters to volume delete Change-Id: Ic47cfcf1cc7d172d7f9d5b093233035f797982f5 --- cinder/api/openstack/api_version_request.py | 3 +- cinder/api/v3/volumes.py | 42 +++++++++++++++++++ cinder/db/api.py | 4 ++ cinder/db/sqlalchemy/api.py | 6 +++ cinder/tests/unit/policy.json | 1 + cinder/tests/unit/test_volume.py | 20 +++++++++ cinder/volume/api.py | 15 +++++-- etc/cinder/policy.json | 1 + .../delete_parameters-6f44fece22a7787d.yaml | 13 ++++++ 9 files changed, 100 insertions(+), 5 deletions(-) create mode 100644 releasenotes/notes/delete_parameters-6f44fece22a7787d.yaml diff --git a/cinder/api/openstack/api_version_request.py b/cinder/api/openstack/api_version_request.py index c14e72d0eb1..b10d274659b 100644 --- a/cinder/api/openstack/api_version_request.py +++ b/cinder/api/openstack/api_version_request.py @@ -72,6 +72,7 @@ REST_API_VERSION_HISTORY = """ volume group. * 3.21 - Show provider_id in detailed view of a volume for admin. * 3.22 - Add filtering based on metadata for snapshot listing. + * 3.23 - Allow passing force parameter to volume delete. """ # The minimum and maximum versions of the API supported @@ -79,7 +80,7 @@ REST_API_VERSION_HISTORY = """ # minimum version of the API supported. # Explicitly using /v1 or /v2 enpoints will still work _MIN_API_VERSION = "3.0" -_MAX_API_VERSION = "3.22" +_MAX_API_VERSION = "3.23" _LEGACY_API_VERSION1 = "1.0" _LEGACY_API_VERSION2 = "2.0" diff --git a/cinder/api/v3/volumes.py b/cinder/api/v3/volumes.py index 94b78e12e66..dec83abf15b 100644 --- a/cinder/api/v3/volumes.py +++ b/cinder/api/v3/volumes.py @@ -15,6 +15,7 @@ from oslo_log import log as logging from oslo_utils import uuidutils +import webob from webob import exc from cinder.api import common @@ -24,6 +25,7 @@ from cinder.api.v3.views import volumes as volume_views_v3 from cinder import exception from cinder import group as group_api from cinder.i18n import _, _LI +import cinder.policy from cinder import utils from cinder.volume import volume_types @@ -32,6 +34,17 @@ LOG = logging.getLogger(__name__) SUMMARY_BASE_MICRO_VERSION = '3.12' +def check_policy(context, action, target_obj=None): + target = { + 'project_id': context.project_id, + 'user_id': context.user_id + } + target.update(target_obj or {}) + + _action = 'volume:%s' % action + cinder.policy.enforce(context, _action, target) + + class VolumeController(volumes_v2.VolumeController): """The Volumes API controller for the OpenStack API V3.""" @@ -41,6 +54,35 @@ class VolumeController(volumes_v2.VolumeController): self.group_api = group_api.API() super(VolumeController, self).__init__(ext_mgr) + def delete(self, req, id): + """Delete a volume.""" + context = req.environ['cinder.context'] + req_version = req.api_version_request + + cascade = utils.get_bool_param('cascade', req.params) + force = False + + params = "" + if req_version.matches('3.23'): + force = utils.get_bool_param('force', req.params) + if cascade or force: + params = "(cascade: %(c)s, force: %(f)s)" % {'c': cascade, + 'f': force} + + msg = _LI("Delete volume with id: %(id)s %(params)s") + LOG.info(msg, {'id': id, 'params': params}, context=context) + + if force: + check_policy(context, 'force_delete') + + volume = self.volume_api.get(context, id) + + self.volume_api.delete(context, volume, + cascade=cascade, + force=force) + + return webob.Response(status_int=202) + def _get_volumes(self, req, is_detail): """Returns a list of volumes, transformed through view builder.""" diff --git a/cinder/db/api.py b/cinder/db/api.py index 291031be31e..82d077a30fa 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -387,6 +387,10 @@ def volume_has_undeletable_snapshots_filter(): return IMPL.volume_has_undeletable_snapshots_filter() +def volume_has_snapshots_in_a_cgsnapshot_filter(): + return IMPL.volume_has_snapshots_in_a_cgsnapshot_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 48fbdf94843..363caf1a28d 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -2384,6 +2384,12 @@ def volume_has_undeletable_snapshots_filter(): models.Snapshot.status.notin_(deletable_statuses)))) +def volume_has_snapshots_in_a_cgsnapshot_filter(): + return sql.exists().where( + and_(models.Volume.id == models.Snapshot.volume_id, + models.Snapshot.cgsnapshot_id.isnot(None))) + + def volume_has_attachments_filter(): return sql.exists().where( and_(models.Volume.id == models.VolumeAttachment.volume_id, diff --git a/cinder/tests/unit/policy.json b/cinder/tests/unit/policy.json index 0179e1cc331..ad8766d5fd3 100644 --- a/cinder/tests/unit/policy.json +++ b/cinder/tests/unit/policy.json @@ -14,6 +14,7 @@ "volume:get_volume_admin_metadata": "rule:admin_api", "volume:update_volume_admin_metadata": "rule:admin_api", "volume:delete": "", + "volume:force_delete": "rule:admin_api", "volume:update": "", "volume:attach": "", "volume:detach": "", diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py index 87e3c6c740d..6b2797daea9 100644 --- a/cinder/tests/unit/test_volume.py +++ b/cinder/tests/unit/test_volume.py @@ -4507,6 +4507,26 @@ class VolumeTestCase(base.BaseVolumeTestCase): volume, cascade=True) + def test_cascade_force_delete_volume_with_snapshots_error(self): + """Test volume force deletion with errored dependent snapshots.""" + volume = tests_utils.create_volume(self.context, + host='fakehost') + + snapshot = create_snapshot(volume.id, + size=volume.size, + status=fields.SnapshotStatus.ERROR_DELETING) + self.volume.create_snapshot(self.context, snapshot) + + volume_api = cinder.volume.api.API() + + volume_api.delete(self.context, volume, cascade=True, force=True) + + snapshot = objects.Snapshot.get_by_id(self.context, snapshot.id) + self.assertEqual('deleting', snapshot.status) + + volume = objects.Volume.get_by_id(self.context, volume.id) + self.assertEqual('deleting', volume.status) + @mock.patch.object(fake_driver.FakeLoggingVolumeDriver, 'get_volume_stats') @mock.patch.object(driver.BaseVD, '_init_vendor_properties') def test_get_capabilities(self, mock_init_vendor, mock_get_volume_stats): diff --git a/cinder/volume/api.py b/cinder/volume/api.py index a18f9ac55cc..0ff334aebcf 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -406,8 +406,13 @@ class API(base.Base): 'error_extending', 'error_managing') if cascade: - # Allow deletion if all snapshots are in an expected state - filters = [~db.volume_has_undeletable_snapshots_filter()] + if force: + # Ignore status checks, but ensure snapshots are not part + # of a cgsnapshot. + filters = [~db.volume_has_snapshots_in_a_cgsnapshot_filter()] + else: + # 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()] @@ -429,9 +434,11 @@ class API(base.Base): if cascade: values = {'status': 'deleting'} - expected = {'status': ('available', 'error', 'deleting'), - 'cgsnapshot_id': None, + expected = {'cgsnapshot_id': None, 'group_snapshot_id': None} + if not force: + expected['status'] = ('available', 'error', 'deleting') + snapshots = objects.snapshot.SnapshotList.get_all_for_volume( context, volume.id) for s in snapshots: diff --git a/etc/cinder/policy.json b/etc/cinder/policy.json index 26e90eab991..6a6cb4bed81 100644 --- a/etc/cinder/policy.json +++ b/etc/cinder/policy.json @@ -6,6 +6,7 @@ "volume:create": "", "volume:delete": "rule:admin_or_owner", + "volume:force_delete": "rule:admin_api", "volume:get": "rule:admin_or_owner", "volume:get_all": "rule:admin_or_owner", "volume:get_volume_metadata": "rule:admin_or_owner", diff --git a/releasenotes/notes/delete_parameters-6f44fece22a7787d.yaml b/releasenotes/notes/delete_parameters-6f44fece22a7787d.yaml new file mode 100644 index 00000000000..a94e432de9b --- /dev/null +++ b/releasenotes/notes/delete_parameters-6f44fece22a7787d.yaml @@ -0,0 +1,13 @@ +--- +features: + - The ``force`` boolean parameter has been added to the volume + delete API. It may be used in combination with ``cascade``. + This also means that volume force delete is available + in the base volume API rather than only in the + ``volume_admin_actions`` extension. +upgrade: + - There is a new policy option ``volume:force_delete`` which + controls access to the ability to specify force delete + via the volume delete API. This is separate from the + pre-existing ``volume-admin-actions:force_delete`` policy + check.