[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]