[PATCH v4 03/11] qemu: snapshot: Introduce helpers for creating overlays on <transient/> disks

Peter Krempa posted 11 patches 5 years, 4 months ago
Only 10 patches received!
[PATCH v4 03/11] qemu: snapshot: Introduce helpers for creating overlays on <transient/> disks
Posted by Peter Krempa 5 years, 4 months ago
To implement <transient/> disks we'll need to install an overlay on top
of the original disk image which will be discarded after the VM is
turned off. This was initially implemented by qemu but libvirt never
picked up this option. With blockdev the qemu feature became unsupported
so we need to do this via the snapshot code anyways.

The helpers introduced in this patch prepare a fake snapshot disk
definition for a disk which is configured as <transient/> and use it to
create a snapshot (without actually modifying metada or persistent def).

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_snapshot.c | 91 +++++++++++++++++++++++++++++++++++++++-
 src/qemu/qemu_snapshot.h |  5 +++
 2 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index ca051071aa..d10e7b6b3a 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -986,6 +986,7 @@ qemuSnapshotDiskPrepareOne(virDomainObjPtr vm,
                            qemuSnapshotDiskDataPtr dd,
                            virHashTablePtr blockNamedNodeData,
                            bool reuse,
+                           bool updateConfig,
                            qemuDomainAsyncJob asyncJob,
                            virJSONValuePtr actions)
 {
@@ -1008,7 +1009,8 @@ qemuSnapshotDiskPrepareOne(virDomainObjPtr vm,
         return -1;

     /* modify disk in persistent definition only when the source is the same */
-    if (vm->newDef &&
+    if (updateConfig &&
+        vm->newDef &&
         (persistdisk = virDomainDiskByTarget(vm->newDef, dd->disk->dst)) &&
         virStorageSourceIsSameLocation(dd->disk->src, persistdisk->src)) {

@@ -1116,6 +1118,54 @@ qemuSnapshotDiskPrepareActiveExternal(virDomainObjPtr vm,
                                        snapctxt->dd + snapctxt->ndd++,
                                        blockNamedNodeData,
                                        reuse,
+                                       true,
+                                       asyncJob,
+                                       snapctxt->actions) < 0)
+            return NULL;
+    }
+
+    return g_steal_pointer(&snapctxt);
+}
+
+
+static qemuSnapshotDiskContextPtr
+qemuSnapshotDiskPrepareDisksTransient(virDomainObjPtr vm,
+                                      virQEMUDriverConfigPtr cfg,
+                                      virHashTablePtr blockNamedNodeData,
+                                      qemuDomainAsyncJob asyncJob)
+{
+    g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
+    size_t i;
+
+    snapctxt = qemuSnapshotDiskContextNew(vm->def->ndisks, vm, asyncJob);
+
+    for (i = 0; i < vm->def->ndisks; i++) {
+        virDomainDiskDefPtr domdisk = vm->def->disks[i];
+        g_autoptr(virDomainSnapshotDiskDef) snapdisk = g_new0(virDomainSnapshotDiskDef, 1);
+
+        if (!domdisk->transient)
+            continue;
+
+        /* validation code makes sure that we do this only for local disks
+         * with a file source */
+        snapdisk->name = g_strdup(domdisk->dst);
+        snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
+        snapdisk->src = virStorageSourceNew();
+        snapdisk->src->type = VIR_STORAGE_TYPE_FILE;
+        snapdisk->src->format = VIR_STORAGE_FILE_QCOW2;
+        snapdisk->src->path = g_strdup_printf("%s.TRANSIENT", domdisk->src->path);
+        if (virFileExists(snapdisk->src->path)) {
+            virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+                           _("Overlay file '%s' for transient disk '%s' already exists"),
+                           snapdisk->src->path, domdisk->dst);
+            return NULL;
+        }
+
+        if (qemuSnapshotDiskPrepareOne(vm, cfg, domdisk, snapdisk,
+                                       snapctxt->dd + snapctxt->ndd++,
+                                       blockNamedNodeData,
+                                       false,
+                                       false,
                                        asyncJob,
                                        snapctxt->actions) < 0)
             return NULL;
@@ -1253,6 +1303,45 @@ qemuSnapshotCreateActiveExternalDisks(virDomainObjPtr vm,
 }


+/**
+ * qemuSnapshotCreateDisksTransient:
+ * @vm: domain object
+ * @asyncJob: qemu async job type
+ *
+ * Creates overlays on top of disks which are configured as <transient/>. Note
+ * that the validation code ensures that <transient> disks have appropriate
+ * configuration.
+ */
+int
+qemuSnapshotCreateDisksTransient(virDomainObjPtr vm,
+                                 qemuDomainAsyncJob asyncJob)
+{
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+    virQEMUDriverPtr driver = priv->driver;
+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+    g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
+    g_autoptr(virHashTable) blockNamedNodeData = NULL;
+
+    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
+        if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob)))
+            return -1;
+
+        if (!(snapctxt = qemuSnapshotDiskPrepareDisksTransient(vm, cfg,
+                                                               blockNamedNodeData,
+                                                               asyncJob)))
+            return -1;
+
+        if (qemuSnapshotDiskCreate(snapctxt, cfg) < 0)
+            return -1;
+    }
+
+    /* the overlays are established, so they can be deleted on a shutdown */
+    priv->inhibitDiskTransientDelete = false;
+
+    return 0;
+}
+
+
 static int
 qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
                                  virDomainObjPtr vm,
diff --git a/src/qemu/qemu_snapshot.h b/src/qemu/qemu_snapshot.h
index 8b3ebe87b1..cf9bfec542 100644
--- a/src/qemu/qemu_snapshot.h
+++ b/src/qemu/qemu_snapshot.h
@@ -21,6 +21,7 @@
 #include "virconftypes.h"
 #include "datatypes.h"
 #include "qemu_conf.h"
+#include "qemu_domainjob.h"

 virDomainMomentObjPtr
 qemuSnapObjFromName(virDomainObjPtr vm,
@@ -53,3 +54,7 @@ int
 qemuSnapshotDelete(virDomainObjPtr vm,
                    virDomainSnapshotPtr snapshot,
                    unsigned int flags);
+
+int
+qemuSnapshotCreateDisksTransient(virDomainObjPtr vm,
+                                 qemuDomainAsyncJob asyncJob);
-- 
2.26.2

Re: [PATCH v4 03/11] qemu: snapshot: Introduce helpers for creating overlays on <transient/> disks
Posted by Masayoshi Mizuma 5 years, 4 months ago
On Thu, Sep 24, 2020 at 01:43:50PM +0200, Peter Krempa wrote:
> To implement <transient/> disks we'll need to install an overlay on top
> of the original disk image which will be discarded after the VM is
> turned off. This was initially implemented by qemu but libvirt never > picked up this option. With blockdev the qemu feature became unsupported
> so we need to do this via the snapshot code anyways.
> 
> The helpers introduced in this patch prepare a fake snapshot disk
> definition for a disk which is configured as <transient/> and use it to
> create a snapshot (without actually modifying metada or persistent def).
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_snapshot.c | 91 +++++++++++++++++++++++++++++++++++++++-
>  src/qemu/qemu_snapshot.h |  5 +++
>  2 files changed, 95 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index ca051071aa..d10e7b6b3a 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -986,6 +986,7 @@ qemuSnapshotDiskPrepareOne(virDomainObjPtr vm,
>                             qemuSnapshotDiskDataPtr dd,
>                             virHashTablePtr blockNamedNodeData,
>                             bool reuse,
> +                           bool updateConfig,
>                             qemuDomainAsyncJob asyncJob,
>                             virJSONValuePtr actions)
>  {
> @@ -1008,7 +1009,8 @@ qemuSnapshotDiskPrepareOne(virDomainObjPtr vm,
>          return -1;
> 
>      /* modify disk in persistent definition only when the source is the same */
> -    if (vm->newDef &&
> +    if (updateConfig &&
> +        vm->newDef &&
>          (persistdisk = virDomainDiskByTarget(vm->newDef, dd->disk->dst)) &&
>          virStorageSourceIsSameLocation(dd->disk->src, persistdisk->src)) {
> 
> @@ -1116,6 +1118,54 @@ qemuSnapshotDiskPrepareActiveExternal(virDomainObjPtr vm,
>                                         snapctxt->dd + snapctxt->ndd++,
>                                         blockNamedNodeData,
>                                         reuse,
> +                                       true,
> +                                       asyncJob,
> +                                       snapctxt->actions) < 0)
> +            return NULL;
> +    }
> +
> +    return g_steal_pointer(&snapctxt);
> +}
> +
> +
> +static qemuSnapshotDiskContextPtr
> +qemuSnapshotDiskPrepareDisksTransient(virDomainObjPtr vm,
> +                                      virQEMUDriverConfigPtr cfg,
> +                                      virHashTablePtr blockNamedNodeData,
> +                                      qemuDomainAsyncJob asyncJob)
> +{
> +    g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
> +    size_t i;
> +
> +    snapctxt = qemuSnapshotDiskContextNew(vm->def->ndisks, vm, asyncJob);
> +
> +    for (i = 0; i < vm->def->ndisks; i++) {
> +        virDomainDiskDefPtr domdisk = vm->def->disks[i];
> +        g_autoptr(virDomainSnapshotDiskDef) snapdisk = g_new0(virDomainSnapshotDiskDef, 1);
> +
> +        if (!domdisk->transient)
> +            continue;
> +
> +        /* validation code makes sure that we do this only for local disks
> +         * with a file source */
> +        snapdisk->name = g_strdup(domdisk->dst);
> +        snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
> +        snapdisk->src = virStorageSourceNew();
> +        snapdisk->src->type = VIR_STORAGE_TYPE_FILE;
> +        snapdisk->src->format = VIR_STORAGE_FILE_QCOW2;
> +        snapdisk->src->path = g_strdup_printf("%s.TRANSIENT", domdisk->src->path);
> +        if (virFileExists(snapdisk->src->path)) {
> +            virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +                           _("Overlay file '%s' for transient disk '%s' already exists"),
> +                           snapdisk->src->path, domdisk->dst);
> +            return NULL;
> +        }
> +
> +        if (qemuSnapshotDiskPrepareOne(vm, cfg, domdisk, snapdisk,
> +                                       snapctxt->dd + snapctxt->ndd++,
> +                                       blockNamedNodeData,
> +                                       false,
> +                                       false,
>                                         asyncJob,
>                                         snapctxt->actions) < 0)
>              return NULL;
> @@ -1253,6 +1303,45 @@ qemuSnapshotCreateActiveExternalDisks(virDomainObjPtr vm,
>  }
> 
> 
> +/**
> + * qemuSnapshotCreateDisksTransient:
> + * @vm: domain object
> + * @asyncJob: qemu async job type
> + *
> + * Creates overlays on top of disks which are configured as <transient/>. Note
> + * that the validation code ensures that <transient> disks have appropriate
> + * configuration.
> + */
> +int
> +qemuSnapshotCreateDisksTransient(virDomainObjPtr vm,
> +                                 qemuDomainAsyncJob asyncJob)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virQEMUDriverPtr driver = priv->driver;
> +    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> +    g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
> +    g_autoptr(virHashTable) blockNamedNodeData = NULL;
> +
> +    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
> +        if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob)))
> +            return -1;
> +
> +        if (!(snapctxt = qemuSnapshotDiskPrepareDisksTransient(vm, cfg,
> +                                                               blockNamedNodeData,
> +                                                               asyncJob)))
> +            return -1;
> +
> +        if (qemuSnapshotDiskCreate(snapctxt, cfg) < 0)
> +            return -1;
> +    }
> +
> +    /* the overlays are established, so they can be deleted on a shutdown */
> +    priv->inhibitDiskTransientDelete = false;
> +
> +    return 0;
> +}
> +
> +
>  static int
>  qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
>                                   virDomainObjPtr vm,
> diff --git a/src/qemu/qemu_snapshot.h b/src/qemu/qemu_snapshot.h
> index 8b3ebe87b1..cf9bfec542 100644
> --- a/src/qemu/qemu_snapshot.h
> +++ b/src/qemu/qemu_snapshot.h
> @@ -21,6 +21,7 @@
>  #include "virconftypes.h"
>  #include "datatypes.h"
>  #include "qemu_conf.h"
> +#include "qemu_domainjob.h"
> 
>  virDomainMomentObjPtr
>  qemuSnapObjFromName(virDomainObjPtr vm,
> @@ -53,3 +54,7 @@ int
>  qemuSnapshotDelete(virDomainObjPtr vm,
>                     virDomainSnapshotPtr snapshot,
>                     unsigned int flags);
> +
> +int
> +qemuSnapshotCreateDisksTransient(virDomainObjPtr vm,
> +                                 qemuDomainAsyncJob asyncJob);
> -- 

Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

Thanks a lot!
Masa

Re: [PATCH v4 03/11] qemu: snapshot: Introduce helpers for creating overlays on <transient/> disks
Posted by Eric Blake 5 years, 4 months ago
On 9/24/20 6:43 AM, Peter Krempa wrote:
> To implement <transient/> disks we'll need to install an overlay on top
> of the original disk image which will be discarded after the VM is
> turned off. This was initially implemented by qemu but libvirt never
> picked up this option. With blockdev the qemu feature became unsupported

You may want to add mention that qemu's -transient command-line option 
lets qemu create the file, which fails under various SELinux scenarios 
(as one of the reasons why libvirt never used it).

> so we need to do this via the snapshot code anyways.
> 
> The helpers introduced in this patch prepare a fake snapshot disk
> definition for a disk which is configured as <transient/> and use it to
> create a snapshot (without actually modifying metada or persistent def).

metadata

> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>   src/qemu/qemu_snapshot.c | 91 +++++++++++++++++++++++++++++++++++++++-
>   src/qemu/qemu_snapshot.h |  5 +++
>   2 files changed, 95 insertions(+), 1 deletion(-)
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v4 03/11] qemu: snapshot: Introduce helpers for creating overlays on <transient/> disks
Posted by Ján Tomko 5 years, 4 months ago
On a Thursday in 2020, Peter Krempa wrote:
>To implement <transient/> disks we'll need to install an overlay on top
>of the original disk image which will be discarded after the VM is
>turned off. This was initially implemented by qemu but libvirt never
>picked up this option. With blockdev the qemu feature became unsupported
>so we need to do this via the snapshot code anyways.
>
>The helpers introduced in this patch prepare a fake snapshot disk
>definition for a disk which is configured as <transient/> and use it to
>create a snapshot (without actually modifying metada or persistent def).
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/qemu/qemu_snapshot.c | 91 +++++++++++++++++++++++++++++++++++++++-
> src/qemu/qemu_snapshot.h |  5 +++
> 2 files changed, 95 insertions(+), 1 deletion(-)
>
>diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
>index ca051071aa..d10e7b6b3a 100644
>--- a/src/qemu/qemu_snapshot.c
>+++ b/src/qemu/qemu_snapshot.c
>@@ -986,6 +986,7 @@ qemuSnapshotDiskPrepareOne(virDomainObjPtr vm,
>                            qemuSnapshotDiskDataPtr dd,
>                            virHashTablePtr blockNamedNodeData,
>                            bool reuse,
>+                           bool updateConfig,
>                            qemuDomainAsyncJob asyncJob,
>                            virJSONValuePtr actions)
> {
>@@ -1008,7 +1009,8 @@ qemuSnapshotDiskPrepareOne(virDomainObjPtr vm,
>         return -1;
>
>     /* modify disk in persistent definition only when the source is the same */
>-    if (vm->newDef &&
>+    if (updateConfig &&
>+        vm->newDef &&
>         (persistdisk = virDomainDiskByTarget(vm->newDef, dd->disk->dst)) &&
>         virStorageSourceIsSameLocation(dd->disk->src, persistdisk->src)) {
>
>@@ -1116,6 +1118,54 @@ qemuSnapshotDiskPrepareActiveExternal(virDomainObjPtr vm,
>                                        snapctxt->dd + snapctxt->ndd++,
>                                        blockNamedNodeData,
>                                        reuse,
>+                                       true,
>+                                       asyncJob,
>+                                       snapctxt->actions) < 0)
>+            return NULL;
>+    }
>+
>+    return g_steal_pointer(&snapctxt);
>+}
>+
>+
>+static qemuSnapshotDiskContextPtr
>+qemuSnapshotDiskPrepareDisksTransient(virDomainObjPtr vm,
>+                                      virQEMUDriverConfigPtr cfg,
>+                                      virHashTablePtr blockNamedNodeData,
>+                                      qemuDomainAsyncJob asyncJob)
>+{
>+    g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
>+    size_t i;
>+
>+    snapctxt = qemuSnapshotDiskContextNew(vm->def->ndisks, vm, asyncJob);
>+
>+    for (i = 0; i < vm->def->ndisks; i++) {
>+        virDomainDiskDefPtr domdisk = vm->def->disks[i];
>+        g_autoptr(virDomainSnapshotDiskDef) snapdisk = g_new0(virDomainSnapshotDiskDef, 1);
>+
>+        if (!domdisk->transient)
>+            continue;
>+
>+        /* validation code makes sure that we do this only for local disks
>+         * with a file source */
>+        snapdisk->name = g_strdup(domdisk->dst);
>+        snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
>+        snapdisk->src = virStorageSourceNew();
>+        snapdisk->src->type = VIR_STORAGE_TYPE_FILE;
>+        snapdisk->src->format = VIR_STORAGE_FILE_QCOW2;
>+        snapdisk->src->path = g_strdup_printf("%s.TRANSIENT", domdisk->src->path);

I'd prefer:
a) an empty line between these assignments and the if()
b) a lowercase name, but I guess standing out is important here as the
file is not meant to stay.

>+        if (virFileExists(snapdisk->src->path)) {
>+            virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>+                           _("Overlay file '%s' for transient disk '%s' already exists"),
>+                           snapdisk->src->path, domdisk->dst);
>+            return NULL;
>+        }
>+
>+        if (qemuSnapshotDiskPrepareOne(vm, cfg, domdisk, snapdisk,
>+                                       snapctxt->dd + snapctxt->ndd++,
>+                                       blockNamedNodeData,
>+                                       false,
>+                                       false,
>                                        asyncJob,
>                                        snapctxt->actions) < 0)
>             return NULL;
>@@ -1253,6 +1303,45 @@ qemuSnapshotCreateActiveExternalDisks(virDomainObjPtr vm,
> }
>
>
>+/**
>+ * qemuSnapshotCreateDisksTransient:
>+ * @vm: domain object
>+ * @asyncJob: qemu async job type
>+ *
>+ * Creates overlays on top of disks which are configured as <transient/>. Note
>+ * that the validation code ensures that <transient> disks have appropriate
>+ * configuration.
>+ */
>+int
>+qemuSnapshotCreateDisksTransient(virDomainObjPtr vm,
>+                                 qemuDomainAsyncJob asyncJob)
>+{
>+    qemuDomainObjPrivatePtr priv = vm->privateData;
>+    virQEMUDriverPtr driver = priv->driver;
>+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>+    g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
>+    g_autoptr(virHashTable) blockNamedNodeData = NULL;
>+
>+    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
>+        if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob)))
>+            return -1;
>+
>+        if (!(snapctxt = qemuSnapshotDiskPrepareDisksTransient(vm, cfg,
>+                                                               blockNamedNodeData,
>+                                                               asyncJob)))
>+            return -1;
>+
>+        if (qemuSnapshotDiskCreate(snapctxt, cfg) < 0)
>+            return -1;
>+    }
>+
>+    /* the overlays are established, so they can be deleted on a shutdown */

*on shutdown
sounds better to me

>+    priv->inhibitDiskTransientDelete = false;
>+
>+    return 0;
>+}
>+
>+
> static int
> qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
>                                  virDomainObjPtr vm,

With the empty line added:
Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano