[libvirt] [PATCH] libxl: support ACPI shutdown flag
Add support for VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag in libxlDomainShutdownFlags(). Inspired by similar functionality in the Xen xl client. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- I considered invoking libxl_send_trigger() immediately when VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag is specified, but in the end decided to only honor the flag if a "normal" shutdown request failed. This behavior is similar to xl and conforms to the virDomainShutdownFlags() docs "If @flags is set to zero, then the hypervisor will choose the method of shutdown it considers best. To have greater control pass one or more of the virDomainShutdownFlagValues. The order in which the hypervisor tries each shutdown method is undefined, and a hypervisor is not required to support all methods." I'm certainly receptive to only invoking libxl_send_trigger() when VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN is specified if folks think that is a better approach. src/libxl/libxl_driver.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b3f8df6..2b30c08 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -868,7 +868,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags) int ret = -1; libxlDomainObjPrivatePtr priv; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN, -1); if (!(vm = libxlDomObjFromDomain(dom))) goto cleanup; @@ -883,17 +883,35 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags) } priv = vm->privateData; - if (libxl_domain_shutdown(priv->ctx, vm->def->id) != 0) { + ret = libxl_domain_shutdown(priv->ctx, vm->def->id); + if (ret == ERROR_NOPARAVIRT) { + if (flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) { + VIR_DEBUG("PV control interface not available, " + "sending ACPI power button event."); + ret = libxl_send_trigger(priv->ctx, vm->def->id, + LIBXL_TRIGGER_POWER, 0); + } else { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("PV control interface not available, " + "preventing external graceful shutdown. " + "Consider using an ACPI power event.")); + ret = -1; + goto cleanup; + } + } + + if (ret != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to shutdown domain '%d' with libxenlight"), vm->def->id); + ret = -1; goto cleanup; } - /* vm is marked shutoff (or removed from domains list if not persistent) + /* ret == 0 == successful shutdown request + * vm is marked shutoff (or removed from domains list if not persistent) * in shutdown event handler. */ - ret = 0; cleanup: if (vm) -- 1.8.1.4
Any comments on this small patch? If possible, I'd like to push it for the 1.2.4 release. Thanks! Jim Jim Fehlig wrote:
Add support for VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag in libxlDomainShutdownFlags(). Inspired by similar functionality in the Xen xl client.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
I considered invoking libxl_send_trigger() immediately when VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag is specified, but in the end decided to only honor the flag if a "normal" shutdown request failed. This behavior is similar to xl and conforms to the virDomainShutdownFlags() docs
"If @flags is set to zero, then the hypervisor will choose the method of shutdown it considers best. To have greater control pass one or more of the virDomainShutdownFlagValues. The order in which the hypervisor tries each shutdown method is undefined, and a hypervisor is not required to support all methods."
I'm certainly receptive to only invoking libxl_send_trigger() when VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN is specified if folks think that is a better approach.
src/libxl/libxl_driver.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b3f8df6..2b30c08 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -868,7 +868,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags) int ret = -1; libxlDomainObjPrivatePtr priv;
- virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN, -1);
if (!(vm = libxlDomObjFromDomain(dom))) goto cleanup; @@ -883,17 +883,35 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags) }
priv = vm->privateData; - if (libxl_domain_shutdown(priv->ctx, vm->def->id) != 0) { + ret = libxl_domain_shutdown(priv->ctx, vm->def->id); + if (ret == ERROR_NOPARAVIRT) { + if (flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) { + VIR_DEBUG("PV control interface not available, " + "sending ACPI power button event."); + ret = libxl_send_trigger(priv->ctx, vm->def->id, + LIBXL_TRIGGER_POWER, 0); + } else { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("PV control interface not available, " + "preventing external graceful shutdown. " + "Consider using an ACPI power event.")); + ret = -1; + goto cleanup; + } + } + + if (ret != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to shutdown domain '%d' with libxenlight"), vm->def->id); + ret = -1; goto cleanup; }
- /* vm is marked shutoff (or removed from domains list if not persistent) + /* ret == 0 == successful shutdown request + * vm is marked shutoff (or removed from domains list if not persistent) * in shutdown event handler. */ - ret = 0;
cleanup: if (vm)
On Mon, 2014-04-28 at 11:24 -0600, Jim Fehlig wrote:
Any comments on this small patch? If possible, I'd like to push it for the 1.2.4 release.
FWIW your use of the libxl interfaces seems correct to me, so Ack in that regard. Ian.
On Tue, Apr 22, 2014 at 11:12:16AM -0600, Jim Fehlig wrote:
Add support for VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag in libxlDomainShutdownFlags(). Inspired by similar functionality in the Xen xl client.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
I considered invoking libxl_send_trigger() immediately when VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag is specified, but in the end decided to only honor the flag if a "normal" shutdown request failed. This behavior is similar to xl and conforms to the virDomainShutdownFlags() docs
"If @flags is set to zero, then the hypervisor will choose the method of shutdown it considers best. To have greater control pass one or more of the virDomainShutdownFlagValues. The order in which the hypervisor tries each shutdown method is undefined, and a hypervisor is not required to support all methods."
I'm certainly receptive to only invoking libxl_send_trigger() when VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN is specified if folks think that is a better approach.
The way I consider shutdown to operate is slightly different to how you've done it in your patch - flags == 0 -> arbitrary upto the hypervisor to decide. Here you invoke libxl_domain_shutdown() and raise error if this fails. This is a valid implementation, though I'd suggest you could choose to try libxl_send_trigger too in this scenario The QEMU driver considers flags == 0, to be equivalent to the union of all flags it supports. ie it effectivel changes flags==0 to be flags = (ACPI_POWER_BTN | GUEST_AGENT) - flags & VIR_DOMAIN_SHUTDOWN_NNNN -> exclusively try the choice specified by the user. When VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN is passed, you are still trying libxl_domain_shutdown() before you try to run libxl_send_trigger(). IMHO this is a bug - it should try libxl_send_trigger exclusively if that was the only bit that was specified in the flags. IIUC libxl_domain_shutdown() will use the Xen paravirt channel to trigger a controlled shutdown in the guest. Currently we have the following flags defined VIR_DOMAIN_SHUTDOWN_DEFAULT = 0, /* hypervisor choice */ VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */ VIR_DOMAIN_SHUTDOWN_GUEST_AGENT = (1 << 1), /* Use guest agent */ VIR_DOMAIN_SHUTDOWN_INITCTL = (1 << 2), /* Use initctl */ VIR_DOMAIN_SHUTDOWN_SIGNAL = (1 << 3), /* Send a signal */ None of those really map to the Xen paravirt shutdown so I think we should define a new flag in the API. VIR_DOMAIN_SHUTDOWN_PARAVIRT = (1 << 4), /* Use paravirt guest control */ Then I think the pseudo code makes sense - If flags == 0 flags = (PARAVIRT | ACPI_POWER_BTN) - If flags & PARAVIRT libxl_domain_shutdown() if (ret == 0) goto done if (ret != ERROR_NOPARAVIT) { virRaiseError(...) goto cleanup; } - If flags & ACPI_POWER_BTN libxl_send_trigger() if (ret == 0) goto done virRaiseError(...) goto cleanup; This gives users/apps full control over which methods are tried. In the flags==0 case it'll try as many as possible. If neccessary the user can force it to only do ACPI power, or only do paravirt. 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 Tue, 2014-04-29 at 10:06 +0100, Daniel P. Berrange wrote:
IIUC libxl_domain_shutdown() will use the Xen paravirt channel to trigger a controlled shutdown in the guest. Currently we have the following flags defined
VIR_DOMAIN_SHUTDOWN_DEFAULT = 0, /* hypervisor choice */ VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */ VIR_DOMAIN_SHUTDOWN_GUEST_AGENT = (1 << 1), /* Use guest agent */ VIR_DOMAIN_SHUTDOWN_INITCTL = (1 << 2), /* Use initctl */ VIR_DOMAIN_SHUTDOWN_SIGNAL = (1 << 3), /* Send a signal */
None of those really map to the Xen paravirt shutdown
It's not a million miles away from VIR_DOMAIN_SHUTDOWN_GUEST_AGENT if you consider the bit of the kernel which responds to the paravirt shutdown request as an agent of sorts. With Windows PV drivers I think this is even literally a userspace component. Not that having a more explicit type is a bad idea, but I thought I'd mention it. Ian.
On Tue, Apr 29, 2014 at 10:53:00AM +0100, Ian Campbell wrote:
On Tue, 2014-04-29 at 10:06 +0100, Daniel P. Berrange wrote:
IIUC libxl_domain_shutdown() will use the Xen paravirt channel to trigger a controlled shutdown in the guest. Currently we have the following flags defined
VIR_DOMAIN_SHUTDOWN_DEFAULT = 0, /* hypervisor choice */ VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */ VIR_DOMAIN_SHUTDOWN_GUEST_AGENT = (1 << 1), /* Use guest agent */ VIR_DOMAIN_SHUTDOWN_INITCTL = (1 << 2), /* Use initctl */ VIR_DOMAIN_SHUTDOWN_SIGNAL = (1 << 3), /* Send a signal */
None of those really map to the Xen paravirt shutdown
It's not a million miles away from VIR_DOMAIN_SHUTDOWN_GUEST_AGENT if you consider the bit of the kernel which responds to the paravirt shutdown request as an agent of sorts. With Windows PV drivers I think this is even literally a userspace component.
Not that having a more explicit type is a bad idea, but I thought I'd mention it.
I guess my rationale for suggesting a new flag is that it is conceivable that Xen could support the virtio-console devices, in which case it would be possible for someone to configure a Xen guest with the QEMU (or another) guest agent provided shutdown facility. In such a scenario, it'd thus be desirable for the API to be able to distinguish Xen paravirt control from the guest agent. 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 Tue, 2014-04-29 at 11:01 +0100, Daniel P. Berrange wrote:
On Tue, Apr 29, 2014 at 10:53:00AM +0100, Ian Campbell wrote:
On Tue, 2014-04-29 at 10:06 +0100, Daniel P. Berrange wrote:
IIUC libxl_domain_shutdown() will use the Xen paravirt channel to trigger a controlled shutdown in the guest. Currently we have the following flags defined
VIR_DOMAIN_SHUTDOWN_DEFAULT = 0, /* hypervisor choice */ VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */ VIR_DOMAIN_SHUTDOWN_GUEST_AGENT = (1 << 1), /* Use guest agent */ VIR_DOMAIN_SHUTDOWN_INITCTL = (1 << 2), /* Use initctl */ VIR_DOMAIN_SHUTDOWN_SIGNAL = (1 << 3), /* Send a signal */
None of those really map to the Xen paravirt shutdown
It's not a million miles away from VIR_DOMAIN_SHUTDOWN_GUEST_AGENT if you consider the bit of the kernel which responds to the paravirt shutdown request as an agent of sorts. With Windows PV drivers I think this is even literally a userspace component.
Not that having a more explicit type is a bad idea, but I thought I'd mention it.
I guess my rationale for suggesting a new flag is that it is conceivable that Xen could support the virtio-console devices, in which case it would be possible for someone to configure a Xen guest with the QEMU (or another) guest agent provided shutdown facility. In such a scenario, it'd thus be desirable for the API to be able to distinguish Xen paravirt control from the guest agent.
That makes sense. Ian.
Daniel P. Berrange wrote:
On Tue, Apr 29, 2014 at 10:53:00AM +0100, Ian Campbell wrote:
On Tue, 2014-04-29 at 10:06 +0100, Daniel P. Berrange wrote:
IIUC libxl_domain_shutdown() will use the Xen paravirt channel to trigger a controlled shutdown in the guest. Currently we have the following flags defined
VIR_DOMAIN_SHUTDOWN_DEFAULT = 0, /* hypervisor choice */ VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */ VIR_DOMAIN_SHUTDOWN_GUEST_AGENT = (1 << 1), /* Use guest agent */ VIR_DOMAIN_SHUTDOWN_INITCTL = (1 << 2), /* Use initctl */ VIR_DOMAIN_SHUTDOWN_SIGNAL = (1 << 3), /* Send a signal */
None of those really map to the Xen paravirt shutdown
It's not a million miles away from VIR_DOMAIN_SHUTDOWN_GUEST_AGENT if you consider the bit of the kernel which responds to the paravirt shutdown request as an agent of sorts. With Windows PV drivers I think this is even literally a userspace component.
Not that having a more explicit type is a bad idea, but I thought I'd mention it.
I guess my rationale for suggesting a new flag is that it is conceivable that Xen could support the virtio-console devices, in which case it would be possible for someone to configure a Xen guest with the QEMU (or another) guest agent provided shutdown facility. In such a scenario, it'd thus be desirable for the API to be able to distinguish Xen paravirt control from the guest agent.
Agreed. I'll rework the patch as suggested. Regards, Jim
participants (3)
-
Daniel P. Berrange -
Ian Campbell -
Jim Fehlig