Signed-off-by: Povilas Kanapickas <povilas(a)radix.lt&gt; --- 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); -- 2.17.1

Hi Peter, Many thanks for the reviews. I replied inline below. On 07/11/2018 17:40, Peter Krempa wrote: 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. 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. 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

Signed-off-by: Povilas Kanapickas <povilas(a)radix.lt&gt; --- 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; -- 2.17.1

Signed-off-by: Povilas Kanapickas <povilas(a)radix.lt&gt; --- 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; } } -- 2.17.1

Signed-off-by: Povilas Kanapickas <povilas(a)radix.lt&gt; --- 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; } -- 2.17.1

On Sun, Oct 21, 2018 at 19:38:52 +0300, Povilas Kanapickas wrote: [....] 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. As said earlier, this should not happen with external snapshots.