[libvirt] [RFC/WIP] [PATCH 0/5] Add support for revert and delete operations to external disk snapshots
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. Regards, Povilas Povilas Kanapickas (5): snapshot: Implement reverting for external disk snapshots snapshot: Add VIR_DEBUG to qemuDomainSnapshotCreateXML() snapshot: Support deleting external disk snapshots when deleting snapshot: Extract qemuDomainSnapshotReparentChildrenMetadata() snapshot: Support reparenting external disk snapshots when deleting src/qemu/qemu_domain.c | 45 ++++- src/qemu/qemu_driver.c | 372 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 379 insertions(+), 38 deletions(-) -- 2.17.1
Signed-off-by: Povilas Kanapickas <povilas@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); -- 2.17.1
On Sun, Oct 21, 2018 at 19:38:48 +0300, Povilas Kanapickas wrote:
Signed-off-by: Povilas Kanapickas <povilas@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? */
This is necessary, but should be known since it's recorded in the overlay file.
+ 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,
There already should be a similar function named "...AlignDisks..." or something like that.
+ 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.
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.
+ - 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)
Please separate refactors from feature implementation.
{ /* 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.
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.
+ 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 */
[...]
@@ -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.
VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0);
Hi Peter, Many thanks for the reviews. I replied inline below. On 07/11/2018 17:40, Peter Krempa wrote:
On Sun, Oct 21, 2018 at 19:38:48 +0300, Povilas Kanapickas wrote: [...]
+ 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.
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
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.
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.
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
+ - 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)
Please separate refactors from feature implementation.
{ /* 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.
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.
+ 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 */
[...]
@@ -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.
VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0);
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hi Peter, On 09/11/2018 20:58, Povilas Kanapickas wrote:
Hi Peter,
Many thanks for the reviews. I replied inline below.
On 07/11/2018 17:40, Peter Krempa wrote: [...]
Just a friendly ping :-) Best regards, Povilas
Signed-off-by: Povilas Kanapickas <povilas@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; -- 2.17.1
On Sun, Oct 21, 2018 at 19:38:49 +0300, Povilas Kanapickas wrote:
Signed-off-by: Povilas Kanapickas <povilas@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);
This is logged by the public wrapper virDomainSnapshotCreateXML: VIR_DOMAIN_DEBUG(domain, "xmlDesc=%s, flags=0x%x", xmlDesc, flags);
Signed-off-by: Povilas Kanapickas <povilas@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; } } -- 2.17.1
On Sun, Oct 21, 2018 at 19:38:50 +0300, Povilas Kanapickas wrote:
Signed-off-by: Povilas Kanapickas <povilas@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.
No any external snapshot does have the state, so the condition below is dead code.
+ 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.
Signed-off-by: Povilas Kanapickas <povilas@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; } -- 2.17.1
Signed-off-by: Povilas Kanapickas <povilas@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; -- 2.17.1
On Sun, Oct 21, 2018 at 19:38:52 +0300, Povilas Kanapickas wrote:
Signed-off-by: Povilas Kanapickas <povilas@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
[....]
@@ -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.
+ 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.
As said earlier, this should not happen with external snapshots.
On 21/10/2018 19:38, Povilas Kanapickas wrote:
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
participants (2)
-
Peter Krempa -
Povilas Kanapickas