6:23 a.m.
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
6:23 a.m.
New subject: [libvirt] [PATCH 1/3] virsh: undefine: Clarify help string for --snapshots-metadata
Reword the end of the help string to make it more obvious that the VM
must be inactive.
Signed-off-by: Peter Krempa <pkrempa(a)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
6:23 a.m.
New subject: [libvirt] [PATCH 2/3] virsh: undefine: Rename --delete-snapshots to --delete-storage-volume-snapshots
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(a)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
6:23 a.m.
New subject: [libvirt] [PATCH 3/3] virsh: undefine: Clarify that --delete-storage-volume-snapshots causes failures
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(a)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
11:11 a.m.
New subject: [libvirt] [PATCH 3/3] virsh: undefine: Clarify that --delete-storage-volume-snapshots causes failures
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(a)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
8:07 a.m.
On Wed, Jun 5, 2019 at 1:27 PM Peter Krempa <pkrempa(a)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(a)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
11:11 a.m.
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
1962
days inactive
1962
days old
6 comments
3 participants
participants (3)
-
Eric Blake -
Fabiano Fidêncio -
Peter Krempa