volume: Add missing 'volume backup *' options

Add a couple of missing options to each command:

  volume backup create
    --no-incremental
    --property
    --availability-zone

  volume backup set
    --property

Most of these are version dependent so we add the relevant version
checks as part of this work. While we're here, we also make the
formatting a little easier on the eye in places.

Change-Id: I328d5c981cb32b2ee9a4b1bd43aa36b22347ff63
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>

doc/source/cli/data/cinder.csv

@@ -15,7 +15,7 @@ backup-list,volume backup list,Lists all backups.
 backup-reset-state,volume backup set --state,Explicitly updates the backup state.
 backup-restore,volume backup restore,Restores a backup.
 backup-show,volume backup show,Show backup details.
-backup-update,,Updates a backup. (Supported by API versions 3.9 - 3.latest)
+backup-update,volume backup set,Updates a backup. (Supported by API versions 3.9 - 3.latest)
 cgsnapshot-create,consistency group snapshot create,Creates a cgsnapshot.
 cgsnapshot-delete,consistency group snapshot delete,Removes one or more cgsnapshots.
 cgsnapshot-list,consistency group snapshot list,Lists all cgsnapshots.

openstackclient/tests/unit/volume/v2/test_volume_backup.py

@@ -15,6 +15,7 @@
 from unittest import mock
 from unittest.mock import call
 
+from cinderclient import api_versions
 from osc_lib import exceptions
 from osc_lib import utils
 
@@ -114,6 +115,104 @@ class TestBackupCreate(TestBackup):
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.data, data)
 
+    def test_backup_create_with_properties(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.43')
+
+        arglist = [
+            "--property", "foo=bar",
+            "--property", "wow=much-cool",
+            self.new_backup.volume_id,
+        ]
+        verifylist = [
+            ("properties", {"foo": "bar", "wow": "much-cool"}),
+            ("volume", self.new_backup.volume_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        columns, data = self.cmd.take_action(parsed_args)
+
+        self.backups_mock.create.assert_called_with(
+            self.new_backup.volume_id,
+            container=None,
+            name=None,
+            description=None,
+            force=False,
+            incremental=False,
+            metadata={"foo": "bar", "wow": "much-cool"},
+        )
+        self.assertEqual(self.columns, columns)
+        self.assertEqual(self.data, data)
+
+    def test_backup_create_with_properties_pre_v343(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.42')
+
+        arglist = [
+            "--property", "foo=bar",
+            "--property", "wow=much-cool",
+            self.new_backup.volume_id,
+        ]
+        verifylist = [
+            ("properties", {"foo": "bar", "wow": "much-cool"}),
+            ("volume", self.new_backup.volume_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        exc = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args)
+        self.assertIn("--os-volume-api-version 3.43 or greater", str(exc))
+
+    def test_backup_create_with_availability_zone(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.51')
+
+        arglist = [
+            "--availability-zone", "my-az",
+            self.new_backup.volume_id,
+        ]
+        verifylist = [
+            ("availability_zone", "my-az"),
+            ("volume", self.new_backup.volume_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        columns, data = self.cmd.take_action(parsed_args)
+
+        self.backups_mock.create.assert_called_with(
+            self.new_backup.volume_id,
+            container=None,
+            name=None,
+            description=None,
+            force=False,
+            incremental=False,
+            availability_zone="my-az",
+        )
+        self.assertEqual(self.columns, columns)
+        self.assertEqual(self.data, data)
+
+    def test_backup_create_with_availability_zone_pre_v351(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.50')
+
+        arglist = [
+            "--availability-zone", "my-az",
+            self.new_backup.volume_id,
+        ]
+        verifylist = [
+            ("availability_zone", "my-az"),
+            ("volume", self.new_backup.volume_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        exc = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args)
+        self.assertIn("--os-volume-api-version 3.51 or greater", str(exc))
+
     def test_backup_create_without_name(self):
         arglist = [
             "--description", self.new_backup.description,
@@ -136,7 +235,6 @@ class TestBackupCreate(TestBackup):
             description=self.new_backup.description,
             force=False,
             incremental=False,
-            snapshot_id=None,
         )
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.data, data)
@@ -240,18 +338,18 @@ class TestBackupList(TestBackup):
     backups = volume_fakes.FakeBackup.create_backups(
         attrs={'volume_id': volume.name}, count=3)
 
-    columns = [
+    columns = (
         'ID',
         'Name',
         'Description',
         'Status',
         'Size',
-    ]
+    )
-    columns_long = columns + [
+    columns_long = columns + (
         'Availability Zone',
         'Volume',
         'Container',
-    ]
+    )
 
     data = []
     for b in backups:
@@ -403,6 +501,9 @@ class TestBackupSet(TestBackup):
         self.cmd = volume_backup.SetVolumeBackup(self.app, None)
 
     def test_backup_set_name(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.9')
+
         arglist = [
             '--name', 'new_name',
             self.backup.id,
@@ -420,7 +521,30 @@ class TestBackupSet(TestBackup):
             self.backup.id, **{'name': 'new_name'})
         self.assertIsNone(result)
 
+    def test_backup_set_name_pre_v39(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.8')
+
+        arglist = [
+            '--name', 'new_name',
+            self.backup.id,
+        ]
+        verifylist = [
+            ('name', 'new_name'),
+            ('backup', self.backup.id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        exc = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args)
+        self.assertIn("--os-volume-api-version 3.9 or greater", str(exc))
+
     def test_backup_set_description(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.9')
+
         arglist = [
             '--description', 'new_description',
             self.backup.id,
@@ -444,6 +568,27 @@ class TestBackupSet(TestBackup):
         )
         self.assertIsNone(result)
 
+    def test_backup_set_description_pre_v39(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.8')
+
+        arglist = [
+            '--description', 'new_description',
+            self.backup.id,
+        ]
+        verifylist = [
+            ('name', None),
+            ('description', 'new_description'),
+            ('backup', self.backup.id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        exc = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args)
+        self.assertIn("--os-volume-api-version 3.9 or greater", str(exc))
+
     def test_backup_set_state(self):
         arglist = [
             '--state', 'error',

openstackclient/volume/v2/volume_backup.py

@@ -14,10 +14,10 @@
 
 """Volume v2 Backup action implementations"""
 
-import copy
 import functools
 import logging
 
+from cinderclient import api_versions
 from cliff import columns as cliff_columns
 from osc_lib.cli import parseractions
 from osc_lib.command import command
@@ -61,7 +61,7 @@ class CreateVolumeBackup(command.ShowOne):
     _description = _("Create new volume backup")
 
     def get_parser(self, prog_name):
-        parser = super(CreateVolumeBackup, self).get_parser(prog_name)
+        parser = super().get_parser(prog_name)
         parser.add_argument(
             "volume",
             metavar="<volume>",
@@ -99,16 +99,67 @@ class CreateVolumeBackup(command.ShowOne):
             default=False,
             help=_("Perform an incremental backup")
         )
+        parser.add_argument(
+            '--no-incremental',
+            action='store_false',
+            help=_("Do not perform an incremental backup")
+        )
+        parser.add_argument(
+            '--property',
+            metavar='<key=value>',
+            action=parseractions.KeyValueAction,
+            dest='properties',
+            help=_(
+                'Set a property on this backup '
+                '(repeat option to remove multiple values) '
+                '(supported by --os-volume-api-version 3.43 or above)'
+            ),
+        )
+        parser.add_argument(
+            '--availability-zone',
+            metavar='<zone-name>',
+            help=_(
+                'AZ where the backup should be stored; by default it will be '
+                'the same as the source '
+                '(supported by --os-volume-api-version 3.51 or above)'
+            ),
+        )
         return parser
 
     def take_action(self, parsed_args):
         volume_client = self.app.client_manager.volume
+
         volume_id = utils.find_resource(
-            volume_client.volumes, parsed_args.volume).id
+            volume_client.volumes, parsed_args.volume,
-        snapshot_id = None
+        ).id
+
+        kwargs = {}
+
         if parsed_args.snapshot:
-            snapshot_id = utils.find_resource(
+            kwargs['snapshot_id'] = utils.find_resource(
-                volume_client.volume_snapshots, parsed_args.snapshot).id
+                volume_client.volume_snapshots, parsed_args.snapshot,
+            ).id
+
+        if parsed_args.properties:
+            if volume_client.api_version < api_versions.APIVersion('3.43'):
+                msg = _(
+                    '--os-volume-api-version 3.43 or greater is required to '
+                    'support the --property option'
+                )
+                raise exceptions.CommandError(msg)
+
+            kwargs['metadata'] = parsed_args.properties
+
+        if parsed_args.availability_zone:
+            if volume_client.api_version < api_versions.APIVersion('3.51'):
+                msg = _(
+                    '--os-volume-api-version 3.51 or greater is required to '
+                    'support the --availability-zone option'
+                )
+                raise exceptions.CommandError(msg)
+
+            kwargs['availability_zone'] = parsed_args.availability_zone
+
         backup = volume_client.backups.create(
             volume_id,
             container=parsed_args.container,
@@ -116,7 +167,7 @@ class CreateVolumeBackup(command.ShowOne):
             description=parsed_args.description,
             force=parsed_args.force,
             incremental=parsed_args.incremental,
-            snapshot_id=snapshot_id,
+            **kwargs,
         )
         backup._info.pop("links", None)
         return zip(*sorted(backup._info.items()))
@@ -148,7 +199,8 @@ class DeleteVolumeBackup(command.Command):
         for i in parsed_args.backups:
             try:
                 backup_id = utils.find_resource(
-                    volume_client.backups, i).id
+                    volume_client.backups, i,
+                ).id
                 volume_client.backups.delete(backup_id, parsed_args.force)
             except Exception as e:
                 result += 1
@@ -158,8 +210,9 @@ class DeleteVolumeBackup(command.Command):
 
         if result > 0:
             total = len(parsed_args.backups)
-            msg = (_("%(result)s of %(total)s backups failed "
+            msg = _("%(result)s of %(total)s backups failed to delete.") % {
-                   "to delete.") % {'result': result, 'total': total})
+                'result': result, 'total': total,
+            }
             raise exceptions.CommandError(msg)
 
 
@@ -182,17 +235,22 @@ class ListVolumeBackup(command.Lister):
         parser.add_argument(
             "--status",
             metavar="<status>",
-            choices=['creating', 'available', 'deleting',
+            choices=[
-                     'error', 'restoring', 'error_restoring'],
+                'creating', 'available', 'deleting',
-            help=_("Filters results by the backup status "
+                'error', 'restoring', 'error_restoring',
-                   "('creating', 'available', 'deleting', "
+            ],
-                   "'error', 'restoring' or 'error_restoring')")
+            help=_(
+                "Filters results by the backup status, one of: "
+                "creating, available, deleting, error, restoring or "
+                "error_restoring"
+            ),
         )
         parser.add_argument(
             "--volume",
             metavar="<volume>",
-            help=_("Filters results by the volume which they "
+            help=_(
-                   "backup (name or ID)")
+                "Filters results by the volume which they backup (name or ID)"
+            ),
         )
         parser.add_argument(
             '--marker',
@@ -212,19 +270,30 @@ class ListVolumeBackup(command.Lister):
             default=False,
             help=_('Include all projects (admin only)'),
         )
+        # TODO(stephenfin): Add once we have an equivalent command for
+        # 'cinder list-filters'
+        # parser.add_argument(
+        #     '--filter',
+        #     metavar='<key=value>',
+        #     action=parseractions.KeyValueAction,
+        #     dest='filters',
+        #     help=_(
+        #         "Filter key and value pairs. Use 'foo' to "
+        #         "check enabled filters from server. Use 'key~=value' for "
+        #         "inexact filtering if the key supports "
+        #         "(supported by --os-volume-api-version 3.33 or above)"
+        #     ),
+        # )
         return parser
 
     def take_action(self, parsed_args):
         volume_client = self.app.client_manager.volume
 
+        columns = ('id', 'name', 'description', 'status', 'size')
+        column_headers = ('ID', 'Name', 'Description', 'Status', 'Size')
         if parsed_args.long:
-            columns = ['ID', 'Name', 'Description', 'Status', 'Size',
+            columns += ('availability_zone', 'volume_id', 'container')
-                       'Availability Zone', 'Volume ID', 'Container']
+            column_headers += ('Availability Zone', 'Volume', 'Container')
-            column_headers = copy.deepcopy(columns)
-            column_headers[6] = 'Volume'
-        else:
-            columns = ['ID', 'Name', 'Description', 'Status', 'Size']
-            column_headers = columns
 
         # Cache the volume list
         volume_cache = {}
@@ -234,17 +303,22 @@ class ListVolumeBackup(command.Lister):
         except Exception:
             # Just forget it if there's any trouble
             pass
-        _VolumeIdColumn = functools.partial(VolumeIdColumn,
+
-                                            volume_cache=volume_cache)
+        _VolumeIdColumn = functools.partial(
+            VolumeIdColumn, volume_cache=volume_cache)
 
         filter_volume_id = None
         if parsed_args.volume:
-            filter_volume_id = utils.find_resource(volume_client.volumes,
+            filter_volume_id = utils.find_resource(
-                                                   parsed_args.volume).id
+                volume_client.volumes, parsed_args.volume,
+            ).id
+
         marker_backup_id = None
         if parsed_args.marker:
-            marker_backup_id = utils.find_resource(volume_client.backups,
+            marker_backup_id = utils.find_resource(
-                                                   parsed_args.marker).id
+                volume_client.backups, parsed_args.marker,
+            ).id
+
         search_opts = {
             'name': parsed_args.name,
             'status': parsed_args.status,
@@ -257,11 +331,14 @@ class ListVolumeBackup(command.Lister):
             limit=parsed_args.limit,
         )
 
-        return (column_headers,
+        return (
-                (utils.get_item_properties(
+            column_headers,
-                    s, columns,
+            (
-                    formatters={'Volume ID': _VolumeIdColumn},
+                utils.get_item_properties(
-                ) for s in data))
+                    s, columns, formatters={'volume_id': _VolumeIdColumn},
+                ) for s in data
+            ),
+        )
 
 
 class RestoreVolumeBackup(command.ShowOne):
@@ -295,7 +372,7 @@ class SetVolumeBackup(command.Command):
     _description = _("Set volume backup properties")
 
     def get_parser(self, prog_name):
-        parser = super(SetVolumeBackup, self).get_parser(prog_name)
+        parser = super().get_parser(prog_name)
         parser.add_argument(
             "backup",
             metavar="<backup>",
@@ -304,28 +381,48 @@ class SetVolumeBackup(command.Command):
         parser.add_argument(
             '--name',
             metavar='<name>',
-            help=_('New backup name')
+            help=_(
+                'New backup name'
+                '(supported by --os-volume-api-version 3.9 or above)'
+            ),
         )
         parser.add_argument(
             '--description',
             metavar='<description>',
-            help=_('New backup description')
+            help=_(
+                'New backup description '
+                '(supported by --os-volume-api-version 3.9 or above)'
+            ),
         )
         parser.add_argument(
             '--state',
             metavar='<state>',
             choices=['available', 'error'],
-            help=_('New backup state ("available" or "error") (admin only) '
+            help=_(
+                'New backup state ("available" or "error") (admin only) '
                 '(This option simply changes the state of the backup '
-                   'in the database with no regard to actual status, '
+                'in the database with no regard to actual status; '
-                   'exercise caution when using)'),
+                'exercise caution when using)'
+            ),
+        )
+        parser.add_argument(
+            '--property',
+            metavar='<key=value>',
+            action=parseractions.KeyValueAction,
+            dest='properties',
+            help=_(
+                'Set a property on this backup '
+                '(repeat option to set multiple values) '
+                '(supported by --os-volume-api-version 3.43 or above)'
+            ),
         )
         return parser
 
     def take_action(self, parsed_args):
         volume_client = self.app.client_manager.volume
-        backup = utils.find_resource(volume_client.backups,
+        backup = utils.find_resource(
-                                     parsed_args.backup)
+            volume_client.backups, parsed_args.backup)
+
         result = 0
         if parsed_args.state:
             try:
@@ -336,21 +433,47 @@ class SetVolumeBackup(command.Command):
                 result += 1
 
         kwargs = {}
+
         if parsed_args.name:
+            if volume_client.api_version < api_versions.APIVersion('3.9'):
+                msg = _(
+                    '--os-volume-api-version 3.9 or greater is required to '
+                    'support the --name option'
+                )
+                raise exceptions.CommandError(msg)
+
             kwargs['name'] = parsed_args.name
+
         if parsed_args.description:
+            if volume_client.api_version < api_versions.APIVersion('3.9'):
+                msg = _(
+                    '--os-volume-api-version 3.9 or greater is required to '
+                    'support the --description option'
+                )
+                raise exceptions.CommandError(msg)
+
             kwargs['description'] = parsed_args.description
+
+        if parsed_args.properties:
+            if volume_client.api_version < api_versions.APIVersion('3.43'):
+                msg = _(
+                    '--os-volume-api-version 3.43 or greater is required to '
+                    'support the --property option'
+                )
+                raise exceptions.CommandError(msg)
+
+            kwargs['metadata'] = parsed_args.properties
+
         if kwargs:
             try:
                 volume_client.backups.update(backup.id, **kwargs)
             except Exception as e:
-                LOG.error(_("Failed to update backup name "
+                LOG.error("Failed to update backup name or description: %s", e)
-                          "or description: %s"), e)
                 result += 1
 
         if result > 0:
-            raise exceptions.CommandError(_("One or more of the "
+            msg = _("One or more of the set operations failed")
-                                          "set operations failed"))
+            raise exceptions.CommandError(msg)
 
 
 class ShowVolumeBackup(command.ShowOne):

releasenotes/notes/add-missing-volume-backup-opts-b9246aded87427ce.yaml

@@ -0,0 +1,15 @@
+---
+features:
+  - |
+    Add ``--no-incremental``, ``--property`` and ``--availability-zone``
+    options to ``volume backup create`` command, allowing users to request a
+    non-incremental backup, set a metadata property on the created backup, and
+    set an availability zone on the created backup, respectively.
+  - |
+    Add ``--property`` option the ``volume backup set`` command to set a
+    metadata property on an existing backup.
+fixes:
+  - |
+    The ``--name`` and ``--description`` options of the ``volume backup set``
+    command will now verify the version requested on the client side.
+    Previously this would fail on the server side.