Re: [libvirt] [PATCHv4 3/4] snapshot: Add flag to allow hypervisor restart when reverting snapshots

On 11/19/2012 03:53 PM, Eric Blake wrote: One more possible problem - the existing virsh code fakes '_FORCE' even for older servers, by first trying without _FORCE, and then only adding it if it would make a difference. Should we also fake '_RESPAWN' in the same manner? Then again, if --allow-respawn fails because the server is too old, you can still use --force; so I think we're okay. Here's what I'm changing before my repost: diff --git i/src/libvirt.c w/src/libvirt.c index a8ce47b..762d829 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -18712,8 +18712,9 @@ error: * * Some snapshot operations may require a restart of the hypervisor to complete * successfuly. This is normally not allowed. To override this behavior add - * VIR_DOMAIN_SNAPSHOT_REVERT_RESPAWN to @flags. The hypervisor driver should - * re-emit the appropriate events to allow reconnect of management applications. + * VIR_DOMAIN_SNAPSHOT_REVERT_RESPAWN to @flags. The hypervisor driver will + * re-emit the appropriate events to allow reconnect of management + * applications. This flag is implied by VIR_DOMAIN_SNAPSHOT_REVERT_FORCE. * * Reverting to any snapshot discards all configuration changes made since * the last snapshot. Additionally, reverting to a snapshot from a running @@ -18735,7 +18736,8 @@ error: * such as VNC or Spice graphics) - this condition arises from active * snapshots that are provably ABI incomaptible, as well as from * inactive snapshots with a @flags request to start the domain after - * the revert. + * the revert (this latter situation is also covered via the weaker + * VIR_DOMAIN_SNAPSHOT_REVERT_RESPAWN, as of libvirt 1.0.1). * * Returns 0 if the creation is successful, -1 on error. */ diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 3de8197..b873604 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -12182,24 +12182,23 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, "yet")); goto cleanup; } - if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { - if (!snap->def->dom) { - virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, - _("snapshot '%s' lacks domain '%s' rollback info"), - snap->def->name, vm->def->name); - goto cleanup; - } - if (virDomainObjIsActive(vm) && - !(snap->def->state == VIR_DOMAIN_RUNNING - || snap->def->state == VIR_DOMAIN_PAUSED) && - (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | - VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { - virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", - _("must respawn qemu to start inactive snapshot")); - goto cleanup; - } + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE) && + !snap->def->dom) { + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, + _("snapshot '%s' lacks domain '%s' rollback info"), + snap->def->name, vm->def->name); + goto cleanup; + } + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RESPAWN) && + virDomainObjIsActive(vm) && + !(snap->def->state == VIR_DOMAIN_RUNNING + || snap->def->state == VIR_DOMAIN_PAUSED) && + (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", + _("must respawn qemu to start inactive snapshot")); + goto cleanup; } - if (vm->current_snapshot) { vm->current_snapshot->def->current = false; diff --git i/tools/virsh.pod w/tools/virsh.pod index 01b3625..cb41352 100644 --- i/tools/virsh.pod +++ w/tools/virsh.pod @@ -2845,25 +2845,18 @@ I<--running> or I<--paused> flags when reverting to a disk snapshot of a transient domain. The I<--stopped> flag cannot be used on snapshots of transient domains. -Some snapshot revert approaches may require a respawn of the hypervisor -process. This is not allowed by default. You may specify I<--allow-respawn> -to override this limit. - -There are two cases where a snapshot revert involves extra risk, which -requires the use of I<--force> to proceed. One is the case of a +There are some cases where a snapshot revert involves extra risk, and +where the revert does not succeed by default but can happen with extra +flags. The first case is any time that reverting from a running domain +would require respawning the hypervisor, rather than reusing the existing +one; this action can impact SPICE and VNC connections, and requires the +I<--allow-respawn> or I<--force> flag. Another problem is the case of a snapshot that lacks full domain information for reverting configuration (such as snapshots created prior to libvirt 0.9.5); since libvirt cannot prove that the current configuration matches what was in use at the time of the snapshot, supplying I<--force> assures libvirt that the snapshot is compatible with the current configuration -(and if it is not, the domain will likely fail to run). The other is -the case of reverting from a running domain to an active state where a -new hypervisor has to be created rather than reusing the existing -hypervisor, because it implies drawbacks such as breaking any existing -VNC or Spice connections; this condition happens with an active -snapshot that uses a provably incompatible configuration, as well as -with an inactive snapshot that is combined with the I<--start> or -I<--pause> flag. +(and if it is not, the domain will likely fail to run). =item B<snapshot-delete> I<domain> {I<snapshot> | I<--current>} [I<--metadata>] [{I<--children> | I<--children-only>}] -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org