On 11/21/12 01:37, Eric Blake wrote:
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)
With this flag, the description of the API function is starting to drift
away from its original "revertTo" semantic. This flag is useful in the
meaning it has but I'm not quite sure if I like the implications it
would have:
1) You cannot do this with a checkpoint unless you wish to revert
without the memory state.
2) Even if you shut down the machine correctly you will need to remember
that and revert a checkpoint to the offline state with this flag.
This is the original reason I was talking about the automatic
meta-snapshots that could be used to revert to a current state of a
branch that was left. When you want to leave a branch in live state you
have to create a checkpoint in any case so this flag would be usable
just for disk snapshots.
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)
and the original meaning of the "revertTo" semantic of this API
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)
... and also invalidates checkpoints.
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.
This might be useful, but is basicaly syntax sugar (when snapshot
deleting will be in place).
[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 agree, this should be part of the revert operation.
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.
Right now, the only way this API can be used is if the snapshot is the
last in the chain or if you specify the FORCE flag, so with the given
state of libvirt right now you lose only the changes that are not a part
of a checkpoint/snapshot if you revert somewhere _or_ you specified
--force and then you sign of that you want to do anything.
As of re-creating the disk images, I tried the approach manually and
qemu-img doesn't care much if the file exists and overwrites it.
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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list