Re: [libvirt] [RFC]: Snapshot API v3

On 03/30/2010 04:40 PM, Jiri Denemark wrote: > Hi. > > ... >> /* NOTE: struct _virDomainSnapshot is a private structure, ala >>  * struct _virDomain. >>  */ >> typedef struct _virDomainSnapshot virDomainSnapshot; >> >> /* Take a snapshot of the current VM state.  Throws an error if >>  * the VM is not currently running */ >> virDomainSnapshotPtr virDomainSnapshotCreateXML(virDomainPtr domain, >>                                                 const char *xmlDesc, >>                                                 unsigned int flags); > > This is probably a leftover from previous versions, but... why do we restrict > this API only for running VMs? Oops, yeah, you are right, I just forgot to change the comment.  It now says: /* Take a snapshot of the current VM state. */ > > ... >> /* Delete a snapshot - with no flags, the snapshot is not used anymore, >>  * but also not removed.  With a MERGE flag, it merges the snapshot into >>  * the parent snapshot (or the base image, if there is no parent snapshot). >>  * Note that if other snapshots would be discarded because of this >>  * MERGE action, this operation will fail.  If that is really what is intended, >>  * use MERGE_FORCE. >>  * >>  * With a DISCARD flag, it deletes the snapshot.  Note that if children snapshots >>  * would be discarded because of this delete action, this operation will >>  * fail.  If this is really what is intended, use DISCARD_FORCE. >>  * >>  * MERGE, MERGE_FORCE, DISCARD, and DISCARD_FORCE are mutually-exclusive. >>  * >>  * Note that this operation can happen when the domain is running or shut >>  * down, though this is hypervisor specific */ >> typedef enum { >>     VIR_DOMAIN_SNAPSHOT_DELETE_MERGE, >>     VIR_DOMAIN_SNAPSHOT_DELETE_MERGE_FORCE, >>     VIR_DOMAIN_SNAPSHOT_DELETE_DISCARD, >>     VIR_DOMAIN_SNAPSHOT_DELETE_DISCARD_FORCE, >> } virDomainSnapshotDelete; > > Merging a snapshot into its parent is probably not the best semantics for > MERGE flag as hypervisors differ in the way merging is implemented. As you Yeah, you are right, we can't just declare this.  It's also an open question to me what qemu does about merging (though bugs with loadvm/delvm are preventing me from testing this at the moment).

> also mention below, VirtualBox merges into all children instead of a parent. > We should allow for both cases. However it influences several things. Firstly, > it makes MERGE_FORCE unnecessary for child merging, which is not a big deal as > it can just be treated in the same way as MERGE. Secondly, it makes a huge > difference when deleting a snapshot with no child. In one case it results in > changes being merged and in other case it results changes begin dropped. > > One option is to refine the semantics to something like: > > - MERGE: merge changes into other snapshot(s) and fail if it would require any >   snapshot to be discarded (even the one which was supposed to be merged) > - MERGE_FORCE: really merge even discarding other snapshots but fail if the >   snapshot itself would actually be discarded > - DISCARD: discard the snapshot and fail if other snapshots would be discarded > - DISCARD_FORCE: discard, no matter what The problem with declaring these semantics is that they are somewhat confusing, so application developers will probably get them wrong.  On the other hand it does allow us to declare a semantic that all 3 hypervisors probably can support, unlike the options below. > > Another option would be to introduce several different APIs for merging into > children, merging into parent, and discarding. That would allow drivers to > implement only supported methods. Even all of them for a very flexible > hypervisor. The problem with this one is that it will be difficult for application writers to write a single application that handles all of the hypervisors.  Imagine trying to write a GUI around this, and you'll see what I mean.  If we were to add 3 new API's like this, we would also probably want to add some data to the capabilities XML for the particular hypervisors so you could grey out specific options in a GUI. On the other hand, it does solve the problem with merging parents vs. children, and also diffuses Paolo's concern about the "SnapshotDelete" API sometimes deleting data and sometimes modifying the base image.

> > And the third option I see would be distinguishing merge direction using new > flags. This one is like the second option, in that you don't know which particular directions a particular hypervisor supports.  You'd still need to add capabilities XML for each hypervisor. I have to say that after thinking about these 3 options, I like the first option the best.  While it's slightly confusing, it is a good semantic.  I'll update the documentation for this. > > Personally, I like the second option best as it provides the easiest way for > application to detect unsupported behavior. > > ... >> Possible issues: >> 1)  I don't see a way to support "managed" save/restore and snapshotting with >> this API.  I think we'll have to have a separate API for managed save/restore. >> 2)  What is the semantic for deleting snapshots from a running domain? >> Virtualbox seems to not allow you to manipulate snapshots while the domain is >> running.  Qemu does allow this, but it's currently unclear what the exact >> semantics are.  VMware seems to allow manipulation of snapshots while the >> domain is running. >> 3)  Do we need a snapshot UUID?  Virtualbox allows you to have multiple snapshots >> with the same name, differentiated by UUID.  Confusingly, they also have a >> "FindByName" method that returns the first depth-first search snapshot that matches >> a given name.  For qemu, if you specify the same name twice it overwrites the previous >> one with the new one.  I don't know what ESX does here. > > Libvirt uses/generates UUIDs for almost everything (networks, vms, ...) so it > might be more consistent to have UUID in snapshot as well. Yeah, that is true, which is why I'm waffling with it.  In general it seems like superflous information, except in the one case of virtualbox duplicate names (ESX doesn't allow duplicate names, I don't think, and qemu blows away duplicates).  My inclination is to declare the semantics of duplicate names to be undefined, since it doesn't seem to be a very useful feature.