On Fri, Jul 19, 2024 at 02:58:23PM +0200, Boris Fiuczynski wrote:

In cases when a QEMU process takes longer than the time sigterm and sigkill are issued to kill the process do not simply fail and leave the VM in state VIR_DOMAIN_SHUTDOWN until the daemon stops. Instead set up an fd on /proc/$pid and get notified when the QEMU process finally has terminated to cleanup the VM state.

Resolves: https://issues.redhat.com/browse/RHEL-28819 Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/qemu/qemu_domain.c | 8 +++ src/qemu/qemu_domain.h | 2 + src/qemu/qemu_driver.c | 18 +++++++ src/qemu/qemu_process.c | 115 ++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_process.h | 1 + 5 files changed, 139 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2134b11038..8147ff02fd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1889,6 +1889,11 @@ qemuDomainObjPrivateFree(void *data)

virChrdevFree(priv->devs);

+ if (priv->pidMonitored >= 0) { + virEventRemoveHandle(priv->pidMonitored); + priv->pidMonitored = -1; + } + /* This should never be non-NULL if we get here, but just in case... */ if (priv->mon) { VIR_ERROR(_("Unexpected QEMU monitor still active during domain deletion")); @@ -1934,6 +1939,8 @@ qemuDomainObjPrivateAlloc(void *opaque) priv->blockjobs = virHashNew(virObjectUnref); priv->fds = virHashNew(g_object_unref);

+ priv->pidMonitored = -1; + /* agent commands block by default, user can choose different behavior */ priv->agentTimeout = VIR_DOMAIN_AGENT_RESPONSE_TIMEOUT_BLOCK; priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX; @@ -11680,6 +11687,7 @@ qemuProcessEventFree(struct qemuProcessEvent *event) case QEMU_PROCESS_EVENT_RESET: case QEMU_PROCESS_EVENT_NBDKIT_EXITED: case QEMU_PROCESS_EVENT_MONITOR_EOF: + case QEMU_PROCESS_EVENT_SHUTDOWN_COMPLETED: case QEMU_PROCESS_EVENT_LAST: break; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index d777559119..a5092dd7f0 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -119,6 +119,7 @@ struct _qemuDomainObjPrivate {

bool beingDestroyed; char *pidfile; + int pidMonitored;

virDomainPCIAddressSet *pciaddrs; virDomainUSBAddressSet *usbaddrs; @@ -469,6 +470,7 @@ typedef enum { QEMU_PROCESS_EVENT_UNATTENDED_MIGRATION, QEMU_PROCESS_EVENT_RESET, QEMU_PROCESS_EVENT_NBDKIT_EXITED, + QEMU_PROCESS_EVENT_SHUTDOWN_COMPLETED,

QEMU_PROCESS_EVENT_LAST } qemuProcessEventType; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9f3013e231..6b1e4084f6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4041,6 +4041,21 @@ processNbdkitExitedEvent(virDomainObj *vm, }

+static void +processShutdownCompletedEvent(virQEMUDriver *driver, + virDomainObj *vm) +{ + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) + return; + + if (virDomainObjIsActive(vm)) + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_UNKNOWN, + VIR_ASYNC_JOB_NONE, 0); + + virDomainObjEndJob(vm); +} + + static void qemuProcessEventHandler(void *data, void *opaque) { struct qemuProcessEvent *processEvent = data; @@ -4101,6 +4116,9 @@ static void qemuProcessEventHandler(void *data, void *opaque) case QEMU_PROCESS_EVENT_NBDKIT_EXITED: processNbdkitExitedEvent(vm, processEvent->data); break; + case QEMU_PROCESS_EVENT_SHUTDOWN_COMPLETED: + processShutdownCompletedEvent(driver, vm); + break; case QEMU_PROCESS_EVENT_LAST: break; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 25dfd04272..86ca495a20 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -25,6 +25,7 @@ #include <unistd.h> #include <signal.h> #include <sys/stat.h> +#include <sys/syscall.h> #if defined(__linux__) # include <linux/capability.h> #elif defined(__FreeBSD__) @@ -8387,9 +8388,107 @@ qemuProcessCreatePretendCmdBuild(virDomainObj *vm, }

+typedef struct { + virDomainObj *vm; + int pidfd; +} qemuProcessInShutdownEventData; + + +static qemuProcessInShutdownEventData* +qemuProcessInShutdownEventDataNew(virDomainObj *vm, int pidfd) +{ + qemuProcessInShutdownEventData *d = g_new(qemuProcessInShutdownEventData, 1); + d->vm = virObjectRef(vm); + d->pidfd = pidfd; + return d; +} + + +static void +qemuProcessInShutdownEventDataFree(qemuProcessInShutdownEventData *d) +{ + virObjectUnref(d->vm); + VIR_FORCE_CLOSE(d->pidfd); + g_free(d); +} + + +static void +qemuProcessInShutdownPidfdCb(int watch, + int fd, + int events G_GNUC_UNUSED, + void *opaque) +{ + qemuProcessInShutdownEventData *data = opaque; + virDomainObj *vm = data->vm; + + VIR_DEBUG("vm=%p name=%s pid=%lld fd=%d", + vm, vm->def->name, (long long)vm->pid, fd); + + virEventRemoveHandle(watch); + + virObjectLock(vm); + + VIR_INFO("QEMU process %lld finally completed termination", + (long long)vm->pid); + + QEMU_DOMAIN_PRIVATE(vm)->pidMonitored = -1; + qemuProcessEventSubmit(vm, QEMU_PROCESS_EVENT_SHUTDOWN_COMPLETED, + 0, 0, NULL); + + virObjectUnlock(vm); +} + + +static int +qemuProcessInShutdownStartMonitor(virDomainObj *vm) +{ + qemuDomainObjPrivate *priv = vm->privateData; + qemuProcessInShutdownEventData *data; + int pidfd; + int ret = -1; + + VIR_DEBUG("vm=%p name=%s pid=%lld pidMonitored=%d", + vm, vm->def->name, (long long)vm->pid, + priv->pidMonitored); + + if (priv->pidMonitored >= 0) { + VIR_DEBUG("Monitoring qemu in-shutdown process %i already set up", vm->pid); + goto cleanup; + } + + pidfd = syscall(SYS_pidfd_open, vm->pid, 0); + if (pidfd < 0) { + if (errno == ESRCH) /* process has already terminated */ + ret = 1; + goto cleanup; + } + + data = qemuProcessInShutdownEventDataNew(vm, pidfd); + if ((priv->pidMonitored = virEventAddHandle(pidfd, + VIR_EVENT_HANDLE_READABLE, + qemuProcessInShutdownPidfdCb, + data, + (virFreeCallback)qemuProcessInShutdownEventDataFree)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to monitor qemu in-shutdown process %1$i"), + vm->pid); + qemuProcessInShutdownEventDataFree(data); + goto cleanup; + } + VIR_DEBUG("Monitoring qemu in-shutdown process %i for termination", vm->pid); + ret = 0; + + cleanup: + return ret; +}

This will need some #ifdef-ery because I believe SYS_pidfd_open won't exist on all Linux versions we need, and certainly won't exist on non-Linux, and nor will syscall() fro that matter.

+ + int qemuProcessKill(virDomainObj *vm, unsigned int flags) { + int ret = -1; + VIR_DEBUG("vm=%p name=%s pid=%lld flags=0x%x", vm, vm->def->name, (long long)vm->pid, flags); @@ -8410,10 +8509,16 @@ qemuProcessKill(virDomainObj *vm, unsigned int flags)

/* Request an extra delay of two seconds per current nhostdevs * to be safe against stalls by the kernel freeing up the resources */ - return virProcessKillPainfullyDelay(vm->pid, - !!(flags & VIR_QEMU_PROCESS_KILL_FORCE), - vm->def->nhostdevs * 2, - false); + ret = virProcessKillPainfullyDelay(vm->pid, + !!(flags & VIR_QEMU_PROCESS_KILL_FORCE), + vm->def->nhostdevs * 2, + false); + + if (ret < 0 && (flags & VIR_QEMU_PROCESS_KILL_MONITOR_ON_ERROR)) + if (qemuProcessInShutdownStartMonitor(vm) == 1) + ret = 0; /* process termination detected */ + + return ret; }

@@ -8438,7 +8543,7 @@ qemuProcessBeginStopJob(virDomainObj *vm, * cleared inside qemuProcessStop */ priv->beingDestroyed = true;

- if (qemuProcessKill(vm, killFlags) < 0) + if (qemuProcessKill(vm, killFlags|VIR_QEMU_PROCESS_KILL_MONITOR_ON_ERROR) < 0) goto error;

/* Wake up anything waiting on domain condition */ diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index cb67bfcd2d..2324aeb7bd 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -180,6 +180,7 @@ typedef enum { VIR_QEMU_PROCESS_KILL_FORCE = 1 << 0, VIR_QEMU_PROCESS_KILL_NOWAIT = 1 << 1, VIR_QEMU_PROCESS_KILL_NOCHECK = 1 << 2, /* bypass the running vm check */ + VIR_QEMU_PROCESS_KILL_MONITOR_ON_ERROR = 1 << 3, /* on error enable process monitor */ } virQemuProcessKillMode;

int qemuProcessKill(virDomainObj *vm, unsigned int flags); -- 2.45.0

With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|