[libvirt PATCH v5 25/32] qemu: Monitor nbdkit process for exit

Jonathon Jongsma posted 32 patches 2 years, 12 months ago
There is a newer version of this series
[libvirt PATCH v5 25/32] qemu: Monitor nbdkit process for exit
Posted by Jonathon Jongsma 2 years, 12 months ago
Adds the ability to monitor the nbdkit process so that we can take
action in case the child exits unexpectedly.

When the nbdkit process exits, we pause the vm, restart nbdkit, and then
resume the vm. This allows the vm to continue working in the event of a
nbdkit failure.

Eventually we may want to generalize this functionality since we may
need something similar for e.g. qemu-storage-daemon, etc.

The process is monitored with the pidfd_open() syscall if it exists
(since linux 5.3). Otherwise it resorts to checking whether the process
is alive once a second. The one-second time period was chosen somewhat
arbitrarily.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
 meson.build             |   7 ++
 src/qemu/qemu_nbdkit.c  | 147 ++++++++++++++++++++++++++++++++++++++--
 src/qemu/qemu_nbdkit.h  |   4 +-
 src/qemu/qemu_process.c |   4 +-
 4 files changed, 155 insertions(+), 7 deletions(-)

diff --git a/meson.build b/meson.build
index e498b49be4..048b15ff71 100644
--- a/meson.build
+++ b/meson.build
@@ -645,6 +645,13 @@ symbols = [
   [ 'sched.h', 'cpu_set_t' ],
 ]
 
+if host_machine.system() == 'linux'
+  symbols += [
+    # process management
+    [ 'sys/syscall.h', 'SYS_pidfd_open' ],
+  ]
+endif
+
 foreach symbol : symbols
   if cc.has_header_symbol(symbol[0], symbol[1], args: '-D_GNU_SOURCE', prefix: symbol.get(2, ''))
     conf.set('WITH_DECL_@0@'.format(symbol[1].to_upper()), 1)
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
index e6d3232a1e..74e2ceee19 100644
--- a/src/qemu/qemu_nbdkit.c
+++ b/src/qemu/qemu_nbdkit.c
@@ -19,9 +19,11 @@
 
 #include <config.h>
 #include <glib.h>
+#include <sys/syscall.h>
 
 #include "vircommand.h"
 #include "virerror.h"
+#include "virevent.h"
 #include "virlog.h"
 #include "virpidfile.h"
 #include "virsecureerase.h"
@@ -35,6 +37,7 @@
 #include "qemu_nbdkit.h"
 #define LIBVIRT_QEMU_NBDKITPRIV_H_ALLOW
 #include "qemu_nbdkitpriv.h"
+#include "qemu_process.h"
 #include "qemu_security.h"
 
 #include <fcntl.h>
@@ -613,6 +616,123 @@ qemuNbdkitCapsCacheNew(const char *cachedir)
 }
 
 
+static void
+qemuNbdkitProcessRestart(qemuNbdkitProcess *proc,
+                         virDomainObj *vm)
+{
+    qemuDomainObjPrivate *vmpriv = vm->privateData;
+    virQEMUDriver *driver = vmpriv->driver;
+
+    /* clean up resources associated with process */
+    qemuNbdkitProcessStop(proc);
+
+    if (qemuNbdkitProcessStart(proc, vm, driver) < 0)
+        VIR_WARN("Unable to restart nbkdit process");
+}
+
+
+typedef struct {
+    qemuNbdkitProcess *proc;
+    virDomainObj *vm;
+} qemuNbdkitProcessEventData;
+
+
+static qemuNbdkitProcessEventData*
+qemuNbdkitProcessEventDataNew(qemuNbdkitProcess *proc,
+                              virDomainObj *vm)
+{
+    qemuNbdkitProcessEventData *d = g_new(qemuNbdkitProcessEventData, 1);
+    d->proc = proc;
+    d->vm = virObjectRef(vm);
+    return d;
+}
+
+
+static void
+qemuNbdkitProcessEventDataFree(qemuNbdkitProcessEventData *d)
+{
+    virObjectUnref(d->vm);
+    g_free(d);
+}
+
+
+#if WITH_DECL_SYS_PIDFD_OPEN
+static void
+qemuNbdkitProcessPidfdCb(int watch G_GNUC_UNUSED,
+                         int fd,
+                         int events G_GNUC_UNUSED,
+                         void *opaque)
+{
+    qemuNbdkitProcessEventData *d = opaque;
+
+    VIR_FORCE_CLOSE(fd);
+    VIR_DEBUG("nbdkit process %i died", d->proc->pid);
+    qemuNbdkitProcessRestart(d->proc, d->vm);
+}
+#else
+static void
+qemuNbdkitProcessTimeoutCb(int timer G_GNUC_UNUSED,
+                           void *opaque)
+{
+    qemuNbdkitProcessEventData *d = opaque;
+
+    if (virProcessKill(d->proc->pid, 0) < 0) {
+        VIR_DEBUG("nbdkit process %i died", d->proc->pid);
+        qemuNbdkitProcessRestart(d->proc, d->vm);
+    }
+}
+#endif /* WITH_DECL_SYS_PIDFD_OPEN */
+
+
+static int
+qemuNbdkitProcessStartMonitor(qemuNbdkitProcess *proc,
+                              virDomainObj *vm)
+{
+#if WITH_DECL_SYS_PIDFD_OPEN
+    int pidfd;
+#endif
+
+#if WITH_DECL_SYS_PIDFD_OPEN
+    pidfd = syscall(SYS_pidfd_open, proc->pid, 0);
+    if (pidfd < 0)
+        return -1;
+
+    proc->eventwatch = virEventAddHandle(pidfd,
+                                         VIR_EVENT_HANDLE_READABLE,
+                                         qemuNbdkitProcessPidfdCb,
+                                         qemuNbdkitProcessEventDataNew(proc, vm),
+                                         (virFreeCallback)qemuNbdkitProcessEventDataFree);
+#else
+    /* fall back to checking once a second */
+    proc->eventwatch = virEventAddTimeout(1000,
+                                          qemuNbdkitProcessTimeoutCb,
+                                          qemuNbdkitProcessEventDataNew(proc, vm),
+                                          (virFreeCallback)qemuNbdkitProcessEventDataFree);
+#endif /* WITH_DECL_SYS_PIDFD_OPEN */
+
+    if (proc->eventwatch < 0)
+        return -1;
+
+    VIR_DEBUG("Monitoring nbdkit process %i for exit", proc->pid);
+
+    return 0;
+}
+
+
+static void
+qemuNbdkitProcessStopMonitor(qemuNbdkitProcess *proc)
+{
+    if (proc->eventwatch > 0) {
+#if WITH_DECL_SYS_PIDFD_OPEN
+        virEventRemoveHandle(proc->eventwatch);
+#else
+        virEventRemoveTimeout(proc->eventwatch);
+#endif /* WITH_DECL_SYS_PIDFD_OPEN */
+        proc->eventwatch = 0;
+    }
+}
+
+
 static qemuNbdkitProcess *
 qemuNbdkitProcessNew(virStorageSource *source,
                      const char *pidfile,
@@ -660,9 +780,11 @@ qemuNbdkitReconnectStorageSource(virStorageSource *source,
 
 
 static void
-qemuNbdkitStorageSourceManageProcessOne(virStorageSource *source)
+qemuNbdkitStorageSourceManageProcessOne(virStorageSource *source,
+                                        virDomainObj *vm)
 {
     qemuDomainStorageSourcePrivate *srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(source);
+    qemuDomainObjPrivate *vmpriv = vm->privateData;
     qemuNbdkitProcess *proc;
 
     if (!srcpriv)
@@ -673,6 +795,9 @@ qemuNbdkitStorageSourceManageProcessOne(virStorageSource *source)
     if (!proc)
         return;
 
+    if (!proc->caps)
+        proc->caps = qemuGetNbdkitCaps(vmpriv->driver);
+
     if (proc->pid <= 0) {
         if (virPidFileReadPath(proc->pidfile, &proc->pid) < 0) {
             VIR_WARN("Unable to read pidfile '%s'", proc->pidfile);
@@ -680,8 +805,14 @@ qemuNbdkitStorageSourceManageProcessOne(virStorageSource *source)
         }
     }
 
-    if (virProcessKill(proc->pid, 0) < 0)
+    if (virProcessKill(proc->pid, 0) < 0) {
         VIR_WARN("nbdkit process %i is not alive", proc->pid);
+        qemuNbdkitProcessRestart(proc, vm);
+        return;
+    }
+
+    if (qemuNbdkitProcessStartMonitor(proc, vm) < 0)
+        VIR_WARN("unable monitor nbdkit process");
 }
 
 /**
@@ -694,11 +825,12 @@ qemuNbdkitStorageSourceManageProcessOne(virStorageSource *source)
  * disk and is attempting to re-connect to active domains.
  */
 void
-qemuNbdkitStorageSourceManageProcess(virStorageSource *source)
+qemuNbdkitStorageSourceManageProcess(virStorageSource *source,
+                                     virDomainObj *vm)
 {
     virStorageSource *backing;
     for (backing = source; backing != NULL; backing = backing->backingStore)
-        qemuNbdkitStorageSourceManageProcessOne(backing);
+        qemuNbdkitStorageSourceManageProcessOne(backing, vm);
 }
 
 
@@ -991,6 +1123,8 @@ qemuNbdkitProcessBuildCommand(qemuNbdkitProcess *proc)
 void
 qemuNbdkitProcessFree(qemuNbdkitProcess *proc)
 {
+    qemuNbdkitProcessStopMonitor(proc);
+
     g_clear_pointer(&proc->pidfile, g_free);
     g_clear_pointer(&proc->socketfile, g_free);
     g_clear_object(&proc->caps);
@@ -1077,6 +1211,9 @@ qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
         goto error;
     }
 
+    if (qemuNbdkitProcessStartMonitor(proc, vm) < 0)
+        goto error;
+
     return 0;
 
  error:
@@ -1098,6 +1235,8 @@ qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
 int
 qemuNbdkitProcessStop(qemuNbdkitProcess *proc)
 {
+    qemuNbdkitProcessStopMonitor(proc);
+
     if (proc->pid < 0)
         return 0;
 
diff --git a/src/qemu/qemu_nbdkit.h b/src/qemu/qemu_nbdkit.h
index 36a2219d82..326f3d5920 100644
--- a/src/qemu/qemu_nbdkit.h
+++ b/src/qemu/qemu_nbdkit.h
@@ -69,7 +69,8 @@ void
 qemuNbdkitStopStorageSource(virStorageSource *src);
 
 void
-qemuNbdkitStorageSourceManageProcess(virStorageSource *src);
+qemuNbdkitStorageSourceManageProcess(virStorageSource *src,
+                                     virDomainObj *vm);
 
 bool
 qemuNbdkitCapsGet(qemuNbdkitCaps *nbdkitCaps,
@@ -91,6 +92,7 @@ struct _qemuNbdkitProcess {
     uid_t user;
     gid_t group;
     pid_t pid;
+    int eventwatch;
 };
 
 int
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d0b797575d..390f1c2862 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -9036,10 +9036,10 @@ qemuProcessReconnect(void *opaque)
     }
 
     for (i = 0; i < obj->def->ndisks; i++)
-        qemuNbdkitStorageSourceManageProcess(obj->def->disks[i]->src);
+        qemuNbdkitStorageSourceManageProcess(obj->def->disks[i]->src, obj);
 
     if (obj->def->os.loader && obj->def->os.loader->nvram)
-        qemuNbdkitStorageSourceManageProcess(obj->def->os.loader->nvram);
+        qemuNbdkitStorageSourceManageProcess(obj->def->os.loader->nvram, obj);
 
     /* update domain state XML with possibly updated state in virDomainObj */
     if (virDomainObjSave(obj, driver->xmlopt, cfg->stateDir) < 0)
-- 
2.39.1
Re: [libvirt PATCH v5 25/32] qemu: Monitor nbdkit process for exit
Posted by Peter Krempa 2 years, 11 months ago
On Tue, Feb 14, 2023 at 11:08:12 -0600, Jonathon Jongsma wrote:
> Adds the ability to monitor the nbdkit process so that we can take
> action in case the child exits unexpectedly.
> 
> When the nbdkit process exits, we pause the vm, restart nbdkit, and then
> resume the vm. This allows the vm to continue working in the event of a
> nbdkit failure.
> 
> Eventually we may want to generalize this functionality since we may
> need something similar for e.g. qemu-storage-daemon, etc.
> 
> The process is monitored with the pidfd_open() syscall if it exists
> (since linux 5.3). Otherwise it resorts to checking whether the process
> is alive once a second. The one-second time period was chosen somewhat
> arbitrarily.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>  meson.build             |   7 ++
>  src/qemu/qemu_nbdkit.c  | 147 ++++++++++++++++++++++++++++++++++++++--
>  src/qemu/qemu_nbdkit.h  |   4 +-
>  src/qemu/qemu_process.c |   4 +-
>  4 files changed, 155 insertions(+), 7 deletions(-)

[...]

> diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
> index e6d3232a1e..74e2ceee19 100644
> --- a/src/qemu/qemu_nbdkit.c
> +++ b/src/qemu/qemu_nbdkit.c

[...]

> +#if WITH_DECL_SYS_PIDFD_OPEN
> +    pidfd = syscall(SYS_pidfd_open, proc->pid, 0);
> +    if (pidfd < 0)
> +        return -1;
> +
> +    proc->eventwatch = virEventAddHandle(pidfd,
> +                                         VIR_EVENT_HANDLE_READABLE,
> +                                         qemuNbdkitProcessPidfdCb,
> +                                         qemuNbdkitProcessEventDataNew(proc, vm),
> +                                         (virFreeCallback)qemuNbdkitProcessEventDataFree);
> +#else
> +    /* fall back to checking once a second */
> +    proc->eventwatch = virEventAddTimeout(1000,
> +                                          qemuNbdkitProcessTimeoutCb,
> +                                          qemuNbdkitProcessEventDataNew(proc, vm),
> +                                          (virFreeCallback)qemuNbdkitProcessEventDataFree);

I don't think we want to do this this polling. I'll rather disable all
of nbdkit than do this if the event interface is not available.

Additionally any failure in this function would result into the default
error of "unable to connect to nbdkit" but this time with no
information.
Re: [libvirt PATCH v5 25/32] qemu: Monitor nbdkit process for exit
Posted by Peter Krempa 2 years, 11 months ago
On Thu, Feb 16, 2023 at 17:23:19 +0100, Peter Krempa wrote:
> On Tue, Feb 14, 2023 at 11:08:12 -0600, Jonathon Jongsma wrote:
> > Adds the ability to monitor the nbdkit process so that we can take
> > action in case the child exits unexpectedly.
> > 
> > When the nbdkit process exits, we pause the vm, restart nbdkit, and then
> > resume the vm. This allows the vm to continue working in the event of a
> > nbdkit failure.
> > 
> > Eventually we may want to generalize this functionality since we may
> > need something similar for e.g. qemu-storage-daemon, etc.
> > 
> > The process is monitored with the pidfd_open() syscall if it exists
> > (since linux 5.3). Otherwise it resorts to checking whether the process
> > is alive once a second. The one-second time period was chosen somewhat
> > arbitrarily.
> > 
> > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > ---
> >  meson.build             |   7 ++
> >  src/qemu/qemu_nbdkit.c  | 147 ++++++++++++++++++++++++++++++++++++++--
> >  src/qemu/qemu_nbdkit.h  |   4 +-
> >  src/qemu/qemu_process.c |   4 +-
> >  4 files changed, 155 insertions(+), 7 deletions(-)
> 
> [...]
> 
> > diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
> > index e6d3232a1e..74e2ceee19 100644
> > --- a/src/qemu/qemu_nbdkit.c
> > +++ b/src/qemu/qemu_nbdkit.c
> 
> [...]
> 
> > +#if WITH_DECL_SYS_PIDFD_OPEN
> > +    pidfd = syscall(SYS_pidfd_open, proc->pid, 0);
> > +    if (pidfd < 0)
> > +        return -1;
> > +
> > +    proc->eventwatch = virEventAddHandle(pidfd,
> > +                                         VIR_EVENT_HANDLE_READABLE,
> > +                                         qemuNbdkitProcessPidfdCb,
> > +                                         qemuNbdkitProcessEventDataNew(proc, vm),
> > +                                         (virFreeCallback)qemuNbdkitProcessEventDataFree);
> > +#else
> > +    /* fall back to checking once a second */
> > +    proc->eventwatch = virEventAddTimeout(1000,
> > +                                          qemuNbdkitProcessTimeoutCb,
> > +                                          qemuNbdkitProcessEventDataNew(proc, vm),
> > +                                          (virFreeCallback)qemuNbdkitProcessEventDataFree);
> 
> I don't think we want to do this this polling. I'll rather disable all
> of nbdkit than do this if the event interface is not available.
> 
> Additionally any failure in this function would result into the default
> error of "unable to connect to nbdkit" but this time with no
> information.

If you get rid of the polling impl:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>