[libvirt] [PATCH v3 0/2] bhyve: implement domainShutdown

Changes from v2: - Split domainDestroy changes and domainShutdown addition into two patches - In bhyveDestroy, close bhyve monitor after running 'bhyvectl --destroy' (reasoning in the commit message) Roman Bogorodskiy (2): bhyve: drop virProcessKillPainfully() from destroy bhyve: implement domainShutdown src/bhyve/bhyve_driver.c | 27 +++++++++++++++++++++++++++ src/bhyve/bhyve_process.c | 43 +++++++++++++++++++++++++++++-------------- src/bhyve/bhyve_process.h | 2 ++ 3 files changed, 58 insertions(+), 14 deletions(-) -- 2.7.4

Currentt implementation of domainDestroy for bhyve calls virProcessKillPainfully() for the bhyve process and then executes "bhyvectl --destroy". This is wrong for two reasons: * bhyvectl --destroy alone is sufficient because it terminates the process * virProcessKillPainfully() first sends SIGTERM and after few attempts sends SIGKILL. As SIGTERM triggers ACPI shutdown that we're not interested in, it creates an unwatned side effect in domainDestroy. Also, destroy monitor only after "bhyvectl --destroy" command succeeded to avoid a case when the command fails and domain remains running, but not being monitored anymore. --- src/bhyve/bhyve_process.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 14588a9..fe61a9a 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -278,26 +278,18 @@ virBhyveProcessStop(bhyveConnPtr driver, return -1; } + if (!(cmd = virBhyveProcessBuildDestroyCmd(driver, vm->def))) + return -1; + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + if ((priv != NULL) && (priv->mon != NULL)) 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); - /* Cleanup network interfaces */ bhyveNetCleanup(vm); - /* No matter if shutdown was successful or not, we - * need to unload the VM */ - if (!(cmd = virBhyveProcessBuildDestroyCmd(driver, vm->def))) - goto cleanup; - - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - ret = 0; virCloseCallbacksUnset(driver->closeCallbacks, vm, -- 2.7.4

On Wed, May 04, 2016 at 07:41:58PM +0300, Roman Bogorodskiy wrote:
Currentt implementation of domainDestroy for bhyve calls virProcessKillPainfully() for the bhyve process and then executes "bhyvectl --destroy".
This is wrong for two reasons:
* bhyvectl --destroy alone is sufficient because it terminates the process * virProcessKillPainfully() first sends SIGTERM and after few attempts sends SIGKILL. As SIGTERM triggers ACPI shutdown that we're not interested in, it creates an unwatned side effect in domainDestroy.
Also, destroy monitor only after "bhyvectl --destroy" command succeeded to avoid a case when the command fails and domain remains running, but not being monitored anymore. --- src/bhyve/bhyve_process.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)
ACK 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 :|

Bhyve supports ACPI shutdown by issuing SIGTERM signal to a 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 by calling virBhyveProcessStop(). --- src/bhyve/bhyve_driver.c | 27 +++++++++++++++++++++++++++ src/bhyve/bhyve_process.c | 23 +++++++++++++++++++++++ src/bhyve/bhyve_process.h | 2 ++ 3 files changed, 52 insertions(+) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 5526bb0..4fc504e 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, @@ -1502,6 +1528,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 fe61a9a..6db070f 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -309,6 +309,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 Wed, May 04, 2016 at 07:41:59PM +0300, Roman Bogorodskiy wrote:
Bhyve supports ACPI shutdown by issuing SIGTERM signal to a 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 by calling virBhyveProcessStop(). --- src/bhyve/bhyve_driver.c | 27 +++++++++++++++++++++++++++ src/bhyve/bhyve_process.c | 23 +++++++++++++++++++++++ src/bhyve/bhyve_process.h | 2 ++ 3 files changed, 52 insertions(+)
ACK 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 Wed, May 04, 2016 at 07:41:59PM +0300, Roman Bogorodskiy wrote:
Bhyve supports ACPI shutdown by issuing SIGTERM signal to a 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 by calling virBhyveProcessStop(). --- src/bhyve/bhyve_driver.c | 27 +++++++++++++++++++++++++++ src/bhyve/bhyve_process.c | 23 +++++++++++++++++++++++ src/bhyve/bhyve_process.h | 2 ++ 3 files changed, 52 insertions(+)
ACK
Pushed both patches (with some typos in commit messages fixed along the way). Thank you and Cole for reviewing! Roman Bogorodskiy
participants (2)
-
Daniel P. Berrange
-
Roman Bogorodskiy