Skip to content

Commit de4a69a

Browse files
Huanxuan Aostephenfin
Huanxuan Ao
authored and
stephenfin
committed
Refactor "volume backup restore" command
Make the positional argument "volume" optional and add a "--force" option (volume v2 only) to the "volume backup restore" command. Closes-Bug: #1597189 Change-Id: If944e10158bd18e8331be63e96187a23e23095d7
1 parent 8c975ba commit de4a69a

File tree

5 files changed

+173
-26
lines changed

5 files changed

+173
-26
lines changed

‎openstackclient/tests/unit/volume/v1/test_volume_backup.py‎

Lines changed: 46 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -319,29 +319,69 @@ class TestBackupRestore(TestBackup):
319319
attrs={'volume_id': volume.id})
320320

321321
def setUp(self):
322-
super(TestBackupRestore, self).setUp()
322+
super().setUp()
323323

324324
self.backups_mock.get.return_value = self.backup
325325
self.volumes_mock.get.return_value = self.volume
326-
self.restores_mock.restore.return_value = None
326+
self.restores_mock.restore.return_value = (
327+
volume_fakes.FakeVolume.create_one_volume(
328+
{'id': self.volume['id']},
329+
)
330+
)
327331
# Get the command object to mock
328332
self.cmd = volume_backup.RestoreVolumeBackup(self.app, None)
329333

330334
def test_backup_restore(self):
331335
arglist = [
332336
self.backup.id,
333-
self.backup.volume_id
334337
]
335338
verifylist = [
336339
("backup", self.backup.id),
337-
("volume", self.backup.volume_id)
340+
("volume", None),
338341
]
339342
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
340343

341344
result = self.cmd.take_action(parsed_args)
342345
self.restores_mock.restore.assert_called_with(self.backup.id,
343-
self.backup.volume_id)
344-
self.assertIsNone(result)
346+
None)
347+
self.assertIsNotNone(result)
348+
349+
def test_backup_restore_with_existing_volume(self):
350+
arglist = [
351+
self.backup.id,
352+
self.backup.volume_id,
353+
]
354+
verifylist = [
355+
("backup", self.backup.id),
356+
("volume", self.backup.volume_id),
357+
]
358+
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
359+
360+
result = self.cmd.take_action(parsed_args)
361+
self.restores_mock.restore.assert_called_with(
362+
self.backup.id, self.backup.volume_id,
363+
)
364+
self.assertIsNotNone(result)
365+
366+
def test_backup_restore_with_invalid_volume(self):
367+
arglist = [
368+
self.backup.id,
369+
"unexist_volume",
370+
]
371+
verifylist = [
372+
("backup", self.backup.id),
373+
("volume", "unexist_volume"),
374+
]
375+
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
376+
with mock.patch.object(
377+
utils, 'find_resource',
378+
side_effect=exceptions.CommandError(),
379+
):
380+
self.assertRaises(
381+
exceptions.CommandError,
382+
self.cmd.take_action,
383+
parsed_args,
384+
)
345385

346386

347387
class TestBackupShow(TestBackup):

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

Lines changed: 67 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -458,35 +458,95 @@ class TestBackupRestore(TestBackup):
458458

459459
volume = volume_fakes.FakeVolume.create_one_volume()
460460
backup = volume_fakes.FakeBackup.create_one_backup(
461-
attrs={'volume_id': volume.id})
461+
attrs={'volume_id': volume.id},
462+
)
462463

463464
def setUp(self):
464-
super(TestBackupRestore, self).setUp()
465+
super().setUp()
465466

466467
self.backups_mock.get.return_value = self.backup
467468
self.volumes_mock.get.return_value = self.volume
468469
self.restores_mock.restore.return_value = (
469470
volume_fakes.FakeVolume.create_one_volume(
470-
{'id': self.volume['id']}))
471+
{'id': self.volume['id']},
472+
)
473+
)
471474
# Get the command object to mock
472475
self.cmd = volume_backup.RestoreVolumeBackup(self.app, None)
473476

474477
def test_backup_restore(self):
478+
self.volumes_mock.get.side_effect = exceptions.CommandError()
479+
self.volumes_mock.find.side_effect = exceptions.CommandError()
480+
arglist = [
481+
self.backup.id
482+
]
483+
verifylist = [
484+
("backup", self.backup.id),
485+
("volume", None),
486+
]
487+
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
488+
489+
result = self.cmd.take_action(parsed_args)
490+
self.restores_mock.restore.assert_called_with(
491+
self.backup.id, None, None,
492+
)
493+
self.assertIsNotNone(result)
494+
495+
def test_backup_restore_with_volume(self):
496+
self.volumes_mock.get.side_effect = exceptions.CommandError()
497+
self.volumes_mock.find.side_effect = exceptions.CommandError()
498+
arglist = [
499+
self.backup.id,
500+
self.backup.volume_id,
501+
]
502+
verifylist = [
503+
("backup", self.backup.id),
504+
("volume", self.backup.volume_id),
505+
]
506+
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
507+
508+
result = self.cmd.take_action(parsed_args)
509+
self.restores_mock.restore.assert_called_with(
510+
self.backup.id, None, self.backup.volume_id,
511+
)
512+
self.assertIsNotNone(result)
513+
514+
def test_backup_restore_with_volume_force(self):
475515
arglist = [
516+
"--force",
476517
self.backup.id,
477-
self.backup.volume_id
518+
self.volume.name,
478519
]
479520
verifylist = [
521+
("force", True),
480522
("backup", self.backup.id),
481-
("volume", self.backup.volume_id)
523+
("volume", self.volume.name),
482524
]
483525
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
484526

485527
result = self.cmd.take_action(parsed_args)
486-
self.restores_mock.restore.assert_called_with(self.backup.id,
487-
self.backup.volume_id)
528+
self.restores_mock.restore.assert_called_with(
529+
self.backup.id, self.volume.id, None,
530+
)
488531
self.assertIsNotNone(result)
489532

533+
def test_backup_restore_with_volume_existing(self):
534+
arglist = [
535+
self.backup.id,
536+
self.volume.name,
537+
]
538+
verifylist = [
539+
("backup", self.backup.id),
540+
("volume", self.volume.name),
541+
]
542+
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
543+
544+
self.assertRaises(
545+
exceptions.CommandError,
546+
self.cmd.take_action,
547+
parsed_args,
548+
)
549+
490550

491551
class TestBackupSet(TestBackup):
492552

‎openstackclient/volume/v1/volume_backup.py‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -231,18 +231,23 @@ def get_parser(self, prog_name):
231231
parser.add_argument(
232232
'volume',
233233
metavar='<volume>',
234-
help=_('Volume to restore to (name or ID)')
234+
nargs='?',
235+
help=_('Volume to restore to (name or ID) (default to None)')
235236
)
236237
return parser
237238

238239
def take_action(self, parsed_args):
239240
volume_client = self.app.client_manager.volume
240-
backup = utils.find_resource(volume_client.backups,
241-
parsed_args.backup)
242-
destination_volume = utils.find_resource(volume_client.volumes,
243-
parsed_args.volume)
244-
return volume_client.restores.restore(backup.id,
245-
destination_volume.id)
241+
backup = utils.find_resource(
242+
volume_client.backups, parsed_args.backup,
243+
)
244+
volume_id = None
245+
if parsed_args.volume is not None:
246+
volume_id = utils.find_resource(
247+
volume_client.volumes,
248+
parsed_args.volume,
249+
).id
250+
return volume_client.restores.restore(backup.id, volume_id)
246251

247252

248253
class ShowVolumeBackup(command.ShowOne):

‎openstackclient/volume/v2/volume_backup.py‎

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -355,18 +355,50 @@ def get_parser(self, prog_name):
355355
parser.add_argument(
356356
"volume",
357357
metavar="<volume>",
358-
help=_("Volume to restore to (name or ID)")
358+
nargs="?",
359+
help=_(
360+
"Volume to restore to "
361+
"(name or ID for existing volume, name only for new volume) "
362+
"(default to None)"
363+
)
364+
)
365+
parser.add_argument(
366+
"--force",
367+
action="store_true",
368+
help=_(
369+
"Restore the backup to an existing volume "
370+
"(default to False)"
371+
)
359372
)
360373
return parser
361374

362375
def take_action(self, parsed_args):
363376
volume_client = self.app.client_manager.volume
377+
364378
backup = utils.find_resource(volume_client.backups, parsed_args.backup)
365-
destination_volume = utils.find_resource(volume_client.volumes,
366-
parsed_args.volume)
367-
backup = volume_client.restores.restore(backup.id,
368-
destination_volume.id)
369-
return zip(*sorted(backup._info.items()))
379+
380+
volume_name = None
381+
volume_id = None
382+
try:
383+
volume_id = utils.find_resource(
384+
volume_client.volumes,
385+
parsed_args.volume,
386+
).id
387+
except Exception:
388+
volume_name = parsed_args.volume
389+
else:
390+
# If we didn't fail, the volume must already exist. We only allow
391+
# this to work if the user forced things
392+
if not parsed_args.force:
393+
msg = _(
394+
"Volume '%s' already exists; if you want to restore the "
395+
"backup to it you need to specify the '--force' option"
396+
) % parsed_args.volume
397+
raise exceptions.CommandError(msg)
398+
399+
return volume_client.restores.restore(
400+
backup.id, volume_id, volume_name,
401+
)
370402

371403

372404
class SetVolumeBackup(command.Command):

‎releasenotes/notes/bug-1597189-02a8d8a402725860.yaml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
features:
3+
- |
4+
The ``volume`` argument of the ``volume backup restore`` command is now
5+
optional and can refer to the name of a new volume that should be created
6+
rather than a name or ID of an existing volume (which would be
7+
overwritten). If not provided, cinder will generate a new volume with a
8+
unique name. To restore a backup to an existing volume, you must now
9+
specify the ``--force`` option (volume v2, v3 only).
10+
[Bug `1597189 <https://bugs.launchpad.net/bugs/1597189>`_]

0 commit comments

Comments
 (0)