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@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list