[libvirt] [PATCHv4 4/4] snapshot: qemu: Implement reverting of external snapshots

Peter Krempa pkrempa at redhat.com
Wed Nov 21 14:16:52 UTC 2012


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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>




More information about the libvir-list mailing list