Fix performance & reliabilty of QMP probing

This previous commit

  commit 1a50ba2cb07d8bb2aa724062889deb9efd7ad9e9
  Author: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
  Date:   Mon Nov 26 15:17:13 2012 +0100

    qemu: Fix QMP Capabability Probing Failure

which attempted to make sure the QEMU process used for probing
ran as the right user id, caused serious performance regression
and unreliability in probing. The -daemonize switch in QEMU
guarantees that the monitor socket is present before the parent
process exits. This means libvirtd is guaranteed to be able to
connect immediately. By switching from -daemonize to the
virCommandDaemonize API libvirtd was no longer synchronized with
QEMU's startup process. The result was that the QEMU monitor
failed to open and went into its 200ms sleep loop. This happened
for all 25 binaries resulting in 5 seconds worth of sleeping
at libvirtd startup. In addition sometimes when libvirt connected,
QEMU would be partially initialized and crash causing total
failure to probe that binary.

This commit reverts the previous change, ensuring we do use the
-daemonize flag to QEMU. Startup delay is cut from 7 seconds
to 2 seconds on my machine, which is on a par with what it was
prior to the capabilities rewrite.

To deal with the fact that QEMU needs to be able to create the
pidfile, we switch pidfile location fron runDir to libDir, which
QEMU is guaranteed to be able to write to.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 95fa3da486aa4734db08753558bf5b4ab8bcba2d..f86c28f2a205b2174f31f7bfbfb01dfddf900d38 100644 (file)
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2291,7 +2291,6 @@ qemuCapsInitQMPBasic(qemuCapsPtr caps)
 static int
 qemuCapsInitQMP(qemuCapsPtr caps,
                 const char *libDir,
-                const char *runDir,
                 uid_t runUid,
                 gid_t runGid)
 {
@@ -2324,8 +2323,11 @@ qemuCapsInitQMP(qemuCapsPtr caps,
 
     /* ".pidfile" suffix is used rather than ".pid" to avoid a possible clash
      * with a qemu domain called "capabilities"
+     * Normally we'd use runDir for pid files, but because we're using
+     * -daemonize we need QEMU to be allowed to create them, rather
+     * than libvirtd. So we're using libDir which QEMU can write to
      */
-    if (virAsprintf(&pidfile, "%s/%s", runDir, "capabilities.pidfile") < 0) {
+    if (virAsprintf(&pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0) {
         virReportOOMError();
         goto cleanup;
     }
@@ -2337,6 +2339,13 @@ qemuCapsInitQMP(qemuCapsPtr caps,
 
     VIR_DEBUG("Try to get caps via QMP caps=%p", caps);
 
+    /*
+     * We explicitly need to use -daemonize here, rather than
+     * virCommandDaemonize, because we need to synchronize
+     * with QEMU creating its monitor socket API. Using
+     * daemonize guarantees control won't return to libvirt
+     * until the socket is present.
+     */
     cmd = virCommandNewArgList(caps->binary,
                                "-S",
                                "-no-user-config",
@@ -2344,14 +2353,14 @@ qemuCapsInitQMP(qemuCapsPtr caps,
                                "-nographic",
                                "-M", "none",
                                "-qmp", monarg,
+                               "-pidfile", pidfile,
+                               "-daemonize",
                                NULL);
     virCommandAddEnvPassCommon(cmd);
     virCommandClearCaps(cmd);
     hookData.runUid = runUid;
     hookData.runGid = runGid;
     virCommandSetPreExecHook(cmd, qemuCapsHook, &hookData);
-    virCommandSetPidFile(cmd, pidfile);
-    virCommandDaemonize(cmd);
 
     if (virCommandRun(cmd, &status) < 0)
         goto cleanup;
@@ -2472,7 +2481,6 @@ cleanup:
 
 qemuCapsPtr qemuCapsNewForBinary(const char *binary,
                                  const char *libDir,
-                                 const char *runDir,
                                  uid_t runUid,
                                  gid_t runGid)
 {
@@ -2502,7 +2510,7 @@ qemuCapsPtr qemuCapsNewForBinary(const char *binary,
         goto error;
     }
 
-    if ((rv = qemuCapsInitQMP(caps, libDir, runDir, runUid, runGid)) < 0)
+    if ((rv = qemuCapsInitQMP(caps, libDir, runUid, runGid)) < 0)
         goto error;
 
     if (!caps->usedQMP &&
@@ -2542,8 +2550,9 @@ qemuCapsHashDataFree(void *payload, const void *key ATTRIBUTE_UNUSED)
 
 
 qemuCapsCachePtr
-qemuCapsCacheNew(const char *libDir, const char *runDir,
-                 uid_t runUid, gid_t runGid)
+qemuCapsCacheNew(const char *libDir,
+                 uid_t runUid,
+                 gid_t runGid)
 {
     qemuCapsCachePtr cache;
 
@@ -2561,8 +2570,7 @@ qemuCapsCacheNew(const char *libDir, const char *runDir,
 
     if (!(cache->binaries = virHashCreate(10, qemuCapsHashDataFree)))
         goto error;
-    if (!(cache->libDir = strdup(libDir)) ||
-        !(cache->runDir = strdup(runDir))) {
+    if (!(cache->libDir = strdup(libDir))) {
         virReportOOMError();
         goto error;
     }
@@ -2594,7 +2602,7 @@ qemuCapsCacheLookup(qemuCapsCachePtr cache, const char *binary)
     if (!ret) {
         VIR_DEBUG("Creating capabilities for %s",
                   binary);
-        ret = qemuCapsNewForBinary(binary, cache->libDir, cache->runDir,
+        ret = qemuCapsNewForBinary(binary, cache->libDir,
                                    cache->runUid, cache->runGid);
         if (ret) {
             VIR_DEBUG("Caching capabilities %p for %s",
@@ -2634,7 +2642,6 @@ qemuCapsCacheFree(qemuCapsCachePtr cache)
         return;
 
     VIR_FREE(cache->libDir);
-    VIR_FREE(cache->runDir);
     virHashFree(cache->binaries);
     virMutexDestroy(&cache->lock);
     VIR_FREE(cache);

diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 089fa304e22a5c9fa536d5bb366ff68013e35ba2..5279d07605929b7878dbe09c21942d1b4fb2adad 100644 (file)
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -179,7 +179,6 @@ qemuCapsPtr qemuCapsNew(void);
 qemuCapsPtr qemuCapsNewCopy(qemuCapsPtr caps);
 qemuCapsPtr qemuCapsNewForBinary(const char *binary,
                                  const char *libDir,
-                                 const char *runDir,
                                  uid_t runUid,
                                  gid_t runGid);
 
@@ -219,7 +218,7 @@ int qemuCapsGetMachineTypesCaps(qemuCapsPtr caps,
 bool qemuCapsIsValid(qemuCapsPtr caps);
 
 
-qemuCapsCachePtr qemuCapsCacheNew(const char *libDir, const char *runDir,
+qemuCapsCachePtr qemuCapsCacheNew(const char *libDir,
                                   uid_t uid, gid_t gid);
 qemuCapsPtr qemuCapsCacheLookup(qemuCapsCachePtr cache, const char *binary);
 qemuCapsPtr qemuCapsCacheLookupCopy(qemuCapsCachePtr cache, const char *binary);