7:55 p.m.
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(a)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(a)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