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