[libvirt PATCH v4 18/31] qemu: include nbdkit state in private xml

Jonathon Jongsma posted 31 patches 3 years ago
There is a newer version of this series
[libvirt PATCH v4 18/31] qemu: include nbdkit state in private xml
Posted by Jonathon Jongsma 3 years ago
Add xml to the private data for a disk source to represent the nbdkit
process so that the state can be re-created if the libvirt daemon is
restarted. Format:

   <nbdkit>
     <pidfile>/path/to/nbdkit.pid</pidfile>
     <socketfile>/path/to/nbdkit.socket</socketfile>
   </nbdkit>

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
 src/qemu/qemu_domain.c                    | 48 ++++++++++++++
 src/qemu/qemu_nbdkit.c                    | 77 +++++++++++++++++++++++
 src/qemu/qemu_nbdkit.h                    |  8 +++
 src/qemu/qemu_process.c                   | 11 ++++
 tests/qemustatusxml2xmldata/modern-in.xml |  4 ++
 5 files changed, 148 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0e3eaf49f8..28d4bddf14 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1911,6 +1911,29 @@ qemuStorageSourcePrivateDataAssignSecinfo(qemuDomainSecretInfo **secinfo,
 }
 
 
+static int
+qemuStorageSourcePrivateDataParseNbdkit(xmlNodePtr node,
+                                        xmlXPathContextPtr ctxt,
+                                        virStorageSource *src)
+{
+    g_autofree char *pidfile = NULL;
+    g_autofree char *socketfile = NULL;
+    VIR_XPATH_NODE_AUTORESTORE(ctxt);
+
+    ctxt->node = node;
+
+    if (!(pidfile = virXPathString("string(./pidfile)", ctxt)))
+        return -1;
+
+    if (!(socketfile = virXPathString("string(./socketfile)", ctxt)))
+        return -1;
+
+    qemuNbdkitReconnectStorageSource(src, pidfile, socketfile);
+
+    return 0;
+}
+
+
 static int
 qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
                                   virStorageSource *src)
@@ -1921,6 +1944,7 @@ qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
     g_autofree char *httpcookiealias = NULL;
     g_autofree char *tlskeyalias = NULL;
     g_autofree char *thresholdEventWithIndex = NULL;
+    xmlNodePtr nbdkitnode = NULL;
 
     src->nodestorage = virXPathString("string(./nodenames/nodename[@type='storage']/@name)", ctxt);
     src->nodeformat = virXPathString("string(./nodenames/nodename[@type='format']/@name)", ctxt);
@@ -1964,6 +1988,10 @@ qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
         virTristateBoolTypeFromString(thresholdEventWithIndex) == VIR_TRISTATE_BOOL_YES)
         src->thresholdEventWithIndex = true;
 
+    if ((nbdkitnode = virXPathNode("nbdkit", ctxt))) {
+        if (qemuStorageSourcePrivateDataParseNbdkit(nbdkitnode, ctxt, src) < 0)
+            return -1;
+    }
     return 0;
 }
 
@@ -1981,6 +2009,23 @@ qemuStorageSourcePrivateDataFormatSecinfo(virBuffer *buf,
 }
 
 
+static void
+qemuStorageSourcePrivateDataFormatNbdkit(qemuNbdkitProcess *nbdkit,
+                                         virBuffer *buf)
+{
+    g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
+
+    if (!nbdkit)
+        return;
+
+    virBufferEscapeString(&childBuf, "<pidfile>%s</pidfile>\n",
+                          nbdkit->pidfile);
+    virBufferEscapeString(&childBuf, "<socketfile>%s</socketfile>\n",
+                          nbdkit->socketfile);
+    virXMLFormatElement(buf, "nbdkit", NULL, &childBuf);
+}
+
+
 static int
 qemuStorageSourcePrivateDataFormat(virStorageSource *src,
                                    virBuffer *buf)
@@ -2019,6 +2064,9 @@ qemuStorageSourcePrivateDataFormat(virStorageSource *src,
     if (src->thresholdEventWithIndex)
         virBufferAddLit(buf, "<thresholdEvent indexUsed='yes'/>\n");
 
+    if (srcPriv)
+        qemuStorageSourcePrivateDataFormatNbdkit(srcPriv->nbdkitProcess, buf);
+
     return 0;
 }
 
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
index 00ca945904..2b26e9bc08 100644
--- a/src/qemu/qemu_nbdkit.c
+++ b/src/qemu/qemu_nbdkit.c
@@ -627,6 +627,83 @@ qemuNbdkitProcessNew(virStorageSource *source,
     return nbdkit;
 }
 
+/**
+ * qemuNbdkitReconnectStorageSource:
+ * @source: a storage source
+ * @pidfile: a pidfile for an nbdkit process
+ * @socketfile: the socket file associated with the nbdkit process
+ *
+ * This function constructs a new qemuNbdkitProcess object with the given values for @pidfile and
+ * @socketfile and stores it in @source. This is intended to be called when the libvirt daemon is
+ * restarted and tries to reconnect to all currently-running domains. Since this function is called
+ * from the code that parses the current daemon state, it should not perform any filesystem
+ * operations, or anything else that might fail. Additional initialization will be done later by
+ * calling qemuNbdkitStorageSourceManageProcess().
+ */
+void
+qemuNbdkitReconnectStorageSource(virStorageSource *source,
+                                 const char *pidfile,
+                                 const char *socketfile)
+{
+    qemuDomainStorageSourcePrivate *srcpriv = qemuDomainStorageSourcePrivateFetch(source);
+
+    if (srcpriv->nbdkitProcess) {
+        VIR_WARN("source already has an nbdkit process");
+        return;
+    }
+
+    srcpriv->nbdkitProcess = qemuNbdkitProcessNew(source, pidfile, socketfile);
+}
+
+
+static int
+qemuNbdkitStorageSourceManageProcessOne(virStorageSource *source)
+{
+    qemuDomainStorageSourcePrivate *srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(source);
+    qemuNbdkitProcess *proc;
+
+    if (!srcpriv)
+        return 0;
+
+    proc = srcpriv->nbdkitProcess;
+
+    if (proc) {
+        if (proc->pid <= 0) {
+            if (virPidFileReadPath(proc->pidfile, &proc->pid) < 0)
+                return -1;
+        }
+
+        if (virProcessKill(proc->pid, 0) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("nbdkit process %i is not alive"), proc->pid);
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
+/**
+ * qemuNbdkitStorageSourceManageProcess:
+ * @source: a storage source
+ * @vm: the vm that owns this storage source
+ *
+ * This function re-enables monitoring of any nbdkit processes associated with the backing chain of
+ * @source. It is intended to be called after libvirt restarts and has loaded its current state from
+ * disk and is attempting to re-connect to active domains.
+ */
+int
+qemuNbdkitStorageSourceManageProcess(virStorageSource *source)
+{
+    virStorageSource *backing;
+    for (backing = source->backingStore; backing != NULL; backing = backing->backingStore) {
+        if (qemuNbdkitStorageSourceManageProcessOne(backing) < 0)
+            return -1;
+    }
+
+    return qemuNbdkitStorageSourceManageProcessOne(source);
+}
+
 
 bool
 qemuNbdkitInitStorageSource(qemuNbdkitCaps *caps,
diff --git a/src/qemu/qemu_nbdkit.h b/src/qemu/qemu_nbdkit.h
index ccd418b7d3..2be46b6002 100644
--- a/src/qemu/qemu_nbdkit.h
+++ b/src/qemu/qemu_nbdkit.h
@@ -54,6 +54,14 @@ qemuNbdkitInitStorageSource(qemuNbdkitCaps *nbdkitCaps,
                             uid_t user,
                             gid_t group);
 
+void
+qemuNbdkitReconnectStorageSource(virStorageSource *source,
+                                 const char *pidfile,
+                                 const char *socketfile);
+
+int
+qemuNbdkitStorageSourceManageProcess(virStorageSource *src);
+
 bool
 qemuNbdkitCapsGet(qemuNbdkitCaps *nbdkitCaps,
                   qemuNbdkitCapsFlags flag);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 6d1751b5d7..7ec31ef6ac 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -9047,6 +9047,17 @@ qemuProcessReconnect(void *opaque)
         }
     }
 
+    for (i = 0; i < obj->def->ndisks; i++) {
+        virDomainDiskDef *disk = obj->def->disks[i];
+        if (qemuNbdkitStorageSourceManageProcess(disk->src) < 0)
+            goto error;
+    }
+
+    if (obj->def->os.loader && obj->def->os.loader->nvram) {
+        if (qemuNbdkitStorageSourceManageProcess(obj->def->os.loader->nvram) < 0)
+            goto error;
+    }
+
     /* update domain state XML with possibly updated state in virDomainObj */
     if (virDomainObjSave(obj, driver->xmlopt, cfg->stateDir) < 0)
         goto error;
diff --git a/tests/qemustatusxml2xmldata/modern-in.xml b/tests/qemustatusxml2xmldata/modern-in.xml
index 7759034f7a..71b3eb4736 100644
--- a/tests/qemustatusxml2xmldata/modern-in.xml
+++ b/tests/qemustatusxml2xmldata/modern-in.xml
@@ -342,6 +342,10 @@
                 <TLSx509 alias='transport-alias'/>
               </objects>
               <thresholdEvent indexUsed='yes'/>
+              <nbdkit>
+                <pidfile>/path/to/nbdkit.pid</pidfile>
+                <socketfile>/path/to/nbdkit.socket</socketfile>
+              </nbdkit>
             </privateData>
           </source>
           <backingStore/>
-- 
2.39.0
Re: [libvirt PATCH v4 18/31] qemu: include nbdkit state in private xml
Posted by Peter Krempa 3 years ago
On Fri, Jan 20, 2023 at 16:03:12 -0600, Jonathon Jongsma wrote:
> Add xml to the private data for a disk source to represent the nbdkit
> process so that the state can be re-created if the libvirt daemon is
> restarted. Format:
> 
>    <nbdkit>
>      <pidfile>/path/to/nbdkit.pid</pidfile>
>      <socketfile>/path/to/nbdkit.socket</socketfile>
>    </nbdkit>
> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>  src/qemu/qemu_domain.c                    | 48 ++++++++++++++
>  src/qemu/qemu_nbdkit.c                    | 77 +++++++++++++++++++++++
>  src/qemu/qemu_nbdkit.h                    |  8 +++
>  src/qemu/qemu_process.c                   | 11 ++++
>  tests/qemustatusxml2xmldata/modern-in.xml |  4 ++
>  5 files changed, 148 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 0e3eaf49f8..28d4bddf14 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1911,6 +1911,29 @@ qemuStorageSourcePrivateDataAssignSecinfo(qemuDomainSecretInfo **secinfo,
>  }
>  
>  
> +static int
> +qemuStorageSourcePrivateDataParseNbdkit(xmlNodePtr node,
> +                                        xmlXPathContextPtr ctxt,
> +                                        virStorageSource *src)
> +{
> +    g_autofree char *pidfile = NULL;
> +    g_autofree char *socketfile = NULL;
> +    VIR_XPATH_NODE_AUTORESTORE(ctxt);
> +
> +    ctxt->node = node;
> +
> +    if (!(pidfile = virXPathString("string(./pidfile)", ctxt)))
> +        return -1;

The XPath apis are a bit misleading/broken. For the basic case, when you
have a correct XPath, but the element is missing they *don't* actually
report an error. They report errors only form programming errors.

Thus you must add error reports here.

> +
> +    if (!(socketfile = virXPathString("string(./socketfile)", ctxt)))
> +        return -1;
> +
> +    qemuNbdkitReconnectStorageSource(src, pidfile, socketfile);
> +
> +    return 0;
> +}

[...]

> diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
> index 00ca945904..2b26e9bc08 100644
> --- a/src/qemu/qemu_nbdkit.c
> +++ b/src/qemu/qemu_nbdkit.c
> @@ -627,6 +627,83 @@ qemuNbdkitProcessNew(virStorageSource *source,

[...]

> +static int
> +qemuNbdkitStorageSourceManageProcessOne(virStorageSource *source)
> +{
> +    qemuDomainStorageSourcePrivate *srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(source);
> +    qemuNbdkitProcess *proc;
> +
> +    if (!srcpriv)
> +        return 0;
> +
> +    proc = srcpriv->nbdkitProcess;
> +
> +    if (proc) {
> +        if (proc->pid <= 0) {
> +            if (virPidFileReadPath(proc->pidfile, &proc->pid) < 0)
> +                return -1;
> +        }
> +
> +        if (virProcessKill(proc->pid, 0) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("nbdkit process %i is not alive"), proc->pid);
> +            return -1;

In later patches you add code for monitoring the nbdkit process which is
actually mean to restart it, so we should not kill the VM if 'nbdkit'
died while libvirt was not looking.

This is not fixed even in 24/31 which adds the monitoring bit and thus
could restart nbdkit.

> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +/**
> + * qemuNbdkitStorageSourceManageProcess:
> + * @source: a storage source
> + * @vm: the vm that owns this storage source
> + *
> + * This function re-enables monitoring of any nbdkit processes associated with the backing chain of
> + * @source. It is intended to be called after libvirt restarts and has loaded its current state from
> + * disk and is attempting to re-connect to active domains.
> + */
> +int
> +qemuNbdkitStorageSourceManageProcess(virStorageSource *source)
> +{
> +    virStorageSource *backing;
> +    for (backing = source->backingStore; backing != NULL; backing = backing->backingStore) {

if you initialize 'backing' to 'source' instead ...

> +        if (qemuNbdkitStorageSourceManageProcessOne(backing) < 0)
> +            return -1;
> +    }
> +
> +    return qemuNbdkitStorageSourceManageProcessOne(source);

You don't have to have this extra invocation, which also makes the order
the processes are checked weird ... eg. backing of the top node only is
processed before parent, but backing of a child note is processed
after the parent

> +}

[...]

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 6d1751b5d7..7ec31ef6ac 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -9047,6 +9047,17 @@ qemuProcessReconnect(void *opaque)
>          }
>      }
>  
> +    for (i = 0; i < obj->def->ndisks; i++) {
> +        virDomainDiskDef *disk = obj->def->disks[i];
> +        if (qemuNbdkitStorageSourceManageProcess(disk->src) < 0)
> +            goto error;

This kills the VM if nbdkit is not running.

> +    }
> +
> +    if (obj->def->os.loader && obj->def->os.loader->nvram) {
> +        if (qemuNbdkitStorageSourceManageProcess(obj->def->os.loader->nvram) < 0)
> +            goto error;
> +    }
> +