From: Pavel Hrdina <phrdina@redhat.com>
Date: Mon, 5 Dec 2022 12:03:32 +0000 (+0100)
Subject: qemu_snapshot: when deleting snapshot invalidate parent snapshot
X-Git-Tag: v9.0.0-rc1~37
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=565bcb5d79dddaf1d0d14cffe1f3ae78d76b10a0;p=thirdparty%2Flibvirt.git

qemu_snapshot: when deleting snapshot invalidate parent snapshot

When deleting external snapshots the operation may fail at any point
which could lead to situation that some disks finished the block commit
operation but for some disks it failed and the libvirt job ends.

In order to make sure that the qcow2 images are in consistent state
introduce new element "<snapshotDeleteInProgress/>" that will mark the
disk in snapshot metadata as invalid until the snapshot delete is
completed successfully.

This will prevent deleting snapshot with the invalid disk and in future
reverting to snapshot with the invalid disk.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
---

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 4b5b908d66..9bf3c78353 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -158,6 +158,11 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
         return -1;
     }
 
+    if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) {
+        def->snapshotDeleteInProgress = !!virXPathNode("./snapshotDeleteInProgress",
+                                                       ctxt);
+    }
+
     if ((cur = virXPathNode("./source", ctxt)) &&
         virDomainStorageSourceParse(cur, ctxt, src, flags, xmlopt) < 0)
         return -1;
@@ -744,6 +749,9 @@ virDomainSnapshotDiskDefFormat(virBuffer *buf,
         virBufferAsprintf(&attrBuf, " snapshot='%s'",
                           virDomainSnapshotLocationTypeToString(disk->snapshot));
 
+    if (disk->snapshotDeleteInProgress)
+        virBufferAddLit(&childBuf, "<snapshotDeleteInProgress/>\n");
+
     if (disk->src->path || disk->src->format != 0) {
         g_auto(virBuffer) driverAttrBuf = VIR_BUFFER_INITIALIZER;
         g_auto(virBuffer) driverChildBuf = VIR_BUFFER_INIT_CHILD(&childBuf);
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index fec4a5a912..96c77ef42b 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -52,6 +52,7 @@ typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef;
 struct _virDomainSnapshotDiskDef {
     char *name;     /* name matching the <target dev='...' of the domain */
     virDomainSnapshotLocation snapshot;
+    bool snapshotDeleteInProgress;
 
     /* details of wrapper external file. src is always non-NULL.
      * XXX optimize this to allow NULL for internal snapshots? */
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 06cc6ef8c0..189fe98299 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2316,6 +2316,13 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm,
         if (snapDisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO)
             continue;
 
+        if (snapDisk->snapshotDeleteInProgress) {
+            virReportError(VIR_ERR_OPERATION_INVALID,
+                           _("snapshot disk '%s' was target of not completed snapshot delete"),
+                           snapDisk->name);
+            return NULL;
+        }
+
         data = g_new0(qemuSnapshotDeleteExternalData, 1);
         data->snapDisk = snapDisk;
 
@@ -2628,6 +2635,53 @@ qemuSnapshotDeleteBlockJobFinishing(virDomainObj *vm,
 }
 
 
+/**
+ * qemuSnapshotSetInvalid:
+ * @vm: vm object
+ * @snap: snapshot object that contains parent disk
+ * @disk: disk from the snapshot we are deleting
+ * @invalid: boolean to set/unset invalid state
+ *
+ * @snap should point to a ancestor snapshot from the snapshot tree that
+ * affected the @disk which doesn't have to be the direct parent.
+ *
+ * When setting snapshot with parent disk as invalid due to snapshot being
+ * deleted we should not mark the whole snapshot as invalid but only the
+ * affected disks because the snapshot can contain other disks that we are
+ * not modifying at the moment.
+ *
+ * Return 0 on success, -1 on error.
+ * */
+static int
+qemuSnapshotSetInvalid(virDomainObj *vm,
+                       virDomainMomentObj *snap,
+                       virDomainSnapshotDiskDef *disk,
+                       bool invalid)
+{
+    ssize_t i;
+    qemuDomainObjPrivate *priv = vm->privateData;
+    virQEMUDriver *driver = priv->driver;
+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+    virDomainSnapshotDef *snapdef = NULL;
+
+    if (!snap)
+        return 0;
+
+    snapdef = virDomainSnapshotObjGetDef(snap);
+    if (!snapdef)
+        return 0;
+
+    for (i = 0; i < snapdef->ndisks; i++) {
+        virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]);
+
+        if (STREQ(snapDisk->name, disk->name))
+            snapDisk->snapshotDeleteInProgress = invalid;
+    }
+
+    return qemuDomainSnapshotWriteMetadata(vm, snap, driver->xmlopt, cfg->snapshotDir);
+}
+
+
 static int
 qemuSnapshotDiscardExternal(virDomainObj *vm,
                             GSList *externalData)
@@ -2644,6 +2698,9 @@ qemuSnapshotDiscardExternal(virDomainObj *vm,
             autofinalize = VIR_TRISTATE_BOOL_YES;
         }
 
+        if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, true) < 0)
+            goto error;
+
         data->job = qemuBlockCommit(vm,
                                     data->domDisk,
                                     data->parentDiskSrc,
@@ -2694,6 +2751,9 @@ qemuSnapshotDiscardExternal(virDomainObj *vm,
         }
 
         qemuBlockJobSyncEnd(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT);
+
+        if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, false) < 0)
+            goto error;
     }
 
     return 0;