[libvirt] [RFC]: Snapshot API

[Daniel P. Berrange]

On Wed, Mar 24, 2010 at 09:52:51AM +0100, Paolo Bonzini wrote:
> >> > /* 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?

Yes, if there were children of this snapshot they would be killed.
Agreed, that we could use a flag to allow that behaviour to be controlled,
either refusing to discard children, or forcing discard.

> 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.

It would only discard snapshots depending on the snapshot being merge
onto, eg in

        +-> F
        |
   A -> B -> C -> D
        |
        +-> G

If you merged D onto C, then A, B, F & G would all still be valid:

        +-> F
        |
   A -> B -> C
        |
        +-> G

If you merged C onto B, then F & G would be invalid, but D would
still be valid

   A -> B ------> D

> 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);

I don't see any need to special case the base image here. The main
virDOmainSnapshotDelete() API already lets you discard all snapshots
until you get to the base image - VMWare/VirtualBox already demonstrate
this is sufficient.

Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|