8:09 a.m.
On 03/24/2010 04:52 AM, Paolo Bonzini wrote:
> > > /* 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?
Good thought. I'll add a VIR_DOMAIN_SNAPSHOT_DEACTIVATE_DISCARD_FORCE flag
(to be renamed later, but the semantic will be what you say).
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.
Right, also a good idea, I'll also add VIR_DOMAIN_SNAPSHOT_DEACTIVATE_MERGE_FORCE.
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);
Given Dan's alternate proposal of virDomainCreateWithSnapshot, I don't
think this is necessary. That is, if you wanted to start a guest from
it's base image, you would call "virDomainCreate". If you wanted to
start a guest from a snapshot, you would call "virDomainCreateWithSnapshot".
If you want to remove a snapshot, you would call virDomainSnapshotDeactivate.
I agree that the fact that virDomainSnapshotDeactivate has the possibility
to modify the base image makes it more dangerous than others, but it won't
be the default and it is an integral part of snapshotting.
> > 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).
Good point. I could allow virDomainSnapshotCreateXML to take a
NULL pointer, which would be interpreted as "<domainsnapshot/>",
but that doesn't seem to fit in with the rest of the libvirt
API. I think I'll just allow virsh to take an optional XML
file, which I'll then pass in as "<domainsnapshot/>".
> > 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.
The problem, though, is what Mattias points out; there is no (easy) way
that, given state C, I can get back to state A to make a new snapshot.
I actually have to be at state A to take a new snapshot with a parent of
A. I think this is a place where we have to make it manual; if you really
want a new snapshot that is a child of A, you'll have to manually shutdown
your domain, boot to snapshot A, then take a snapshot of A.
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.
That's more or less what the <parent> field is supposed to mean, although
I'm not sure I understand your proposal about --inactive. How would you
go about doing that?
--
Chris Lalancette