Re: [libvirt] [RFC PATCH 1/n] snapshot: add revert-and-create branching of external snapshots

On 11/13/2012 08:26 AM, Peter Krempa wrote: Hmm, if the guest is offline and you are branching away from an external snapshot, the disk image already contains the old state. But you are right that for an online guest, it might be worth having a way to save a checkpoint just before the revert - and this is true whether the point you are going to was an internal or an external snapshot (although for first implementation, I will only allow branching from an external snapshot). In fact, this is true even if we don't add the atomic revert-and-create operation - even a plain revert can abandon runtime state that might be nice to remember. My original idea was that specifying <branch> means that <memory> is ignored on the incoming <domainsnapshot> XML; but maybe I should instead state that the <memory> controls what to do with the branch-point being abandoned: snapshot=no says discard the runtime state (what revert has always done, even without the create operation merged in), and 'snapshot=internal name=...' or 'snapshot=external file=...' says to create a new checkpoint before abandoning running state (so the revert-and-create branch operation would actually be creating two snapshots at once). Or we could just say that this is the responsibility of management apps to avoid abandoning running state - anyone wanting to return to a running domain is responsible for first pausing and saving that domain as an independent operation, and then only use the revert-and-create from an offline or paused state. In a way, it would be easier to insist that branching only ever be done from an offline state; but transient domains throw a wrench in that idea (there is never an offline state for transient domains). Another thought - maybe snapshots should remember if the domain has ever been unpaused after the snapshot was first taken. That is, a snapshot starts life clean as long as the domain remains offline or paused, but is marked dirty the first time the guest is allowed to run (or if any changes are made to the domain definition, such as a hotplug); knowing if a snapshot is dirty would determine whether an additional snapshot is even needed before leaving that branch in a revert operation. Maybe virDomainRevertToSnapshot() needs a way to do a create-and-revert mode of operation; and then virDomainSnapshotCreateXML needs a create-revert-create operation. Perhaps it is time to introduce a new API to give better control over a snapshot being created before the revert, vs. the snapshot being created after the revert; although that would not be something that can be backported to older releases (sometimes there are interesting design trade-offs for what can be done without changing the .so entry points). I'll think about that some more as I continue writing patches. But for now, since our existing revert has always discarded runtime state (and left it to management apps to do their own snapshot prior to revert if they don't want to lose anything), keeping that as the status quo doesn't hurt. Now done in a separate thread. For an online guest, if we allow the create-before-revert, then the solution is to revert to the checkpoint taken before we did the revert. But for an offline guest, we shouldn't need a magic snapshot at the end of a branch; so I still think we need a new mode to revert that says to revert to the changes beyond the external snapshot. Or use hard-links instead of copies, and let the file system do the reference counting for us :) But if we do that, _and_ we do the idea of create-revert-create for marking the old point prior to creating the new branch, then the user has to be able to specify the external memory file name of both snapshots. Lots of ideas; thankfully, I don't see a problem with adding features incrementally. Well, the phrase about 'create a new snapshot at the same time as the revert' applies to this patch, but the phrase about 'request to delete all invalidated snapshots' indeed goes better with yours. I'm okay if the documentation is slightly inaccurate as our patches converge, as long as the final documentation in 1.0.1 matches what we have together. So it may be best to just separate this into its own patch, to be applied after both our code approaches have been merged. Oops. Proof that I need to add a test case for RNG validation. Actually, if we add a create-revert-create ability, then _LIVE would apply to the first create (the branch we are leaving). But it definitely doesn't work for the branch we are creating. Indeed, also a candidate for a final doc cleanup patch after both of our improvements are in. Copy and paste, so I'll also fix the point it copied from. In fact, the names we generate for --disk-only are generated by libvirt (snapshot_conf.c:virDomainSnapshotAlignDisks), not virsh; so if we come up with a reasonable approach for auto-naming an external memory snapshot, it should be done in the same place. But yes, an ability to autoname external snapshots would be a nice independent patch. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org