From 8eb1a183fe82e341812d0895ff14894d1668615c Mon Sep 17 00:00:00 2001 From: Stephen Finucane <stephenfin@redhat.com> Date: Fri, 7 Mar 2025 17:39:33 +0000 Subject: [PATCH] volume: Migrate 'backup set', 'backup unset' to SDK Change-Id: Iced346df828faab1ff08a2645ff64f4cfea25cb1 Signed-off-by: Stephen Finucane <stephenfin@redhat.com> --- .../unit/volume/v2/test_volume_backup.py | 63 ++++---- .../unit/volume/v3/test_volume_backup.py | 150 +++++++++--------- openstackclient/volume/v2/volume_backup.py | 12 +- openstackclient/volume/v3/volume_backup.py | 43 ++--- 4 files changed, 135 insertions(+), 133 deletions(-) diff --git a/openstackclient/tests/unit/volume/v2/test_volume_backup.py b/openstackclient/tests/unit/volume/v2/test_volume_backup.py index faf24899af..b2401afd4a 100644 --- a/openstackclient/tests/unit/volume/v2/test_volume_backup.py +++ b/openstackclient/tests/unit/volume/v2/test_volume_backup.py @@ -13,26 +13,15 @@ from unittest.mock import call +from openstack.block_storage.v2 import backup as _backup +from openstack import exceptions as sdk_exceptions +from openstack.test import fakes as sdk_fakes from osc_lib import exceptions from openstackclient.tests.unit.volume.v2 import fakes as volume_fakes from openstackclient.volume.v2 import volume_backup -class TestBackupLegacy(volume_fakes.TestVolume): - def setUp(self): - super().setUp() - - self.backups_mock = self.volume_client.backups - self.backups_mock.reset_mock() - self.volumes_mock = self.volume_client.volumes - self.volumes_mock.reset_mock() - self.snapshots_mock = self.volume_client.volume_snapshots - self.snapshots_mock.reset_mock() - self.restores_mock = self.volume_client.restores - self.restores_mock.reset_mock() - - class TestBackupCreate(volume_fakes.TestVolume): volume = volume_fakes.create_one_volume() snapshot = volume_fakes.create_one_snapshot() @@ -476,17 +465,15 @@ class TestBackupRestore(volume_fakes.TestVolume): ) -class TestBackupSet(TestBackupLegacy): - backup = volume_fakes.create_one_backup( - attrs={'metadata': {'wow': 'cool'}}, - ) - +class TestBackupSet(volume_fakes.TestVolume): def setUp(self): super().setUp() - self.backups_mock.get.return_value = self.backup + self.backup = sdk_fakes.generate_fake_resource( + _backup.Backup, metadata={'wow': 'cool'} + ) + self.volume_sdk_client.find_backup.return_value = self.backup - # Get the command object to test self.cmd = volume_backup.SetVolumeBackup(self.app, None) def test_backup_set_state(self): @@ -496,26 +483,34 @@ class TestBackupSet(TestBackupLegacy): parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - self.backups_mock.reset_state.assert_called_once_with( - self.backup.id, 'error' - ) self.assertIsNone(result) + self.volume_sdk_client.find_backup.assert_called_with( + self.backup.id, ignore_missing=False + ) + self.volume_sdk_client.reset_backup_status.assert_called_with( + self.backup, status='error' + ) + def test_backup_set_state_failed(self): - self.backups_mock.reset_state.side_effect = exceptions.CommandError() + self.volume_sdk_client.reset_backup_status.side_effect = ( + sdk_exceptions.NotFoundException('foo') + ) + arglist = ['--state', 'error', self.backup.id] verifylist = [('state', 'error'), ('backup', self.backup.id)] parsed_args = self.check_parser(self.cmd, arglist, verifylist) - try: - self.cmd.take_action(parsed_args) - self.fail('CommandError should be raised.') - except exceptions.CommandError as e: - self.assertEqual( - 'One or more of the set operations failed', str(e) - ) - self.backups_mock.reset_state.assert_called_with( - self.backup.id, 'error' + exc = self.assertRaises( + exceptions.CommandError, self.cmd.take_action, parsed_args + ) + self.assertEqual('One or more of the set operations failed', str(exc)) + + self.volume_sdk_client.find_backup.assert_called_with( + self.backup.id, ignore_missing=False + ) + self.volume_sdk_client.reset_backup_status.assert_called_with( + self.backup, status='error' ) diff --git a/openstackclient/tests/unit/volume/v3/test_volume_backup.py b/openstackclient/tests/unit/volume/v3/test_volume_backup.py index fb0a46508c..5069bee565 100644 --- a/openstackclient/tests/unit/volume/v3/test_volume_backup.py +++ b/openstackclient/tests/unit/volume/v3/test_volume_backup.py @@ -13,26 +13,15 @@ from unittest.mock import call +from openstack.block_storage.v3 import backup as _backup +from openstack import exceptions as sdk_exceptions +from openstack.test import fakes as sdk_fakes from osc_lib import exceptions from openstackclient.tests.unit.volume.v3 import fakes as volume_fakes from openstackclient.volume.v3 import volume_backup -class TestBackupLegacy(volume_fakes.TestVolume): - def setUp(self): - super().setUp() - - self.backups_mock = self.volume_client.backups - self.backups_mock.reset_mock() - self.volumes_mock = self.volume_client.volumes - self.volumes_mock.reset_mock() - self.snapshots_mock = self.volume_client.volume_snapshots - self.snapshots_mock.reset_mock() - self.restores_mock = self.volume_client.restores - self.restores_mock.reset_mock() - - class TestBackupCreate(volume_fakes.TestVolume): volume = volume_fakes.create_one_volume() snapshot = volume_fakes.create_one_snapshot() @@ -574,17 +563,15 @@ class TestBackupRestore(volume_fakes.TestVolume): ) -class TestBackupSet(TestBackupLegacy): - backup = volume_fakes.create_one_backup( - attrs={'metadata': {'wow': 'cool'}}, - ) - +class TestBackupSet(volume_fakes.TestVolume): def setUp(self): super().setUp() - self.backups_mock.get.return_value = self.backup + self.backup = sdk_fakes.generate_fake_resource( + _backup.Backup, metadata={'wow': 'cool'} + ) + self.volume_sdk_client.find_backup.return_value = self.backup - # Get the command object to test self.cmd = volume_backup.SetVolumeBackup(self.app, None) def test_backup_set_name(self): @@ -601,14 +588,16 @@ class TestBackupSet(TestBackupLegacy): ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) - # In base command class ShowOne in cliff, abstract method take_action() - # returns nothing result = self.cmd.take_action(parsed_args) - self.backups_mock.update.assert_called_once_with( - self.backup.id, **{'name': 'new_name'} - ) self.assertIsNone(result) + self.volume_sdk_client.find_backup.assert_called_with( + self.backup.id, ignore_missing=False + ) + self.volume_sdk_client.update_backup.assert_called_once_with( + self.backup, name='new_name' + ) + def test_backup_set_name_pre_v39(self): self.set_volume_api_version('3.8') @@ -644,14 +633,15 @@ class TestBackupSet(TestBackupLegacy): parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - - # Set expected values - kwargs = {'description': 'new_description'} - self.backups_mock.update.assert_called_once_with( - self.backup.id, **kwargs - ) self.assertIsNone(result) + self.volume_sdk_client.find_backup.assert_called_with( + self.backup.id, ignore_missing=False + ) + self.volume_sdk_client.update_backup.assert_called_once_with( + self.backup, description='new_description' + ) + def test_backup_set_description_pre_v39(self): self.set_volume_api_version('3.8') @@ -679,26 +669,34 @@ class TestBackupSet(TestBackupLegacy): parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - self.backups_mock.reset_state.assert_called_once_with( - self.backup.id, 'error' - ) self.assertIsNone(result) + self.volume_sdk_client.find_backup.assert_called_with( + self.backup.id, ignore_missing=False + ) + self.volume_sdk_client.reset_backup_status.assert_called_with( + self.backup, status='error' + ) + def test_backup_set_state_failed(self): - self.backups_mock.reset_state.side_effect = exceptions.CommandError() + self.volume_sdk_client.reset_backup_status.side_effect = ( + sdk_exceptions.NotFoundException('foo') + ) + arglist = ['--state', 'error', self.backup.id] verifylist = [('state', 'error'), ('backup', self.backup.id)] parsed_args = self.check_parser(self.cmd, arglist, verifylist) - try: - self.cmd.take_action(parsed_args) - self.fail('CommandError should be raised.') - except exceptions.CommandError as e: - self.assertEqual( - 'One or more of the set operations failed', str(e) - ) - self.backups_mock.reset_state.assert_called_with( - self.backup.id, 'error' + exc = self.assertRaises( + exceptions.CommandError, self.cmd.take_action, parsed_args + ) + self.assertEqual('One or more of the set operations failed', str(exc)) + + self.volume_sdk_client.find_backup.assert_called_with( + self.backup.id, ignore_missing=False + ) + self.volume_sdk_client.reset_backup_status.assert_called_with( + self.backup, status='error' ) def test_backup_set_no_property(self): @@ -715,16 +713,15 @@ class TestBackupSet(TestBackupLegacy): parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - - # Set expected values - kwargs = { - 'metadata': {}, - } - self.backups_mock.update.assert_called_once_with( - self.backup.id, **kwargs - ) self.assertIsNone(result) + self.volume_sdk_client.find_backup.assert_called_with( + self.backup.id, ignore_missing=False + ) + self.volume_sdk_client.update_backup.assert_called_once_with( + self.backup, metadata={} + ) + def test_backup_set_no_property_pre_v343(self): self.set_volume_api_version('3.42') @@ -758,16 +755,15 @@ class TestBackupSet(TestBackupLegacy): parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - - # Set expected values - kwargs = { - 'metadata': {'wow': 'cool', 'foo': 'bar'}, - } - self.backups_mock.update.assert_called_once_with( - self.backup.id, **kwargs - ) self.assertIsNone(result) + self.volume_sdk_client.find_backup.assert_called_with( + self.backup.id, ignore_missing=False + ) + self.volume_sdk_client.update_backup.assert_called_once_with( + self.backup, metadata={'wow': 'cool', 'foo': 'bar'} + ) + def test_backup_set_property_pre_v343(self): self.set_volume_api_version('3.42') @@ -788,17 +784,16 @@ class TestBackupSet(TestBackupLegacy): self.assertIn("--os-volume-api-version 3.43 or greater", str(exc)) -class TestBackupUnset(TestBackupLegacy): - backup = volume_fakes.create_one_backup( - attrs={'metadata': {'foo': 'bar'}}, - ) - +class TestBackupUnset(volume_fakes.TestVolume): def setUp(self): super().setUp() - self.backups_mock.get.return_value = self.backup + self.backup = sdk_fakes.generate_fake_resource( + _backup.Backup, metadata={'foo': 'bar', 'wow': 'cool'} + ) + self.volume_sdk_client.find_backup.return_value = self.backup + self.volume_sdk_client.delete_backup_metadata.return_value = None - # Get the command object to test self.cmd = volume_backup.UnsetVolumeBackup(self.app, None) def test_backup_unset_property(self): @@ -816,16 +811,15 @@ class TestBackupUnset(TestBackupLegacy): parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - - # Set expected values - kwargs = { - 'metadata': {}, - } - self.backups_mock.update.assert_called_once_with( - self.backup.id, **kwargs - ) self.assertIsNone(result) + self.volume_sdk_client.find_backup.assert_called_with( + self.backup.id, ignore_missing=False + ) + self.volume_sdk_client.delete_backup_metadata.assert_called_once_with( + self.backup, keys=['wow'] + ) + def test_backup_unset_property_pre_v343(self): self.set_volume_api_version('3.42') @@ -907,7 +901,9 @@ class TestBackupShow(volume_fakes.TestVolume): parsed_args = self.check_parser(self.cmd, arglist, verifylist) columns, data = self.cmd.take_action(parsed_args) - self.volume_sdk_client.find_backup.assert_called_with(self.backup.id) + self.volume_sdk_client.find_backup.assert_called_with( + self.backup.id, ignore_missing=False + ) self.assertEqual(self.columns, columns) self.assertEqual(self.data, data) diff --git a/openstackclient/volume/v2/volume_backup.py b/openstackclient/volume/v2/volume_backup.py index f6746eec85..55cde32f1d 100644 --- a/openstackclient/volume/v2/volume_backup.py +++ b/openstackclient/volume/v2/volume_backup.py @@ -417,13 +417,19 @@ class SetVolumeBackup(command.Command): 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) + volume_client = self.app.client_manager.sdk_connection.volume + + backup = volume_client.find_backup( + parsed_args.backup, + ignore_missing=False, + ) result = 0 if parsed_args.state: try: - volume_client.backups.reset_state(backup.id, parsed_args.state) + volume_client.reset_backup_status( + backup, status=parsed_args.state + ) except Exception as e: LOG.error(_("Failed to set backup state: %s"), e) result += 1 diff --git a/openstackclient/volume/v3/volume_backup.py b/openstackclient/volume/v3/volume_backup.py index 9892eadb06..7df46f8e69 100644 --- a/openstackclient/volume/v3/volume_backup.py +++ b/openstackclient/volume/v3/volume_backup.py @@ -18,7 +18,6 @@ import copy import functools import logging -from cinderclient import api_versions from cliff import columns as cliff_columns from openstack import utils as sdk_utils from osc_lib.cli import parseractions @@ -512,13 +511,19 @@ class SetVolumeBackup(command.Command): 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) + volume_client = self.app.client_manager.sdk_connection.volume + + backup = volume_client.find_backup( + parsed_args.backup, + ignore_missing=False, + ) result = 0 if parsed_args.state: try: - volume_client.backups.reset_state(backup.id, parsed_args.state) + volume_client.reset_backup_status( + backup, status=parsed_args.state + ) except Exception as e: LOG.error(_("Failed to set backup state: %s"), e) result += 1 @@ -526,7 +531,7 @@ class SetVolumeBackup(command.Command): kwargs = {} if parsed_args.name: - if volume_client.api_version < api_versions.APIVersion('3.9'): + if not sdk_utils.supports_microversion(volume_client, '3.9'): msg = _( '--os-volume-api-version 3.9 or greater is required to ' 'support the --name option' @@ -536,7 +541,7 @@ class SetVolumeBackup(command.Command): kwargs['name'] = parsed_args.name if parsed_args.description: - if volume_client.api_version < api_versions.APIVersion('3.9'): + if not sdk_utils.supports_microversion(volume_client, '3.9'): msg = _( '--os-volume-api-version 3.9 or greater is required to ' 'support the --description option' @@ -546,7 +551,7 @@ class SetVolumeBackup(command.Command): kwargs['description'] = parsed_args.description if parsed_args.no_property: - if volume_client.api_version < api_versions.APIVersion('3.43'): + if not sdk_utils.supports_microversion(volume_client, '3.43'): msg = _( '--os-volume-api-version 3.43 or greater is required to ' 'support the --no-property option' @@ -554,14 +559,14 @@ class SetVolumeBackup(command.Command): raise exceptions.CommandError(msg) if parsed_args.properties: - if volume_client.api_version < api_versions.APIVersion('3.43'): + if not sdk_utils.supports_microversion(volume_client, '3.43'): msg = _( '--os-volume-api-version 3.43 or greater is required to ' 'support the --property option' ) raise exceptions.CommandError(msg) - if volume_client.api_version >= api_versions.APIVersion('3.43'): + if sdk_utils.supports_microversion(volume_client, '3.43'): metadata = copy.deepcopy(backup.metadata) if parsed_args.no_property: @@ -572,7 +577,7 @@ class SetVolumeBackup(command.Command): if kwargs: try: - volume_client.backups.update(backup.id, **kwargs) + volume_client.update_backup(backup, **kwargs) except Exception as e: LOG.error("Failed to update backup: %s", e) result += 1 @@ -608,16 +613,18 @@ class UnsetVolumeBackup(command.Command): return parser def take_action(self, parsed_args): - volume_client = self.app.client_manager.volume + volume_client = self.app.client_manager.sdk_connection.volume - if volume_client.api_version < api_versions.APIVersion('3.43'): + if not sdk_utils.supports_microversion(volume_client, '3.43'): msg = _( '--os-volume-api-version 3.43 or greater is required to ' 'support the --property option' ) raise exceptions.CommandError(msg) - backup = utils.find_resource(volume_client.backups, parsed_args.backup) + backup = volume_client.find_backup( + parsed_args.backup, ignore_missing=False + ) metadata = copy.deepcopy(backup.metadata) for key in parsed_args.properties: @@ -632,11 +639,7 @@ class UnsetVolumeBackup(command.Command): del metadata[key] - kwargs = { - 'metadata': metadata, - } - - volume_client.backups.update(backup.id, **kwargs) + volume_client.delete_backup_metadata(backup, keys=list(metadata)) class ShowVolumeBackup(command.ShowOne): @@ -653,7 +656,9 @@ class ShowVolumeBackup(command.ShowOne): def take_action(self, parsed_args): volume_client = self.app.client_manager.sdk_connection.volume - backup = volume_client.find_backup(parsed_args.backup) + backup = volume_client.find_backup( + parsed_args.backup, ignore_missing=False + ) columns: tuple[str, ...] = ( "availability_zone", "container",