[libvirt] [RFC]: Snapshot API

[Paolo Bonzini]

> > > /* Return the number of snapshots for this domain */
> > > int virDomainSnapshotNum(virDomainPtr domain, unsigned int flags);
> >
> > Shouldn't this be called virDomainNumOfSnapshots to be consistent with
> > the naming or other num-of functions?
>
> Gah.  I struggled with this; I like the fact that all of the rest of them
> start with virDomainSnapshot; this one doesn't fit the mold.  I don't care
> too much either way.

virDomain*Snapshots for APIs that take a virDomainPtr seems consistent too.

> > > /* Deactivate 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 base image.  With a DISCARD flag, it deletes the snapshot.  MERGE
> > >   * and DISCARD are mutually-exclusive.  Note that this operation can
> > >   * generally only happen when the domain is shut down, though this is
> > >   * hypervisor-specific */
> > > typedef enum {
> > >     VIR_DOMAIN_SNAPSHOT_DEACTIVATE_MERGE,
> > >     VIR_DOMAIN_SNAPSHOT_DEACTIVATE_DISCARD,
> > > } virDomainSnapshotDeactivate;
> > > int virDomainSnapshotDeactivate(virDomainSnapshotPtr snapshot,
> > >                                 unsigned int flags);
> >
> > I'm not sure if virDomainSnapshotDeactivate is a good name.
>
> I agree it's not a great name.  I didn't like Dan's original
> proposal of "virDomainSnapshotDelete", though, since it doesn't
> exactly seem to fit the situation either.  Any more suggestions for
> a name?

Several things are not clear about this API to me too. :-)

1) Would discarding also discard all the snapshots depending on the 
discarded one.  If so, what about a --force flag that you need to pass 
if the delete operation would delete other snapshots recursively?

2) Likewise, merging would invalidate all the other snapshots depending 
on the base image (and discard them).  The same --force option then 
could apply as well.

However, I think there is a fundamental difference between the two 
cases; only one of them modifies the base image I'm not sure that 
something that modifies the base image rightfully belongs into the 
virDomainSnapshot* namespace.  So my proposal would be

    /* Start using the base image again.  If snapshot != NULL, merge
       the given snapshot in the base image again.  */
    virDomainDisableSnapshots(virDomainPtr domain,
                              virDomainSnapshotPtr snapshot, int flags);

   /* Discard the given snapshot and, if --force is given, all that
      depend on it.  Fail if it is active.  */
    virDomainSnapshotDiscard(virDomainSnapshotPtr snapshot, int flags);

> > Why would I deactivate a snapshot, but not merge/discard it? What's
> > the use case for this?
>
> The use case I was thinking about mostly involved debugging (and comes
> from my experience debugging qemu guests).  Say you took a snapshot of
> a guest, and after running for a while discovered some sort of bug in
> the guest software.  You might then keep the memory image of that snapshot
> around so that you could run "crash" or "gdb" against it to extract some data.
> It's kind of an esoteric use-case, I'll admit, but I have found it
> somewhat useful in the past.

If your guest crashes in production (not just a kernel crash, even an 
application crash) and you want to restart it from the previous existing 
snapshot, you probably still want to keep the other snapshot around so 
that you can copy it to another machine, start it there, and do 
post-mortem debugging.

In this case, however, you would likely activate another snapshot rather 
than deactivating the one that crashed.  See below for another 
suggestion about how to handle this.

> > > int virDomainSnapshotFree(virDomainSnapshotPtr snapshot);
> > >
> > > NOTE: During snapshot creation, *none* of the fields are required.  That is,
> > > you can call virDomainSnapshotCreateXML() with an XML of "<domainsnapshot/>".

Maybe it's worthwhile having a simple virDomainSnapshotCreate() API for 
this?  (Or at least making the xml argument optional in virsh).

> > How can<parent>  be settable? If I have snapshots A and B
> >
> >    A ->  B ->  current state
> >
> > and I create a new snapshot C, then B will be the parent of C.
> >
> >    A ->  B ->  C ->  current state
> >
> > If I create another snapshot D now and specify A to be its parent,
> > what's supposed to happen then?
>
> You are right, that doesn't make that much sense.  I have to admit that
> the tree structure is the part I thought about least, so I'll take that
> part back. <parent> is just going to be an informational field about
> which snapshot was current (if any) when this one was created.

If discarding a snapshot also discards the children, it would definitely 
make sense to be able to specify the parent.

If you do not want to allow setting the parent, you can also add a flag 
--inactive to virsh snapshot-create that would create the snapshot 
without making it active.  Then you would make <parent> an informational 
field about which snapshot was *active* when the new one was created.

In the end the virsh commands would be

   virsh snapshot-create <dom> [<xmlfile>] [--compress] [--inactive]
   virsh snapshot-list <dom>
   virsh snapshot-dumpxml <dom> <name>
   virsh snapshot-activate <dom> <snapshotname>
   virsh snapshot-discard <dom> <snapshotname> [--force]
   virsh snapshot-disable <dom> [<snapshotname>]

What do you think?

Paolo