[libvirt] [PATCH] Allow off-line removal of devices via xend
# HG changeset patch # User john.levon@sun.com # Date 1233765613 28800 # Node ID bda41ea0cbbdea409447686c30b7afb10b9cae85 # Parent e6b17082b9b9440ce14ad76bd2f4c003ae2404db Allow off-line removal of devices via xend We must avoid xenstore if the domain isn't running. For disks, this works in all xend versions; for NICs, however, this requires a change in netif.py to allow matching by MAC address. Preferring the device ID but falling back to the device ref works best with all versions of xend. Signed-off-by: John Levon <john.levon@sun.com> diff --git a/src/xend_internal.c b/src/xend_internal.c --- a/src/xend_internal.c +++ b/src/xend_internal.c @@ -92,11 +92,17 @@ xenDaemonFormatSxprNet(virConnectPtr con int xendConfigVersion, int isAttach); static int -virDomainXMLDevID(virDomainPtr domain, - virDomainDeviceDefPtr dev, - char *class, - char *ref, - int ref_len); +virDomainDevRef(virDomainPtr domain, + virDomainDeviceDefPtr dev, + char *class, + char *ref, + int ref_len); +static int +virDomainDevID(virDomainPtr domain, + int type, + const char *ref, + char *devid, + int devid_len); #endif #define virXendError(conn, code, fmt...) \ @@ -3857,7 +3863,7 @@ xenDaemonAttachDevice(virDomainPtr domai virDomainDeviceDefPtr dev = NULL; virDomainDefPtr def = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - char class[8], ref[80]; + char class[8], ref[80], devid[80]; if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { virXendError((domain ? domain->conn : NULL), VIR_ERR_INVALID_ARG, @@ -3913,15 +3919,24 @@ xenDaemonAttachDevice(virDomainPtr domai sexpr = virBufferContentAndReset(&buf); - if (virDomainXMLDevID(domain, dev, class, ref, sizeof(ref))) { + devid[0] = '\0'; + + if (virDomainDevRef(domain, dev, class, ref, sizeof(ref)) == -1) + goto cleanup; + + if (domain->id > 0 && + virDomainDevID(domain, dev->type, ref, devid, sizeof(devid)) != -1) { + VIR_DEBUG("device_configure(ref = %s, devid = %s): %s", + ref, devid, sexpr); + /* device exists, attempt to modify it */ + ret = xend_op(domain->conn, domain->name, "op", "device_configure", + "config", sexpr, "dev", devid, NULL); + } else { + VIR_DEBUG("device_create(ref = %s, devid = %s): %s", + ref, devid, sexpr); /* device doesn't exist, define it */ ret = xend_op(domain->conn, domain->name, "op", "device_create", "config", sexpr, NULL); - } - else { - /* device exists, attempt to modify it */ - ret = xend_op(domain->conn, domain->name, "op", "device_configure", - "config", sexpr, "dev", ref, NULL); } cleanup: @@ -3944,7 +3959,7 @@ xenDaemonDetachDevice(virDomainPtr domai xenDaemonDetachDevice(virDomainPtr domain, const char *xml) { xenUnifiedPrivatePtr priv; - char class[8], ref[80]; + char class[8], ref[80], devid[80]; virDomainDeviceDefPtr dev = NULL; virDomainDefPtr def = NULL; int ret = -1; @@ -3975,11 +3990,40 @@ xenDaemonDetachDevice(virDomainPtr domai def, xml, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; - if (virDomainXMLDevID(domain, dev, class, ref, sizeof(ref))) + if (dev->type != VIR_DOMAIN_DEVICE_NET && + dev->type != VIR_DOMAIN_DEVICE_DISK) { + virXendError(domain->conn, VIR_ERR_INTERNAL_ERROR, + _("can't detach device of type %d"), dev->type); goto cleanup; + } + + /* + * First acquire a static reference to the device (MAC address or + * destination disk). We'll use this if the domain isn't running, + * otherwise we'll rely upon the XenStore devid. + */ + + if (virDomainDevRef(domain, dev, class, ref, sizeof(ref)) == -1) { + virXendError(domain->conn, VIR_ERR_INTERNAL_ERROR, + _("can't detach device: not found ")); + goto cleanup; + } + + if (domain->id >= 0) { + if (virDomainDevID(domain, dev->type, ref, devid, sizeof(devid))) + virXendError(domain->conn, VIR_ERR_INTERNAL_ERROR, + _("can't detach device: not found ")); + goto cleanup; + } else { + strncpy(devid, ref, 80); + } + + VIR_DEBUG("device_destroy(type = %s, ref = %s, devid = %s)", + class, ref, devid); ret = xend_op(domain->conn, domain->name, "op", "device_destroy", - "type", class, "dev", ref, "force", "0", "rm_cfg", "1", NULL); + "type", class, "dev", devid, "force", "0", + "rm_cfg", "1", NULL); cleanup: virDomainDefFree(def); @@ -5501,12 +5545,48 @@ error: /** - * virDomainXMLDevID: + * virDomainXMLDevRef: * @domain: pointer to domain object * @dev: pointer to device config object * @class: Xen device class "vbd" or "vif" (OUT) * @ref: Xen device reference (OUT) * + * Return the class (vbd/vif) and reference (MAC address / disk + * destination) of the given device. + * + * Returns 0 in case of success, -1 in case of failure. + */ +static int +virDomainDevRef(virDomainPtr domain, + virDomainDeviceDefPtr dev, + char *class, + char *ref, + int ref_len) +{ + if (dev->type == VIR_DOMAIN_DEVICE_DISK) { + if (dev->data.disk->dst == NULL) + return -1; + strncpy(ref, dev->data.disk->dst, ref_len); + strcpy(class, "vbd"); + } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { + virFormatMacAddr(dev->data.net->mac, ref); + strcpy(class, "vif"); + } else { + virXendError(NULL, VIR_ERR_NO_SUPPORT, + _("no support for device type %d"), dev->type); + return -1; + } + + return 0; +} + +/** + * virDomainXMLDevID: + * @domain: pointer to domain object + * @class: either 'vbd' or 'vif' + * @ref: Xen device reference + * @devid: Xen device ID (OUT) + * * Set class according to XML root, and: * - if disk, copy in ref the target name from description * - if network, get MAC address from description, scan XenStore and @@ -5515,54 +5595,41 @@ error: * Returns 0 in case of success, -1 in case of failure. */ static int -virDomainXMLDevID(virDomainPtr domain, - virDomainDeviceDefPtr dev, - char *class, - char *ref, - int ref_len) +virDomainDevID(virDomainPtr domain, + int type, + const char *ref, + char *devid, + int devid_len) { xenUnifiedPrivatePtr priv = domain->conn->privateData; - char *xref; - - if (dev->type == VIR_DOMAIN_DEVICE_DISK) { - strcpy(class, "vbd"); - if (dev->data.disk->dst == NULL) - return -1; - xenUnifiedLock(priv); - xref = xenStoreDomainGetDiskID(domain->conn, domain->id, - dev->data.disk->dst); - xenUnifiedUnlock(priv); - if (xref == NULL) - return -1; - - strncpy(ref, xref, ref_len); - free(xref); - ref[ref_len - 1] = '\0'; - } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { - char mac[30]; - virDomainNetDefPtr def = dev->data.net; - snprintf(mac, sizeof(mac), "%02x:%02x:%02x:%02x:%02x:%02x", - def->mac[0], def->mac[1], def->mac[2], - def->mac[3], def->mac[4], def->mac[5]); - - strcpy(class, "vif"); - - xenUnifiedLock(priv); - xref = xenStoreDomainGetNetworkID(domain->conn, domain->id, - mac); - xenUnifiedUnlock(priv); - if (xref == NULL) - return -1; - - strncpy(ref, xref, ref_len); - free(xref); - ref[ref_len - 1] = '\0'; + char *id = NULL; + + if (domain->id < 0) { + virXendError(domain->conn, VIR_ERR_INTERNAL_ERROR, + _("devid not available for shutdown domain")); + return -1; + } + + xenUnifiedLock(priv); + + if (type == VIR_DOMAIN_DEVICE_DISK) { + id = xenStoreDomainGetDiskID(domain->conn, domain->id, ref); + + } else if (type == VIR_DOMAIN_DEVICE_NET) { + id = xenStoreDomainGetNetworkID(domain->conn, domain->id, ref); } else { virXendError(NULL, VIR_ERR_NO_SUPPORT, - "%s", _("hotplug of device type not supported")); + _("no support for device type %d"), type); + } + + xenUnifiedUnlock(priv); + + if (id == NULL) return -1; - } - + + strncpy(devid, id, devid_len); + VIR_FREE(id); + devid[devid_len - 1] = '\0'; return 0; }
john.levon@sun.com wrote:
# HG changeset patch # User john.levon@sun.com # Date 1233765613 28800 # Node ID bda41ea0cbbdea409447686c30b7afb10b9cae85 # Parent e6b17082b9b9440ce14ad76bd2f4c003ae2404db Allow off-line removal of devices via xend
We must avoid xenstore if the domain isn't running. For disks, this works in all xend versions; for NICs, however, this requires a change in netif.py to allow matching by MAC address. Preferring the device ID but falling back to the device ref works best with all versions of xend.
No fault of this patch, but this makes me ask the question: what exactly are attachDevice and detachDevice supposed to do? In the qemu driver, it is entirely hotplug/hotunplug, and deliberately fails on an inactive domain. This patch seems to add offline support akin to 'remove disk device from xml and redefine' (or maybe it's always attempted that in the xen driver?). The documentation in libvirt.c is pretty ambiguous, so it would be nice if we could clarify this. Thanks, Cole
On Mon, Feb 09, 2009 at 11:30:30AM -0500, Cole Robinson wrote:
No fault of this patch, but this makes me ask the question: what exactly are attachDevice and detachDevice supposed to do?
In the qemu driver, it is entirely hotplug/hotunplug, and deliberately fails on an inactive domain.
So to add a device with qemu, you need to edit XML? Reason alone for these methods to work on offline domains I think :) regards john
On Mon, Feb 09, 2009 at 11:30:30AM -0500, Cole Robinson wrote: [..snip..]
No fault of this patch, but this makes me ask the question: what exactly are attachDevice and detachDevice supposed to do?
In the qemu driver, it is entirely hotplug/hotunplug, and deliberately fails on an inactive domain. This patch seems to add offline support We don't even persist the devices being hot{added,removed} into the domain xml so acting on inactive domains has no meaning or am I missing something? Cheers, -- Guido
Guido Günther wrote:
On Mon, Feb 09, 2009 at 11:30:30AM -0500, Cole Robinson wrote: [..snip..]
No fault of this patch, but this makes me ask the question: what exactly are attachDevice and detachDevice supposed to do?
In the qemu driver, it is entirely hotplug/hotunplug, and deliberately fails on an inactive domain. This patch seems to add offline support We don't even persist the devices being hot{added,removed} into the domain xml so acting on inactive domains has no meaning or am I missing something?
When the domain is enabled, and you store it... you basically get that behavior. Stefan
On Tue, Feb 10, 2009 at 09:32:49PM +0100, Stefan de Konink wrote:
Guido Günther wrote:
On Mon, Feb 09, 2009 at 11:30:30AM -0500, Cole Robinson wrote: [..snip..]
No fault of this patch, but this makes me ask the question: what exactly are attachDevice and detachDevice supposed to do?
In the qemu driver, it is entirely hotplug/hotunplug, and deliberately fails on an inactive domain. This patch seems to add offline support We don't even persist the devices being hot{added,removed} into the domain xml so acting on inactive domains has no meaning or am I missing something?
When the domain is enabled, and you store it... you basically get that behavior. Exactly my point: when the domain is *enabled*. -- Guido
On Wed, Feb 11, 2009 at 10:01:24AM +0100, Guido Günther wrote:
In the qemu driver, it is entirely hotplug/hotunplug, and deliberately fails on an inactive domain. This patch seems to add offline support We don't even persist the devices being hot{added,removed} into the domain xml so acting on inactive domains has no meaning or am I missing something?
When the domain is enabled, and you store it... you basically get that behavior. Exactly my point: when the domain is *enabled*.
This is the xend driver, not the qemu one. regards john
John Levon wrote:
On Wed, Feb 11, 2009 at 10:01:24AM +0100, Guido Günther wrote:
In the qemu driver, it is entirely hotplug/hotunplug, and deliberately fails on an inactive domain. This patch seems to add offline support We don't even persist the devices being hot{added,removed} into the domain xml so acting on inactive domains has no meaning or am I missing something? When the domain is enabled, and you store it... you basically get that behavior. Exactly my point: when the domain is *enabled*.
This is the xend driver, not the qemu one.
I am unable to see the point? This function is very good for developers :) So apply :) Stefan
Guido Günther wrote:
On Tue, Feb 10, 2009 at 09:32:49PM +0100, Stefan de Konink wrote:
Guido Günther wrote:
On Mon, Feb 09, 2009 at 11:30:30AM -0500, Cole Robinson wrote: [..snip..]
No fault of this patch, but this makes me ask the question: what exactly are attachDevice and detachDevice supposed to do?
In the qemu driver, it is entirely hotplug/hotunplug, and deliberately fails on an inactive domain. This patch seems to add offline support We don't even persist the devices being hot{added,removed} into the domain xml so acting on inactive domains has no meaning or am I missing something? When the domain is enabled, and you store it... you basically get that behavior. Exactly my point: when the domain is *enabled*.
I'm a little lost. My point was basically that with this patch to xend, attach/detach has different meanings between the qemu + xend driver. qemu : vm active : attempt to hotplug the device and persist xml changes on success, raise error if device is not not hotpluggable vm inactive : deliberately fail: 'vm must be active' xen : vm active : attempt to hotplug device, unknown if changes are persisted, unknown result if device is not hotpluggable (unknown to me, anyways) vm inactive : _dont_ fail, add device to VM config The libvirt docs for attach/detach device are: attachDevice: Create a virtual device attachment to backend. detachDevice: Destroy a virtual device attachment to backend. Not very helpful. All I want is some understanding of what attach/detach is supposed to mean :) Adding the device to the xml for an inactive domain is fine by me (though it would essentially be a convenience function), but I would like to ensure that if the domain is active and the device is not hotpluggable, we throw a failure. Without that, there isn't any way to determine that attachDevice on the active domain did anything immediately useful. Thanks, Cole
On Fri, Feb 13, 2009 at 11:35:09AM -0500, Cole Robinson wrote:
Adding the device to the xml for an inactive domain is fine by me (though it would essentially be a convenience function), but I would like to ensure that if the domain is active and the device is not hotpluggable, we throw a failure. Without that, there isn't any way to determine that attachDevice on the active domain did anything immediately useful.
But there's then also no way to attach a device for after a reboot. The interfaces have messed up the idea of runtime versus temporary versus permanent changes, and I think we have to muddle along as best we can... regards john
On Fri, Feb 13, 2009 at 11:35:09AM -0500, Cole Robinson wrote:
Guido Günther wrote:
On Tue, Feb 10, 2009 at 09:32:49PM +0100, Stefan de Konink wrote:
Guido Günther wrote:
On Mon, Feb 09, 2009 at 11:30:30AM -0500, Cole Robinson wrote: [..snip..]
No fault of this patch, but this makes me ask the question: what exactly are attachDevice and detachDevice supposed to do?
In the qemu driver, it is entirely hotplug/hotunplug, and deliberately fails on an inactive domain. This patch seems to add offline support We don't even persist the devices being hot{added,removed} into the domain xml so acting on inactive domains has no meaning or am I missing something? When the domain is enabled, and you store it... you basically get that behavior. Exactly my point: when the domain is *enabled*.
I'm a little lost. My point was basically that with this patch to xend, attach/detach has different meanings between the qemu + xend driver.
qemu : vm active : attempt to hotplug the device and persist xml
changes on success, raise error if device is not not hotpluggable
vm inactive : deliberately fail: 'vm must be active'
xen : vm active : attempt to hotplug device, unknown if changes are persisted, unknown result if device is not hotpluggable (unknown to me, anyways)
vm inactive : _dont_ fail, add device to VM config
Actually, that's not correct. For Xen <= 3.0.3, the XM driver is used for inactive guests, and that already supports attach/detach. IMHO, it is reasonable to support these operations for inactive domains, so we should do so for all drivers, where practical.
The libvirt docs for attach/detach device are:
attachDevice: Create a virtual device attachment to backend. detachDevice: Destroy a virtual device attachment to backend.
Not very helpful. All I want is some understanding of what attach/detach is supposed to mean :)
When first added it was only implemented for the XenD driver for active domains. The XM driver support came later. The docs should be updated to to - If the VM is active, it attempts to hotplug the device to the guest - If the VM is inactive, it updates the VM config file. We cannot guarentee whether the active hotplugging will update the persistent config or not, but if possible it should. Currenty XenD appears to update the config from what I see, though haven't tested it for real yet Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Feb 13, 2009 at 05:00:40PM +0000, Daniel P. Berrange wrote:
- If the VM is active, it attempts to hotplug the device to the guest - If the VM is inactive, it updates the VM config file.
We cannot guarentee whether the active hotplugging will update the persistent config or not, but if possible it should. Currenty XenD appears to update the config from what I see, though haven't tested it for real yet
It will update the persistent config, but there's an annoying bug: the in-xend-memory version of the config isn't updated. So when the domain shuts down, you have to restart xend to get the real config. I haven't got around to working out the fix for this yet. regards john
On Fri, Feb 13, 2009 at 05:00:40PM +0000, Daniel P. Berrange wrote: [..snip..]
We cannot guarentee whether the active hotplugging will update the persistent config or not, but if possible it should. Currenty XenD appears to update the config from what I see, though haven't tested it for real yet Qemu doesn't update the persistent config, so should we add this? Or woutld it be better to add a new set of functions that have a flags argument like VIRT_PERSIST (I think you suggested somehting like this once) and leave the behaviour for attacheDevice/detachDevice undefined.
The current behaviour of attachDevice/detachDevice is a bit of a nightmare for tools like virt-manager since it's the behaviour is by no means hypervisor independent. -- Guido
participants (7)
-
Cole Robinson -
Daniel P. Berrange -
Guido Günther -
John Levon -
John Levon -
john.levon＠sun.com
-
Stefan de Konink