[PATCH 0/4] ch: Improve domain destroy
*** BLURB HERE *** Michal Prívozník (4): virprocess: Report errno if virProcessAbort() fails ch: Make sure the cloud-hypervisor process is killed in virCHProcessStop() ch: Introduce flags to virCHProcessStop() ch: Implement VIR_DOMAIN_DESTROY_GRACEFUL flag support src/ch/ch_driver.c | 13 ++++++++++--- src/ch/ch_events.c | 2 +- src/ch/ch_process.c | 22 ++++++++++++++++------ src/ch/ch_process.h | 8 +++++++- src/util/virprocess.c | 3 ++- 5 files changed, 36 insertions(+), 12 deletions(-) -- 2.49.1
From: Michal Privoznik <mprivozn@redhat.com> The aim of virProcessAbort() is to reap a child process. It does so by waitpid()-ing and possibly sending SIGTERM/SIGKILL to the child process (and waitpid()-ing again). Nevertheless, if everything fails a debug message is printed. But the message mentions only the PID and not errno (set by previous waitpid()) which may be useful. For instance when virProcessAbort() is called over a PID that's not our child: failed to reap child 16325, abandoning it: No child processes Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virprocess.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 3889ba90f9..e8120c1bdc 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -168,7 +168,8 @@ virProcessAbort(pid_t pid) } } } - VIR_DEBUG("failed to reap child %lld, abandoning it", (long long) pid); + VIR_DEBUG("failed to reap child %lld, abandoning it: %s", + (long long) pid, g_strerror(errno)); cleanup: errno = saved_errno; -- 2.49.1
From: Michal Privoznik <mprivozn@redhat.com> Currently, virCHProcessStop() is called either when the cloud-hypervisor process dies gracefully (e.g. on shutdown initiated from within the guest) or when virDomainDestroy() is called (or on failed start attempt, but that's not important right now). At any rate, if the cloud-hypervisor process is running it's not a child process of libvirtd rather than the init (per virCommandDaemonize() called inside of virCHMonitorNew()). This distinction is important because virCHProcessStop() then calls virProcessAbort() thinking it'll kill the process. Well, virProcessAbort() works only on child processes. Switch to virProcessKillPainfully() which does work in such cases. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 5195d3f5da..6b779285e1 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -1028,7 +1028,7 @@ virCHProcessStop(virCHDriver *driver, virErrorPreserveLast(&orig_err); if (priv->monitor) { - virProcessAbort(vm->pid); + virProcessKillPainfully(vm->pid, true); g_clear_pointer(&priv->monitor, virCHMonitorClose); } -- 2.49.1
From: Michal Privoznik <mprivozn@redhat.com> A caller (e.g. chDomainDestroyFlags()) might want to chose whether to kill emulator process forcefully or gracefully (the @force argument of virProcessKillPainfully()). Invent a flag to virCHProcessStop() for this. And to keep consistent behaviour, pass the flag everywhere for now. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_driver.c | 8 ++++++-- src/ch/ch_events.c | 2 +- src/ch/ch_process.c | 22 ++++++++++++++++------ src/ch/ch_process.h | 8 +++++++- 4 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 760fccba82..019994b202 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -686,8 +686,11 @@ chDomainDestroyFlags(virDomainPtr dom, unsigned int flags) if (virDomainObjCheckActive(vm) < 0) goto endjob; - if (virCHProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED) < 0) + if (virCHProcessStop(driver, vm, + VIR_DOMAIN_SHUTOFF_DESTROYED, + VIR_CH_PROCESS_STOP_FORCE) < 0) { goto endjob; + } event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, @@ -818,7 +821,8 @@ chDoDomainSave(virCHDriver *driver, goto end; } - if (virCHProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED) < 0) { + if (virCHProcessStop(driver, vm, + VIR_DOMAIN_SHUTOFF_SAVED, VIR_CH_PROCESS_STOP_FORCE) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to shutoff after domain save")); goto end; diff --git a/src/ch/ch_events.c b/src/ch/ch_events.c index 3d4e3c41e1..be572dfde3 100644 --- a/src/ch/ch_events.c +++ b/src/ch/ch_events.c @@ -59,7 +59,7 @@ virCHEventStopProcess(virDomainObj *vm, virObjectLock(vm); if (virDomainObjBeginJob(vm, VIR_JOB_DESTROY)) return -1; - virCHProcessStop(driver, vm, reason); + virCHProcessStop(driver, vm, reason, VIR_CH_PROCESS_STOP_FORCE); virDomainObjEndJob(vm); virObjectUnlock(vm); diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 6b779285e1..54b21b0baf 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -1003,7 +1003,9 @@ virCHProcessStart(virCHDriver *driver, cleanup: if (ret) - virCHProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED); + virCHProcessStop(driver, vm, + VIR_DOMAIN_SHUTOFF_FAILED, + VIR_CH_PROCESS_STOP_FORCE); return ret; } @@ -1011,7 +1013,8 @@ virCHProcessStart(virCHDriver *driver, int virCHProcessStop(virCHDriver *driver, virDomainObj *vm, - virDomainShutoffReason reason) + virDomainShutoffReason reason, + unsigned int flags) { g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver); int ret; @@ -1022,13 +1025,18 @@ virCHProcessStop(virCHDriver *driver, virErrorPtr orig_err = NULL; size_t i; - VIR_DEBUG("Stopping VM name=%s pid=%d reason=%d", - vm->def->name, (int)vm->pid, (int)reason); + VIR_DEBUG("Stopping VM name=%s pid=%d reason=%d flags=0x%x", + vm->def->name, (int)vm->pid, (int)reason, flags); virErrorPreserveLast(&orig_err); if (priv->monitor) { - virProcessKillPainfully(vm->pid, true); + bool force = false; + + if (flags & VIR_CH_PROCESS_STOP_FORCE) + force = true; + + virProcessKillPainfully(vm->pid, force); g_clear_pointer(&priv->monitor, virCHMonitorClose); } @@ -1180,6 +1188,8 @@ virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, const char *from if (tapfds) chCloseFDs(tapfds, ntapfds); if (ret) - virCHProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED); + virCHProcessStop(driver, vm, + VIR_DOMAIN_SHUTOFF_FAILED, + VIR_CH_PROCESS_STOP_FORCE); return ret; } diff --git a/src/ch/ch_process.h b/src/ch/ch_process.h index 70ae8f700d..a22790bb5c 100644 --- a/src/ch/ch_process.h +++ b/src/ch/ch_process.h @@ -26,9 +26,15 @@ int virCHProcessStart(virCHDriver *driver, virDomainObj *vm, virDomainRunningReason reason); + +typedef enum { + VIR_CH_PROCESS_STOP_FORCE = 1 << 0, +} virCHProcessStopFlags; + int virCHProcessStop(virCHDriver *driver, virDomainObj *vm, - virDomainShutoffReason reason); + virDomainShutoffReason reason, + unsigned int flags); int virCHProcessStartRestore(virCHDriver *driver, virDomainObj *vm, -- 2.49.1
From: Michal Privoznik <mprivozn@redhat.com> The virDomainDestroyFlags() API has several flags, including VIR_DOMAIN_DESTROY_GRACEFUL which is documented to send only SIGTERM to the emulator process. Implement its support into CH driver. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_driver.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 019994b202..0a516f3384 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -670,9 +670,13 @@ chDomainDestroyFlags(virDomainPtr dom, unsigned int flags) virCHDriver *driver = dom->conn->privateData; virDomainObj *vm; virObjectEvent *event = NULL; + unsigned int stopFlags = 0; int ret = -1; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_DESTROY_GRACEFUL, -1); + + if (!(flags & VIR_DOMAIN_DESTROY_GRACEFUL)) + stopFlags |= VIR_CH_PROCESS_STOP_FORCE; if (!(vm = virCHDomainObjFromDomain(dom))) goto cleanup; @@ -687,8 +691,7 @@ chDomainDestroyFlags(virDomainPtr dom, unsigned int flags) goto endjob; if (virCHProcessStop(driver, vm, - VIR_DOMAIN_SHUTOFF_DESTROYED, - VIR_CH_PROCESS_STOP_FORCE) < 0) { + VIR_DOMAIN_SHUTOFF_DESTROYED, stopFlags) < 0) { goto endjob; } -- 2.49.1
On a Tuesday in 2025, Michal Privoznik via Devel wrote:
*** BLURB HERE ***
Michal Prívozník (4): virprocess: Report errno if virProcessAbort() fails ch: Make sure the cloud-hypervisor process is killed in virCHProcessStop() ch: Introduce flags to virCHProcessStop() ch: Implement VIR_DOMAIN_DESTROY_GRACEFUL flag support
src/ch/ch_driver.c | 13 ++++++++++--- src/ch/ch_events.c | 2 +- src/ch/ch_process.c | 22 ++++++++++++++++------ src/ch/ch_process.h | 8 +++++++- src/util/virprocess.c | 3 ++- 5 files changed, 36 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik