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 <[email protected]>
Co-authored-by: Brian Rosmaita <[email protected]>

Implements: blueprint fix-snapshot-create-force
Change-Id: I6d45aeab065197a85ce62740fc95306bce9dfc45

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"
         }
     ]
 }

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"
         }
     ]
 }

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
 --------------
 

cinder/api/microversions.py

@@ -169,6 +169,8 @@
 
 USE_QUOTA = '3.65'
 
+SNAPSHOT_IN_USE = '3.66'
+
 
 def get_mv_header(version):
     """Gets a formatted HTTP microversion header.

cinder/api/openstack/api_version_request.py

@@ -151,14 +151,15 @@
     * 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

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.

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 @@ def _items(self, req, is_detail=True):
                                                         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))

cinder/tests/unit/api/v3/test_snapshots.py

@@ -17,6 +17,7 @@
 
 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 @@ def test_snapshot_list_with_metadata_unsupported_microversion(
 
         # 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])

cinder/tests/unit/volume/test_snapshot.py

@@ -335,7 +335,7 @@ def test_create_snapshot_failed_host_is_None(self):
     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 @@ def test_create_snapshot_force(self):
         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 @@ def test_create_snapshot_force(self):
         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."""

cinder/volume/api.py

@@ -850,12 +850,13 @@ def _create_snapshot(self, context,
                          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 @@ def create_snapshot_in_db(self, context,
                               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 @@ def _create_snapshot_in_db_options(self, context, volume,
     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

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.