[libvirt] [PATCH RFC v2] 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 alows 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 volumes to be undefined. Each of the volumes is removed from the domain definition and afterwards removed from storage pools. 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. --- tools/virsh.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 12 ++++ 2 files changed, 179 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 3b060bf..67125c9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1858,6 +1858,9 @@ 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) (see domblklist)")}, + {"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} @@ -1874,6 +1877,8 @@ 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_volumes = false; /* Positive if these items exist. */ int has_managed_save = 0; int has_snapshots_metadata = 0; @@ -1883,6 +1888,25 @@ 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; + const char *volume; + char *saveptr = NULL; + char *def = NULL; + char *xpath_str = NULL; + char *volume_path = NULL; + char *device_desc = NULL; + xmlNodePtr node = NULL; + xmlXPathContextPtr ctxt_dom = NULL; + xmlXPathContextPtr ctxt_dev = NULL; + xmlBufferPtr xml_buf = NULL; + xmlDocPtr xml_dom = NULL; + xmlDocPtr xml_dev = NULL; + virStorageVolPtr vol = NULL; + bool wipe_failed; + + virBuffer xpath = VIR_BUFFER_INITIALIZER; if (managed_save) { flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE; @@ -1893,12 +1917,19 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) snapshots_safe = true; } + if (!vshConnectionUsability(ctl, ctl->conn)) return false; 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) { + remove_volumes = true; + volumes = vshStrdup(ctl, volumes_arg); + } + /* 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 @@ -1961,6 +1992,138 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) snapshots_safe = true; } + /* try to undefine storage volumes associated with this domain, if it's requested */ + if (remove_volumes) { + if (running) { + vshError(ctl, _("Refusing to remove storage for running domain")); + goto cleanup; + } + + /* check if managed save is being removed, as the domain removal + * might fail and leave it without storage */ + if (has_managed_save && !managed_save) { + vshError(ctl, "%s", + _("Refusing to undefine with storage volumes while domain managed save " + "image exists")); + goto cleanup; + } + + /* get domain description */ + if (!(def = virDomainGetXMLDesc(dom, 0))) + goto cleanup; + + xml_dom = virXMLParseStringCtxt(def, _("(domain definition)"), &ctxt_dom); + VIR_FREE(def); + + if (!xml_dom) + goto cleanup; + + xml_buf = xmlBufferCreate(); + if (!xml_buf) + goto cleanup; + + /* tokenize the string of devices to remove */ + volume = strtok_r(volumes, ",", &saveptr); + + while (volume) { + + /* find and try to remove each volume selected by the user */ + virBufferAsprintf(&xpath, "/domain/devices/disk[target[@dev='%s']]", volume); + xpath_str = virBufferContentAndReset(&xpath); + + node = virXPathNode(xpath_str, ctxt_dom); + VIR_FREE(xpath_str); + if (!node) { + vshPrint(ctl, _("Volume %s not found. Skipping\n"), volume); + goto next_volume; + } + + xmlBufferEmpty(xml_buf); + + /* dump and detatch the device from persistent conf. */ + + if (xmlNodeDump(xml_buf, xml_dom, node, 0, 0) < 0) { + vshError(ctl, _("Failed to create device description")); + goto cleanup; + } + + device_desc = (char *) xmlBufferContent(xml_buf); + + if (virDomainDetachDeviceFlags(dom, device_desc, VIR_DOMAIN_AFFECT_CURRENT) < 0) { + vshError(ctl, + _("Failed to remove storage device '%s' from definition. " + "If domain undefinition fails, this domain may remain inconsistent"), + volume); + } + + xml_dev = virXMLParseStringCtxt(device_desc, + _("(storage volume definition)"), + &ctxt_dev); + + /* find the target and lookup the image in storage pools */ + if (!xml_dev) + goto cleanup; + + volume_path = virXPathString("string(//disk/source/@file" + "| //disk/source/@dev" + "| //disk/source/@dir" + "| //disk/source/@name)", ctxt_dev); + + if (!volume_path) { + vshPrint(ctl, + _("Virtual storage device '%s' has no associated volume. Skipping."), + volume); + goto next_volume; + } + + vol = virStorageVolLookupByPath(ctl->conn, volume_path); + + if (!vol) { + vshPrint(ctl, + _("Storage volume '%s' is not managed by libvirt. Remove it manualy.\n"), + volume_path); + goto next_volume; + } + + wipe_failed = false; + + /* wipe the volume */ + if (wipe_storage) { + vshPrint(ctl, _("Wiping volume '%s' ... "), volume_path); + if (virStorageVolWipe(vol, 0) < 0) { + vshPrint(ctl, _("Failed!\n")); + wipe_failed = true; + } else { + vshPrint(ctl, _("Done.\n")); + } + } + + + /* delete the volume */ + if (wipe_failed || virStorageVolDelete(vol, 0) < 0) { + vshError(ctl, + _("Failed to remove storage volume '%s'. Remove it manualy."), + volume_path); + } else { + vshPrint(ctl, _("Volume '%s' deleted.\n"), volume_path); + } + + +next_volume: + VIR_FREE(volume_path); + if (vol) + virStorageVolFree(vol); + vol = NULL; + xmlXPathFreeContext(ctxt_dev); + ctxt_dev = NULL; + xmlFreeDoc(xml_dev); + xml_dev = NULL; + + volume = strtok_r(NULL, ",", &saveptr); + } + /* domain's storage removed */ + } + /* 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 @@ -2013,6 +2176,10 @@ out: } cleanup: + xmlFreeDoc(xml_dom); + xmlBufferFree(xml_buf); + xmlXPathFreeContext(ctxt_dom); + VIR_FREE(volumes); virDomainFree(dom); return ret; } diff --git a/tools/virsh.pod b/tools/virsh.pod index e82567d..87a1e7d 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1067,6 +1067,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 volumes>] [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, @@ -1082,6 +1083,17 @@ 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 volumes, that is a comma separated +list of volume target names of storage volumes to be removed along with +the undefined domain. This operation is valid only for a inactive domain. +If one or more of the operations while removing disk storage fail, the +domain may still be undefined and some volumes may remain undeleted. +(See B<domblklist> for list of target names associated to a domain). +Example: --storage vda,vdb,vdc + +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 09/13/2011 09:00 AM, Peter Krempa wrote:
Add an option for virsh undefine command, to remove associated storage volumes while undefining a domain. This patch alows 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 volumes to be undefined. Each of the volumes is removed from the domain definition and afterwards removed from storage pools.
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.
Probably too much of a feature, even though it only touches virsh, that I'm 60-40 towards delaying this until post-0.9.5 release. At any rate, it needs more work, although I like the overall idea (it doesn't affect the API, which was the sticking point of v1, but just the user experience; where error reporting can be a bit more granular over what worked and what failed than by lumping it all into a single API call). Here's hoping v3 is nicer :)
@@ -1961,6 +1992,138 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) snapshots_safe = true; }
+ /* try to undefine storage volumes associated with this domain, if it's requested */ + if (remove_volumes) { + if (running) { + vshError(ctl, _("Refusing to remove storage for running domain")); + goto cleanup; + }
Wrong order. Undefine the domain first, _then_ undefine storage volumes. As written, if you undefine a storage volume, then the undefine fails, you've caused irreparable damage (the domain cannot be restarted); but if you undefine the domain first, then storage volumes, your error message can at least list which volumes still remain, or you are guaranteed that the domain still exists and can still be run. Okay, you _do_ need to parse out domains prior to undefining the domain, but then save that list until after the domain undefine succeeds.
+ + /* check if managed save is being removed, as the domain removal + * might fail and leave it without storage */ + if (has_managed_save&& !managed_save) {
Once you fix the ordering (domain before storage), then this check is pointless (managed save either already prevented undefining the domain, or else there is no longer a managed save to worry about because the undefine domain succeeded, whether by the new API or by the virsh faking it with old API).
+ vshError(ctl, "%s", + _("Refusing to undefine with storage volumes while domain managed save " + "image exists")); + goto cleanup; + } + + /* get domain description */ + if (!(def = virDomainGetXMLDesc(dom, 0))) + goto cleanup; + + xml_dom = virXMLParseStringCtxt(def, _("(domain definition)"),&ctxt_dom); + VIR_FREE(def); + + if (!xml_dom) + goto cleanup; + + xml_buf = xmlBufferCreate(); + if (!xml_buf) + goto cleanup; + + /* tokenize the string of devices to remove */ + volume = strtok_r(volumes, ",",&saveptr); + + while (volume) { + + /* find and try to remove each volume selected by the user */ + virBufferAsprintf(&xpath, "/domain/devices/disk[target[@dev='%s']]", volume); + xpath_str = virBufferContentAndReset(&xpath);
This only parses by dev name, but you should also support parsing by source path. That is, we need a virsh counterpart to domain_conf.c virDomainDiskIndexByName() which operates on domain XML instead of virDomainDefPtr, and maps an input string of two possible formats back into the canonical device name to later be manipulated.
+ + node = virXPathNode(xpath_str, ctxt_dom); + VIR_FREE(xpath_str); + if (!node) { + vshPrint(ctl, _("Volume %s not found. Skipping\n"), volume); + goto next_volume; + } + + xmlBufferEmpty(xml_buf); + + /* dump and detatch the device from persistent conf. */
s/detatch/detach/ But shouldn't be needed - if the domain is undefined first, then there's no need to detach the disk from a domain.
+ + if (xmlNodeDump(xml_buf, xml_dom, node, 0, 0)< 0) { + vshError(ctl, _("Failed to create device description")); + goto cleanup; + } + + device_desc = (char *) xmlBufferContent(xml_buf); + + if (virDomainDetachDeviceFlags(dom, device_desc, VIR_DOMAIN_AFFECT_CURRENT)< 0) { + vshError(ctl, + _("Failed to remove storage device '%s' from definition. " + "If domain undefinition fails, this domain may remain inconsistent"), + volume); + } + + xml_dev = virXMLParseStringCtxt(device_desc, + _("(storage volume definition)"), +&ctxt_dev); + + /* find the target and lookup the image in storage pools */ + if (!xml_dev) + goto cleanup; + + volume_path = virXPathString("string(//disk/source/@file" + "| //disk/source/@dev" + "| //disk/source/@dir" + "| //disk/source/@name)", ctxt_dev);
Hmm, looks like your helper function that maps names back to devices should return both the disk device name (vda) and source path (/path/to/image).
+ + if (!volume_path) { + vshPrint(ctl, + _("Virtual storage device '%s' has no associated volume. Skipping."), + volume); + goto next_volume; + } + + vol = virStorageVolLookupByPath(ctl->conn, volume_path);
We _really_ need a libvirt function that returns a list of virStorageVolPtr objects for all disks associated with a domain (as well as virsh machinery to fake that when talking to older servers that lack the API). Something like that would also help my snapshot work, but it's on my post-0.9.5 plate. It would also be handy to operate on virStorageVolPtr that does not belong to virStoragePoolPtr (that is, a rogue image file with no associated pool), which means we also need a way to get from a virStorageVolPtr back to its owning pool or back to NULL if the volume is rogue.
+ + if (!vol) { + vshPrint(ctl, + _("Storage volume '%s' is not managed by libvirt. Remove it manualy.\n"),
s/manualy/manually/
+ volume_path); + goto next_volume;
When printing messages like this, be sure to change the exit status - that is, the undefine succeeds, but the overall status should be non-zero.
+ } + + wipe_failed = false; + + /* wipe the volume */ + if (wipe_storage) { + vshPrint(ctl, _("Wiping volume '%s' ... "), volume_path); + if (virStorageVolWipe(vol, 0)< 0) { + vshPrint(ctl, _("Failed!\n")); + wipe_failed = true; + } else { + vshPrint(ctl, _("Done.\n")); + } + } + + + /* delete the volume */ + if (wipe_failed || virStorageVolDelete(vol, 0)< 0) { + vshError(ctl, + _("Failed to remove storage volume '%s'. Remove it manualy."),
s/manualy/manually/
+ volume_path); + } else { + vshPrint(ctl, _("Volume '%s' deleted.\n"), volume_path); + } + + +next_volume: + VIR_FREE(volume_path); + if (vol) + virStorageVolFree(vol); + vol = NULL; + xmlXPathFreeContext(ctxt_dev); + ctxt_dev = NULL; + xmlFreeDoc(xml_dev); + xml_dev = NULL; + + volume = strtok_r(NULL, ",",&saveptr); + } + /* domain's storage removed */ + } + /* 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
See above comments about reordering things - parse out the disks before domain undefine, but don't act on them until after domain undefine.
=item B<undefine> I<domain-id> [I<--managed-save>] [I<--snapshots-metadata] +[I<--storage volumes>] [I<--wipe-storage>]
--wipe-storage only makes sense with --storage volumes. Also, I'd format volumes in bold, rather than italic, like this: [I<--storage> B<volumes> [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, @@ -1082,6 +1083,17 @@ 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 volumes, that is a comma separated
s/parameter volumes/parameter B<volumes>/
+list of volume target names of storage volumes to be removed along with +the undefined domain. This operation is valid only for a inactive domain. +If one or more of the operations while removing disk storage fail, the +domain may still be undefined and some volumes may remain undeleted.
Tweak wording to state: Volume deletion is only attempted after the domain is undefined; if not all of the requested volumes could be deleted, then the error message indicates what still remains behind.
+(See B<domblklist> for list of target names associated to a domain). +Example: --storage vda,vdb,vdc + +The Flag I<--wipe-storage> specifies, that the storage volumes should be
s/Flag/flag/ s/specifies, that/specifies that/
+wiped before removal. + NOTE: For an inactive domain, the domain name or UUID must be used as the I<domain-id>.
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa