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.)

5 files changed, 98 insertions, 38 deletions

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index cca6a5ddc0..798ab071d3 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -1191,12 +1191,6 @@ virConnectPtr           virDomainGetConnect     (virDomainPtr domain);
  * Domain creation and destruction
  */
 
-
-/*
- * typedef enum {
- * } virDomainDestroyFlagsValues;
- */
-
 virDomainPtr            virDomainCreateXML      (virConnectPtr conn,
                                                  const char *xmlDesc,
                                                  unsigned int flags);
@@ -1231,6 +1225,18 @@ int                     virDomainReset          (virDomainPtr domain,
                                                  unsigned int flags);
 
 int                     virDomainDestroy        (virDomainPtr domain);
+
+/**
+ * virDomainDestroyFlagsValues:
+ *
+ * Flags used to provide specific behaviour to the
+ * virDomainDestroyFlags() function
+ */
+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 11e26358bb..8035add37e 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -2181,6 +2181,15 @@ error:
  * does not free the associated virDomainPtr object.
  * This function may require privileged access
  *
+ * virDomainDestroy first requests that a guest terminate
+ * (e.g. SIGTERM), then waits for it to comply. After a reasonable
+ * timeout, if the guest still exists, virDomainDestory will
+ * forcefully terminate the guest (e.g. SIGKILL) if necessary (which
+ * may produce undesirable results, for example unflushed disk cache
+ * in the guest). To avoid this possibility, it's recommended to
+ * instead call virDomainDestroyFlags, sending the
+ * VIR_DOMAIN_DESTROY_GRACEFUL flag.
+ *
  * If the domain is transient and has any snapshot metadata (see
  * virDomainSnapshotNum()), then that metadata will automatically
  * be deleted when the domain quits.
@@ -2233,10 +2242,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 1daf4fca7c..b396ba0a8d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1769,7 +1769,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);
@@ -1790,7 +1790,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 116a828bbe..2d92d66a0e 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; /* kill 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; /* kill 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 062d79ec35..4ae6b4b618 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,