Current cleanup routine for the domain start errors misses quite a few important steps. It does not set the proper domain state, it does not clean up sockets and so forth. The only thing it does is destroying of the actual domain and performing network cleanup.
To properly clean all resources, replace running virBhyveProcessBuildDestroyCmd() command with executing virBhyveProcessStop() which already does the necessary steps. Extend virBhyveProcessStop() with the forceCleanup boolean argument which instructs it not to error out on some checks to account for inconsistent state after the partial startup. Signed-off-by: Roman Bogorodskiy <[email protected]> --- src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_monitor.c | 6 ++-- src/bhyve/bhyve_process.c | 68 ++++++++++++++++++++++++--------------- src/bhyve/bhyve_process.h | 3 +- 4 files changed, 48 insertions(+), 31 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 668e46ad37..a0d97d3f86 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -1019,7 +1019,7 @@ bhyveDomainDestroyFlags(virDomainPtr dom, unsigned int flags) if (virDomainObjCheckActive(vm) < 0) goto cleanup; - ret = virBhyveProcessStop(privconn, vm, VIR_DOMAIN_SHUTOFF_DESTROYED); + ret = virBhyveProcessStop(privconn, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, false); event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); diff --git a/src/bhyve/bhyve_monitor.c b/src/bhyve/bhyve_monitor.c index 8cf4b41ca3..a24696cad5 100644 --- a/src/bhyve/bhyve_monitor.c +++ b/src/bhyve/bhyve_monitor.c @@ -153,7 +153,7 @@ bhyveMonitorIO(int watch, int kq, int events G_GNUC_UNUSED, void *opaque) virReportError(VIR_ERR_INTERNAL_ERROR, _("Guest %1$s got signal %2$d and crashed"), name, WTERMSIG(status)); - virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED); + virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, false); } else if (WIFEXITED(status)) { if (WEXITSTATUS(status) == 0 || mon->reboot) { /* 0 - reboot */ @@ -162,11 +162,11 @@ bhyveMonitorIO(int watch, int kq, int events G_GNUC_UNUSED, void *opaque) } else if (WEXITSTATUS(status) < 3) { /* 1 - shutdown, 2 - halt, 3 - triple fault. others - error */ VIR_INFO("Guest %s shut itself down; destroying domain.", name); - virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN); + virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN, false); } else { VIR_INFO("Guest %s had an error and exited with status %d; destroying domain.", name, WEXITSTATUS(status)); - virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_UNKNOWN); + virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_UNKNOWN, false); } } } diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 4d93ba8fac..d501aa8bcc 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -66,7 +66,7 @@ bhyveProcessAutoDestroy(virDomainObj *vm, bhyveDomainObjPrivate *priv = vm->privateData; struct _bhyveConn *driver = priv->driver; - virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED); + virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, false); if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); @@ -437,18 +437,9 @@ virBhyveProcessStartImpl(struct _bhyveConn *driver, devmap_file); } - if (ret < 0) { - int exitstatus; /* Needed to avoid logging non-zero status */ - g_autoptr(virCommand) destroy_cmd = NULL; - if ((destroy_cmd = virBhyveProcessBuildDestroyCmd(driver, - vm->def)) != NULL) { - virCommandSetOutputFD(load_cmd, &logfd); - virCommandSetErrorFD(load_cmd, &logfd); - ignore_value(virCommandRun(destroy_cmd, &exitstatus)); - } - - bhyveNetCleanup(vm); - } + if (ret < 0) + ignore_value(virBhyveProcessStop(driver, vm, + VIR_DOMAIN_SHUTOFF_FAILED, true)); return ret; } @@ -634,26 +625,49 @@ bhyveProcessRemoveDomainStatus(const char *statusDir, } } +/** + * @driver: bhyve driver + * @vm: domain object + * @reason: shutoff reason + * @forceCleanup: boolean controlling cleanup + * + * Stops the domain and cleans up its resources. + * It could be used whether as a direct call or as a cleanup routine. + * In the latter case, @forceCleanup should be set to `true`, so it + * tries to clean up all resources instead of exiting early if the domain + * is not active for example. + * + * Does not run the shutdown hooks if the domain PID was not obtained. + * + * Returns 0 on success, -1 on error. + */ int virBhyveProcessStop(struct _bhyveConn *driver, virDomainObj *vm, - virDomainShutoffReason reason) + virDomainShutoffReason reason, + bool forceCleanup) { int ret = 0; size_t i = 0; g_autoptr(virCommand) cmd = NULL; bhyveDomainObjPrivate *priv = vm->privateData; + bool vm_started = false; - if (!virDomainObjIsActive(vm)) { - VIR_DEBUG("VM '%s' not active", vm->def->name); - return 0; - } + if (vm->pid != 0) + vm_started = true; - if (vm->pid == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid PID %1$d for VM"), - (int)vm->pid); - return -1; + if (!forceCleanup) { + if (!virDomainObjIsActive(vm)) { + VIR_DEBUG("VM '%s' not active", vm->def->name); + return 0; + } + + if (!vm_started) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid PID %1$d for VM"), + (int)vm->pid); + return -1; + } } /* Destroy monitor before running the actual destroy command to prevent @@ -669,7 +683,8 @@ virBhyveProcessStop(struct _bhyveConn *driver, VIR_WARN("Failed to run the domain destroy command"); } - bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_STOPPED); + if (vm_started) + bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_STOPPED); /* Cleanup network interfaces */ bhyveNetCleanup(vm); @@ -702,7 +717,8 @@ virBhyveProcessStop(struct _bhyveConn *driver, vm->pid = 0; vm->def->id = -1; - bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_RELEASE); + if (vm_started) + bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_RELEASE); virPidFileDelete(BHYVE_STATE_DIR, vm->def->name); bhyveProcessRemoveDomainStatus(BHYVE_STATE_DIR, vm->def->name); @@ -736,7 +752,7 @@ int virBhyveProcessRestart(struct _bhyveConn *driver, virDomainObj *vm) { - if (virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN) < 0) + if (virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN, false) < 0) return -1; if (virBhyveProcessStartImpl(driver, vm, VIR_DOMAIN_RUNNING_BOOTED) < 0) diff --git a/src/bhyve/bhyve_process.h b/src/bhyve/bhyve_process.h index bf82f748a6..339e5d429a 100644 --- a/src/bhyve/bhyve_process.h +++ b/src/bhyve/bhyve_process.h @@ -44,7 +44,8 @@ int virBhyveProcessStart(bhyveConn *driver, int virBhyveProcessStop(struct _bhyveConn *driver, virDomainObj *vm, - virDomainShutoffReason reason); + virDomainShutoffReason reason, + bool forceCleanup); int virBhyveProcessRestart(struct _bhyveConn *driver, virDomainObj *vm); -- 2.52.0
