[libvirt] [PATCH 0/3] virsh: undefine: Clarify things around snapshots
Peter Krempa (3): virsh: undefine: Clarify help string for --snapshots-metadata virsh: undefine: Rename --delete-snapshots to --delete-storage-volume-snapshots virsh: undefine: Clarify that --delete-storage-volume-snapshots causes failures tools/virsh-domain.c | 6 +++++- tools/virsh.pod | 10 ++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) -- 2.21.0
Reword the end of the help string to make it more obvious that the VM must be inactive. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 82140feb57..a6e4469160 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3627,7 +3627,7 @@ static const vshCmdOptDef opts_undefine[] = { }, {.name = "snapshots-metadata", .type = VSH_OT_BOOL, - .help = N_("remove all domain snapshot metadata, if inactive") + .help = N_("remove all domain snapshot metadata (vm must be inactive)") }, {.name = "nvram", .type = VSH_OT_BOOL, -- 2.21.0
The old flag name confused some users into thinking it's the correct way to undefine a VM with libvirt (not storage volume) snapshots. The correct flag in that case is way less obvious: --snapshots-metadata. Rename the flag (by adding an alias) to something which will promote looking up the actual purpose of the flag. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 4 ++++ tools/virsh.pod | 7 ++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a6e4469160..828ae30789 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3617,6 +3617,10 @@ static const vshCmdOptDef opts_undefine[] = { .help = N_("remove all associated storage volumes (use with caution)") }, {.name = "delete-snapshots", + .type = VSH_OT_ALIAS, + .help = "delete-storage-volume-snapshots" + }, + {.name = "delete-storage-volume-snapshots", .type = VSH_OT_BOOL, .help = N_("delete snapshots associated with volume(s), requires " "--remove-all-storage (must be supported by storage driver)") diff --git a/tools/virsh.pod b/tools/virsh.pod index 9f5bfd27a0..fd9ba00d1f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2896,8 +2896,8 @@ is not available the processes will provide an exit code of 1. =item B<undefine> I<domain> [I<--managed-save>] [I<--snapshots-metadata>] [I<--nvram>] [I<--keep-nvram>] -[ {I<--storage> B<volumes> | I<--remove-all-storage> [I<--delete-snapshots>]} -I<--wipe-storage>] +[ {I<--storage> B<volumes> | I<--remove-all-storage> +[I<--delete-storage-volume-snapshots>]} I<--wipe-storage>] Undefine a domain. If the domain is running, this converts it to a transient domain, without stopping it. If the domain is inactive, @@ -2932,7 +2932,8 @@ Example: --storage vda,/path/to/storage.img The I<--remove-all-storage> flag specifies that all of the domain's storage volumes should be deleted. -The I<--delete-snapshots> flag specifies that any snapshots associated with +The I<--delete-storage-volume-snapshots> (previously I<--delete-snapshots>) +flag specifies that any snapshots associated with the storage volume should be deleted as well. Requires the I<--remove-all-storage> flag to be provided. Not all storage drivers support this option, presently only rbd. -- 2.21.0
The flag causes undefine to fail if trying to remove a non-RBD disk. Add a warning about that. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh.pod | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/virsh.pod b/tools/virsh.pod index fd9ba00d1f..11e853deea 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2936,7 +2936,8 @@ The I<--delete-storage-volume-snapshots> (previously I<--delete-snapshots>) flag specifies that any snapshots associated with the storage volume should be deleted as well. Requires the I<--remove-all-storage> flag to be provided. Not all storage drivers -support this option, presently only rbd. +support this option, presently only rbd. Using this when also removing volumes +handled by storage driver which does not support the flag will result in failure. The flag I<--wipe-storage> specifies that the storage volumes should be wiped before removal. -- 2.21.0
On 6/5/19 6:23 AM, Peter Krempa wrote:
The flag causes undefine to fail if trying to remove a non-RBD disk. Add a warning about that.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh.pod | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/virsh.pod b/tools/virsh.pod index fd9ba00d1f..11e853deea 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2936,7 +2936,8 @@ The I<--delete-storage-volume-snapshots> (previously I<--delete-snapshots>) flag specifies that any snapshots associated with the storage volume should be deleted as well. Requires the I<--remove-all-storage> flag to be provided. Not all storage drivers -support this option, presently only rbd. +support this option, presently only rbd. Using this when also removing volumes +handled by storage driver which does not support the flag will result in failure.
s/by/by a/ -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Wed, Jun 5, 2019 at 1:27 PM Peter Krempa <pkrempa@redhat.com> wrote:
Peter Krempa (3): virsh: undefine: Clarify help string for --snapshots-metadata virsh: undefine: Rename --delete-snapshots to --delete-storage-volume-snapshots virsh: undefine: Clarify that --delete-storage-volume-snapshots causes failures
tools/virsh-domain.c | 6 +++++- tools/virsh.pod | 10 ++++++---- 2 files changed, 11 insertions(+), 5 deletions(-)
Reviewed-by: Fabiano Fidêncio <fidencio@redhat.com> Please, take this review with a grain of salt and feel free to wait for a libvirt developer to act on this. Best Regards, -- Fabiano Fidêncio
On 6/5/19 6:23 AM, Peter Krempa wrote:
Peter Krempa (3): virsh: undefine: Clarify help string for --snapshots-metadata virsh: undefine: Rename --delete-snapshots to --delete-storage-volume-snapshots virsh: undefine: Clarify that --delete-storage-volume-snapshots causes failures
Interesting - I think I had copied '--delete-snapshots' into '--delete-checkpoints' in my incremental backup work because I had fallen prey to the wrong meaning that you are trying to clean up here. I guess that's another thing I get to revisit before posting v9. ACK series once the grammar in 3/3 is fixed. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (3)
-
Eric Blake -
Fabiano Fidêncio -
Peter Krempa