From 9ed7c16ef0c8b190cb727604467819cdf23531b0 Mon Sep 17 00:00:00 2001 From: Eric Harney Date: Mon, 3 May 2021 15:49:38 +0000 Subject: [PATCH] Snapshot in-use volumes without force flag Introduces API microversion 3.66, which allows snapshot creation on in-use volumes without the force flag being passed. Co-authored-by: Eric Harney Co-authored-by: Brian Rosmaita Implements: blueprint fix-snapshot-create-force Change-Id: I6d45aeab065197a85ce62740fc95306bce9dfc45 --- .../versions/version-show-response.json | 4 +- .../samples/versions/versions-response.json | 4 +- api-ref/source/v3/volumes-v3-snapshots.inc | 6 ++ cinder/api/microversions.py | 2 + cinder/api/openstack/api_version_request.py | 5 +- .../openstack/rest_api_version_history.rst | 7 +++ cinder/api/v3/snapshots.py | 63 +++++++++++++++++++ cinder/tests/unit/api/v3/test_snapshots.py | 54 ++++++++++++++++ cinder/tests/unit/volume/test_snapshot.py | 55 +++++++++++++++- cinder/volume/api.py | 17 +++-- ...in-use-without-force-86c6d74ebc9c0d60.yaml | 8 +++ 11 files changed, 212 insertions(+), 13 deletions(-) create mode 100644 releasenotes/notes/snapshot-in-use-without-force-86c6d74ebc9c0d60.yaml diff --git a/api-ref/source/v3/samples/versions/version-show-response.json b/api-ref/source/v3/samples/versions/version-show-response.json index 1d79bf38f19..63d41fad4a9 100644 --- a/api-ref/source/v3/samples/versions/version-show-response.json +++ b/api-ref/source/v3/samples/versions/version-show-response.json @@ -21,8 +21,8 @@ ], "min_version": "3.0", "status": "CURRENT", - "updated": "2021-08-25T00:00:00Z", - "version": "3.65" + "updated": "2021-09-16T00:00:00Z", + "version": "3.66" } ] } diff --git a/api-ref/source/v3/samples/versions/versions-response.json b/api-ref/source/v3/samples/versions/versions-response.json index dd7b843d105..92bfafc5b98 100644 --- a/api-ref/source/v3/samples/versions/versions-response.json +++ b/api-ref/source/v3/samples/versions/versions-response.json @@ -21,8 +21,8 @@ ], "min_version": "3.0", "status": "CURRENT", - "updated": "2021-08-25T00:00:00Z", - "version": "3.65" + "updated": "2021-09-16T00:00:00Z", + "version": "3.66" } ] } diff --git a/api-ref/source/v3/volumes-v3-snapshots.inc b/api-ref/source/v3/volumes-v3-snapshots.inc index f3d6608577f..cede89a4210 100644 --- a/api-ref/source/v3/volumes-v3-snapshots.inc +++ b/api-ref/source/v3/volumes-v3-snapshots.inc @@ -107,6 +107,12 @@ Create a snapshot Creates a volume snapshot, which is a point-in-time, complete copy of a volume. You can create a volume from a snapshot. +Prior to API version 3.66, a 'force' flag was required to create a snapshot of +an in-use volume, but this is no longer the case. From API version 3.66, the +'force' flag is invalid when passed in a volume snapshot request. (For +backward compatibility, however, a 'force' flag with a value evaluating to +True is silently ignored.) + Response codes -------------- diff --git a/cinder/api/microversions.py b/cinder/api/microversions.py index 909dcd875fb..0c06e6c9c0a 100644 --- a/cinder/api/microversions.py +++ b/cinder/api/microversions.py @@ -169,6 +169,8 @@ ENCRYPTION_KEY_ID_IN_DETAILS = '3.64' USE_QUOTA = '3.65' +SNAPSHOT_IN_USE = '3.66' + def get_mv_header(version): """Gets a formatted HTTP microversion header. diff --git a/cinder/api/openstack/api_version_request.py b/cinder/api/openstack/api_version_request.py index 7f0769e9b19..183e904bf21 100644 --- a/cinder/api/openstack/api_version_request.py +++ b/cinder/api/openstack/api_version_request.py @@ -151,14 +151,15 @@ REST_API_VERSION_HISTORY = """ * 3.65 - Include 'consumes_quota' in volume and snapshot details - Accept 'consumes_quota' filter in volume and snapshot list operation. + * 3.66 - Allow snapshotting in-use volumes without force flag. """ # The minimum and maximum versions of the API supported # The default api version request is defined to be the # minimum version of the API supported. _MIN_API_VERSION = "3.0" -_MAX_API_VERSION = "3.65" -UPDATED = "2021-08-25T00:00:00Z" +_MAX_API_VERSION = "3.66" +UPDATED = "2021-09-16T00:00:00Z" # NOTE(cyeoh): min and max versions declared as functions so we can diff --git a/cinder/api/openstack/rest_api_version_history.rst b/cinder/api/openstack/rest_api_version_history.rst index 9344b282a61..666771a640c 100644 --- a/cinder/api/openstack/rest_api_version_history.rst +++ b/cinder/api/openstack/rest_api_version_history.rst @@ -498,3 +498,10 @@ whether the resource is consuming quota or not. Also, accept a ``consumes_quota`` filter, which takes a boolean value, in the volume and snapshot list requests. (The default listing behavior is not to use this filter.) + +3.66 (Maximum in Xena) +---------------------- +Volume snapshots of in-use volumes can be created without the 'force' flag. +Although the 'force' flag is now considered invalid when passed in a volume +snapshot request, for backward compatibility, the 'force' flag with a value +evaluating to True is silently ignored. diff --git a/cinder/api/v3/snapshots.py b/cinder/api/v3/snapshots.py index d5c1a47ba9e..b4494112cac 100644 --- a/cinder/api/v3/snapshots.py +++ b/cinder/api/v3/snapshots.py @@ -16,19 +16,30 @@ """The volumes snapshots V3 API.""" import ast +from http import HTTPStatus from oslo_log import log as logging +from oslo_utils import strutils +from webob import exc from cinder.api import api_utils from cinder.api import common from cinder.api import microversions as mv from cinder.api.openstack import wsgi +from cinder.api.schemas import snapshots as snapshot from cinder.api.v2 import snapshots as snapshots_v2 from cinder.api.v3.views import snapshots as snapshot_views +from cinder.api import validation from cinder import utils LOG = logging.getLogger(__name__) +SNAPSHOT_IN_USE_FLAG_MSG = ( + f"Since microversion {mv.SNAPSHOT_IN_USE} the 'force' flag is " + "invalid for this request. For backward compatability, however, when " + "the 'force' flag is passed with a value evaluating to True, it is " + "silently ignored.") + class SnapshotsController(snapshots_v2.SnapshotsController): """The Snapshots API controller for the OpenStack API.""" @@ -131,6 +142,58 @@ class SnapshotsController(snapshots_v2.SnapshotsController): total_count) return snapshots + @wsgi.response(HTTPStatus.ACCEPTED) + @validation.schema(snapshot.create) + def create(self, req, body): + """Creates a new snapshot.""" + kwargs = {} + context = req.environ['cinder.context'] + snapshot = body['snapshot'] + kwargs['metadata'] = snapshot.get('metadata', None) + volume_id = snapshot['volume_id'] + volume = self.volume_api.get(context, volume_id) + req_version = req.api_version_request + force_flag = snapshot.get('force') + force = False + if force_flag is not None: + # note: this won't raise because it passed schema validation + force = strutils.bool_from_string(force_flag, strict=True) + + if req_version.matches(mv.SNAPSHOT_IN_USE): + # strictly speaking, the 'force' flag is invalid for + # mv.SNAPSHOT_IN_USE, but we silently ignore a True + # value for backward compatibility + if force is False: + raise exc.HTTPBadRequest( + explanation=SNAPSHOT_IN_USE_FLAG_MSG) + + LOG.info("Create snapshot from volume %s", volume_id) + + self.validate_name_and_description(snapshot, check_length=False) + if 'name' in snapshot: + snapshot['display_name'] = snapshot.pop('name') + + if force: + new_snapshot = self.volume_api.create_snapshot_force( + context, + volume, + snapshot.get('display_name'), + snapshot.get('description'), + **kwargs) + else: + if req_version.matches(mv.SNAPSHOT_IN_USE): + kwargs['allow_in_use'] = True + + new_snapshot = self.volume_api.create_snapshot( + context, + volume, + snapshot.get('display_name'), + snapshot.get('description'), + **kwargs) + req.cache_db_snapshot(new_snapshot) + + return self._view_builder.detail(req, new_snapshot) + def create_resource(ext_mgr): return wsgi.Resource(SnapshotsController(ext_mgr)) diff --git a/cinder/tests/unit/api/v3/test_snapshots.py b/cinder/tests/unit/api/v3/test_snapshots.py index 473ae5b89be..3f205edc95a 100644 --- a/cinder/tests/unit/api/v3/test_snapshots.py +++ b/cinder/tests/unit/api/v3/test_snapshots.py @@ -17,6 +17,7 @@ from unittest import mock import ddt from oslo_utils import strutils +from webob import exc from cinder.api import microversions as mv from cinder.api.v3 import snapshots @@ -400,3 +401,56 @@ class SnapshotApiTest(test.TestCase): # verify some snapshot is returned self.assertNotEqual(0, len(res_dict['snapshots'])) + + @mock.patch('cinder.volume.api.API.create_snapshot') + def test_snapshot_create_allow_in_use(self, mock_create): + req = create_snapshot_query_with_metadata( + '{"key2": "val2"}', + mv.SNAPSHOT_IN_USE) + + body = {'snapshot': {'volume_id': fake.VOLUME_ID}} + + self.controller.create(req, body=body) + self.assertIn('allow_in_use', mock_create.call_args_list[0][1]) + self.assertTrue(mock_create.call_args_list[0][1]['allow_in_use']) + + @mock.patch('cinder.volume.api.API.create_snapshot') + def test_snapshot_create_allow_in_use_negative(self, mock_create): + req = create_snapshot_query_with_metadata( + '{"key2": "val2"}', + mv.get_prior_version(mv.SNAPSHOT_IN_USE)) + + body = {'snapshot': {'volume_id': fake.VOLUME_ID}} + + self.controller.create(req, body=body) + self.assertNotIn('allow_in_use', mock_create.call_args_list[0][1]) + + @ddt.data(False, 'false', 'f', 'no', 'n', '0', 'off') + @mock.patch('cinder.volume.api.API.create_snapshot') + def test_snapshot_create_force_false(self, force_flag, mock_create): + snapshot_name = 'Snapshot Test Name' + snapshot_description = 'Snapshot Test Desc' + snapshot = { + "volume_id": fake.VOLUME_ID, + "force": force_flag, + "name": snapshot_name, + "description": snapshot_description + } + + body = dict(snapshot=snapshot) + req = create_snapshot_query_with_metadata( + '{"key2": "val2"}', + mv.SNAPSHOT_IN_USE) + self.assertRaises(exc.HTTPBadRequest, + self.controller.create, + req, + body=body) + mock_create.assert_not_called() + + # prevent regression -- shouldn't raise for pre-mv-3.66 + req = create_snapshot_query_with_metadata( + '{"key2": "val2"}', + mv.get_prior_version(mv.SNAPSHOT_IN_USE)) + self.controller.create(req, body=body) + # ... but also shouldn't allow an in-use snapshot + self.assertNotIn('allow_in_use', mock_create.call_args_list[0][1]) diff --git a/cinder/tests/unit/volume/test_snapshot.py b/cinder/tests/unit/volume/test_snapshot.py index 9f6129ad915..caa8bdb41e5 100644 --- a/cinder/tests/unit/volume/test_snapshot.py +++ b/cinder/tests/unit/volume/test_snapshot.py @@ -335,7 +335,7 @@ class SnapshotTestCase(base.BaseVolumeTestCase): def test_create_snapshot_force(self): """Test snapshot in use can be created forcibly.""" - instance_uuid = '12345678-1234-5678-1234-567812345678' + instance_uuid = '12345678-1234-4678-1234-567812345678' # create volume and attach to the instance volume = tests_utils.create_volume(self.context, **self.volume_params) self.volume.create_volume(self.context, volume) @@ -359,6 +359,7 @@ class SnapshotTestCase(base.BaseVolumeTestCase): snapshot_ref.destroy() db.volume_destroy(self.context, volume['id']) + def test_create_snapshot_force_host(self): # create volume and attach to the host volume = tests_utils.create_volume(self.context, **self.volume_params) self.volume.create_volume(self.context, volume) @@ -382,6 +383,58 @@ class SnapshotTestCase(base.BaseVolumeTestCase): snapshot_ref.destroy() db.volume_destroy(self.context, volume['id']) + def test_create_snapshot_in_use(self): + """Test snapshot in use can be created forcibly.""" + + instance_uuid = 'a14dc210-d43b-4792-a608-09fe0824de54' + # create volume and attach to the instance + volume = tests_utils.create_volume(self.context, **self.volume_params) + self.volume.create_volume(self.context, volume) + values = {'volume_id': volume['id'], + 'instance_uuid': instance_uuid, + 'attach_status': fields.VolumeAttachStatus.ATTACHING, } + attachment = db.volume_attach(self.context, values) + db.volume_attached(self.context, attachment['id'], instance_uuid, + None, '/dev/sda1') + + volume_api = cinder.volume.api.API() + volume = volume_api.get(self.context, volume['id']) + self.assertRaises(exception.InvalidVolume, + volume_api.create_snapshot, + self.context, volume, + 'fake_name', 'fake_description') + snapshot_ref = volume_api.create_snapshot(self.context, + volume, + 'fake_name', + 'fake_description', + allow_in_use=True) + snapshot_ref.destroy() + db.volume_destroy(self.context, volume['id']) + + # create volume and attach to the host + volume = tests_utils.create_volume(self.context, **self.volume_params) + self.volume.create_volume(self.context, volume) + values = {'volume_id': volume['id'], + 'attached_host': 'fake_host', + 'attach_status': fields.VolumeAttachStatus.ATTACHING, } + attachment = db.volume_attach(self.context, values) + db.volume_attached(self.context, attachment['id'], None, + 'fake_host', '/dev/sda1') + + volume_api = cinder.volume.api.API() + volume = volume_api.get(self.context, volume['id']) + self.assertRaises(exception.InvalidVolume, + volume_api.create_snapshot, + self.context, volume, + 'fake_name', 'fake_description') + snapshot_ref = volume_api.create_snapshot(self.context, + volume, + 'fake_name', + 'fake_description', + allow_in_use=True) + snapshot_ref.destroy() + db.volume_destroy(self.context, volume['id']) + @mock.patch('cinder.image.image_utils.qemu_img_info') def test_create_snapshot_from_bootable_volume(self, mock_qemu_info): """Test create snapshot from bootable volume.""" diff --git a/cinder/volume/api.py b/cinder/volume/api.py index ae2142756d3..8482c4d210f 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -850,12 +850,13 @@ class API(base.Base): volume, name, description, force=False, metadata=None, cgsnapshot_id=None, - group_snapshot_id=None): + group_snapshot_id=None, + allow_in_use=False): volume.assert_not_frozen() snapshot = self.create_snapshot_in_db( context, volume, name, description, force, metadata, cgsnapshot_id, - True, group_snapshot_id) + True, group_snapshot_id, allow_in_use) # NOTE(tommylikehu): We only wrap the 'size' attribute here # because only the volume's host is passed and only capacity is # validated in the scheduler now. @@ -872,12 +873,15 @@ class API(base.Base): force, metadata, cgsnapshot_id, commit_quota=True, - group_snapshot_id=None): + group_snapshot_id=None, + allow_in_use=False): self._create_snapshot_in_db_validate(context, volume) utils.check_metadata_properties(metadata) - valid_status = ["available", "in-use"] if force else ["available"] + valid_status = ('available',) + if force or allow_in_use: + valid_status = ('available', 'in-use') if volume['status'] not in valid_status: msg = _("Volume %(vol_id)s status must be %(status)s, " @@ -1063,10 +1067,11 @@ class API(base.Base): def create_snapshot(self, context, volume, name, description, metadata=None, cgsnapshot_id=None, - group_snapshot_id=None): + group_snapshot_id=None, + allow_in_use=False): result = self._create_snapshot(context, volume, name, description, False, metadata, cgsnapshot_id, - group_snapshot_id) + group_snapshot_id, allow_in_use) LOG.info("Snapshot create request issued successfully.", resource=result) return result diff --git a/releasenotes/notes/snapshot-in-use-without-force-86c6d74ebc9c0d60.yaml b/releasenotes/notes/snapshot-in-use-without-force-86c6d74ebc9c0d60.yaml new file mode 100644 index 00000000000..01283dc8d30 --- /dev/null +++ b/releasenotes/notes/snapshot-in-use-without-force-86c6d74ebc9c0d60.yaml @@ -0,0 +1,8 @@ +--- +features: + - | + As of API version 3.66, volume snapshots of in-use volumes can be created + without passing the 'force' flag, and the 'force' flag is considered + invalid for this request. For backward compatability, however, when the + 'force' flag is passed with a value evaluating to True, it is silently + ignored.