[libvirt] [PATCH 2/5] snapshot: Add VIR_DEBUG to qemuDomainSnapshotCreateXML()

[Povilas Kanapickas]

Signed-off-by: Povilas Kanapickas <***@radix.lt>
---
src/qemu/qemu_driver.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 279e5d93aa..eb104d306b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15298,6 +15298,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
const char *xmlDesc,
unsigned int flags)
{
+ VIR_DEBUG("domain=%p, flags=0x%x, xmlDesc=%s", domain, flags, xmlDesc);
+
virQEMUDriverPtr driver = domain->conn->privateData;
virDomainObjPtr vm = NULL;
char *xml = NULL;

[Povilas Kanapickas]

Signed-off-by: Povilas Kanapickas <***@radix.lt>
---
src/qemu/qemu_driver.c | 246 +++++++++++++++++++++++++++++++++++++----
1 file changed, 224 insertions(+), 22 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a52e2495d5..279e5d93aa 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15952,19 +15952,194 @@ qemuDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot,
return ret;
}

+static int
+qemuDomainSnapshotRevertExternalSingleDisk(virQEMUDriverPtr driver,
+ virDomainDiskDefPtr revert_disk,
+ virDomainDiskDefPtr backing_disk)
+{
+ virCommandPtr cmd = NULL;
+ const char *qemuImgPath;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ int ret = -1;
+
+ if (!(qemuImgPath = qemuFindQemuImgBinary(driver)))
+ goto cleanup;
+
+ if (unlink(revert_disk->src->path) < 0)
+ VIR_WARN("Failed to overwrite current image for snapshot '%s'",
+ revert_disk->src->path);
+
+ /* TODO: maybe figure out the format from the backing_disk? */
+ revert_disk->src->format = VIR_STORAGE_FILE_QCOW2;
+ /* FIXME: what about revert_disk->src->backingStore ? */
+
+ /* creates cmd line args: qemu-img create -f qcow2 -o */
+ if (!(cmd = virCommandNewArgList(qemuImgPath,
+ "create",
+ "-f",
+ virStorageFileFormatTypeToString(revert_disk->src->format),
+ "-o",
+ NULL)))
+ goto cleanup;
+
+ /* adds cmd line arg: backing_file=/path/to/backing/file,backing_fmt=format */
+ virCommandAddArgFormat(cmd, "backing_file=%s,backing_fmt=%s",
+ backing_disk->src->path,
+ virStorageFileFormatTypeToString(backing_disk->src->format));
+
+ /* adds cmd line args: /path/to/target/file */
+ virCommandAddArg(cmd, revert_disk->src->path);
+
+ if (virCommandRun(cmd, NULL) < 0)
+ goto cleanup;
+
+ virCommandFree(cmd);
+ cmd = NULL;
+ ret = 0;
+
+ cleanup:
+ virCommandFree(cmd);
+ virObjectUnref(cfg);
+
+ return ret;
+}
+
+static void
+qemuComputeSnapshotDiskToDomainDiskMapping(virDomainSnapshotDefPtr snap_def,
+ virDomainDefPtr dom_def,
+ int* out_mapping)
+{
+ size_t i, j;
+ int found_idx;
+ virDomainSnapshotDiskDefPtr snap_disk;
+
+ for (i = 0; i < snap_def->ndisks; ++i) {
+ snap_disk = &(snap_def->disks[i]);
+ if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ continue;
+
+ found_idx = -1;
+ for (j = 0; j < dom_def->ndisks; ++j) {
+ if (STREQ(dom_def->disks[j]->dst, snap_disk->name)) {
+ found_idx = j;
+ break;
+ }
+ }
+ out_mapping[i] = found_idx;
+ }
+}
+
+/* This function reverts an external snapshot disk state. With external disks
+ we can't just revert to the disks listed in the domain stored within the
+ snapshot, because it's read-only and might be used by other VMs in different
+ backing chains. Since the contents of the current disks will be discarded
+ in favor of data in the snapshot, we reuse them by resetting them and
+ pointing the backing image link to the disks listed within the snapshot.
+
+ The domain is expected to be inactive.
+
+ new_def is the new domain definition that will be stored to vm sometime in
+ the future.
+ */
+static int
+qemuDomainSnapshotRevertInactiveExternal(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainSnapshotObjPtr snap,
+ virDomainDefPtr new_def)
+{
+ /* We have the following disks recorded in the snapshot and the current
+ domain definition:
+
+ - the current disk state before the revert (vm->def->disks). We'll
+ discard and reuse them.
+ - the lists of disks that were snapshotted (snap->def->disks). We'll
+ use this information to figure out which disks to actually revert.
+ - the original disk state stored in the snapshot
+ (snap->def->dom->disks). We'll point reverted disks to use these
+ disks as backing images.
+
+ FIXME: what to do with disks that weren't included in all snapshots
+ within the hierrachy?
+ */
+ size_t i;
+ int* snap_disk_to_vm_def_disk_idx_map = NULL;
+ int* snap_disk_to_new_def_disk_idx_map = NULL;
+ int ret = -1;
+ virDomainSnapshotDiskDefPtr snap_disk;
+ virDomainDiskDefPtr backing_disk;
+ virDomainDiskDefPtr revert_disk;
+ virDomainDiskDefPtr new_disk;
+
+ if (VIR_ALLOC_N(snap_disk_to_vm_def_disk_idx_map, snap->def->ndisks) < 0)
+ goto cleanup;
+ if (VIR_ALLOC_N(snap_disk_to_new_def_disk_idx_map, snap->def->ndisks) < 0)
+ goto cleanup;
+
+ /* Figure out the matching disks from the current VM state. */
+ qemuComputeSnapshotDiskToDomainDiskMapping(snap->def, vm->def,
+ snap_disk_to_vm_def_disk_idx_map);
+ qemuComputeSnapshotDiskToDomainDiskMapping(snap->def, new_def,
+ snap_disk_to_new_def_disk_idx_map);
+
+ for (i = 0; i < snap->def->ndisks; ++i) {
+ snap_disk = &(snap->def->disks[i]);
+ if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ continue;
+ if (snap_disk_to_vm_def_disk_idx_map[i] < 0 ||
+ snap_disk_to_new_def_disk_idx_map[i] < 0) {
+ // FIXME: we could create additional disks, but for now it's not
+ // supported.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("all disks in the snapshots must have "
+ "equivalents in the current VM state."));
+ goto cleanup;
+ }
+ }
+
+ /* Discard old disk contents and point them to the backing disks */
+ for (i = 0; i < snap->def->ndisks; ++i) {
+ snap_disk = &(snap->def->disks[i]);
+
+ // Note that at the moment we don't support mixing internal and
+ // external snapshot modes for different disks, but skip non-external
+ // disks just in case.
+ if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ continue;
+
+ backing_disk = snap->def->dom->disks[snap_disk->idx];
+ revert_disk = vm->def->disks[snap_disk_to_vm_def_disk_idx_map[i]];
+ if (qemuDomainSnapshotRevertExternalSingleDisk(driver, revert_disk,
+ backing_disk) < 0) {
+ goto cleanup;
+ }
+
+ /* FIXME: figure out which data exactly needs copying.
+ */
+ new_disk = new_def->disks[snap_disk_to_new_def_disk_idx_map[i]];
+ new_disk->src->format = revert_disk->src->format;
+ if (VIR_STRDUP(new_disk->src->path, revert_disk->src->path) < 0) {
+ goto cleanup;
+ }
+ }
+ ret = 0;
+
+cleanup:
+ VIR_FREE(snap_disk_to_vm_def_disk_idx_map);
+ VIR_FREE(snap_disk_to_new_def_disk_idx_map);
+ return ret;
+}

/* The domain is expected to be locked and inactive. */
static int
-qemuDomainSnapshotRevertInactive(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainSnapshotObjPtr snap)
+qemuDomainSnapshotRevertInactiveInternal(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainSnapshotObjPtr snap)
{
/* Try all disks, but report failure if we skipped any. */
int ret = qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-a", true);
return ret > 0 ? -1 : ret;
}

-
static int
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
unsigned int flags)
@@ -15981,6 +16156,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virDomainDefPtr config = NULL;
virQEMUDriverConfigPtr cfg = NULL;
virCapsPtr caps = NULL;
+ bool is_external = false;
bool was_stopped = false;
qemuDomainSaveCookiePtr cookie;
virCPUDefPtr origCPU = NULL;
@@ -16043,11 +16219,13 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto endjob;
}

- if (virDomainSnapshotIsExternal(snap)) {
+ if (snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("revert to external snapshot not supported yet"));
+ _("revert to external snapshot with memory state is "
+ "not supported yet"));
goto endjob;
}
+ is_external = virDomainSnapshotIsExternal(snap);

if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) {
if (!snap->def->dom) {
@@ -16090,6 +16268,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
driver->xmlopt, NULL, true);
if (!config)
goto endjob;
+ } else if (is_external) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("revert to a an external snapshot without the VM "
+ "definition recorded is not supported."));
+ goto endjob;
}

cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;
@@ -16110,8 +16293,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
/* Transitions 5, 6, 8, 9 */
/* Check for ABI compatibility. We need to do this check against
* the migratable XML or it will always fail otherwise */
+ bool compatible = true;
if (config) {
- bool compatible;

/* Replace the CPU in config and put the original one in priv
* once we're done. When we have the updated CPU def in the
@@ -16152,22 +16335,27 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto endjob;
}
virResetError(err);
- qemuProcessStop(driver, vm,
- VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
- QEMU_ASYNC_JOB_START, 0);
- virDomainAuditStop(vm, "from-snapshot");
- detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
- event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_STOPPED,
- detail);
- virObjectEventStateQueue(driver->domainEventState, event);
- /* Start after stop won't be an async start job, so
- * reset to none */
- jobType = QEMU_ASYNC_JOB_NONE;
- goto load;
}
}

+ if (!compatible || is_external) {
+ // If the snapshot is external we completely stop QEMU, adjust
+ // the disk state and restart it.
+ qemuProcessStop(driver, vm,
+ VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
+ QEMU_ASYNC_JOB_START, 0);
+ virDomainAuditStop(vm, "from-snapshot");
+ detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
+ event = virDomainEventLifecycleNewFromObj(vm,
+ VIR_DOMAIN_EVENT_STOPPED,
+ detail);
+ virObjectEventStateQueue(driver->domainEventState, event);
+ /* Start after stop won't be an async start job, so
+ * reset to none */
+ jobType = QEMU_ASYNC_JOB_NONE;
+ goto load;
+ }
+
priv = vm->privateData;
if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
/* Transitions 5, 6 */
@@ -16208,7 +16396,13 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
} else {
/* Transitions 2, 3 */
load:
+
was_stopped = true;
+
+ if (is_external &&
+ qemuDomainSnapshotRevertInactiveExternal(driver, vm, snap, config) < 0)
+ goto cleanup;
+
if (config)
virDomainObjAssignDef(vm, config, false, NULL);

@@ -16221,7 +16415,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,

rc = qemuProcessStart(snapshot->domain->conn, driver, vm,
cookie ? cookie->cpu : NULL,
- jobType, NULL, -1, NULL, snap,
+ jobType, NULL, -1, NULL,
+ is_external ? NULL : snap,
VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
start_flags);
virDomainAuditStart(vm, "from-snapshot", rc >= 0);
@@ -16291,11 +16486,18 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
detail);
}

- if (qemuDomainSnapshotRevertInactive(driver, vm, snap) < 0) {
+ if (is_external) {
+ rc = qemuDomainSnapshotRevertInactiveExternal(driver, vm, snap, config);
+ } else {
+ rc = qemuDomainSnapshotRevertInactiveInternal(driver, vm, snap);
+ }
+
+ if (rc < 0) {
qemuDomainRemoveInactive(driver, vm);
qemuProcessEndJob(driver, vm);
goto cleanup;
}
+
if (config)
virDomainObjAssignDef(vm, config, false, NULL);

[Peter Krempa]

Post by Povilas Kanapickas
---
src/qemu/qemu_driver.c | 246 +++++++++++++++++++++++++++++++++++++----
1 file changed, 224 insertions(+), 22 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a52e2495d5..279e5d93aa 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15952,19 +15952,194 @@ qemuDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot,
return ret;
}
+static int
+qemuDomainSnapshotRevertExternalSingleDisk(virQEMUDriverPtr driver,
+ virDomainDiskDefPtr revert_disk,
+ virDomainDiskDefPtr backing_disk)
+{
+ virCommandPtr cmd = NULL;
+ const char *qemuImgPath;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ int ret = -1;
+
+ if (!(qemuImgPath = qemuFindQemuImgBinary(driver)))
+ goto cleanup;
+
+ if (unlink(revert_disk->src->path) < 0)
+ VIR_WARN("Failed to overwrite current image for snapshot '%s'",
+ revert_disk->src->path);
+
+ /* TODO: maybe figure out the format from the backing_disk? */

This is necessary, but should be known since it's recorded in the
overlay file.

Post by Povilas Kanapickas
+ revert_disk->src->format = VIR_STORAGE_FILE_QCOW2;
+ /* FIXME: what about revert_disk->src->backingStore ? */
+
+ /* creates cmd line args: qemu-img create -f qcow2 -o */
+ if (!(cmd = virCommandNewArgList(qemuImgPath,
+ "create",
+ "-f",
+ virStorageFileFormatTypeToString(revert_disk->src->format),
+ "-o",
+ NULL)))
+ goto cleanup;
+
+ /* adds cmd line arg: backing_file=/path/to/backing/file,backing_fmt=format */
+ virCommandAddArgFormat(cmd, "backing_file=%s,backing_fmt=%s",
+ backing_disk->src->path,
+ virStorageFileFormatTypeToString(backing_disk->src->format));
+
+ /* adds cmd line args: /path/to/target/file */
+ virCommandAddArg(cmd, revert_disk->src->path);
+
+ if (virCommandRun(cmd, NULL) < 0)
+ goto cleanup;
+
+ virCommandFree(cmd);
+ cmd = NULL;
+ ret = 0;
+
+ virCommandFree(cmd);
+ virObjectUnref(cfg);
+
+ return ret;
+}
+
+static void
+qemuComputeSnapshotDiskToDomainDiskMapping(virDomainSnapshotDefPtr snap_def,

There already should be a similar function named "...AlignDisks..." or
something like that.

Post by Povilas Kanapickas
+ virDomainDefPtr dom_def,
+ int* out_mapping)
+{
+ size_t i, j;
+ int found_idx;
+ virDomainSnapshotDiskDefPtr snap_disk;
+
+ for (i = 0; i < snap_def->ndisks; ++i) {
+ snap_disk = &(snap_def->disks[i]);
+ if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ continue;
+
+ found_idx = -1;
+ for (j = 0; j < dom_def->ndisks; ++j) {
+ if (STREQ(dom_def->disks[j]->dst, snap_disk->name)) {
+ found_idx = j;
+ break;
+ }
+ }
+ out_mapping[i] = found_idx;
+ }
+}
+
+/* This function reverts an external snapshot disk state. With external disks
+ we can't just revert to the disks listed in the domain stored within the
+ snapshot, because it's read-only and might be used by other VMs in different
+ backing chains. Since the contents of the current disks will be discarded
+ in favor of data in the snapshot, we reuse them by resetting them and
+ pointing the backing image link to the disks listed within the snapshot.
+
+ The domain is expected to be inactive.
+
+ new_def is the new domain definition that will be stored to vm sometime in
+ the future.
+ */
+static int
+qemuDomainSnapshotRevertInactiveExternal(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainSnapshotObjPtr snap,
+ virDomainDefPtr new_def)
+{
+ /* We have the following disks recorded in the snapshot and the current
+
+ - the current disk state before the revert (vm->def->disks). We'll
+ discard and reuse them.

So this has multiple problems:

1) You can have a chain of snapshots and revert in middle of that:
- this will destroy any snapshots which depend on the one you are
reverting to and that is not acceptable

2) The snapshot can have a different mapping of disks. When reverting
the old topology should be kept as it existed at the time when the
snapshot was created.

3) you can't return to the state that you are leaving (might be
desirable but needs to be opt-in)

This basically means that we need a new reversion API which will allow
to take an XML and will basically fork the snapshot into an alternate
history and also will mark the state you are leaving so that we can
return to that state.

Reverting to that "new" state will result into just swithcing the
definitions and continuing to use the images and make changes. (Which
should be done for any leaf snapshot in the snapshot tree).

Deleting the state as you do is IMO not acceptable.

Post by Povilas Kanapickas
+ - the lists of disks that were snapshotted (snap->def->disks). We'll
+ use this information to figure out which disks to actually revert.
+ - the original disk state stored in the snapshot
+ (snap->def->dom->disks). We'll point reverted disks to use these
+ disks as backing images.
+
+ FIXME: what to do with disks that weren't included in all snapshots
+ within the hierrachy?
+ */
+ size_t i;
+ int* snap_disk_to_vm_def_disk_idx_map = NULL;
+ int* snap_disk_to_new_def_disk_idx_map = NULL;
+ int ret = -1;
+ virDomainSnapshotDiskDefPtr snap_disk;
+ virDomainDiskDefPtr backing_disk;
+ virDomainDiskDefPtr revert_disk;
+ virDomainDiskDefPtr new_disk;
+
+ if (VIR_ALLOC_N(snap_disk_to_vm_def_disk_idx_map, snap->def->ndisks) < 0)
+ goto cleanup;
+ if (VIR_ALLOC_N(snap_disk_to_new_def_disk_idx_map, snap->def->ndisks) < 0)
+ goto cleanup;
+
+ /* Figure out the matching disks from the current VM state. */
+ qemuComputeSnapshotDiskToDomainDiskMapping(snap->def, vm->def,
+ snap_disk_to_vm_def_disk_idx_map);
+ qemuComputeSnapshotDiskToDomainDiskMapping(snap->def, new_def,
+ snap_disk_to_new_def_disk_idx_map);
+
+ for (i = 0; i < snap->def->ndisks; ++i) {
+ snap_disk = &(snap->def->disks[i]);
+ if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ continue;
+ if (snap_disk_to_vm_def_disk_idx_map[i] < 0 ||
+ snap_disk_to_new_def_disk_idx_map[i] < 0) {
+ // FIXME: we could create additional disks, but for now it's not
+ // supported.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("all disks in the snapshots must have "
+ "equivalents in the current VM state."));
+ goto cleanup;
+ }
+ }
+
+ /* Discard old disk contents and point them to the backing disks */
+ for (i = 0; i < snap->def->ndisks; ++i) {
+ snap_disk = &(snap->def->disks[i]);
+
+ // Note that at the moment we don't support mixing internal and
+ // external snapshot modes for different disks, but skip non-external
+ // disks just in case.
+ if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ continue;
+
+ backing_disk = snap->def->dom->disks[snap_disk->idx];
+ revert_disk = vm->def->disks[snap_disk_to_vm_def_disk_idx_map[i]];
+ if (qemuDomainSnapshotRevertExternalSingleDisk(driver, revert_disk,
+ backing_disk) < 0) {
+ goto cleanup;
+ }
+
+ /* FIXME: figure out which data exactly needs copying.
+ */
+ new_disk = new_def->disks[snap_disk_to_new_def_disk_idx_map[i]];
+ new_disk->src->format = revert_disk->src->format;
+ if (VIR_STRDUP(new_disk->src->path, revert_disk->src->path) < 0) {
+ goto cleanup;
+ }
+ }
+ ret = 0;
+
+ VIR_FREE(snap_disk_to_vm_def_disk_idx_map);
+ VIR_FREE(snap_disk_to_new_def_disk_idx_map);
+ return ret;
+}
/* The domain is expected to be locked and inactive. */
static int
-qemuDomainSnapshotRevertInactive(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainSnapshotObjPtr snap)
+qemuDomainSnapshotRevertInactiveInternal(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainSnapshotObjPtr snap)

Please separate refactors from feature implementation.

Post by Povilas Kanapickas
{
/* Try all disks, but report failure if we skipped any. */
int ret = qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-a", true);
return ret > 0 ? -1 : ret;
}
-
static int
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
unsigned int flags)
@@ -15981,6 +16156,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virDomainDefPtr config = NULL;
virQEMUDriverConfigPtr cfg = NULL;
virCapsPtr caps = NULL;
+ bool is_external = false;
bool was_stopped = false;
qemuDomainSaveCookiePtr cookie;
virCPUDefPtr origCPU = NULL;
@@ -16043,11 +16219,13 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto endjob;
}
- if (virDomainSnapshotIsExternal(snap)) {
+ if (snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("revert to external snapshot not supported yet"));
+ _("revert to external snapshot with memory state is "
+ "not supported yet"));

Fair enough in RFC state, but kind of useless otherwise.

Post by Povilas Kanapickas
goto endjob;
}
+ is_external = virDomainSnapshotIsExternal(snap);
if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) {
if (!snap->def->dom) {
@@ -16090,6 +16268,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
driver->xmlopt, NULL, true);
if (!config)
goto endjob;
+ } else if (is_external) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("revert to a an external snapshot without the VM "
+ "definition recorded is not supported."));

This can't happen. External snapshot without definition can't exist
since libvirt already tracked definitions when external snapshots were
added.

Post by Povilas Kanapickas
+ goto endjob;
}
cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;
@@ -16110,8 +16293,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
/* Transitions 5, 6, 8, 9 */
/* Check for ABI compatibility. We need to do this check against
* the migratable XML or it will always fail otherwise */
+ bool compatible = true;
if (config) {
- bool compatible;
/* Replace the CPU in config and put the original one in priv
* once we're done. When we have the updated CPU def in the
@@ -16152,22 +16335,27 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto endjob;
}
virResetError(err);
- qemuProcessStop(driver, vm,
- VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
- QEMU_ASYNC_JOB_START, 0);
- virDomainAuditStop(vm, "from-snapshot");
- detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
- event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_STOPPED,
- detail);
- virObjectEventStateQueue(driver->domainEventState, event);
- /* Start after stop won't be an async start job, so
- * reset to none */
- jobType = QEMU_ASYNC_JOB_NONE;
- goto load;
}
}
+ if (!compatible || is_external) {
+ // If the snapshot is external we completely stop QEMU, adjust
+ // the disk state and restart it.
+ qemuProcessStop(driver, vm,
+ VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
+ QEMU_ASYNC_JOB_START, 0);
+ virDomainAuditStop(vm, "from-snapshot");
+ detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
+ event = virDomainEventLifecycleNewFromObj(vm,
+ VIR_DOMAIN_EVENT_STOPPED,
+ detail);
+ virObjectEventStateQueue(driver->domainEventState, event);
+ /* Start after stop won't be an async start job, so
+ * reset to none */
+ jobType = QEMU_ASYNC_JOB_NONE;
+ goto load;
+ }
+
priv = vm->privateData;
if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
/* Transitions 5, 6 */

[...]

Post by Povilas Kanapickas
@@ -16221,7 +16415,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
rc = qemuProcessStart(snapshot->domain->conn, driver, vm,
cookie ? cookie->cpu : NULL,
- jobType, NULL, -1, NULL, snap,
+ jobType, NULL, -1, NULL,
+ is_external ? NULL : snap,

I suppose this misses support for active snapshots with memory, where
also the memory state needs to be reverted.

Post by Povilas Kanapickas
VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
start_flags);
virDomainAuditStart(vm, "from-snapshot", rc >= 0);

[Povilas Kanapickas]

Hi Peter,

Many thanks for the reviews. I replied inline below.

Post by Peter Krempa
[...]

Post by Povilas Kanapickas
+ int* out_mapping)
+{
+ size_t i, j;
+ int found_idx;
+ virDomainSnapshotDiskDefPtr snap_disk;
+
+ for (i = 0; i < snap_def->ndisks; ++i) {
+ snap_disk = &(snap_def->disks[i]);
+ if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ continue;
+
+ found_idx = -1;
+ for (j = 0; j < dom_def->ndisks; ++j) {
+ if (STREQ(dom_def->disks[j]->dst, snap_disk->name)) {
+ found_idx = j;
+ break;
+ }
+ }
+ out_mapping[i] = found_idx;
+ }
+}
+
+/* This function reverts an external snapshot disk state. With external disks
+ we can't just revert to the disks listed in the domain stored within the
+ snapshot, because it's read-only and might be used by other VMs in different
+ backing chains. Since the contents of the current disks will be discarded
+ in favor of data in the snapshot, we reuse them by resetting them and
+ pointing the backing image link to the disks listed within the snapshot.
+
+ The domain is expected to be inactive.
+
+ new_def is the new domain definition that will be stored to vm sometime in
+ the future.
+ */
+static int
+qemuDomainSnapshotRevertInactiveExternal(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainSnapshotObjPtr snap,
+ virDomainDefPtr new_def)
+{
+ /* We have the following disks recorded in the snapshot and the current
+
+ - the current disk state before the revert (vm->def->disks). We'll
+ discard and reuse them.

- this will destroy any snapshots which depend on the one you are
reverting to and that is not acceptable

The intention was to drop just the most current disk, the one that does
not have a snapshot yet. Then we can create another file and point its
backing image to the disk storing the snapshot that we want to revert
to. As far as I understand, this does not affect any later snapshots in
the chain.

Post by Peter Krempa
2) The snapshot can have a different mapping of disks. When reverting
the old topology should be kept as it existed at the time when the
snapshot was created.

Agreed. But what would be the best outcome in situations when a snapshot
does not include a certain disk? My thinking is that if user does not
include a disk into a snapshot then he doesn't care about the change of
its contents since the last snapshot. Thus we could iterate the parent
snapshots until we find one that includes that disk and then revert the
state to that disk. If neither the snapshot or its parents include that
disk then we could probably leave the current disk state unchanged.

Post by Peter Krempa
3) you can't return to the state that you are leaving (might be
desirable but needs to be opt-in)
This basically means that we need a new reversion API which will allow
to take an XML and will basically fork the snapshot into an alternate
history and also will mark the state you are leaving so that we can
return to that state.
Reverting to that "new" state will result into just swithcing the
definitions and continuing to use the images and make changes. (Which
should be done for any leaf snapshot in the snapshot tree).
Deleting the state as you do is IMO not acceptable.

I think we could simplify that a lot if we accepted that revert is by
definition a lossy operation and any changes since last snapshot would
be lost.

Otherwise storing the previous state would just complicate things:
consider the situation where we are repeatedly reverting back and forth
between two snapshots. Either we will need to save all states between
the reverts and implement ability to store potentially more than one
previous state as a child of any snapshot. Or we will need to accept
that at some point data loss is acceptable and, say, limit the number of
state children of each snapshot to one.

I suggest that if we accepted snapshot operation as lossy, then your use
case could be implemented by a pair of <take snapshot> and <lossy revert
to other snapshot> operations. Bonus would be that such snapshots
pointing to previous states could be manipulated by existing APIs, so
tools won't need to be updated and so on.

The main problem I see currently is that user needs to somehow specify
new paths for all disks in the snapshot he wants to revert to. All paths
listed in the snapshot currently may be part of other snapshot chains.
I've written a bit about this issue and proposed solutions in this
email:
https://www.redhat.com/archives/libvir-list/2018-October/msg01110.html .
Have you looked into it by chance?

Thank you a lot for your time!

Regards,
Povilas

Post by Peter Krempa

Post by Povilas Kanapickas
+ - the lists of disks that were snapshotted (snap->def->disks). We'll
+ use this information to figure out which disks to actually revert.
+ - the original disk state stored in the snapshot
+ (snap->def->dom->disks). We'll point reverted disks to use these
+ disks as backing images.
+
+ FIXME: what to do with disks that weren't included in all snapshots
+ within the hierrachy?
+ */
+ size_t i;
+ int* snap_disk_to_vm_def_disk_idx_map = NULL;
+ int* snap_disk_to_new_def_disk_idx_map = NULL;
+ int ret = -1;
+ virDomainSnapshotDiskDefPtr snap_disk;
+ virDomainDiskDefPtr backing_disk;
+ virDomainDiskDefPtr revert_disk;
+ virDomainDiskDefPtr new_disk;
+
+ if (VIR_ALLOC_N(snap_disk_to_vm_def_disk_idx_map, snap->def->ndisks) < 0)
+ goto cleanup;
+ if (VIR_ALLOC_N(snap_disk_to_new_def_disk_idx_map, snap->def->ndisks) < 0)
+ goto cleanup;
+
+ /* Figure out the matching disks from the current VM state. */
+ qemuComputeSnapshotDiskToDomainDiskMapping(snap->def, vm->def,
+ snap_disk_to_vm_def_disk_idx_map);
+ qemuComputeSnapshotDiskToDomainDiskMapping(snap->def, new_def,
+ snap_disk_to_new_def_disk_idx_map);
+
+ for (i = 0; i < snap->def->ndisks; ++i) {
+ snap_disk = &(snap->def->disks[i]);
+ if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ continue;
+ if (snap_disk_to_vm_def_disk_idx_map[i] < 0 ||
+ snap_disk_to_new_def_disk_idx_map[i] < 0) {
+ // FIXME: we could create additional disks, but for now it's not
+ // supported.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("all disks in the snapshots must have "
+ "equivalents in the current VM state."));
+ goto cleanup;
+ }
+ }
+
+ /* Discard old disk contents and point them to the backing disks */
+ for (i = 0; i < snap->def->ndisks; ++i) {
+ snap_disk = &(snap->def->disks[i]);
+
+ // Note that at the moment we don't support mixing internal and
+ // external snapshot modes for different disks, but skip non-external
+ // disks just in case.
+ if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ continue;
+
+ backing_disk = snap->def->dom->disks[snap_disk->idx];
+ revert_disk = vm->def->disks[snap_disk_to_vm_def_disk_idx_map[i]];
+ if (qemuDomainSnapshotRevertExternalSingleDisk(driver, revert_disk,
+ backing_disk) < 0) {
+ goto cleanup;
+ }
+
+ /* FIXME: figure out which data exactly needs copying.
+ */
+ new_disk = new_def->disks[snap_disk_to_new_def_disk_idx_map[i]];
+ new_disk->src->format = revert_disk->src->format;
+ if (VIR_STRDUP(new_disk->src->path, revert_disk->src->path) < 0) {
+ goto cleanup;
+ }
+ }
+ ret = 0;
+
+ VIR_FREE(snap_disk_to_vm_def_disk_idx_map);
+ VIR_FREE(snap_disk_to_new_def_disk_idx_map);
+ return ret;
+}
/* The domain is expected to be locked and inactive. */
static int
-qemuDomainSnapshotRevertInactive(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainSnapshotObjPtr snap)
+qemuDomainSnapshotRevertInactiveInternal(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainSnapshotObjPtr snap)

Please separate refactors from feature implementation.

Post by Povilas Kanapickas
{
/* Try all disks, but report failure if we skipped any. */
int ret = qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-a", true);
return ret > 0 ? -1 : ret;
}
-
static int
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
unsigned int flags)
@@ -15981,6 +16156,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virDomainDefPtr config = NULL;
virQEMUDriverConfigPtr cfg = NULL;
virCapsPtr caps = NULL;
+ bool is_external = false;
bool was_stopped = false;
qemuDomainSaveCookiePtr cookie;
virCPUDefPtr origCPU = NULL;
@@ -16043,11 +16219,13 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto endjob;
}
- if (virDomainSnapshotIsExternal(snap)) {
+ if (snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("revert to external snapshot not supported yet"));
+ _("revert to external snapshot with memory state is "
+ "not supported yet"));

Fair enough in RFC state, but kind of useless otherwise.

Post by Povilas Kanapickas
goto endjob;
}
+ is_external = virDomainSnapshotIsExternal(snap);
if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) {
if (!snap->def->dom) {
@@ -16090,6 +16268,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
driver->xmlopt, NULL, true);
if (!config)
goto endjob;
+ } else if (is_external) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("revert to a an external snapshot without the VM "
+ "definition recorded is not supported."));

This can't happen. External snapshot without definition can't exist
since libvirt already tracked definitions when external snapshots were
added.

Post by Povilas Kanapickas
+ goto endjob;
}
cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;
@@ -16110,8 +16293,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
/* Transitions 5, 6, 8, 9 */
/* Check for ABI compatibility. We need to do this check against
* the migratable XML or it will always fail otherwise */
+ bool compatible = true;
if (config) {
- bool compatible;
/* Replace the CPU in config and put the original one in priv
* once we're done. When we have the updated CPU def in the
@@ -16152,22 +16335,27 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto endjob;
}
virResetError(err);
- qemuProcessStop(driver, vm,
- VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
- QEMU_ASYNC_JOB_START, 0);
- virDomainAuditStop(vm, "from-snapshot");
- detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
- event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_STOPPED,
- detail);
- virObjectEventStateQueue(driver->domainEventState, event);
- /* Start after stop won't be an async start job, so
- * reset to none */
- jobType = QEMU_ASYNC_JOB_NONE;
- goto load;
}
}
+ if (!compatible || is_external) {
+ // If the snapshot is external we completely stop QEMU, adjust
+ // the disk state and restart it.
+ qemuProcessStop(driver, vm,
+ VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
+ QEMU_ASYNC_JOB_START, 0);
+ virDomainAuditStop(vm, "from-snapshot");
+ detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
+ event = virDomainEventLifecycleNewFromObj(vm,
+ VIR_DOMAIN_EVENT_STOPPED,
+ detail);
+ virObjectEventStateQueue(driver->domainEventState, event);
+ /* Start after stop won't be an async start job, so
+ * reset to none */
+ jobType = QEMU_ASYNC_JOB_NONE;
+ goto load;
+ }
+
priv = vm->privateData;
if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
/* Transitions 5, 6 */

[...]

Post by Povilas Kanapickas
@@ -16221,7 +16415,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
rc = qemuProcessStart(snapshot->domain->conn, driver, vm,
cookie ? cookie->cpu : NULL,
- jobType, NULL, -1, NULL, snap,
+ jobType, NULL, -1, NULL,
+ is_external ? NULL : snap,

I suppose this misses support for active snapshots with memory, where
also the memory state needs to be reverted.

Post by Povilas Kanapickas
VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
start_flags);
virDomainAuditStart(vm, "from-snapshot", rc >= 0);
--
libvir-list mailing list
https://www.redhat.com/mailman/listinfo/libvir-list

[Povilas Kanapickas]

Hi Peter,

Post by Povilas Kanapickas
Hi Peter,
Many thanks for the reviews. I replied inline below.
[...]

Just a friendly ping :-)

Best regards,
Povilas

[Povilas Kanapickas]

Signed-off-by: Povilas Kanapickas <***@radix.lt>
---
src/qemu/qemu_domain.c | 45 ++++++++++++++++++++++++++++++++++++++----
src/qemu/qemu_driver.c | 7 ++++---
2 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ba3fff607a..73c41788f8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8244,6 +8244,38 @@ qemuDomainSnapshotForEachQcow2(virQEMUDriverPtr driver,
op, try_all, def->ndisks);
}

+static int
+qemuDomainSnapshotDiscardExternal(virDomainSnapshotObjPtr snap)
+{
+ int i;
+ virDomainSnapshotDiskDefPtr snap_disk;
+
+ // FIXME: libvirt stores the VM definition and the list of disks that
+ // snapshot has been taken of since 0.9.5. Before that we must assume that
+ // all disks from within the VM definition have been snapshotted and that
+ // they all were internal disks. Did libvirt support external snapshots
+ // before 0.9.5? If so, if we ever end up in this code path we may incur
+ // data loss as we'll delete whole QEMU file thinking it's an external
+ // snapshot.
+ if (!snap->def->dom) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("Snapshots created using libvirt 9.5 and containing "
+ "external disk snapshots cannot be safely "
+ "discarded."));
+ return -1;
+ }
+
+ for (i = 0; i < snap->def->ndisks; ++i) {
+ snap_disk = &(snap->def->disks[i]);
+ if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ continue;
+ if (unlink(snap_disk->src->path) < 0)
+ VIR_WARN("Failed to unlink snapshot disk '%s'",
+ snap_disk->src->path);
+ }
+ return 0;
+}
+
/* Discard one snapshot (or its metadata), without reparenting any children. */
int
qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
@@ -8260,10 +8292,15 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,

if (!metadata_only) {
if (!virDomainObjIsActive(vm)) {
- /* Ignore any skipped disks */
- if (qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-d",
- true) < 0)
- goto cleanup;
+ if (virDomainSnapshotIsExternal(snap)) {
+ if (qemuDomainSnapshotDiscardExternal(snap) < 0)
+ goto cleanup;
+ } else {
+ /* Ignore any skipped disks */
+ if (qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-d",
+ true) < 0)
+ goto cleanup;
+ }
} else {
priv = vm->privateData;
qemuDomainObjEnterMonitor(driver, vm);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index eb104d306b..7cd44d6957 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16662,7 +16662,7 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot)))
goto endjob;

- if (!metadata_only) {
+ if (!metadata_only && virDomainObjIsActive(vm)) {
if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) &&
virDomainSnapshotIsExternal(snap))
external++;
@@ -16673,8 +16673,9 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
&external);
if (external) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("deletion of %d external disk snapshots not "
- "supported yet"), external);
+ _("deletion of %d external disk snapshots while the "
+ "domain is active is not supported yet"),
+ external);
goto endjob;
}
}

[Peter Krempa]

Post by Povilas Kanapickas
---
src/qemu/qemu_domain.c | 45 ++++++++++++++++++++++++++++++++++++++----
src/qemu/qemu_driver.c | 7 ++++---
2 files changed, 45 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ba3fff607a..73c41788f8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8244,6 +8244,38 @@ qemuDomainSnapshotForEachQcow2(virQEMUDriverPtr driver,
op, try_all, def->ndisks);
}
+static int
+qemuDomainSnapshotDiscardExternal(virDomainSnapshotObjPtr snap)
+{
+ int i;
+ virDomainSnapshotDiskDefPtr snap_disk;
+
+ // FIXME: libvirt stores the VM definition and the list of disks that
+ // snapshot has been taken of since 0.9.5. Before that we must assume that
+ // all disks from within the VM definition have been snapshotted and that
+ // they all were internal disks. Did libvirt support external snapshots
+ // before 0.9.5? If so, if we ever end up in this code path we may incur
+ // data loss as we'll delete whole QEMU file thinking it's an external
+ // snapshot.

No any external snapshot does have the state, so the condition below is
dead code.

Post by Povilas Kanapickas
+ if (!snap->def->dom) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("Snapshots created using libvirt 9.5 and containing "
+ "external disk snapshots cannot be safely "
+ "discarded."));
+ return -1;
+ }
+
+ for (i = 0; i < snap->def->ndisks; ++i) {
+ snap_disk = &(snap->def->disks[i]);
+ if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ continue;
+ if (unlink(snap_disk->src->path) < 0)
+ VIR_WARN("Failed to unlink snapshot disk '%s'",
+ snap_disk->src->path);

So this will ever only work properly for leaf snapshots. If you try to
delete a snapshot which has children you'll destroy any of it's
children. Since they are not deleted here we can't do it without that
since reverting to them would be broken.

Ideally we want to merge the changes to all relevant snapshots so that
the intermediate files and snapshot metadata can be deleted without data
loss even in the middle of the chain/tree.

Also one other note, snapshots are possible on networked storage where
unlink will not work, so this needs to be made more general.

[Povilas Kanapickas]

Signed-off-by: Povilas Kanapickas <***@radix.lt>
---
src/qemu/qemu_driver.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7cd44d6957..227ec1c6d9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16600,6 +16600,23 @@ struct _virQEMUSnapReparent {
};


+static int
+qemuDomainSnapshotReparentChildrenMetadata(virDomainSnapshotObjPtr snap,
+ virQEMUSnapReparentPtr rep)
+{
+ VIR_FREE(snap->def->parent);
+ snap->parent = rep->parent;
+
+ if (rep->parent->def &&
+ VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) {
+ return -1;
+ }
+
+ return qemuDomainSnapshotWriteMetadata(rep->vm, snap,
+ rep->caps, rep->xmlopt,
+ rep->cfg->snapshotDir);
+}
+
static int
qemuDomainSnapshotReparentChildren(void *payload,
const void *name ATTRIBUTE_UNUSED,
@@ -16611,21 +16628,14 @@ qemuDomainSnapshotReparentChildren(void *payload,
if (rep->err < 0)
return 0;

- VIR_FREE(snap->def->parent);
- snap->parent = rep->parent;
-
- if (rep->parent->def &&
- VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) {
+ if (qemuDomainSnapshotReparentChildrenMetadata(snap, rep) < 0) {
rep->err = -1;
- return 0;
+ goto cleanup;
}

+cleanup:
if (!snap->sibling)
rep->last = snap;
-
- rep->err = qemuDomainSnapshotWriteMetadata(rep->vm, snap,
- rep->caps, rep->xmlopt,
- rep->cfg->snapshotDir);
return 0;
}

[Povilas Kanapickas]

Signed-off-by: Povilas Kanapickas <***@radix.lt>
---
src/qemu/qemu_driver.c | 89 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 89 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 227ec1c6d9..a3ffc19122 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16590,6 +16590,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
typedef struct _virQEMUSnapReparent virQEMUSnapReparent;
typedef virQEMUSnapReparent *virQEMUSnapReparentPtr;
struct _virQEMUSnapReparent {
+ virQEMUDriverPtr driver;
virQEMUDriverConfigPtr cfg;
virDomainSnapshotObjPtr parent;
virDomainObjPtr vm;
@@ -16617,6 +16618,88 @@ qemuDomainSnapshotReparentChildrenMetadata(virDomainSnapshotObjPtr snap,
rep->cfg->snapshotDir);
}

+static int
+qemuDomainSnapshotReparentDiskExternal(virQEMUDriverPtr driver,
+ virDomainSnapshotObjPtr parent_snap,
+ virDomainSnapshotDiskDefPtr disk)
+{
+ const char* qemu_img_path = NULL;
+ virCommandPtr cmd = NULL;
+ int i;
+ int ret = -1;
+ const char* parent_disk_path = NULL;
+
+ // Find the path to the disk we should use as the base when reparenting
+ // FIXME: what if there's no parent snapshot? i.e. we need to reparent on
+ // "empty" disk?
+ for (i = 0; i < parent_snap->def->dom->ndisks; ++i) {
+ if (STREQ(parent_snap->def->dom->disks[i]->dst, disk->name)) {
+ parent_disk_path = parent_snap->def->dom->disks[i]->src->path;
+ break;
+ }
+ }
+
+ if (parent_disk_path == NULL) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not find disk to reparent '%s' to"),
+ disk->src->path);
+ goto cleanup;
+ }
+
+ if (!(qemu_img_path = qemuFindQemuImgBinary(driver))) {
+ goto cleanup;
+ }
+
+ if (!(cmd = virCommandNewArgList(qemu_img_path,
+ "rebase", "-b", parent_disk_path,
+ disk->src->path,
+ NULL))) {
+ goto cleanup;
+ }
+
+ if (virCommandRun(cmd, NULL) < 0)
+ goto cleanup;
+
+ ret = 0;
+cleanup:
+ virCommandFree(cmd);
+ return ret;
+}
+
+static int
+qemuDomainSnapshotReparentDisks(virDomainSnapshotObjPtr snap,
+ virQEMUSnapReparentPtr rep)
+{
+ int i;
+ virDomainSnapshotDiskDefPtr snap_disk;
+
+ if (!snap->def->dom) {
+ VIR_WARN("Any external disk snapshots in a snapshot created with "
+ "pre-0.9.5 libvirt will not be correctly reparented.");
+ // In snapshots created with pre-0.9.5 libvirt we don't have information
+ // needed to correctly reparent external disk snapshots but we also
+ // don't even know whether external disk snapshots were used so that
+ // we could report this as an error. We can only emit a warning in that
+ // case.
+ return 0;
+ }
+
+ for (i = 0; i < snap->def->ndisks; ++i) {
+ snap_disk = &(snap->def->disks[i]);
+
+ // FIXME: do we need to explicitly reparent internal snapshots to
+ // e.g. prevent long non-visible snapshot chains that might affect
+ // performance?
+ if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ continue;
+
+ if (qemuDomainSnapshotReparentDiskExternal(rep->driver, rep->parent,
+ snap_disk) < 0)
+ return -1;
+ }
+ return 0;
+}
+
static int
qemuDomainSnapshotReparentChildren(void *payload,
const void *name ATTRIBUTE_UNUSED,
@@ -16628,6 +16711,11 @@ qemuDomainSnapshotReparentChildren(void *payload,
if (rep->err < 0)
return 0;

+ if (qemuDomainSnapshotReparentDisks(snap, rep) < 0) {
+ rep->err = -1;
+ goto cleanup;
+ }
+
if (qemuDomainSnapshotReparentChildrenMetadata(snap, rep) < 0) {
rep->err = -1;
goto cleanup;
@@ -16718,6 +16806,7 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
vm->current_snapshot = snap;
}
} else if (snap->nchildren) {
+ rep.driver = driver;
rep.cfg = cfg;
rep.parent = snap->parent;
rep.vm = vm;

[Peter Krempa]

Post by Povilas Kanapickas
---
src/qemu/qemu_driver.c | 89 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 89 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 227ec1c6d9..a3ffc19122 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c

[....]

Post by Povilas Kanapickas
@@ -16617,6 +16618,88 @@ qemuDomainSnapshotReparentChildrenMetadata(virDomainSnapshotObjPtr snap,
rep->cfg->snapshotDir);
}
+static int
+qemuDomainSnapshotReparentDiskExternal(virQEMUDriverPtr driver,
+ virDomainSnapshotObjPtr parent_snap,
+ virDomainSnapshotDiskDefPtr disk)
+{
+ const char* qemu_img_path = NULL;
+ virCommandPtr cmd = NULL;
+ int i;
+ int ret = -1;
+ const char* parent_disk_path = NULL;
+
+ // Find the path to the disk we should use as the base when reparenting
+ // FIXME: what if there's no parent snapshot? i.e. we need to reparent on
+ // "empty" disk?
+ for (i = 0; i < parent_snap->def->dom->ndisks; ++i) {
+ if (STREQ(parent_snap->def->dom->disks[i]->dst, disk->name)) {
+ parent_disk_path = parent_snap->def->dom->disks[i]->src->path;
+ break;
+ }
+ }
+
+ if (parent_disk_path == NULL) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not find disk to reparent '%s' to"),
+ disk->src->path);
+ goto cleanup;
+ }
+
+ if (!(qemu_img_path = qemuFindQemuImgBinary(driver))) {
+ goto cleanup;
+ }
+
+ if (!(cmd = virCommandNewArgList(qemu_img_path,
+ "rebase", "-b", parent_disk_path,
+ disk->src->path,
+ NULL))) {

Okay, this solves the problems that I've mentioned previously. There
still might be problems with matching which images actually get deleted
if the definition changed.

Also note that it needs to support network disks.

This code also should be added together with the deletion code as they
are tightly related.

Post by Povilas Kanapickas
+ goto cleanup;
+ }
+
+ if (virCommandRun(cmd, NULL) < 0)
+ goto cleanup;
+
+ ret = 0;
+ virCommandFree(cmd);
+ return ret;
+}
+
+static int
+qemuDomainSnapshotReparentDisks(virDomainSnapshotObjPtr snap,
+ virQEMUSnapReparentPtr rep)
+{
+ int i;
+ virDomainSnapshotDiskDefPtr snap_disk;
+
+ if (!snap->def->dom) {
+ VIR_WARN("Any external disk snapshots in a snapshot created with "
+ "pre-0.9.5 libvirt will not be correctly reparented.");
+ // In snapshots created with pre-0.9.5 libvirt we don't have information
+ // needed to correctly reparent external disk snapshots but we also
+ // don't even know whether external disk snapshots were used so that
+ // we could report this as an error. We can only emit a warning in that
+ // case.

As said earlier, this should not happen with external snapshots.

[Povilas Kanapickas]

Hey all,
Currently libvirt only supports creation of external disk snapshots, but not
reversion and deletion which are essential for any serious use of this feature.
I've looked into implementing removal and reversion of external disk snapshots
and came up with some prototype code that works with my simple test VMs (see
attached patches).
I'd like to discuss about how these features could be implemented properly. As
I've never significantly contributed to libvirt yet, I wanted to delay the
discussion until I understand the problem space myself so that the discussion
could be productive.
My current approach is relatively simple. For snapshot deletion we either
simply remove the disk or use `qemu-img rebase` to reparent a snapshot on top
of the parent of the snapshot that is being deleted. For reversion we delete
the current overlay disk and create another that uses the image of the
snapshot we want to revert to as the backing disk.
Are the attached patches good in principle? Are there any major blockers aside
from lack of tests, code formatting, bugs and so on? Are there any design
issues which prevent a simple implementation of external disk snapshot
support that I didn't see?
If there aren't significant blockers, my plan would be to continue work on the
feature until I have something that could actually be reviewed and possibly
merged.

Friendly ping :-)

Regards,
Povilas

[Peter Krempa]

Post by Povilas Kanapickas
---
src/qemu/qemu_driver.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 279e5d93aa..eb104d306b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15298,6 +15298,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
const char *xmlDesc,
unsigned int flags)
{
+ VIR_DEBUG("domain=%p, flags=0x%x, xmlDesc=%s", domain, flags, xmlDesc);

This is logged by the public wrapper virDomainSnapshotCreateXML:

VIR_DOMAIN_DEBUG(domain, "xmlDesc=%s, flags=0x%x", xmlDesc, flags);