6:37 p.m.
On 11/19/2012 04:51 PM, Eric Blake wrote:
On 11/19/2012 09:06 AM, Peter Krempa wrote:
> This patch adds support for reverting of external snapshots. The support
> is somewhat limited yet (you can only revert to a snapshot that has no
> children or delete the children that would have their image chains
> invalidated).
>
> While reverting an external snapshot, the domain has to be taken offline
> as there's no possibility to start a migration from file on a running
> machine. This poses a few difficulties when the user has virt-viewer
> attached as appropriate events need to be re-emitted even if the machine
> doesn't change states.
> ---
> Adapt for the revert flag and a few minor fixes.
> + /* wipe and re-create disk images - qemu-img doesn't
care if it exists*/
> + if (qemuDomainSnapshotCreateInactiveExternal(driver, vm, snap, false) < 0)
> + goto endjob;
This comment says you are throwing away state, but without the use of
any flag guarding things that the user meant to throw away state. I'm a
bit worried that we're missing a flag here to state that we are
reverting to the point of the external snapshot _and_ want to throw away
all changes that happened in the external file.
Also, does qemuDomainSnapshotCreateInactiveExternal(,,,false) actually
do what you want here? I thought that invocation fails if the file
already exists (and passing true assumes that the file already exists,
but does not otherwise touch it). Do we instead need to change that
boolean into an 'int', with multiple values (0 and 1 for when the user
first creates the snapshot, depending on whether they passed the
REUSE_EXT flag, and 2 for this function, which says to forcefully recreate)?
After thinking a bit more, I think we need the following additional
flags to virDomainRevertToSnapshot, and require that exactly one of
these three mutually exclusive flags is present when reverting to an
external snapshot:
VIR_DOMAIN_SNAPSHOT_REVERT_FOLLOW - revert to the current state of disks
after an external snapshot (I'm about to post work on this flag - yes,
it will conflict with what you've done so far)
VIR_DOMAIN_SNAPSHOT_REVERT_RESET - revert to the point where an external
snapshot was taken, and reset the external files to be exactly that
state again (the semantics of _this_ patch)
VIR_DOMAIN_SNAPSHOT_REVERT_COMMIT - revert to the current state of disks
after an external snapshot, but commit the state of the external files
into the backing files. Not possible if there is another snapshot
branched off the same backing file (unless we want to let _FORCE allow
us to potentially invalidate those other branches)
I can also see the use for a revert-and-delete operation rolled into
one, via a new flag:
VIR_DOMAIN_SNAPSHOT_REVERT_DISCARD - after reverting to a snapshot,
delete it (that is, we no longer plan to revert to it again in the
future). When mixed with FOLLOW, we keep the backing chain with no
change to disk contents; when mixed with RESET, we keep the backing
chain but reset the backing chain to start fresh; and when mixed with
COMMIT, we shorten the backing chain back to its pre-snapshot length.
[I was debating whether the combination delete-and-revert fits better as
part of the revert operation, via the REVERT_DISCARD flag, or whether it
fits better as part of the delete operation, via a new flag there; but
if we make it part of delete, then delete has to learn whether the
domain should be running, paused, or stopped after an associated revert,
whereas revert already has that logic.]
I'm still the most worried about the concept of whether we are blindly
discarding user disk state since the snapshot was created, and whether
we are properly recreating external files if that's what the user really
wanted. I'm not sure whether to say ACK and then fix the fallout, or to
wait a bit longer to see what else you come up with in the series. I
guess we still have a few more development days before the 1.0.1 freeze
to decide, so for now, I'd like to see what you have in store for
snapshot deletion, and to also get my patches posted for snapshot
revert-and-create, before I decide on this one.
I think I could ACK this patch if you respun it to require my proposed
RESET flag to match your intent of discarding user disk state. Also,
before you decide too much, you'd better read up on my respin with my
proposed FOLLOW flag.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org