On Thu, Feb 02, 2012 at 12:54:28PM -0500, Laine Stump wrote:
> 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);
Please add here a standard description comment for the datatype:
/**
* virDomainDestroyFlagsValues:
*
* Flags used to provide specific behaviour to the
* virDomainDestroyFlags() function
*/
for automatic extraction :-)
> +
> +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 */
s/nuke/kill/ :-)
> + 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 */
idem :-)
> + 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
Except for the 2 nits that looks fine to me
ACK
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list