Support Block Storage API mv 3.66

Block Storage API mv 3.66 enables snapshots of in-use volumes
without requiring a 'force' flag.  For backward compatibility,
the API silently accepts force=true, even though the 'force' flag
is considered invalid for that call.  That behavior is replicated
in the client, where --force with a true value is silently accepted.
The --force option is not advertised in the shell and an option
value that doesn't evaluate to true raises an UnsupportedAttribute
error.  Similar behavior from the v3 Snapshot class, except it
raises a ValueError under similar circumstances.

Change-Id: I7408d0e3a5ed7f4cbcaf65cf3434ad60aaed511d

6 files changed, 254 insertions, 57 deletions

diff --git a/cinderclient/api_versions.py b/cinderclient/api_versions.py
index 78bb8a0..48e00b7 100644
--- a/cinderclient/api_versions.py
+++ b/cinderclient/api_versions.py
@@ -26,7 +26,7 @@ LOG = logging.getLogger(__name__)
 
 # key is unsupported version, value is appropriate supported alternative
 REPLACEMENT_VERSIONS = {"1": "3", "2": "3"}
-MAX_VERSION = "3.65"
+MAX_VERSION = "3.66"
 MIN_VERSION = "3.0"
 
 _SUBSTITUTIONS = {}
diff --git a/cinderclient/tests/unit/v3/test_shell.py b/cinderclient/tests/unit/v3/test_shell.py
index 9a6852f..8f00525 100644
--- a/cinderclient/tests/unit/v3/test_shell.py
+++ b/cinderclient/tests/unit/v3/test_shell.py
@@ -900,6 +900,67 @@ class ShellTest(utils.TestCase):
                          'manageable-list --cluster dest')
         self.assert_called('GET', '/manageable_volumes/detail?cluster=dest')
 
+    @ddt.data(True, False, 'Nonboolean')
+    @mock.patch('cinderclient.utils.find_resource')
+    def test_snapshot_create_pre_3_66(self, force_value, mock_find_vol):
+        mock_find_vol.return_value = volumes.Volume(
+            self, {'id': '123456'}, loaded=True)
+        snap_body_3_65 = {
+            'snapshot': {
+                'volume_id': '123456',
+                'force': f'{force_value}',
+                'name': None,
+                'description': None,
+                'metadata': {}
+            }
+        }
+        self.run_command('--os-volume-api-version 3.65 '
+                         f'snapshot-create --force {force_value} 123456')
+        self.assert_called_anytime('POST', '/snapshots', body=snap_body_3_65)
+
+    SNAP_BODY_3_66 = {
+        'snapshot': {
+            'volume_id': '123456',
+            'name': None,
+            'description': None,
+            'metadata': {}
+        }
+    }
+
+    @ddt.data(True, 'true', 'on', '1')
+    @mock.patch('cinderclient.utils.find_resource')
+    def test_snapshot_create_3_66_with_force_true(self, f_val, mock_find_vol):
+        mock_find_vol.return_value = volumes.Volume(
+            self, {'id': '123456'}, loaded=True)
+        mock_find_vol.return_value = volumes.Volume(self,
+                                                    {'id': '123456'},
+                                                    loaded=True)
+        self.run_command('--os-volume-api-version 3.66 '
+                         f'snapshot-create --force {f_val} 123456')
+        self.assert_called_anytime('POST', '/snapshots',
+                                   body=self.SNAP_BODY_3_66)
+
+    @ddt.data(False, 'false', 'no', '0', 'whatever')
+    @mock.patch('cinderclient.utils.find_resource')
+    def test_snapshot_create_3_66_with_force_not_true(
+            self, f_val, mock_find_vol):
+        mock_find_vol.return_value = volumes.Volume(
+            self, {'id': '123456'}, loaded=True)
+        uae = self.assertRaises(exceptions.UnsupportedAttribute,
+                                self.run_command,
+                                '--os-volume-api-version 3.66 '
+                                f'snapshot-create --force {f_val} 123456')
+        self.assertIn('not allowed after microversion 3.65', str(uae))
+
+    @mock.patch('cinderclient.utils.find_resource')
+    def test_snapshot_create_3_66(self, mock_find_vol):
+        mock_find_vol.return_value = volumes.Volume(
+            self, {'id': '123456'}, loaded=True)
+        self.run_command('--os-volume-api-version 3.66 '
+                         'snapshot-create 123456')
+        self.assert_called_anytime('POST', '/snapshots',
+                                   body=self.SNAP_BODY_3_66)
+
     def test_snapshot_manageable_list(self):
         self.run_command('--os-volume-api-version 3.8 '
                          'snapshot-manageable-list fakehost')
diff --git a/cinderclient/v3/shell.py b/cinderclient/v3/shell.py
index 195d859..18f94d3 100644
--- a/cinderclient/v3/shell.py
+++ b/cinderclient/v3/shell.py
@@ -2193,6 +2193,144 @@ def do_snapshot_list(cs, args):
     AppendFilters.filters = []
 
 
+@api_versions.wraps("3.0", "3.65")
+@utils.arg('volume',
+           metavar='<volume>',
+           help='Name or ID of volume to snapshot.')
+@utils.arg('--force',
+           metavar='<True|False>',
+           const=True,
+           nargs='?',
+           default=False,
+           end_version='3.65',
+           help='Allows or disallows snapshot of '
+           'a volume when the volume is attached to an instance. '
+           'If set to True, ignores the current status of the '
+           'volume when attempting to snapshot it rather '
+           'than forcing it to be available. From microversion 3.66, '
+           'all snapshots are "forced" and this option is invalid. '
+           'Default=False.')
+@utils.arg('--force',
+           metavar='<True>',
+           nargs='?',
+           default=None,
+           start_version='3.66',
+           help=argparse.SUPPRESS)
+@utils.arg('--name',
+           metavar='<name>',
+           default=None,
+           help='Snapshot name. Default=None.')
+@utils.arg('--display-name',
+           help=argparse.SUPPRESS)
+@utils.arg('--display_name',
+           help=argparse.SUPPRESS)
+@utils.arg('--description',
+           metavar='<description>',
+           default=None,
+           help='Snapshot description. Default=None.')
+@utils.arg('--display-description',
+           help=argparse.SUPPRESS)
+@utils.arg('--display_description',
+           help=argparse.SUPPRESS)
+@utils.arg('--metadata',
+           nargs='*',
+           metavar='<key=value>',
+           default=None,
+           help='Snapshot metadata key and value pairs. Default=None.')
+def do_snapshot_create(cs, args):
+    """Creates a snapshot."""
+    if args.display_name is not None:
+        args.name = args.display_name
+
+    if args.display_description is not None:
+        args.description = args.display_description
+
+    snapshot_metadata = None
+    if args.metadata is not None:
+        snapshot_metadata = shell_utils.extract_metadata(args)
+
+    volume = utils.find_volume(cs, args.volume)
+    snapshot = cs.volume_snapshots.create(volume.id,
+                                          args.force,
+                                          args.name,
+                                          args.description,
+                                          metadata=snapshot_metadata)
+    shell_utils.print_volume_snapshot(snapshot)
+
+
+@api_versions.wraps("3.66")
+@utils.arg('volume',
+           metavar='<volume>',
+           help='Name or ID of volume to snapshot.')
+@utils.arg('--force',
+           nargs='?',
+           help=argparse.SUPPRESS)
+@utils.arg('--name',
+           metavar='<name>',
+           default=None,
+           help='Snapshot name. Default=None.')
+@utils.arg('--display-name',
+           help=argparse.SUPPRESS)
+@utils.arg('--display_name',
+           help=argparse.SUPPRESS)
+@utils.arg('--description',
+           metavar='<description>',
+           default=None,
+           help='Snapshot description. Default=None.')
+@utils.arg('--display-description',
+           help=argparse.SUPPRESS)
+@utils.arg('--display_description',
+           help=argparse.SUPPRESS)
+@utils.arg('--metadata',
+           nargs='*',
+           metavar='<key=value>',
+           default=None,
+           help='Snapshot metadata key and value pairs. Default=None.')
+def do_snapshot_create(cs, args):  # noqa: F811
+    """Creates a snapshot."""
+
+    # TODO(rosmaita): we really need to look into removing this v1
+    # compatibility code and the v1 options entirely.  Note that if you
+    # include the --name and also --display_name, the latter will be used.
+    # Not sure that's desirable, but it is consistent with all the other
+    # functions in this file, so we'll do it here too.
+    if args.display_name is not None:
+        args.name = args.display_name
+    if args.display_description is not None:
+        args.description = args.display_description
+
+    snapshot_metadata = None
+    if args.metadata is not None:
+        snapshot_metadata = shell_utils.extract_metadata(args)
+
+    force = getattr(args, 'force', None)
+
+    volume = utils.find_volume(cs, args.volume)
+
+    # this is a little weird, but for consistency with the API we
+    # will silently ignore the --force option when it's passed with
+    # a value that evaluates to True; otherwise, we report that the
+    # --force option is illegal for this call
+    try:
+        snapshot = cs.volume_snapshots.create(volume.id,
+                                              force=force,
+                                              name=args.name,
+                                              description=args.description,
+                                              metadata=snapshot_metadata)
+    except ValueError as ve:
+        # make sure it's the exception we expect
+        em = cinderclient.v3.volume_snapshots.MV_3_66_FORCE_FLAG_ERROR
+        if em == str(ve):
+            raise exceptions.UnsupportedAttribute(
+                'force',
+                start_version=None,
+                end_version=api_versions.APIVersion('3.65'))
+        else:
+            raise
+
+    shell_utils.print_volume_snapshot(snapshot)
+
+
 @api_versions.wraps('3.27')
 @utils.arg('--all-tenants',
            dest='all_tenants',
diff --git a/cinderclient/v3/shell_base.py b/cinderclient/v3/shell_base.py
index 4dc6da9..0fc1b5d 100644
--- a/cinderclient/v3/shell_base.py
+++ b/cinderclient/v3/shell_base.py
@@ -595,62 +595,6 @@ def do_snapshot_show(cs, args):
     shell_utils.print_volume_snapshot(snapshot)
 
 
-@utils.arg('volume',
-           metavar='<volume>',
-           help='Name or ID of volume to snapshot.')
-@utils.arg('--force',
-           metavar='<True|False>',
-           const=True,
-           nargs='?',
-           default=False,
-           help='Allows or disallows snapshot of '
-           'a volume when the volume is attached to an instance. '
-           'If set to True, ignores the current status of the '
-           'volume when attempting to snapshot it rather '
-           'than forcing it to be available. '
-           'Default=False.')
-@utils.arg('--name',
-           metavar='<name>',
-           default=None,
-           help='Snapshot name. Default=None.')
-@utils.arg('--display-name',
-           help=argparse.SUPPRESS)
-@utils.arg('--display_name',
-           help=argparse.SUPPRESS)
-@utils.arg('--description',
-           metavar='<description>',
-           default=None,
-           help='Snapshot description. Default=None.')
-@utils.arg('--display-description',
-           help=argparse.SUPPRESS)
-@utils.arg('--display_description',
-           help=argparse.SUPPRESS)
-@utils.arg('--metadata',
-           nargs='*',
-           metavar='<key=value>',
-           default=None,
-           help='Snapshot metadata key and value pairs. Default=None.')
-def do_snapshot_create(cs, args):
-    """Creates a snapshot."""
-    if args.display_name is not None:
-        args.name = args.display_name
-
-    if args.display_description is not None:
-        args.description = args.display_description
-
-    snapshot_metadata = None
-    if args.metadata is not None:
-        snapshot_metadata = shell_utils.extract_metadata(args)
-
-    volume = utils.find_volume(cs, args.volume)
-    snapshot = cs.volume_snapshots.create(volume.id,
-                                          args.force,
-                                          args.name,
-                                          args.description,
-                                          metadata=snapshot_metadata)
-    shell_utils.print_volume_snapshot(snapshot)
-
-
 @utils.arg('snapshot',
            metavar='<snapshot>', nargs='+',
            help='Name or ID of the snapshot(s) to delete.')
diff --git a/cinderclient/v3/volume_snapshots.py b/cinderclient/v3/volume_snapshots.py
index ce7f4e0..9a94422 100644
--- a/cinderclient/v3/volume_snapshots.py
+++ b/cinderclient/v3/volume_snapshots.py
@@ -15,10 +15,18 @@
 
 """Volume snapshot interface (v3 extension)."""
 
+from oslo_utils import strutils
+
 from cinderclient import api_versions
 from cinderclient.apiclient import base as common_base
 from cinderclient import base
 
+MV_3_66_FORCE_FLAG_ERROR = (
+    "Since microversion 3.66 of the Block Storage API,  the 'force' option is "
+    "invalid for this request.  For backward compatibility, however, when the "
+    "'force' flag is passed with a value evaluating to True, it is silently "
+    "ignored.")
+
 
 class Snapshot(base.Resource):
     """A Snapshot is a point-in-time snapshot of an openstack volume."""
@@ -80,6 +88,7 @@ class SnapshotManager(base.ManagerWithFind):
     """Manage :class:`Snapshot` resources."""
     resource_class = Snapshot
 
+    @api_versions.wraps("3.0", "3.65")
     def create(self, volume_id, force=False,
                name=None, description=None, metadata=None):
 
@@ -106,6 +115,42 @@ class SnapshotManager(base.ManagerWithFind):
                              'metadata': snapshot_metadata}}
         return self._create('/snapshots', body, 'snapshot')
 
+    @api_versions.wraps("3.66")
+    def create(self, volume_id, force=None,  # noqa: F811
+               name=None, description=None, metadata=None):
+
+        """Creates a snapshot of the given volume.
+
+        :param volume_id: The ID of the volume to snapshot.
+        :param force: This is technically not valid after mv 3.66, but the
+        API silently accepts force=True for backward compatibility, so this
+        function will, too
+        :param name: Name of the snapshot
+        :param description: Description of the snapshot
+        :param metadata: Metadata of the snapshot
+        :raises: ValueError if 'force' is not passed with a value that
+        evaluates to true
+        :rtype: :class:`Snapshot`
+        """
+
+        if metadata is None:
+            snapshot_metadata = {}
+        else:
+            snapshot_metadata = metadata
+
+        body = {'snapshot': {'volume_id': volume_id,
+                             'name': name,
+                             'description': description,
+                             'metadata': snapshot_metadata}}
+        if force is not None:
+            try:
+                force = strutils.bool_from_string(force, strict=True)
+                if not force:
+                    raise ValueError()
+            except ValueError:
+                raise ValueError(MV_3_66_FORCE_FLAG_ERROR)
+        return self._create('/snapshots', body, 'snapshot')
+
     def get(self, snapshot_id):
         """Shows snapshot details.
 
diff --git a/releasenotes/notes/support-bs-mv-3.66-5214deb20d164056.yaml b/releasenotes/notes/support-bs-mv-3.66-5214deb20d164056.yaml
new file mode 100644
index 0000000..c4028b0
--- /dev/null
+++ b/releasenotes/notes/support-bs-mv-3.66-5214deb20d164056.yaml
@@ -0,0 +1,9 @@
+---
+features:
+  - |
+    Adds support for Block Storage API version 3.66, which drops the
+    requirement of a 'force' flag to create a snapshot of an in-use
+    volume.  Although the 'force' flag is invalid for the ``snapshot-create``
+    call for API versions 3.66 and higher, for backward compatibility the
+    cinderclient follows the Block Storage API in silently ignoring the
+    flag when it is passed with a value that evaluates to True.