[libvirt] [PATCH RFC 0/3] undefine: Interface for undefinig storage devices along with domains
This extension of flags used while undefining domains grants the user ability to remove virtual storage devices while undefining a domain. This is only a demonstration, implemented only for the QEMU/KVM emulators. I'd like to know your opinions and comments and soon i'll provide patches for other emulators as well. Thanks for your precious time :) Peter Peter Krempa (3): undefine: Add flags for removing storage files when undefining a domain. undefine: QEMU driver - remove storage associated with a domain while undefining undefine: Add coresponding switches to virsh, for removing storage devices associated with domain include/libvirt/libvirt.h.in | 2 ++ src/qemu/qemu_driver.c | 32 +++++++++++++++++++++++++++++++- tools/virsh.c | 17 ++++++++++++++++- 3 files changed, 49 insertions(+), 2 deletions(-) -- 1.7.6
Adds option for virDomainUndefineFlags to remove storage associated with the domain while undefining it. VIR_DOMAIN_UNDEFINE_DISK_FILE - remove disk files, that aren't managed by storage pools. VIR_DOMAIN_UNDEFINE_DISK_BACKEND - remove storage devices that are managed using libvirt storage pools. --- include/libvirt/libvirt.h.in | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 40ce0fc..f99bc8b 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1203,6 +1203,8 @@ int virDomainUndefine (virDomainPtr domain); typedef enum { VIR_DOMAIN_UNDEFINE_MANAGED_SAVE = (1 << 0), + VIR_DOMAIN_UNDEFINE_DISK_FILE = (1 << 1), + VIR_DOMAIN_UNDEFINE_DISK_BACKEND = (1 << 2), /* Future undefine control flags should come here. */ } virDomainUndefineFlagsValues; -- 1.7.6
On 07/21/2011 09:12 AM, Peter Krempa wrote:
Adds option for virDomainUndefineFlags to remove storage associated with the domain while undefining it.
VIR_DOMAIN_UNDEFINE_DISK_FILE - remove disk files, that aren't managed by storage pools.
VIR_DOMAIN_UNDEFINE_DISK_BACKEND - remove storage devices that are managed using libvirt storage pools. --- include/libvirt/libvirt.h.in | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 40ce0fc..f99bc8b 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1203,6 +1203,8 @@ int virDomainUndefine (virDomainPtr domain);
typedef enum { VIR_DOMAIN_UNDEFINE_MANAGED_SAVE = (1<< 0), + VIR_DOMAIN_UNDEFINE_DISK_FILE = (1<< 1), + VIR_DOMAIN_UNDEFINE_DISK_BACKEND = (1<< 2),
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Jul 21, 2011 at 05:12:14PM +0200, Peter Krempa wrote:
Adds option for virDomainUndefineFlags to remove storage associated with the domain while undefining it.
VIR_DOMAIN_UNDEFINE_DISK_FILE - remove disk files, that aren't managed by storage pools.
VIR_DOMAIN_UNDEFINE_DISK_BACKEND - remove storage devices that are managed using libvirt storage pools. --- include/libvirt/libvirt.h.in | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 40ce0fc..f99bc8b 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1203,6 +1203,8 @@ int virDomainUndefine (virDomainPtr domain);
typedef enum { VIR_DOMAIN_UNDEFINE_MANAGED_SAVE = (1 << 0), + VIR_DOMAIN_UNDEFINE_DISK_FILE = (1 << 1), + VIR_DOMAIN_UNDEFINE_DISK_BACKEND = (1 << 2),
/* Future undefine control flags should come here. */ } virDomainUndefineFlagsValues;
I don't really like us going down this route. While it was easy for managed save files, when you start thinking about deleting disk storage life becomes alot more complicated, and you almost certainly need to make different decisions for different disks in a VM. As such having a VM level flag for deleting all disks, or not is wrong and IMHO quite dangerous. Dealing with failure of the API when using these flags also gets alot more complicated. If this API fails with these flags set you may or may not have deleted one or more disks. The application has to then do a bunch more API calls to try and figure out just which part of the delete request failed & whether they need to issue further API calls to clean up the mess. So to actually use these flags and correctly cope with failure ends up being just as much code as you would have to write to just call our existing storage volume APIs, or possibly even more. NACK to this concept I'm afraid. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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
On Thu, Jul 21, 2011 at 05:12:15PM +0200, Peter Krempa wrote:
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); + } + } + } + }
The way this code deals with errors deleting disks is the pefect example of why adding these flags to virDomainUndefine is a bad idea. Any serious user of these APIs, now needs to use all the virStorageVol APIs anyway to check if the disks for the VM really were deleted. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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
On Thu, Jul 21, 2011 at 05:12:16PM +0200, Peter Krempa wrote:
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
I don't think this is the right level of granularity because you really want control over individual disks to be removed, the disctinction of whether a disk is from a storage pool or not is not relevant to the task at hand. eg, Consider a VM with 4 disks in the config, sda, sdb, sdc and sdd and you want to delete sda and sdc, but not sdb since that it on iSCSI which we can't remote delete, and not sdd since that's a data disk we want to keep for a future VM. virsh undefine foo --remove-disks=sda,sdc Or delete all writable disks: virsh undefine foo --remove-disks It would be best if virsh would print a confirmation for each disk removed, so the admin knows whether it succeeded or not # virsh undefine foo --remove-disk-names=sda,sdc Deleted disk /path/to/disk/for/sda Failed to delete disk /path/to/disk/for/sdc # virsh undefine foo --remove-disk-names=sda,sdc Deleted disk /path/to/disk/for/sda Deleted disk /path/to/disk/for/sdc Undefined VM foo Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/21/2011 11:11 AM, Daniel P. Berrange wrote:
It would be best if virsh would print a confirmation for each disk removed, so the admin knows whether it succeeded or not
# virsh undefine foo --remove-disk-names=sda,sdc Deleted disk /path/to/disk/for/sda Failed to delete disk /path/to/disk/for/sdc
Note also that an idea like this is implementable in virsh even without adding a new flag to the virDomainUndefineFlags API. That is, there's nothing wrong with a single virsh command making multiple libvirt API calls to nicely tie together multiple concepts into an easy-to-use UI. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa