Re: [libvirt] [PATCH 1/5] snapshot: Implement reverting for external disk snapshots

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

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); > > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list