[libvirt] [PATCH] bhyve: implement domainShutdown
Bhyve supports ACPI shutdown by issuing SIGTERM signal to the bhyve process. Add the bhyveDomainShutdown() function and virBhyveProcessShutdown() helper function that just sends SIGTERM to VM's bhyve process. If a guest supports ACPI shutdown then process will be terminated and this event will be noticed by the bhyve monitor code that will handle setting proper status and clean up VM's resources. Also, remove a warning in domainDestroy in case if virProcessKillPainfully() returns 1, meaning that it killed process using SIGKILL. This behavior should be expected when using 'destroy'. --- src/bhyve/bhyve_driver.c | 27 +++++++++++++++++++++++++++ src/bhyve/bhyve_process.c | 30 ++++++++++++++++++++++++++---- src/bhyve/bhyve_process.h | 2 ++ 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 9219890..b9eacb9 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -1018,6 +1018,32 @@ bhyveDomainDestroy(virDomainPtr dom) } static int +bhyveDomainShutdown(virDomainPtr dom) +{ + virDomainObjPtr vm; + int ret = -1; + + if (!(vm = bhyveDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainShutdownEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("Domain is not running")); + goto cleanup; + } + + ret = virBhyveProcessShutdown(vm); + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + +static int bhyveDomainOpenConsole(virDomainPtr dom, const char *dev_name ATTRIBUTE_UNUSED, virStreamPtr st, @@ -1507,6 +1533,7 @@ static virHypervisorDriver bhyveHypervisorDriver = { .domainCreateWithFlags = bhyveDomainCreateWithFlags, /* 1.2.3 */ .domainCreateXML = bhyveDomainCreateXML, /* 1.2.4 */ .domainDestroy = bhyveDomainDestroy, /* 1.2.2 */ + .domainShutdown = bhyveDomainShutdown, /* 1.3.3 */ .domainLookupByUUID = bhyveDomainLookupByUUID, /* 1.2.2 */ .domainLookupByName = bhyveDomainLookupByName, /* 1.2.2 */ .domainLookupByID = bhyveDomainLookupByID, /* 1.2.3 */ diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 14588a9..fba3896 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -282,10 +282,9 @@ virBhyveProcessStop(bhyveConnPtr driver, bhyveMonitorClose(priv->mon); /* First, try to kill 'bhyve' process */ - if (virProcessKillPainfully(vm->pid, true) != 0) - VIR_WARN("Failed to gracefully stop bhyve VM '%s' (pid: %d)", - vm->def->name, - (int)vm->pid); + if (virProcessKillPainfully(vm->pid, true) == -1) + VIR_WARN("Failed to kill bhyve process for VM '%s': %s", + vm->def->name, virGetLastErrorMessage()); /* Cleanup network interfaces */ bhyveNetCleanup(vm); @@ -317,6 +316,29 @@ virBhyveProcessStop(bhyveConnPtr driver, } int +virBhyveProcessShutdown(virDomainObjPtr vm) +{ + if (vm->pid <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid PID %d for VM"), + (int)vm->pid); + return -1; + } + + /* Bhyve tries to perform ACPI shutdown when it receives + * SIGTERM signal. So we just issue SIGTERM here and rely + * on the bhyve monitor to clean things up if process disappears. + */ + if (virProcessKill(vm->pid, SIGTERM) != 0) { + VIR_WARN("Failed to terminate bhyve process for VM '%s': %s", + vm->def->name, virGetLastErrorMessage()); + return -1; + } + + return 0; +} + +int virBhyveGetDomainTotalCpuStats(virDomainObjPtr vm, unsigned long long *cpustats) { diff --git a/src/bhyve/bhyve_process.h b/src/bhyve/bhyve_process.h index cfa80af..ebabe17 100644 --- a/src/bhyve/bhyve_process.h +++ b/src/bhyve/bhyve_process.h @@ -34,6 +34,8 @@ int virBhyveProcessStop(bhyveConnPtr driver, virDomainObjPtr vm, virDomainShutoffReason reason); +int virBhyveProcessShutdown(virDomainObjPtr vm); + int virBhyveGetDomainTotalCpuStats(virDomainObjPtr vm, unsigned long long *cpustats); -- 2.7.4
On Mon, Mar 28, 2016 at 10:27:18AM +0300, Roman Bogorodskiy wrote:
Bhyve supports ACPI shutdown by issuing SIGTERM signal to the bhyve process. Add the bhyveDomainShutdown() function and virBhyveProcessShutdown() helper function that just sends SIGTERM to VM's bhyve process. If a guest supports ACPI shutdown then process will be terminated and this event will be noticed by the bhyve monitor code that will handle setting proper status and clean up VM's resources.
Also, remove a warning in domainDestroy in case if virProcessKillPainfully() returns 1, meaning that it killed process using SIGKILL. This behavior should be expected when using 'destroy'.
Hmm, so destroy is supposed to be equivalent to physically removing the power plug. The existing code is calling virProcessShutdownPainfully which starts by sending SIGTERM and then switches to SIGKILL. So this means that your virDomainDestroy implementation is mistakenly trying todo a graceful shutdown initially and then switching to hard shutdown after a bit of a delay. Has bhyve always used SIGTERM to trigger ACPI shutdown, or is this a recent addition ? I would tend to suggest we need to go straight to SIGKILL for virDomainDestroy to avoid doing ACPI shutdown when we don't want it. It is kind of a shame they used SIGTERM for triggering ACPI imho but oh well. 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 :|
Daniel P. Berrange wrote:
On Mon, Mar 28, 2016 at 10:27:18AM +0300, Roman Bogorodskiy wrote:
Bhyve supports ACPI shutdown by issuing SIGTERM signal to the bhyve process. Add the bhyveDomainShutdown() function and virBhyveProcessShutdown() helper function that just sends SIGTERM to VM's bhyve process. If a guest supports ACPI shutdown then process will be terminated and this event will be noticed by the bhyve monitor code that will handle setting proper status and clean up VM's resources.
Also, remove a warning in domainDestroy in case if virProcessKillPainfully() returns 1, meaning that it killed process using SIGKILL. This behavior should be expected when using 'destroy'.
Hmm, so destroy is supposed to be equivalent to physically removing the power plug. The existing code is calling virProcessShutdownPainfully which starts by sending SIGTERM and then switches to SIGKILL. So this means that your virDomainDestroy implementation is mistakenly trying todo a graceful shutdown initially and then switching to hard shutdown after a bit of a delay.
Indeed, you're right. Probably doesn't make practical difference due to timeout being quite low, but makes no sense to wait. Will fix.
Has bhyve always used SIGTERM to trigger ACPI shutdown, or is this a recent addition ? I would tend to suggest we need to go straight to SIGKILL for virDomainDestroy to avoid doing ACPI shutdown when we don't want it.
It was actually added back in the end of 2013, so it was in -CURRENT for quite some time already. It's also available in 10.1, the earliest supported version [1] of the 10.x branch. It's not in 10.0 though.
It is kind of a shame they used SIGTERM for triggering ACPI imho but oh well.
Agreed, it doesn't seem very flexible. At least I don't see a way how to probe if this feature is supported by the given bhyve binary and how to check if the command was successful or not (except relying on the libvirt's bhyve/bhyve_monitor.c code to detect process termination). 1: https://www.freebsd.org/security/security.html#sup
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 :|
Roman Bogorodskiy
On 03/29/2016 08:24 AM, Daniel P. Berrange wrote:
Bhyve supports ACPI shutdown by issuing SIGTERM signal to the bhyve process. Add the bhyveDomainShutdown() function and virBhyveProcessShutdown() helper function that just sends SIGTERM to VM's bhyve process. If a guest supports ACPI shutdown then process will be terminated and this event will be noticed by the bhyve monitor code that will handle setting proper status and clean up VM's resources.
Also, remove a warning in domainDestroy in case if virProcessKillPainfully() returns 1, meaning that it killed process using SIGKILL. This behavior should be expected when using 'destroy'. Hmm, so destroy is supposed to be equivalent to physically removing
On Mon, Mar 28, 2016 at 10:27:18AM +0300, Roman Bogorodskiy wrote: the power plug. The existing code is calling virProcessShutdownPainfully which starts by sending SIGTERM and then switches to SIGKILL. So this means that your virDomainDestroy implementation is mistakenly trying todo a graceful shutdown initially and then switching to hard shutdown after a bit of a delay.
For context - the qemu driver does this for destroy as well. I think at least one of the reasons is to allow the qemu process to flush the buffers for the disk image files. I recall this was causing some amount of pain for ovirt (or maybe someone else, I forget)
Has bhyve always used SIGTERM to trigger ACPI shutdown, or is this a recent addition ? I would tend to suggest we need to go straight to SIGKILL for virDomainDestroy to avoid doing ACPI shutdown when we don't want it.
It is kind of a shame they used SIGTERM for triggering ACPI imho but oh well.
Yes, it means there is no way to implement a "clean power off" (which would give bhyve the chance to clean up some critical things before pulling the plug).
On Tue, Mar 29, 2016 at 10:52:09AM -0400, Laine Stump wrote:
On 03/29/2016 08:24 AM, Daniel P. Berrange wrote:
Bhyve supports ACPI shutdown by issuing SIGTERM signal to the bhyve process. Add the bhyveDomainShutdown() function and virBhyveProcessShutdown() helper function that just sends SIGTERM to VM's bhyve process. If a guest supports ACPI shutdown then process will be terminated and this event will be noticed by the bhyve monitor code that will handle setting proper status and clean up VM's resources.
Also, remove a warning in domainDestroy in case if virProcessKillPainfully() returns 1, meaning that it killed process using SIGKILL. This behavior should be expected when using 'destroy'. Hmm, so destroy is supposed to be equivalent to physically removing
On Mon, Mar 28, 2016 at 10:27:18AM +0300, Roman Bogorodskiy wrote: the power plug. The existing code is calling virProcessShutdownPainfully which starts by sending SIGTERM and then switches to SIGKILL. So this means that your virDomainDestroy implementation is mistakenly trying todo a graceful shutdown initially and then switching to hard shutdown after a bit of a delay.
For context - the qemu driver does this for destroy as well. I think at least one of the reasons is to allow the qemu process to flush the buffers for the disk image files. I recall this was causing some amount of pain for ovirt (or maybe someone else, I forget)
Yep it is different in QEMU - SIGTERM doesn't trigger any guest OS ACPI event with QEMU - it merely lets QEMU gracefully close the block layer functions and flush I/O.
Has bhyve always used SIGTERM to trigger ACPI shutdown, or is this a recent addition ? I would tend to suggest we need to go straight to SIGKILL for virDomainDestroy to avoid doing ACPI shutdown when we don't want it.
It is kind of a shame they used SIGTERM for triggering ACPI imho but oh well.
Yes, it means there is no way to implement a "clean power off" (which would give bhyve the chance to clean up some critical things before pulling the plug).
Yeah, exactly, it is desirable to be able to do a immediate forced shutdown of guest OS while still letting the host process exit cleanly. 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 :|
Daniel P. Berrange wrote:
On Tue, Mar 29, 2016 at 10:52:09AM -0400, Laine Stump wrote:
On 03/29/2016 08:24 AM, Daniel P. Berrange wrote:
Bhyve supports ACPI shutdown by issuing SIGTERM signal to the bhyve process. Add the bhyveDomainShutdown() function and virBhyveProcessShutdown() helper function that just sends SIGTERM to VM's bhyve process. If a guest supports ACPI shutdown then process will be terminated and this event will be noticed by the bhyve monitor code that will handle setting proper status and clean up VM's resources.
Also, remove a warning in domainDestroy in case if virProcessKillPainfully() returns 1, meaning that it killed process using SIGKILL. This behavior should be expected when using 'destroy'. Hmm, so destroy is supposed to be equivalent to physically removing
On Mon, Mar 28, 2016 at 10:27:18AM +0300, Roman Bogorodskiy wrote: the power plug. The existing code is calling virProcessShutdownPainfully which starts by sending SIGTERM and then switches to SIGKILL. So this means that your virDomainDestroy implementation is mistakenly trying todo a graceful shutdown initially and then switching to hard shutdown after a bit of a delay.
For context - the qemu driver does this for destroy as well. I think at least one of the reasons is to allow the qemu process to flush the buffers for the disk image files. I recall this was causing some amount of pain for ovirt (or maybe someone else, I forget)
Yep it is different in QEMU - SIGTERM doesn't trigger any guest OS ACPI event with QEMU - it merely lets QEMU gracefully close the block layer functions and flush I/O.
Has bhyve always used SIGTERM to trigger ACPI shutdown, or is this a recent addition ? I would tend to suggest we need to go straight to SIGKILL for virDomainDestroy to avoid doing ACPI shutdown when we don't want it.
It is kind of a shame they used SIGTERM for triggering ACPI imho but oh well.
Yes, it means there is no way to implement a "clean power off" (which would give bhyve the chance to clean up some critical things before pulling the plug).
Yeah, exactly, it is desirable to be able to do a immediate forced shutdown of guest OS while still letting the host process exit cleanly.
That's an interesting point. I noticed that there's also 'bhyvectl --force-poweroff' command, if it does what it looks like it should do then I guess it would be good to use it instead of the kill -9 call. I'll check its behaviour and update the patch accordingly.
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 :|
Roman Bogorodskiy
participants (3)
-
Daniel P. Berrange -
Laine Stump -
Roman Bogorodskiy