diff --git a/doc/source/cli/data/cinder.csv b/doc/source/cli/data/cinder.csv index f94552ea93..649cd8f59a 100644 --- a/doc/source/cli/data/cinder.csv +++ b/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. diff --git a/openstackclient/tests/unit/volume/v2/test_volume_backup.py b/openstackclient/tests/unit/volume/v2/test_volume_backup.py index 13513ed8ad..7b5a965e09 100644 --- a/openstackclient/tests/unit/volume/v2/test_volume_backup.py +++ b/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', diff --git a/openstackclient/volume/v2/volume_backup.py b/openstackclient/volume/v2/volume_backup.py index c336f6c9ac..a171164e3a 100644 --- a/openstackclient/volume/v2/volume_backup.py +++ b/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="", @@ -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='', + 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='', + 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 - snapshot_id = None + volume_client.volumes, parsed_args.volume, + ).id + + kwargs = {} + if parsed_args.snapshot: - snapshot_id = utils.find_resource( - volume_client.volume_snapshots, parsed_args.snapshot).id + kwargs['snapshot_id'] = utils.find_resource( + 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 " - "to delete.") % {'result': result, 'total': total}) + msg = _("%(result)s of %(total)s backups failed to delete.") % { + 'result': result, 'total': total, + } raise exceptions.CommandError(msg) @@ -182,17 +235,22 @@ class ListVolumeBackup(command.Lister): parser.add_argument( "--status", metavar="", - choices=['creating', 'available', 'deleting', - 'error', 'restoring', 'error_restoring'], - help=_("Filters results by the backup status " - "('creating', 'available', 'deleting', " - "'error', 'restoring' or 'error_restoring')") + choices=[ + 'creating', 'available', 'deleting', + 'error', 'restoring', 'error_restoring', + ], + help=_( + "Filters results by the backup status, one of: " + "creating, available, deleting, error, restoring or " + "error_restoring" + ), ) parser.add_argument( "--volume", metavar="", - help=_("Filters results by the volume which they " - "backup (name or ID)") + help=_( + "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='', + # 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', - 'Availability Zone', 'Volume ID', 'Container'] - column_headers = copy.deepcopy(columns) - column_headers[6] = 'Volume' - else: - columns = ['ID', 'Name', 'Description', 'Status', 'Size'] - column_headers = columns + columns += ('availability_zone', 'volume_id', 'container') + column_headers += ('Availability Zone', 'Volume', 'Container') # 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, - parsed_args.volume).id + filter_volume_id = utils.find_resource( + volume_client.volumes, parsed_args.volume, + ).id + marker_backup_id = None if parsed_args.marker: - marker_backup_id = utils.find_resource(volume_client.backups, - parsed_args.marker).id + marker_backup_id = utils.find_resource( + 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, - (utils.get_item_properties( - s, columns, - formatters={'Volume ID': _VolumeIdColumn}, - ) for s in data)) + return ( + column_headers, + ( + utils.get_item_properties( + 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="", @@ -304,28 +381,48 @@ class SetVolumeBackup(command.Command): parser.add_argument( '--name', metavar='', - help=_('New backup name') + help=_( + 'New backup name' + '(supported by --os-volume-api-version 3.9 or above)' + ), ) parser.add_argument( '--description', metavar='', - help=_('New backup description') + help=_( + 'New backup description ' + '(supported by --os-volume-api-version 3.9 or above)' + ), ) parser.add_argument( '--state', metavar='', choices=['available', 'error'], - 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, ' - 'exercise caution when using)'), + 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; ' + 'exercise caution when using)' + ), + ) + parser.add_argument( + '--property', + metavar='', + 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, - parsed_args.backup) + backup = utils.find_resource( + 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 " - "or description: %s"), e) + LOG.error("Failed to update backup name or description: %s", e) result += 1 if result > 0: - raise exceptions.CommandError(_("One or more of the " - "set operations failed")) + msg = _("One or more of the set operations failed") + raise exceptions.CommandError(msg) class ShowVolumeBackup(command.ShowOne): diff --git a/releasenotes/notes/add-missing-volume-backup-opts-b9246aded87427ce.yaml b/releasenotes/notes/add-missing-volume-backup-opts-b9246aded87427ce.yaml new file mode 100644 index 0000000000..883cb0d564 --- /dev/null +++ b/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.