Merge "Refactor "volume backup restore" command"

5 files changed, 173 insertions, 26 deletions

diff --git a/openstackclient/tests/unit/volume/v1/test_volume_backup.py b/openstackclient/tests/unit/volume/v1/test_volume_backup.py
index f25a5ffa..2f568929 100644
--- a/openstackclient/tests/unit/volume/v1/test_volume_backup.py
+++ b/openstackclient/tests/unit/volume/v1/test_volume_backup.py
@@ -319,29 +319,69 @@ class TestBackupRestore(TestBackup):
         attrs={'volume_id': volume.id})
 
     def setUp(self):
-        super(TestBackupRestore, self).setUp()
+        super().setUp()
 
         self.backups_mock.get.return_value = self.backup
         self.volumes_mock.get.return_value = self.volume
-        self.restores_mock.restore.return_value = None
+        self.restores_mock.restore.return_value = (
+            volume_fakes.FakeVolume.create_one_volume(
+                {'id': self.volume['id']},
+            )
+        )
         # Get the command object to mock
         self.cmd = volume_backup.RestoreVolumeBackup(self.app, None)
 
     def test_backup_restore(self):
         arglist = [
             self.backup.id,
-            self.backup.volume_id
         ]
         verifylist = [
             ("backup", self.backup.id),
-            ("volume", self.backup.volume_id)
+            ("volume", None),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
         result = self.cmd.take_action(parsed_args)
         self.restores_mock.restore.assert_called_with(self.backup.id,
-                                                      self.backup.volume_id)
-        self.assertIsNone(result)
+                                                      None)
+        self.assertIsNotNone(result)
+
+    def test_backup_restore_with_existing_volume(self):
+        arglist = [
+            self.backup.id,
+            self.backup.volume_id,
+        ]
+        verifylist = [
+            ("backup", self.backup.id),
+            ("volume", self.backup.volume_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+        self.restores_mock.restore.assert_called_with(
+            self.backup.id, self.backup.volume_id,
+        )
+        self.assertIsNotNone(result)
+
+    def test_backup_restore_with_invalid_volume(self):
+        arglist = [
+            self.backup.id,
+            "unexist_volume",
+        ]
+        verifylist = [
+            ("backup", self.backup.id),
+            ("volume", "unexist_volume"),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        with mock.patch.object(
+            utils, 'find_resource',
+            side_effect=exceptions.CommandError(),
+        ):
+            self.assertRaises(
+                exceptions.CommandError,
+                self.cmd.take_action,
+                parsed_args,
+            )
 
 
 class TestBackupShow(TestBackup):
diff --git a/openstackclient/tests/unit/volume/v2/test_volume_backup.py b/openstackclient/tests/unit/volume/v2/test_volume_backup.py
index 4b9212d0..ffd84901 100644
--- a/openstackclient/tests/unit/volume/v2/test_volume_backup.py
+++ b/openstackclient/tests/unit/volume/v2/test_volume_backup.py
@@ -458,35 +458,95 @@ class TestBackupRestore(TestBackup):
 
     volume = volume_fakes.FakeVolume.create_one_volume()
     backup = volume_fakes.FakeBackup.create_one_backup(
-        attrs={'volume_id': volume.id})
+        attrs={'volume_id': volume.id},
+    )
 
     def setUp(self):
-        super(TestBackupRestore, self).setUp()
+        super().setUp()
 
         self.backups_mock.get.return_value = self.backup
         self.volumes_mock.get.return_value = self.volume
         self.restores_mock.restore.return_value = (
             volume_fakes.FakeVolume.create_one_volume(
-                {'id': self.volume['id']}))
+                {'id': self.volume['id']},
+            )
+        )
         # Get the command object to mock
         self.cmd = volume_backup.RestoreVolumeBackup(self.app, None)
 
     def test_backup_restore(self):
+        self.volumes_mock.get.side_effect = exceptions.CommandError()
+        self.volumes_mock.find.side_effect = exceptions.CommandError()
+        arglist = [
+            self.backup.id
+        ]
+        verifylist = [
+            ("backup", self.backup.id),
+            ("volume", None),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+        self.restores_mock.restore.assert_called_with(
+            self.backup.id, None, None,
+        )
+        self.assertIsNotNone(result)
+
+    def test_backup_restore_with_volume(self):
+        self.volumes_mock.get.side_effect = exceptions.CommandError()
+        self.volumes_mock.find.side_effect = exceptions.CommandError()
+        arglist = [
+            self.backup.id,
+            self.backup.volume_id,
+        ]
+        verifylist = [
+            ("backup", self.backup.id),
+            ("volume", self.backup.volume_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+        self.restores_mock.restore.assert_called_with(
+            self.backup.id, None, self.backup.volume_id,
+        )
+        self.assertIsNotNone(result)
+
+    def test_backup_restore_with_volume_force(self):
         arglist = [
+            "--force",
             self.backup.id,
-            self.backup.volume_id
+            self.volume.name,
         ]
         verifylist = [
+            ("force", True),
             ("backup", self.backup.id),
-            ("volume", self.backup.volume_id)
+            ("volume", self.volume.name),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
         result = self.cmd.take_action(parsed_args)
-        self.restores_mock.restore.assert_called_with(self.backup.id,
-                                                      self.backup.volume_id)
+        self.restores_mock.restore.assert_called_with(
+            self.backup.id, self.volume.id, None,
+        )
         self.assertIsNotNone(result)
 
+    def test_backup_restore_with_volume_existing(self):
+        arglist = [
+            self.backup.id,
+            self.volume.name,
+        ]
+        verifylist = [
+            ("backup", self.backup.id),
+            ("volume", self.volume.name),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args,
+        )
+
 
 class TestBackupSet(TestBackup):
 
diff --git a/openstackclient/volume/v1/volume_backup.py b/openstackclient/volume/v1/volume_backup.py
index 1a83a3c0..790cb463 100644
--- a/openstackclient/volume/v1/volume_backup.py
+++ b/openstackclient/volume/v1/volume_backup.py
@@ -231,18 +231,23 @@ class RestoreVolumeBackup(command.Command):
         parser.add_argument(
             'volume',
             metavar='<volume>',
-            help=_('Volume to restore to (name or ID)')
+            nargs='?',
+            help=_('Volume to restore to (name or ID) (default to None)')
         )
         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)
-        destination_volume = utils.find_resource(volume_client.volumes,
-                                                 parsed_args.volume)
-        return volume_client.restores.restore(backup.id,
-                                              destination_volume.id)
+        backup = utils.find_resource(
+            volume_client.backups, parsed_args.backup,
+        )
+        volume_id = None
+        if parsed_args.volume is not None:
+            volume_id = utils.find_resource(
+                volume_client.volumes,
+                parsed_args.volume,
+            ).id
+        return volume_client.restores.restore(backup.id, volume_id)
 
 
 class ShowVolumeBackup(command.ShowOne):
diff --git a/openstackclient/volume/v2/volume_backup.py b/openstackclient/volume/v2/volume_backup.py
index f2d89dc7..d96b28e9 100644
--- a/openstackclient/volume/v2/volume_backup.py
+++ b/openstackclient/volume/v2/volume_backup.py
@@ -363,18 +363,50 @@ class RestoreVolumeBackup(command.ShowOne):
         parser.add_argument(
             "volume",
             metavar="<volume>",
-            help=_("Volume to restore to (name or ID)")
+            nargs="?",
+            help=_(
+                "Volume to restore to "
+                "(name or ID for existing volume, name only for new volume) "
+                "(default to None)"
+            )
+        )
+        parser.add_argument(
+            "--force",
+            action="store_true",
+            help=_(
+                "Restore the backup to an existing volume "
+                "(default to False)"
+            )
         )
         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)
-        destination_volume = utils.find_resource(volume_client.volumes,
-                                                 parsed_args.volume)
-        backup = volume_client.restores.restore(backup.id,
-                                                destination_volume.id)
-        return zip(*sorted(backup._info.items()))
+
+        volume_name = None
+        volume_id = None
+        try:
+            volume_id = utils.find_resource(
+                volume_client.volumes,
+                parsed_args.volume,
+            ).id
+        except Exception:
+            volume_name = parsed_args.volume
+        else:
+            # If we didn't fail, the volume must already exist. We only allow
+            # this to work if the user forced things
+            if not parsed_args.force:
+                msg = _(
+                    "Volume '%s' already exists; if you want to restore the "
+                    "backup to it you need to specify the '--force' option"
+                ) % parsed_args.volume
+                raise exceptions.CommandError(msg)
+
+        return volume_client.restores.restore(
+            backup.id, volume_id, volume_name,
+        )
 
 
 class SetVolumeBackup(command.Command):
diff --git a/releasenotes/notes/bug-1597189-02a8d8a402725860.yaml b/releasenotes/notes/bug-1597189-02a8d8a402725860.yaml
new file mode 100644
index 00000000..69d1eb77
--- /dev/null
+++ b/releasenotes/notes/bug-1597189-02a8d8a402725860.yaml
@@ -0,0 +1,10 @@
+---
+features:
+  - |
+    The ``volume`` argument of the ``volume backup restore`` command is now
+    optional and can refer to the name of a new volume that should be created
+    rather than a name or ID of an existing volume (which would be
+    overwritten). If not provided, cinder will generate a new volume with a
+    unique name. To restore a backup to an existing volume, you must now
+    specify the ``--force`` option (volume v2, v3 only).
+    [Bug `1597189 <https://bugs.launchpad.net/bugs/1597189>`_]