[libvirt] [PATCH v4] virsh: Add option to undefine storage with domains

Add an option for virsh undefine command, to remove associated storage volumes while undefining a domain. This patch allows the user to remove associated (libvirt managed ) storage volumes while undefining a domain. The new option --storage for the undefine command takes a string argument that consists of comma separated list of target or source path of volumes to be undefined. Volumes are removed after the domain has been successfully undefined, If a volume is not part of a storage pool, the user is warned to remove the volume in question himself. Option --wipe-storage may be specified along with this, that ensures the image is wiped before removing. Option --remove-all-storage enables the user to remove all storage. The name is chosen long as the users should be aware what they're about to do. --- Changes to v3: - fix ton'o'typos (I should install a spell checker.) - add a new variable to hold copy of argument to avoid confusion, typecasting ... - break volume string into tokens just once and store it in an array - add both target and source as volume description ( both are needed, as the user should know which volume he shoud delete, if the domain vanishes ) - reformat argument string in man - add example containing source path tools/virsh.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 18 ++++++ 2 files changed, 198 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index f6571f7..84b5e61 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1924,6 +1924,13 @@ static const vshCmdInfo info_undefine[] = { static const vshCmdOptDef opts_undefine[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name or uuid")}, {"managed-save", VSH_OT_BOOL, 0, N_("remove domain managed state file")}, + {"storage", VSH_OT_DATA, VSH_OFLAG_NONE, + N_("remove associated storage volumes (comma separated list of targets " + "or source paths) (see domblklist)")}, + {"remove-all-storage", VSH_OT_BOOL, 0, + N_("remove all associated storage volumes (use with caution)")}, + {"wipe-storage", VSH_OT_BOOL, VSH_OFLAG_NONE, + N_("wipe data on the removed volumes")}, {"snapshots-metadata", VSH_OT_BOOL, 0, N_("remove all domain snapshot metadata, if inactive")}, {NULL, 0, 0, NULL} @@ -1940,6 +1947,9 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) /* User-requested actions. */ bool managed_save = vshCommandOptBool(cmd, "managed-save"); bool snapshots_metadata = vshCommandOptBool(cmd, "snapshots-metadata"); + bool wipe_storage = vshCommandOptBool(cmd, "wipe-storage"); + bool remove_storage = false; + bool remove_all_storage = vshCommandOptBool(cmd, "remove-all-storage"); /* Positive if these items exist. */ int has_managed_save = 0; int has_snapshots_metadata = 0; @@ -1949,6 +1959,24 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) bool snapshots_safe = false; int rc = -1; int running; + /* list of volumes to remove along with this domain */ + const char *volumes_arg = NULL; + char *volumes = NULL; + char **volume_tokens = NULL; + char *volume_tok = NULL; + int nvolume_tokens = 0; + char *def = NULL; + char *source = NULL; + char *target = NULL; + int vol_i; + int tok_i; + xmlDocPtr doc = NULL; + xmlXPathContextPtr ctxt = NULL; + xmlNodePtr *vol_nodes = NULL; + int nvolumes = 0; + virStorageVolPtr vol = NULL; + bool vol_del_failed = false; if (managed_save) { flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE; @@ -1965,6 +1993,17 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; + /* check if a string that should contain list of volumes to remove is present */ + if (vshCommandOptString(cmd, "storage", &volumes_arg) > 0) { + volumes = vshStrdup(ctl, volumes_arg); + + if (remove_all_storage) { + vshError(ctl, _("Specified both --storage and --remove-all-storage")); + goto cleanup; + } + remove_storage = true; + } + /* Do some flag manipulation. The goal here is to disable bits * from flags to reduce the likelihood of a server rejecting * unknown flag bits, as well as to track conditions which are @@ -2027,6 +2066,20 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) snapshots_safe = true; } + /* Stash domain description for later use */ + if (remove_storage || remove_all_storage) { + if (running) { + vshError(ctl, _("Storage volume deletion is supported only on stopped domains")); + goto cleanup; + } + + if (!(def = virDomainGetXMLDesc(dom, 0))) { + vshError(ctl, _("Could not retrieve domain XML description")); + goto cleanup; + } + } + /* Generally we want to try the new API first. However, while * virDomainUndefineFlags was introduced at the same time as * VIR_DOMAIN_UNDEFINE_MANAGED_SAVE in 0.9.4, the @@ -2076,9 +2129,138 @@ out: ret = true; } else { vshError(ctl, _("Failed to undefine domain %s"), name); + goto cleanup; + } + + /* try to undefine storage volumes associated with this domain, if it's requested */ + if (remove_storage || remove_all_storage) { + ret = false; + + /* tokenize the string from user and save it's parts into an array */ + if (volumes) { + /* count the delimiters */ + volume_tok = volumes; + nvolume_tokens = 1; /* we need at least one member */ + while (*volume_tok) { + if (*volume_tok == ',') + nvolume_tokens++; + volume_tok++; + } + + volume_tokens = vshCalloc(ctl, nvolume_tokens, sizeof(char *)); + + /* tokenize the input string */ + nvolume_tokens = 0; + volume_tok = volumes; + do { + volume_tokens[nvolume_tokens] = strsep(&volume_tok, ","); + nvolume_tokens++; + } while (volume_tok); + } + + doc = virXMLParseStringCtxt(def, _("(domain_definition)"), &ctxt); + if (!doc) + goto cleanup; + + nvolumes = virXPathNodeSet("./devices/disk", ctxt, &vol_nodes); + + if (nvolumes < 0) + goto cleanup; + + for (vol_i = 0; vol_i < nvolumes; vol_i++) { + ctxt->node = vol_nodes[vol_i]; + VIR_FREE(target); + VIR_FREE(source); + if (vol) { + virStorageVolFree(vol); + vol = NULL; + } + + /* get volume source and target paths */ + if (!(target = virXPathString("string(./target/@dev)", ctxt))) { + vshError(ctl, _("Failed to enumerate devices")); + goto cleanup; + } + + if (!(source = virXPathString("string(" + "./source/@file|" + "./source/@dir|" + "./source/@name|" + "./source/@dev)", ctxt)) && + virGetLastError()) + goto cleanup; + + /* lookup if volume was selected by user */ + if (volumes) { + volume_tok = NULL; + for (tok_i = 0; tok_i < nvolume_tokens; tok_i++) { + if (volume_tokens[tok_i] && + (STREQ_NULLABLE(volume_tokens[tok_i], target) || + STREQ_NULLABLE(volume_tokens[tok_i], source))) { + volume_tok = volume_tokens[tok_i]; + volume_tokens[tok_i] = NULL; + break; + } + } + if (!volume_tok) + continue; + } + + if (!source) + continue; + + if (!(vol = virStorageVolLookupByPath(ctl->conn, source))) { + vshPrint(ctl, + _("Storage volume '%s'(%s) is not managed by libvirt. " + "Remove it manually.\n"), target, source); + virResetLastError(); + continue; + } + + if (wipe_storage) { + vshPrint(ctl, _("Wiping volume '%s'(%s) ... "), target, source); + fflush(stdout); + if (virStorageVolWipe(vol, 0) < 0) { + vshError(ctl, _("Failed! Volume not removed.")); + vol_del_failed = true; + continue; + } else { + vshPrint(ctl, _("Done.\n")); + } + } + + /* delete the volume */ + if (virStorageVolDelete(vol, 0) < 0) { + vshError(ctl, _("Failed to remove storage volume '%s'(%s)"), + target, source); + vol_del_failed = true; + } + vshPrint(ctl, _("Volume '%s' removed.\n"), volume_tok?volume_tok:source); + } + + /* print volumes specified by user that were not found in domain definition */ + if (volumes) { + for (tok_i = 0; tok_i < nvolume_tokens; tok_i++) { + if (volume_tokens[tok_i]) + vshPrint(ctl, _("Volume '%s' was not found in domain's " + "definition.\n"), + volume_tokens[tok_i]); + } + } + + if (!vol_del_failed) + ret = true; } cleanup: + VIR_FREE(source); + VIR_FREE(target); + VIR_FREE(volumes); + VIR_FREE(volume_tokens); + VIR_FREE(def); + VIR_FREE(vol_nodes); + xmlFreeDoc(doc); + xmlXPathFreeContext(ctxt); virDomainFree(dom); return ret; } diff --git a/tools/virsh.pod b/tools/virsh.pod index c468f13..3976166 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1181,6 +1181,7 @@ Output the device used for the TTY console of the domain. If the information is not available the processes will provide an exit code of 1. =item B<undefine> I<domain-id> [I<--managed-save>] [I<--snapshots-metadata>] +[I<--storage> B<volumes> | I<--remove-all-storage> 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, @@ -1196,6 +1197,23 @@ domain. Without the flag, attempts to undefine an inactive domain with snapshot metadata will fail. If the domain is active, this flag is ignored. +The I<--storage> flag takes a parameter B<volumes>, that is a comma separated +list of volume target names or source paths of storage volumes to be removed +along with the undefined domain. Volumes can be undefined and thus removed only +on inactive domains. Volume deletion is only attempted after the domain is +undefined; if not all of the requested volumes could be deleted, the +error message indicates what still remains behind. If a volume path is not +found in the domain definition, it's treated as if the volume was successfully +deleteted. +(See B<domblklist> for list of target names associated to a domain). +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 flag I<--wipe-storage> specifies that the storage volumes should be +wiped before removal. + NOTE: For an inactive domain, the domain name or UUID must be used as the I<domain-id>. -- 1.7.3.4

On 12/15/2011 08:20 AM, Peter Krempa wrote:
Add an option for virsh undefine command, to remove associated storage volumes while undefining a domain. This patch allows the user to remove associated (libvirt managed ) storage volumes while undefining a domain.
The new option --storage for the undefine command takes a string argument that consists of comma separated list of target or source path of volumes to be undefined. Volumes are removed after the domain has been successfully undefined,
If a volume is not part of a storage pool, the user is warned to remove the volume in question himself.
Option --wipe-storage may be specified along with this, that ensures the image is wiped before removing.
Option --remove-all-storage enables the user to remove all storage. The name is chosen long as the users should be aware what they're about to do. --- Changes to v3: - fix ton'o'typos (I should install a spell checker.) - add a new variable to hold copy of argument to avoid confusion, typecasting ... - break volume string into tokens just once and store it in an array - add both target and source as volume description ( both are needed, as the user should know which volume he shoud
Dare I mention the typo here? Nah, since it won't be part of the final commit. :)
@@ -2027,6 +2066,20 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) snapshots_safe = true; }
+ /* Stash domain description for later use */ + if (remove_storage || remove_all_storage) { + if (running) { + vshError(ctl, _("Storage volume deletion is supported only on stopped domains")); + goto cleanup; + }
Doesn't hold up this patch, but I wonder if we should have a counterpart virsh operation that allows deletion of volumes after destroying a transient domain. On the other hand, virsh isn't used as often with transient domains, and any management app (I'm thinking things like VDSM) that already prefers transient domains probably already knows how to properly wipe storage after the fact, without needing syntactic sugar in virsh. Overall, the code looks nice now!
=item B<undefine> I<domain-id> [I<--managed-save>] [I<--snapshots-metadata>] +[I<--storage> B<volumes> | I<--remove-all-storage> I<--wipe-storage>]
We want to make it obvious that --wipe-storage applies only if you use either --storage or --remove-all-storage, so this has to be: [ {I<--storage> B<volume> | I<--remove-all-storage>} [I<--wipe-storage]] The outer [] says storage removal in general is optional, then the first {} says that storage removal is an either-or between the two styles, and the second [] says that --wipe-storage goes with either style.
+The I<--storage> flag takes a parameter B<volumes>, that is a comma separated
s/that/which/
+list of volume target names or source paths of storage volumes to be removed +along with the undefined domain. Volumes can be undefined and thus removed only +on inactive domains. Volume deletion is only attempted after the domain is +undefined; if not all of the requested volumes could be deleted, the +error message indicates what still remains behind. If a volume path is not +found in the domain definition, it's treated as if the volume was successfully +deleteted.
s/deleteted/deleted/
+(See B<domblklist> for list of target names associated to a domain). +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 flag I<--wipe-storage> specifies that the storage volumes should be +wiped before removal.
ACK with nits in virsh.pod fixed; no need to send a v5. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 12/15/2011 09:17 PM, Eric Blake wrote:
On 12/15/2011 08:20 AM, Peter Krempa wrote:
Changes to v3: - fix ton'o'typos (I should install a spell checker.) - add a new variable to hold copy of argument to avoid confusion, typecasting ... - break volume string into tokens just once and store it in an array - add both target and source as volume description ( both are needed, as the user should know which volume he shoud
Dare I mention the typo here? Nah, since it won't be part of the final commit. :)
Dang. Happens _every_ time :(
@@ -2027,6 +2066,20 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) snapshots_safe = true; }
+ /* Stash domain description for later use */ + if (remove_storage || remove_all_storage) { + if (running) { + vshError(ctl, _("Storage volume deletion is supported only on stopped domains")); + goto cleanup; + }
Doesn't hold up this patch, but I wonder if we should have a counterpart virsh operation that allows deletion of volumes after destroying a transient domain. On the other hand, virsh isn't used as often with transient domains, and any management app (I'm thinking things like VDSM) that already prefers transient domains probably already knows how to properly wipe storage after the fact, without needing syntactic sugar in virsh.
Well, I think that would require remembering that the user wishes to remove storage in (transient) domain's configuration, and when the daemon stops the domain, it would remove the storage. Or virsh would have to wait if the domain disappears and then undefine the storage, but this would have limited usability as it would require virsh to run while the transient domain is being destroyed.
=item B<undefine> I<domain-id> [I<--managed-save>] [I<--snapshots-metadata>] +[I<--storage> B<volumes> | I<--remove-all-storage> I<--wipe-storage>]
We want to make it obvious that --wipe-storage applies only if you use either --storage or --remove-all-storage, so this has to be:
[ {I<--storage> B<volume> | I<--remove-all-storage>} [I<--wipe-storage]]
The outer [] says storage removal in general is optional, then the first {} says that storage removal is an either-or between the two styles, and the second [] says that --wipe-storage goes with either style.
I see your point. This is this definitely the best option to write it, but I somehow overlooked the curly braces while reading your v3 review :/
ACK with nits in virsh.pod fixed; no need to send a v5.
Thanks, fixed and pushed. Peter
participants (2)
-
Eric Blake
-
Peter Krempa