On 07/21/2011 09:12 AM, Peter Krempa wrote: At first glance, I like the idea; however, it needs documentation in libvirt.c (that is, the official virDomainUndefineFlags must mention the semantic of these flags, so that we can uniformly implement those semantics across all hypervisors where we add support). The names could be tweaked: VIR_DOMAIN_UNDEFINE_REMOVE_UNMANAGED_DISKS - affect only disks not belonging to a pool VIR_DOMAIN_UNDEFINE_REMOVE_MANAGED_DISKS - affect only disks belonging to a pool But on thinking further... We have to be careful - since undefine is supposed to work on running guests (turning persistent into transient), these flags must cause undefine to fail on a running guest. Conversely, if we have a transient guest, we use virDomainShutdown/virDomainDestroy to end the domain, not virDomainUndefine, so those APIs would _also_ need the same flags argument, to give the same semantics of storage cleanup on death of the domain. Are those flags sufficient? What about the distinction in readonly/shared/readwrite disks - do you delete a readonly .iso file mounted as a virtual cdrom disk, or just readwrite storage? Meanwhile, this is all-or-nothing, on the other hand, if you have a scenario where you want some, but not all, disks removed, then you still have access to the virStorageVol APIs to do that removal, at least for managed disks. I don't know if there is any way to make it more fine-tuned, short of adding some XML attributes to each <disk> element of the domain definition on whether the backing storage of the disk gets auto-deleted on undefine. And if you update XML to support auto-deletion of a disk on domain, then it can be argued that you no longer need this flag on the virDomainUndefineFlags path - instead, the undefine path inspects the XML and fails if the requested auto-deletion fails. Or maybe we combine the aspects - the XML has an optional element to say what happens on undefine, but for every disk where that element is not present, then: if the disk is shared or readonly, the default is leave the storage alone, and if the disk is readwrite, the default is to raise an undefine error if no flags were passed. Then you have two mutually exclusive flags to undefine - one says to change a default error to instead leave the storage untouched, and the other says to change a default error to instead remove the storage. Neither flag is needed if the XML fully specifies the behavior on all disks. Meanwhile, this points out that we are probably missing some API for unmanaged disks (that is, a volume in use by a domain, but not belonging to a storage pool). Other holes - we don't have any way, given a virStorageVolPtr, to get back to the virStoragePoolPtr that contains it (or, if we use virStorageVolPtr for unmanaged disks, to return an error indicating that it is unmanaged). Also, we don't have a way, given a virDomainPtr, to enumerate all the storage volumes associated with that domain as a list of virStorageVolPtr. Adding more APIs for storage volume management would also be useful for my work with snapshots. I guess we add things incrementally as we go. Anyways, thanks for bringing up the ideas - there's lots of potential for improvement in this area! -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

qemu specific code to remove associated storage to domains being undefined with the virDomainUndefineFlags function. Storage devices marked as shared aren't removed. The code checks path of domain's storage devices in the storage pools and acts accordingly to flags passed to the undefine function. --- src/qemu/qemu_driver.c | 32 +++++++++++++++++++++++++++++++- 1 files changed, 31 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cd65bce..03a079f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4320,8 +4320,14 @@ qemuDomainUndefineFlags(virDomainPtr dom, virDomainEventPtr event = NULL; char *name = NULL; int ret = -1; + int i = 0; + virDomainDiskDefPtr disk = NULL; + virConnectPtr conn = NULL; + virStorageVolPtr volume = NULL; - virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE, -1); + virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | + VIR_DOMAIN_UNDEFINE_DISK_FILE | + VIR_DOMAIN_UNDEFINE_DISK_BACKEND , -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -4366,6 +4372,30 @@ qemuDomainUndefineFlags(virDomainPtr dom, } } + // remove disk images while undefinig the domain + if ((flags & VIR_DOMAIN_UNDEFINE_DISK_FILE || flags & VIR_DOMAIN_UNDEFINE_DISK_BACKEND) && + ((conn = virDomainGetConnect(dom)) != NULL)) { + + for (i = 0; i < vm->def->ndisks; i++) { + disk = (vm->def->disks)[i]; + + // lookup the device in the storage pool + if ((volume = virStorageVolLookupByPath(conn, disk->src)) == NULL) { + if (disk->type == VIR_DOMAIN_DISK_TYPE_FILE && !disk->shared) { + if (unlink(disk->src) < 0) + VIR_WARN("Couldn't remove disk image file: '%s' while undefining domain '%s'", disk->src, vm->def->name); + else + VIR_INFO("Disk image '%s' removed.", disk->src); + } + } else { + if (!disk->shared) { + if(virStorageVolDelete(volume, 0) == -1) + VIR_WARN("Couldn't remove storage volume with path '%s' while undefining domain '%s'", disk->src, vm->def->name); + } + } + } + } + if (virDomainDeleteConfig(driver->configDir, driver->autostartDir, vm) < 0) goto cleanup; -- 1.7.6

Adds switches for the virsh command "undefine", for managing removal of associated storage devices for the undefined domain. --remove-unmanaged-disks - removes only disk images, that aren't a part of any libvirt storage pool. Only file-based storage devices are removed. --remove-managed-disks - removes only libvirt managed storage devices --remove-all-disks - removes both managed and unmanaged storage devices --- tools/virsh.c | 17 ++++++++++++++++- 1 files changed, 16 insertions(+), 1 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index ca92f0c..9d2808f 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1421,6 +1421,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")}, + {"remove-unmanaged-disks", VSH_OT_BOOL, 0, N_("remove unmanaged storage images")}, + {"remove-managed-disks", VSH_OT_BOOL, 0, N_("remove disks managed in storage pools")}, + {"remove-all-disks", VSH_OT_BOOL, 0, N_("remove all disks associated with the undefined domain")}, {NULL, 0, 0, NULL} }; @@ -1433,13 +1436,25 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) int id; int flags = 0; int managed_save = vshCommandOptBool(cmd, "managed-save"); + int remove_unmanaged_disks = vshCommandOptBool(cmd, "remove-unmanaged-disks"); + int remove_backend_disks = vshCommandOptBool(cmd, "remove-managed-disks"); + int remove_all_disks = vshCommandOptBool(cmd, "remove-all-disks"); int has_managed_save = 0; int rc = -1; if (managed_save) flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE; - if (!managed_save) + if (remove_unmanaged_disks || remove_all_disks) + flags |= VIR_DOMAIN_UNDEFINE_DISK_FILE; + + if (remove_backend_disks || remove_all_disks) + flags |= VIR_DOMAIN_UNDEFINE_DISK_BACKEND; + + if (!managed_save && + !remove_unmanaged_disks && + !remove_backend_disks && + !remove_all_disks) flags = -1; if (!vshConnectionUsability(ctl, ctl->conn)) -- 1.7.6