[libvirt] [PATCH 5/8] backup: Introduce virDomainCheckpoint APIs

[Eric Blake]

On 06/25/2018 06:16 PM, John Ferlan wrote:
> 
> 
> On 06/13/2018 12:42 PM, Eric Blake wrote:
>> Introduce a bunch of new public APIs related to backup checkpoints.
>> Checkpoints are modeled heavily after virDomainSnapshotPtr (both
>> represent a point in time of the guest), although a snapshot exists
>> with the intent of rolling back to that state, while a checkpoint
>> exists to make it possible to create an incremental backup at a later
>> time.
>>
>> Signed-off-by: Eric Blake <eblake at redhat.com>
>> ---
>>   docs/Makefile.am                            |   3 +
>>   docs/apibuild.py                            |   2 +
>>   docs/docs.html.in                           |   1 +
>>   include/libvirt/libvirt-domain-checkpoint.h | 147 ++++++
>>   include/libvirt/libvirt.h                   |   5 +-
>>   libvirt.spec.in                             |   1 +
>>   mingw-libvirt.spec.in                       |   2 +
>>   po/POTFILES                                 |   1 +
>>   src/Makefile.am                             |   2 +
>>   src/driver-hypervisor.h                     |  60 ++-
>>   src/libvirt-domain-checkpoint.c             | 708 ++++++++++++++++++++++++++++
>>   src/libvirt_public.syms                     |  16 +
>>   12 files changed, 944 insertions(+), 4 deletions(-)
>>   create mode 100644 include/libvirt/libvirt-domain-checkpoint.h
>>   create mode 100644 src/libvirt-domain-checkpoint.c
>>
> 
> In a word... Overwhelming!

Yeah, it's been a lot of code on my end, and more still to come.

> 
> I have concerns related to committing the API before everyone is sure
> about the underlying hypervisor code. No sense in baking in an API only
> to find out later needs/issues. It seems we

Incomplete sentence? But it definitely explains why I want a working 
demo of the API in use, even if targeting what is currently experimental 
qemu commands, to show that the API does what we want.

> 
> I see checkpoint code borrows the domain connection - is that similar to
> snapshots?

Yes, I was very heavily borrowing the code for snapshots, as both items 
are sub-objects that are related to a domain at a point in time.

> 
> I won't go too in depth here - mostly just scan to look for obvious issues.
> 

>> +++ b/include/libvirt/libvirt-domain-checkpoint.h
>> @@ -0,0 +1,147 @@
>> +/*
>> + * libvirt-domain-checkpoint.h
>> + * Summary: APIs for management of domain checkpoints
>> + * Description: Provides APIs for the management of domain checkpoints
>> + * Author: Eric Blake <eblake at redhat.com>
>> + *
>> + * Copyright (C) 2006-2018 Red Hat, Inc.
> 
> Since it's created in 2018 - shouldn't it just list that? Not my area of
> expertise by any stretch of the imagination though.

Since I copied-and-pasted from snapshots, I like to keep the full range 
of years from my template code; I could use just 2018 by calling it new 
code, but I don't see it being much of an issue either way (no one will 
use this header in isolation, so whether it has the project's full 
copyright years, or just this file's earliest year of existence, doesn't 
really matter when git history will paint a full picture).

>> +/**
>> + * virDomainCheckpointListFlags:
>> + *
>> + * Flags valid for virDomainListCheckpoints() and
>> + * virDomainCheckpointListChildren().  Note that the interpretation of
>> + * flag (1<<0) depends on which function it is passed to; but serves
>> + * to toggle the per-call default of whether the listing is shallow or
>> + * recursive.  Remaining bits come in groups; if all bits from a group
>> + * are 0, then that group is not used to filter results.  */
>                                                             ^^
> There's an extra space here

Yeah, I tend to do two spaces after full stop (old-school typewriting 
convention). I can make it a point to use just one when revising this 
patch, although I don't think it makes too much difference either way.

> 
>> +typedef enum {
>> +    VIR_DOMAIN_CHECKPOINT_LIST_ROOTS       = (1 << 0), /* Filter by checkpoints
>> +                                                          with no parents, when
>> +                                                          listing a domain */
>> +    VIR_DOMAIN_CHECKPOINT_LIST_DESCENDANTS = (1 << 0), /* List all descendants,
>> +                                                          not just children, when
>> +                                                          listing a checkpoint */
> 
> There's two "1 << 0" entries - ironically the doc page render lists
> these in the opposite order.  Still I see this is essentially a copy of
> the SnapshotListFlags. So do we really need to keep that when there's
> only 2 API's?

That's the same way snapshots did it, and yes, two separate names makes 
the most sense for both consistency and usage.  That is, you call either:

virDomainListCheckpoints(,0) (list all checkpoints, by recursing)
virDomainListCheckpoints(,_LIST_ROOTS) (list only roots, by not recursing)

virDomainCheckpointListChildren(,0) (list only direct children, by not 
recursing)
virDomainCheckpointListChildren(,_LIST_DESCENDENTS) (list all 
generations, by recursing)

but it was easier to declare one set of flags than two separate enums 
for the one difference.  There's also the fact that the recurse-or-not 
bit has a different sense between the two APIs, so having two different 
names for the bit makes it easier to see which sense you are getting.

> 
>> +
>> +    VIR_DOMAIN_CHECKPOINT_LIST_LEAVES      = (1 << 1), /* Filter by checkpoints
>> +                                                          with no children */
>> +    VIR_DOMAIN_CHECKPOINT_LIST_NO_LEAVES   = (1 << 2), /* Filter by checkpoints
>> +                                                          that have children */
>> +
>> +    VIR_DOMAIN_CHECKPOINT_LIST_METADATA    = (1 << 3), /* Filter by checkpoints
>> +                                                          which have metadata */
>> +    VIR_DOMAIN_CHECKPOINT_LIST_NO_METADATA = (1 << 4), /* Filter by checkpoints
>> +                                                          with no metadata */
>> +} virDomainCheckpointListFlags;
> 
> Not quite sure where/how metadata comes into play. What metadata?
> Where/when was that introduced?

I'm still not convinced whether we need this flag; we could remove it 
for initial introduction, and add it later if it proves useful; however, 
it is modeled after what proved useful for snapshots.  The idea here is 
that if qemu bitmaps learn the ability to track their parent bitmap, 
then libvirt could reconstruct the checkpoint chain purely by reading 
the qcow2 metadata, instead of having to track XML itself.  Such a 
tracking will be limited (libvirt wants to store extra metadata such as 
'description', 'timestamp', and the full 'domain' layout at the time of 
the checkpoint, while do not have room in qcow2 to be stored there); but 
if we allow libvirt to import checkpoints by reading what bitmaps 
already exist in the qcow2 file, then knowing which checkpoints have no 
metadata (the ones reconstructed from qcow2) vs. DO have metadata (the 
ones that libvirt has tracked in secondary XML files) will be useful.


>> +/* Delete a checkpoint */
>> +typedef enum {
>> +    VIR_DOMAIN_CHECKPOINT_DELETE_CHILDREN      = (1 << 0), /* Also delete children */
>> +    VIR_DOMAIN_CHECKPOINT_DELETE_METADATA_ONLY = (1 << 1), /* Delete just metadata */
>> +    VIR_DOMAIN_CHECKPOINT_DELETE_CHILDREN_ONLY = (1 << 2), /* Delete just children */
>> +} virDomainCheckpointDeleteFlags;
> 
> Again, not sure what the metadata entails here.  Although this perhaps
> answer a different question I had about converging bitmaps by deleting
> "middle" checkpoints.

Again, modeled after snapshots.

The 8 combinations result in:

children | metadata_only | children_only
0  0  0  - delete one snapshot (both the bitmap in qcow2 and the libvirt 
XML)
0  0  1  - invalid (children_only requires children)
0  1  0  - delete the libvirt xml, but leave the bitmap in qcow2
0  1  1  - invalid
1  0  0  - delete a full tree of snapshots (this one and all its 
children), including libvirt XML
1  0  1  - delete a partial tree of snapshots (all the children, but 
leave this one intact)
1  1  0  - delete full tree of libvirt xml but leave bitmaps in qcow2
1  1  1  - delete partial tree of libvirt xml but leave bitmaps

Hmm - I didn't document _CHILDREN or _CHILDREN_ONLY in the .c file. 
Maybe I should just delete those flags from here for now (again, we can 
always add flags later).

>> @@ -355,6 +356,7 @@ rm -rf $RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh
>>   %{mingw64_includedir}/libvirt/libvirt.h
>>   %{mingw64_includedir}/libvirt/libvirt-common.h
>>   %{mingw64_includedir}/libvirt/libvirt-domain.h
>> +%{mingw64_includedir}/libvirt/libvirt-domain-checkpoint.h
>>   %{mingw64_includedir}/libvirt/libvirt-domain-snapshot.h
>>   %{mingw64_includedir}/libvirt/libvirt-event.h
>>   %{mingw64_includedir}/libvirt/libvirt-host.h
> 
> Not an area I know well, but as long as the various make with rpm
> options is tested, then great. Seems we always seem to forget something
> related to some obscure option every time we add something new!

Also I found a lot of these places by grepping for domain snapshots - if 
a file mentioned one, it should probably mention the other :)

>> +/**
>> + * virDomainCheckpointGetDomain:
>> + * @checkpoint: a checkpoint object
>> + *
>> + * Provides the domain pointer associated with a checkpoint.  The
>> + * reference counter on the domain is not increased by this
>> + * call.
> 
> Seems call could fit on previous line.

Maybe. And sometimes emacs is funny when it reflows paragraphs.  It 
doesn't affect the generated html docs, though.


>> +/**
>> + * virDomainCheckpointCreateXML:
>> + * @domain: a domain object
>> + * @xmlDesc: description of the checkpoint to create
>> + * @flags: bitwise-OR of supported virDomainCheckpointCreateFlags
>> + *
>> + * Create a new checkpoint using @xmlDesc on a running @domain.
>> + * Typically, it is more common to create a new checkpoint as part of
>> + * kicking off a backup job with virDomainBackupBegin(); however, it
>> + * is also possible to start a checkpoint without a backup.
> 
> Should we state that the domain needs to have an open connection already
> or is that for free (too lazy to check right now ;-))

A valid virDomainCheckpointPtr implies an open connection already.

> 
>> + *
>> + * See formatcheckpoint.html#CheckpointAttributes document for more
>> + * details on @xmlDesc.
>> + *
>> + * If @flags includes VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE, then this
>> + * is a request to reinstate checkpoint metadata that was previously
>> + * discarded, rather than creating a new checkpoint.  When redefining
>> + * checkpoint metadata, the current checkpoint will not be altered
>> + * unless the VIR_DOMAIN_CHECKPOINT_CREATE_CURRENT flag is also
>> + * present.  It is an error to request the
>> + * VIR_DOMAIN_CHECKPOINT_CREATE_CURRENT flag without
>> + * VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE.
>> + *
>> + * If @flags includes VIR_DOMAIN_CHECKPOINT_CREATE_NO_METADATA, then
>> + * the domain's disk images are modified according to @xmlDesc, but
>> + * then the just-created checkpoint has its metadata deleted.  This
>> + * flag is incompatible with VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE.
>> + *
>> + * Returns an (opaque) new virDomainCheckpointPtr on success, or NULL
>> + * on failure.
>> + */
> 
> Not sure I quite yet understand the "metadata" reference... Not sure how
> much of this is cut-n-paste from the snapshot world.

I'm not opposed to trimming the METADATA flag from the first revision, 
then adding it later if it makes sense.

>> +/**
>> + * virDomainCheckpointGetXMLDesc:
>> + * @checkpoint: a domain checkpoint object
>> + * @flags: bitwise-OR of subset of virDomainXMLFlags
>> + *
>> + * Provide an XML description of the domain checkpoint.
>> + *
>> + * No security-sensitive data will be included unless @flags contains
>> + * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only
>> + * connections.  For this API, @flags should not contain either
>> + * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU.
> 
> New paragraph for the last sentence and perhaps just state "This API
> does not support the xx or xx flags.

Copy-and-paste from snapshots, but sure, I can clean it up.

>> +/**
>> + * virDomainListCheckpoints:
>> + * @domain: a domain object
>> + * @checkpoints: pointer to variable to store the array containing checkpoint
>> + *               objects, or NULL if the list is not required (just returns
> 
> s/objects,/objects/
> 
>> + *               number of checkpoints)
>> + * @flags: bitwise-OR of supported virDomainCheckpoinListFlags
> 
> CheckpointListFlags
> 
>> + *
>> + * Collect the list of domain checkpoints for the given domain, and allocate
>> + * an array to store those objects.
>> + *
>> + * By default, this command covers all checkpoints; it is also possible to
>> + * limit things to just checkpoints with no parents, when @flags includes
>> + * VIR_DOMAIN_CHECKPOINT_LIST_ROOTS.  Additional filters are provided in
>> + * groups, where each group contains bits that describe mutually exclusive
>> + * attributes of a checkpoint, and where all bits within a group describe
>> + * all possible checkpoints.  Some hypervisors might reject explicit bits
>> + * from a group where the hypervisor cannot make a distinction.  For a
>> + * group supported by a given hypervisor, the behavior when no bits of a
>> + * group are set is identical to the behavior when all bits in that group
>> + * are set.  When setting bits from more than one group, it is possible to
>> + * select an impossible combination, in that case a hypervisor may return
>> + * either 0 or an error.
> 
> Huh, what?  This is really a confusing statement.

Oh well, copied from snapshots.

> 
> Considering "other factors" - rather than returning multiple levels of
> data, maybe we ought to consider simplifying our lives and only return
> the main/top level checkpoint object. Forcing the consumer to handle all
> the iteration logic if they so desire. The concern being you end up 100
> levels deep, too much data, and timeouts (think backingStore issues).
> Of course that alters the name of the API a bit.
> 
> Assuming the driver level could easily return a count of checkpoints,
> that allows the consumer all the ammunition they need I would think.

Snapshots already had an API that returned a count - it was racy (by the 
time you queried the count, another thread could have changed the 
count).  We've already learned that the List* functions should return a 
filterable list of objects, and then provide enough filters to be useful 
so that a user can narrow the list rather than getting too much data.

> 
> The way logic works now is that none or all type thing. Let the consumer
> decide how far they want to chase.

The other issue is that if I have a sequence of checkpoints:

Mon .. Tue .. Wed

where Mon is the root, but I want to perform an incremental backup of 
just the data modified since Wed, then having to do 
virDomainListCheckpoints(mydom) to get Mon, then 
virDomainCheckpointListChildren(Mon) to get Tue, then 
virDomainCheckpointListChildren(Tue) to get Wed, is slower than just 
doing virDomainListCheckpoints(mydom) to get the set 'Mon, Tue, Wed' up 
front.


>> +
>> +/**
>> + * virDomainCheckpointListChildren:
>> + * @checkpoint: a domain checkpoint object
>> + * @children: pointer to variable to store the array containing checkpoint
>> + *            objects, or NULL if the list is not required (just returns
> 
> s/objects,/objects
> 
>> + *            number of checkpoints)
>> + * @flags: bitwise-OR of supported virDomainCheckpointListFlags
>> + *
>> + * Collect the list of domain checkpoints that are children of the given
>> + * checkpoint, and allocate an array to store those objects.
> 
> assuming using the open domain connection pointer for the provided
> checkpoint.

Yes, and similar to snapshots.


>> +
>> +    if (conn->driver->domainCheckpointCurrent) {
>> +        virDomainCheckpointPtr snap;
> 
> s/snap/checkpoint

Yep, there's probably a few of these still lurking in my series.

>> +/**
>> + * virDomainCheckpointRef:
>> + * @checkpoint: the checkpoint to hold a reference on
>> + *
>> + * Increment the reference count on the checkpoint. For each
>> + * additional call to this method, there shall be a corresponding
>> + * call to virDomainCheckpointFree to release the reference count, once
>> + * the caller no longer needs the reference to this object.
>> + *
>> + * This method is typically useful for applications where multiple
>> + * threads are using a connection, and it is required that the
>> + * connection and domain remain open until all threads have finished
>> + * using the checkpoint. ie, each new thread using a checkpoint would
>> + * increment the reference count.
> 
> Kind of the "gray area" when checkpoints use domain objects which would
> own the connection.  Almost makes me wonder if by creating a checkpoint
> object, then should we just make the domm->conn ref to ensure that conn
> doesn't get free'd inadvertently.

This one is copied directly from snapshots; if one should grab a 
reference to the domain and/or connection, then both should (right now, 
neither do; the object is valid only as long as your domain object and 
connection also remain valid).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org