libxl: wait for QEMU exit before restarting a domain

The PID-based device model destruction path returns immediately after sending SIGHUP, before QEMU has necessarily exited. When an HVM guest reboots with VNC enabled, the new QEMU process may fail to bind the VNC port because the old process still holds it.

Wait for QEMU to exit in a child process after singaling it for at most

60 seconds. If waiting fails or times out, log the error and retain the existing domain destruction behaviour.

tools/libs/light/libxl_dm.c +4 -0

+        assert(rc <= 0 && rc >= -125);
+        _exit(-rc);
+    }
+    return;
 
  out:
      /*

Quoted 5 lines · Jiaqing Zhao

The PID-based device model destruction path returns immediately after sending SIGHUP, before QEMU has necessarily exited. When an HVM guest reboots with VNC enabled, the new QEMU process may fail to bind the VNC port because the old process still holds it.

Quoted 1 line

Wait for QEMU to exit in a child process after singaling it for at most

"signaling"

Quoted 2 lines

60 seconds. If waiting fails or times out, log the error and retain the existing domain destruction behaviour.

60 seconds seems like a lot.  Did you measure the time to see how long it takes?  It's an upper bound, but maybe 10 seconds would be better?

tools/libs/light/libxl_dm.c void libxl__destroy_device_model(libxl__egc *egc, · line 3566

Quoted 8 lines

Signed-off-by Jiaqing Zhao <[email protected]>

tools/libs/light/libxl_dm.c +1 -2

@@ -3547,12 +3564,28 @@ void libxl__destroy_device_model(libxl__egc *egc,
       LOGD(DEBUG, domid, "Didn't find dm UID; destroying by pid");
         path = GCSPRINTF("/local/domain/%d/image/device-model-pid", domid);
-    rc = kill_device_model(gc, path);
-
+    rc = kill_device_model(gc, path, &dm_pid);

If you just read the pid directly here, you don't need to change kill_device_model().  But your approach is only a single read, so maybe it's better.  I'm undecided, but maybe someone else has an opinion.

tools/libs/light/libxl_dm.c void libxl__destroy_device_model(libxl__egc *egc, · line 3576

Quoted 9 lines

tools/libs/light/libxl_dm.c +6 -0

       if (rc) {
           ACCUMULATE_RC(ddms->rc);
           LOGD(ERROR, domid, "Killing device model pid from path %s", path);
+        goto out;
+    }
+
+    LOGD(DEBUG, domid, "Waiting for device model [%d] to exit", dm_pid);
+    reaper_pid = libxl__ev_child_fork(gc, &ddms->destroyer,
+                                      kill_device_model_uid_cb);

The reaper_pid handling below is duplicated from
if (dm_kill_uid_str) {

Put the above pid code in an else, and use a single implementation of the reaper_pid handling.

tools/libs/light/libxl_dm.c void libxl__destroy_device_model(libxl__egc *egc, · line 3584

Quoted 8 lines

tools/libs/light/libxl_dm.c +7 -0

+    if (reaper_pid < 0) {
+        rc = ERROR_FAIL;
+        ACCUMULATE_RC(ddms->rc);
+        LOGD(ERROR, domid, "Unable to fork device model exit waiter");
+        goto out;
       }
+    if (!reaper_pid) {
+        rc = wait_device_model_pid_child(gc, domid, dm_pid);

Though this would need an additional conditional.

Thanks,
Jason

Quoted 6 lines

tools/libs/light/libxl_dm.c +4 -0

+        assert(rc <= 0 && rc >= -125);
+        _exit(-rc);
+    }
+    return;

    out:
      /*

Quoted 6 lines · Jiaqing Zhao

The PID-based device model destruction path returns immediately after sending SIGHUP, before QEMU has necessarily exited. When an HVM guest reboots with VNC enabled, the new QEMU process may fail to bind the VNC port because the old process still holds it.

Wait for QEMU to exit in a child process after singaling it for at most

"signaling"

Quoted 2 lines

60 seconds. If waiting fails or times out, log the error and retain the existing domain destruction behaviour.

60 seconds seems like a lot. Did you measure the time to see how long it takes? It's an upper bound, but maybe 10 seconds would be better?

Quoted 13 lines · Jason Andryuk

Quoted 6 lines · Jiaqing Zhao

The PID-based device model destruction path returns immediately after sending SIGHUP, before QEMU has necessarily exited. When an HVM guest reboots with VNC enabled, the new QEMU process may fail to bind the VNC port because the old process still holds it.

Wait for QEMU to exit in a child process after singaling it for at most

"signaling"

Quoted 2 lines

60 seconds. If waiting fails or times out, log the error and retain the existing domain destruction behaviour.

Quoted 2 lines

60 seconds seems like a lot. Did you measure the time to see how long it takes? It's an upper bound, but maybe 10 seconds would be better?

Is 10s enough for it to time out upon disconnecting pass-through devices? I'm seeing this happen routinely, albeit with an older version of qemu.

Jan

Quoted 11 lines · Jan Beulich

Quoted 10 lines · Jason Andryuk

Quoted 6 lines · Jiaqing Zhao

The PID-based device model destruction path returns immediately after sending SIGHUP, before QEMU has necessarily exited. When an HVM guest reboots with VNC enabled, the new QEMU process may fail to bind the VNC port because the old process still holds it.

Wait for QEMU to exit in a child process after singaling it for at most

"signaling"

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

60 seconds. If waiting fails or times out, log the error and retain the existing domain destruction behaviour.

60 seconds seems like a lot. Did you measure the time to see how long it takes? It's an upper bound, but maybe 10 seconds would be better?

Is 10s enough for it to time out upon disconnecting pass-through devices? I'm seeing this happen routinely, albeit with an older version of qemu.

I checked the code, passthorugh devices are removed via QMP, with pci_remove_qmp_query_cb(), before sending SIGHUP to terminate QEMU.

Passing through a PCI SD card controller on my system took ~1.2s from xl destory to sending SIGHUP to QEMU, and QEMU then took ~50ms to exit. So 10s is pretty enough here I think.

The PID-based device model destruction path returns immediately after sending SIGHUP, before QEMU has necessarily exited. When an HVM guest reboots with VNC enabled, the new QEMU process may fail to bind the VNC port because the old process still holds it.

Wait for QEMU to exit in a child process after singaling it for at most 60 seconds. If waiting fails or times out, log the error and retain the existing domain destruction behaviour.

Signed-off-by Jiaqing Zhao <[email protected]>

---
Cc: Anthony PERARD <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Roger Pau Monné <[email protected]>
Cc: Stefano Stabellini <[email protected]>
Cc: Penny Zheng <[email protected]>

---
 tools/libs/light/libxl_aoutils.c  |  8 ++++--
 tools/libs/light/libxl_dm.c       | 45 ++++++++++++++++++++++++++-----
 tools/libs/light/libxl_domain.c   |  2 +-
 tools/libs/light/libxl_internal.h |  3 ++-
 4 files changed, 48 insertions(+), 10 deletions(-)

tools/libs/light/libxl_aoutils.c +6 -2

index c2d42e7cac..977a2705a8 100644
@@ -628,10 +628,11 @@ void libxl__kill(libxl__gc *gc, pid_t pid, int sig, const char *what)
 
 /* Generic function to signal (HUP) a pid stored in xenstore */
 int libxl__kill_xs_path(libxl__gc *gc, const char *xs_path_pid,
-                        const char *what)
+                        const char *what, pid_t *pid_r)
 {
     const char *xs_pid;
-    int ret, pid;
+    int ret;
+    pid_t pid;
 
     ret = libxl__xs_read_checked(gc, XBT_NULL, xs_path_pid, &xs_pid);
     if (ret || !xs_pid) {

@@ -640,6 +641,9 @@ int libxl__kill_xs_path(libxl__gc *gc, const char *xs_path_pid,
         goto out;
     }
     pid = atoi(xs_pid);
+    if (pid_r) {
+        *pid_r = pid;
+    }
 
     ret = kill(pid, SIGHUP);
     if (ret < 0 && errno == ESRCH) {

tools/libs/light/libxl_dm.c +5 -4

index 511ec76a65..ac1db9fe6a 100644
@@ -2859,7 +2859,7 @@ static void device_model_probe_spawn_outcome(libxl__egc *egc,
 
     /* Ensure our QEMU command line probe is killed. */
     rc = libxl__kill_xs_path(gc, dmss->spawn.pidpath,
-                             "qemu command-line probe");
+                             "qemu command-line probe", NULL);
     if (rc) {
         LOGD(WARN, dmss->guest_domid,
              "Killing qemu command-line probe pid from path %s",

@@ -3420,9 +3420,10 @@ out:
 }
 
 /* Generic function to signal a Qemu instance to exit */
-static int kill_device_model(libxl__gc *gc, const char *xs_path_pid)
+static int kill_device_model(libxl__gc *gc, const char *xs_path_pid,
+                             pid_t *pid_r)
 {
-    return libxl__kill_xs_path(gc, xs_path_pid, "Device Model");
+    return libxl__kill_xs_path(gc, xs_path_pid, "Device Model", pid_r);
 }
 
 /* Helper to destroy a Qdisk backend */

@@ -3433,7 +3434,7 @@ int libxl__destroy_qemu_xenpv_backend(libxl__gc *gc, uint32_t domid)
 
     pid_path = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
 
-    rc = kill_device_model(gc, pid_path);
+    rc = kill_device_model(gc, pid_path, NULL);
     if (rc)
         goto out;
 

Quoted 87 lines · Jiaqing Zhao

The PID-based device model destruction path returns immediately after sending SIGHUP, before QEMU has necessarily exited. When an HVM guest reboots with VNC enabled, the new QEMU process may fail to bind the VNC port because the old process still holds it.

Wait for QEMU to exit in a child process after singaling it for at most 60 seconds. If waiting fails or times out, log the error and retain the existing domain destruction behaviour.

Signed-off-by Jiaqing Zhao <[email protected]>

---
Cc: Anthony PERARD <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Roger Pau Monné <[email protected]>
Cc: Stefano Stabellini <[email protected]>
Cc: Penny Zheng <[email protected]>

---
 tools/libs/light/libxl_aoutils.c  |  8 ++++--
 tools/libs/light/libxl_dm.c       | 45 ++++++++++++++++++++++++++-----
 tools/libs/light/libxl_domain.c   |  2 +-
 tools/libs/light/libxl_internal.h |  3 ++-
 4 files changed, 48 insertions(+), 10 deletions(-)

tools/libs/light/libxl_aoutils.c +6 -2

index c2d42e7cac..977a2705a8 100644
@@ -628,10 +628,11 @@ void libxl__kill(libxl__gc *gc, pid_t pid, int sig, const char *what)
 
 /* Generic function to signal (HUP) a pid stored in xenstore */
 int libxl__kill_xs_path(libxl__gc *gc, const char *xs_path_pid,
-                        const char *what)
+                        const char *what, pid_t *pid_r)
 {
     const char *xs_pid;
-    int ret, pid;
+    int ret;
+    pid_t pid;
 
     ret = libxl__xs_read_checked(gc, XBT_NULL, xs_path_pid, &xs_pid);
     if (ret || !xs_pid) {

@@ -640,6 +641,9 @@ int libxl__kill_xs_path(libxl__gc *gc, const char *xs_path_pid,
         goto out;
     }
     pid = atoi(xs_pid);
+    if (pid_r) {
+        *pid_r = pid;
+    }
 
     ret = kill(pid, SIGHUP);
     if (ret < 0 && errno == ESRCH) {

tools/libs/light/libxl_dm.c +5 -4

index 511ec76a65..ac1db9fe6a 100644
@@ -2859,7 +2859,7 @@ static void device_model_probe_spawn_outcome(libxl__egc *egc,
 
     /* Ensure our QEMU command line probe is killed. */
     rc = libxl__kill_xs_path(gc, dmss->spawn.pidpath,
-                             "qemu command-line probe");
+                             "qemu command-line probe", NULL);
     if (rc) {
         LOGD(WARN, dmss->guest_domid,
              "Killing qemu command-line probe pid from path %s",

@@ -3420,9 +3420,10 @@ out:
 }
 
 /* Generic function to signal a Qemu instance to exit */
-static int kill_device_model(libxl__gc *gc, const char *xs_path_pid)
+static int kill_device_model(libxl__gc *gc, const char *xs_path_pid,
+                             pid_t *pid_r)
 {
-    return libxl__kill_xs_path(gc, xs_path_pid, "Device Model");
+    return libxl__kill_xs_path(gc, xs_path_pid, "Device Model", pid_r);
 }
 
 /* Helper to destroy a Qdisk backend */

@@ -3433,7 +3434,7 @@ int libxl__destroy_qemu_xenpv_backend(libxl__gc *gc, uint32_t domid)
 
     pid_path = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
 
-    rc = kill_device_model(gc, pid_path);
+    rc = kill_device_model(gc, pid_path, NULL);
     if (rc)
         goto out;
 

tools/libs/light/libxl_dm.c static void kill_device_model_uid_cb(libxl__egc *egc, · line 3464

Quoted 10 lines

tools/libs/light/libxl_dm.c +7 -0

@@ -3454,6 +3455,21 @@ static void kill_device_model_uid_cb(libxl__egc *egc,
                                      libxl__ev_child *destroyer,
                                      pid_t pid, int status);
 
+/* Run in a child so waiting for QEMU does not block the libxl event loop. */
+static int wait_device_model_pid_child(libxl__gc *gc, uint32_t domid, pid_t pid)
+{
+    unsigned int i;
+
+    for (i = 0; i < LIBXL_DEVICE_MODEL_STOP_TIMEOUT * 10; i++) {
+        if (kill(pid, 0) && errno == ESRCH)

Isn't this theoretically racy? How can you guarantee the PID hasn't been reused, and thus you end up killing an unexpected process on further loops?

I see you hook this up into the events interface by spawning a process that does the spinning, have you looked whether it's possible to get PID state changes as notification on a file descriptor? That would be more natural to hook into the events interface, and would avoid the race mentioned above AFAICT.

Thanks, Roger.