[libvirt] [PATCH RFC v2] virsh: Add option to undefine storage with domains
Eric Blake
eblake at redhat.com
Tue Sep 13 23:35:59 UTC 2011
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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list