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 4b5b908d66ccb0af9962721bf8735e46ce2ba806..9bf3c783531546e7564a74bcf7bdced4272cfbf7 100644 (file)
--- 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 fec4a5a912e42a3380dce267ad4ffa0d3823fd96..96c77ef42b8c3f5b8cbe99c92ea2b8c6ab810a82 100644 (file)
--- 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 06cc6ef8c03dd934cb26bc6361e718d3de22f084..189fe98299d777da9d08341ff6a5eb1a75c85e7e 100644 (file)
--- 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;