Re: [libvirt] [PATCHv2 1/2] qemu: new GRACEFUL flag for virDomainDestroy w/ QEMU support

When libvirt's virDomainDestroy API is shutting down the qemu process, it first sends SIGTERM, then waits for 1.6 seconds and, if it sees the process still there, sends a SIGKILL. There have been reports that this behavior can lead to data loss because the guest running in qemu doesn't have time to flush its disk cache buffers before it's unceremoniously whacked. This patch maintains that default behavior, but provides a new flag VIR_DOMAIN_DESTROY_GRACEFUL to alter the behavior. If this flag is set in the call to virDomainDestroyFlags, SIGKILL will never be sent to the qemu process; instead, if the timeout is reached and the qemu process still exists, virDomainDestroy will return an error. Once this patch is in, the recommended method for applications to call virDomainDestroyFlags will be with VIR_DOMAIN_DESTROY_GRACEFUL included. If that fails, then the application can decide if and when to call virDomainDestroyFlags again without VIR_DOMAIN_DESTROY_GRACEFUL (to force the issue with SIGKILL). (Note that this does not address the issue of existing applications that have not yet been modified to use VIR_DOMAIN_DESTROY_GRACEFUL. That is a separate patch.) --- include/libvirt/libvirt.h.in | 12 ++++---- src/libvirt.c | 20 ++++++++++-- src/qemu/qemu_driver.c | 12 ++++++- src/qemu/qemu_process.c | 68 +++++++++++++++++++++++++++--------------- src/qemu/qemu_process.h | 9 ++++- 5 files changed, 83 insertions(+), 38 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index d9b9b95..9440751 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1186,12 +1186,6 @@ virConnectPtr virDomainGetConnect (virDomainPtr domain); * Domain creation and destruction */ - -/* - * typedef enum { - * } virDomainDestroyFlagsValues; - */ - virDomainPtr virDomainCreateXML (virConnectPtr conn, const char *xmlDesc, unsigned int flags); @@ -1226,6 +1220,12 @@ int virDomainReset (virDomainPtr domain, unsigned int flags); int virDomainDestroy (virDomainPtr domain);

+ +typedef enum { + VIR_DOMAIN_DESTROY_DEFAULT = 0, /* Default behavior - could lead to data loss!! */ + VIR_DOMAIN_DESTROY_GRACEFUL = 1 << 0, /* only SIGTERM, no SIGKILL */ +} virDomainDestroyFlagsValues; + int virDomainDestroyFlags (virDomainPtr domain, unsigned int flags); int virDomainRef (virDomainPtr domain); diff --git a/src/libvirt.c b/src/libvirt.c index c609202..5833d8d 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -2233,10 +2233,22 @@ error: * This does not free the associated virDomainPtr object. * This function may require privileged access. * - * Calling this function with no @flags set (equal to zero) - * is equivalent to calling virDomainDestroy. Using virDomainShutdown() - * may produce cleaner results for the guest's disks, but depends on guest - * support. + * Calling this function with no @flags set (equal to zero) is + * equivalent to calling virDomainDestroy, and after a reasonable + * timeout will forcefully terminate the guest (e.g. SIGKILL) if + * necessary (which may produce undesirable results, for example + * unflushed disk cache in the guest). Including + * VIR_DOMAIN_DESTROY_GRACEFUL in the flags will prevent the forceful + * termination of the guest, and virDomainDestroyFlags will instead + * return an error if the guest doesn't terminate by the end of the + * timeout; at that time, the management application can decide if + * calling again without VIR_DOMAIN_DESTROY_GRACEFUL is appropriate. + * + * Another alternative which may produce cleaner results for the + * guest's disks is to use virDomainShutdown() instead, but that + * depends on guest support (some hypervisor/guest combinations may + * ignore the shutdown request). + * * * Returns 0 in case of success and -1 in case of failure. */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1b147a9..ad81e64 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1754,7 +1754,7 @@ qemuDomainDestroyFlags(virDomainPtr dom, virDomainEventPtr event = NULL; qemuDomainObjPrivatePtr priv; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_DESTROY_GRACEFUL, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -1775,7 +1775,15 @@ qemuDomainDestroyFlags(virDomainPtr dom, * can kill the process even if a job is active. Killing * it now means the job will be released */ - qemuProcessKill(vm, false); + if (flags & VIR_DOMAIN_DESTROY_GRACEFUL) { + if (qemuProcessKill(vm, 0) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to kill qemu process with SIGTERM")); + goto cleanup; + } + } else { + ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_FORCE)); + } /* We need to prevent monitor EOF callback from doing our work (and sending * misleading events) while the vm is unlocked inside BeginJob API diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 116a828..1bbb55c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1,7 +1,7 @@ /* * qemu_process.h: QEMU process management * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -587,7 +587,7 @@ endjob: cleanup: if (vm) { if (ret == -1) - qemuProcessKill(vm, false); + ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_FORCE)); if (virDomainObjUnref(vm) > 0) virDomainObjUnlock(vm); } @@ -612,12 +612,12 @@ qemuProcessShutdownOrReboot(struct qemud_driver *driver, qemuProcessFakeReboot, vm) < 0) { VIR_ERROR(_("Failed to create reboot thread, killing domain")); - qemuProcessKill(vm, true); + ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_NOWAIT)); /* Safe to ignore value since ref count was incremented above */ ignore_value(virDomainObjUnref(vm)); } } else { - qemuProcessKill(vm, true); + ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_NOWAIT)); } } @@ -3532,45 +3532,65 @@ cleanup: } -void qemuProcessKill(virDomainObjPtr vm, bool gracefully) +int qemuProcessKill(virDomainObjPtr vm, unsigned int flags) { int i; - VIR_DEBUG("vm=%s pid=%d gracefully=%d", - vm->def->name, vm->pid, gracefully); + const char *signame = "TERM"; + + VIR_DEBUG("vm=%s pid=%d flags=%x", + vm->def->name, vm->pid, flags); if (!virDomainObjIsActive(vm)) { VIR_DEBUG("VM '%s' not active", vm->def->name); - return; + return 0; } - /* This loop sends SIGTERM, then waits a few iterations - * (1.6 seconds) to see if it dies. If still alive then - * it does SIGKILL, and waits a few more iterations (1.6 - * seconds more) to confirm that it has really gone. + /* This loop sends SIGTERM (or SIGKILL if flags has + * VIR_QEMU_PROCESS_KILL_FORCE and VIR_QEMU_PROCESS_KILL_NOWAIT), + * then waits a few iterations (3 seconds) to see if it + * dies. Halfway through this wait, if the qemu process still + * hasn't exited, and VIR_QEMU_PROCESS_KILL_FORCE is requested, a + * SIGKILL will be sent. Note that the FORCE mode could result + * in lost data in the guest, so it should only be used if the + * guest is hung and can't be destroyed in any other manner. */ - for (i = 0 ; i < 15 ; i++) { + for (i = 0 ; i < 15; i++) { int signum; - if (i == 0) - signum = SIGTERM; - else if (i == 8) - signum = SIGKILL; - else + if (i == 0) { + if ((flags & VIR_QEMU_PROCESS_KILL_FORCE) && + (flags & VIR_QEMU_PROCESS_KILL_NOWAIT)) { + signum = SIGKILL; /* nuke it immediately */

+ signame="KILL"; + } else { + signum = SIGTERM; /* kindly suggest it should exit */ + } + } else if ((i == 8) & (flags & VIR_QEMU_PROCESS_KILL_FORCE)) { + VIR_WARN("Timed out waiting after SIG%s to process %d, " + "sending SIGKILL", signame, vm->pid); + signum = SIGKILL; /* nuke it after a grace period */

+ signame="KILL"; + } else { signum = 0; /* Just check for existence */ + } if (virKillProcess(vm->pid, signum) < 0) { if (errno != ESRCH) { char ebuf[1024]; - VIR_WARN("Failed to kill process %d %s", - vm->pid, virStrerror(errno, ebuf, sizeof ebuf)); + VIR_WARN("Failed to terminate process %d with SIG%s: %s", + vm->pid, signame, + virStrerror(errno, ebuf, sizeof ebuf)); + return -1; } - break; + return 0; /* process is dead */ } - if (i == 0 && gracefully) - break; + if (i == 0 && (flags & VIR_QEMU_PROCESS_KILL_NOWAIT)) + return 0; usleep(200 * 1000); } + VIR_WARN("Timed out waiting after SIG%s to process %d", signame, vm->pid); + return -1; } @@ -3659,7 +3679,7 @@ void qemuProcessStop(struct qemud_driver *driver, } /* shut it off for sure */ - qemuProcessKill(vm, false); + ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_FORCE)); /* Stop autodestroy in case guest is restarted */ qemuProcessAutoDestroyRemove(driver, vm); diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 062d79e..4ae6b4b 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -1,7 +1,7 @@ /* * qemu_process.c: QEMU process management * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -68,7 +68,12 @@ int qemuProcessAttach(virConnectPtr conn, virDomainChrSourceDefPtr monConfig, bool monJSON); -void qemuProcessKill(virDomainObjPtr vm, bool gracefully); +typedef enum { + VIR_QEMU_PROCESS_KILL_FORCE = 1 << 0, + VIR_QEMU_PROCESS_KILL_NOWAIT = 1 << 1, +} virQemuProcessKillMode; + +int qemuProcessKill(virDomainObjPtr vm, unsigned int flags); int qemuProcessAutoDestroyInit(struct qemud_driver *driver); void qemuProcessAutoDestroyRun(struct qemud_driver *driver, -- 1.7.7.5