1:15 p.m.
On 2/11/19 3:19 PM, John Ferlan wrote:
On 2/6/19 2:18 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.
>
> The full list of new API:
> virDomainCheckpointCreateXML;
> virDomainCheckpointCurrent;
> virDomainCheckpointDelete;
> virDomainCheckpointFree;
> virDomainCheckpointGetConnect;
> virDomainCheckpointGetDomain;
> virDomainCheckpointGetParent;
> virDomainCheckpointGetXMLDesc;
> virDomainCheckpointHasMetadata;
> virDomainCheckpointIsCurrent;
> virDomainCheckpointListChildren;
> virDomainCheckpointLookupByName;
> virDomainCheckpointRef;
> virDomainHasCurrentCheckpoint;
> virDomainListCheckpoints;
> virDomainCheckpointGetName;
After reading and commenting further I came back here and started
wondering if we should just name this 'libvirt-domain-backup' since
checkpoint is part of backup... Then of course adjust various comments
accordingly to indicate checkpoint and backup.
Of course there's also something to be said for separating out the
domain-backup API's into their own module too.
So while you may see comments later about splitting just keep this
secondary thought in mind.
Long-term, I'd like to have both virDomainBackupBegin() and some form of
virDomainSnapshotCreateXML() be able to kick off the creation of a
checkpoint object. Object-wise, we have virDomainSnapshotPtr and
virDomainCheckpointPtr (two distinct objects that are long-term XML
entities present regardless of what else you do), but no
virDomainBackupPtr (a backup job is just that - a job, that goes away
when it is complete). I'm actually thinking that it is better to have
virDomainBackupBegin() and friends live directly in libvirt-domain.h,
alongside other job-like APIs (virDomainMigrate(), virDomainBlockCopy(),
...), especially if I fix snapshots to refer to checkpoints.
So there's that to think about as well when considering where to place
things.
> diff --git a/include/libvirt/libvirt-domain-checkpoint.h
b/include/libvirt/libvirt-domain-checkpoint.h
> new file mode 100644
> index 0000000000..9006a46c6e
> --- /dev/null
> +++ b/include/libvirt/libvirt-domain-checkpoint.h
> @@ -0,0 +1,155 @@
> +/*
> + * libvirt-domain-checkpoint.h
> + * Summary: APIs for management of domain checkpoints
> + * Description: Provides APIs for the management of domain checkpoints
> + *
> + * Copyright (C) 2006-2019 Red Hat, Inc.
New file right, so should this just be 2019? There's varying opinions to
follow on this...
Heavily copied from an existing file, so I copied the copyright as part
of that.
> +typedef enum {
> + VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE = (1 << 0), /* Restore or alter
> + metadata */
> + VIR_DOMAIN_CHECKPOINT_CREATE_CURRENT = (1 << 1), /* With redefine,
make
> + checkpoint current */
> + VIR_DOMAIN_CHECKPOINT_CREATE_NO_METADATA = (1 << 2), /* Make checkpoint
without
> + remembering it */
> + /* TODO: VIR_DOMAIN_CHECKPOINT_CREATE_QUIESCE */
Need to remove the TODO or add the flag of course...
I'll see if I can implement it (it probably missed 5.1, but then again,
this whole series needs a respin with freeze today; but should be easy
enough to get by 5.2)
> @@ -1580,6 +1627,17 @@ struct _virHypervisorDriver {
> virDrvConnectBaselineHypervisorCPU connectBaselineHypervisorCPU;
> virDrvNodeGetSEVInfo nodeGetSEVInfo;
> virDrvDomainGetLaunchSecurityInfo domainGetLaunchSecurityInfo;
> + virDrvDomainCheckpointCreateXML domainCheckpointCreateXML;
> + virDrvDomainCheckpointGetXMLDesc domainCheckpointGetXMLDesc;
> + virDrvDomainListCheckpoints domainListCheckpoints;
> + virDrvDomainCheckpointListChildren domainCheckpointListChildren;
> + virDrvDomainCheckpointLookupByName domainCheckpointLookupByName;
> + virDrvDomainHasCurrentCheckpoint domainHasCurrentCheckpoint;
> + virDrvDomainCheckpointGetParent domainCheckpointGetParent;
> + virDrvDomainCheckpointCurrent domainCheckpointCurrent;
> + virDrvDomainCheckpointIsCurrent domainCheckpointIsCurrent;
> + virDrvDomainCheckpointHasMetadata domainCheckpointHasMetadata;
> + virDrvDomainCheckpointDelete domainCheckpointDelete;
> };
Since we're adding new I think we should be consistent w/ naming
virDrvDomainCheckpoint* and domainCheckpoint*
DomainListCheckpoints and DomainHasCurrent break the mold, so to speak.
Here, I'm copying heavily from virDomainSnapshot. The APIs that deal
directly with a virDomain object (namely, listing how many checkpoints
belong to the domain, and whether the domain has a current checkpoint)
are distinct from the APIs that deal with a virDomainCheckpointObject
(creation, getting XML, looking up children). But you are also right
that there are some other misleading names
(virDomainCheckpointLookupByName and virDomainCheckpointCurrent are
virDomain operations).
Here's what snapshots have, and how I differ:
virDomainSnapshotCreateXML;
virDomainSnapshotGetXMLDesc;
Both operate on snapshots. Counterparts virDomainCheckpointCreateXML,
virDomainCheckpointGetXMLDesc.
virDomainSnapshotNum;
virDomainSnapshotListNames;
No checkpoint counterpart for either (no need to do the racy API when
ListAllSnapshots is better)
virDomainSnapshotLookupByName;
virDomainHasCurrentSnapshot;
virDomainSnapshotCurrent;
All 3 operate on domain. Counterparts virDomainCheckpointLookupByName,
virDomainHasCurrentCheckpoint, virDomainCheckpointCurrent
virDomainRevertToSnapshot;
Operates on domain. No counterpart (you don't revert to a checkpoint,
but instead use it as the basis to an incremental virDomainBackupBegin)
virDomainSnapshotDelete;
virDomainSnapshotFree;
virDomainSnapshotGetConnect;
virDomainSnapshotGetDomain;
virDomainSnapshotGetName;
virDomainSnapshotGetParent;
virDomainSnapshotRef;
All 7 operate on snapshot, counterparts are obvious (some don't require
driver callbacks, because they obtained directly from the object).
virDomainSnapshotListChildrenNames;
virDomainSnapshotNumChildren;
Both operate on snapshot, no counterpart (no need to do the racy API
when ListAllChildren is better)
virDomainSnapshotHasMetadata;
virDomainSnapshotIsCurrent;
Both operate on snapshot, counterparts virDomainSnapshotHasMetadata,
virDomainSnapshotIsCurrent
virDomainListAllSnapshots;
virDomainSnapshotListAllChildren;
One operates on domain, the other on a snapshot. For the checkpoint
counterpoints, since I didn't copy the older racy names, I didn't see
the point of keeping 'All' in the name, so I used
virDomainListCheckpoints and virDomainCheckpointListChildren.
> +/**
> + * 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.
NIT, but no big deal... Patch order has virDomainBackupBegin in the next
patch...
Correct; I don't know how much churn to put to keep the docs consistent
mid-series, or whether to just let it work out in the end.
> + *
> + * See <a href=formatcheckpoint.html#CheckpointAttributes">Checkpoint
XML</a>
> + * for more details on @xmlDesc. In particular, some hypervisors may require
> + * particular disk formats, such as qcow2, in order to support this
> + * command; where @xmlDesc can be used to limit the checkpoint to a working
> + * subset of the domain's disks.
> + *
> + * 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
s/just-created/just created/
copy-and-paste from snapshots. (I can touch those up as a separate
trivial patch to match any grammar changes I make here based on your
review).
> + *
> + * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error.
s/, or/ or/
> + * the caller must free() the returned value.
s/the/The/
the indention is probably unnecessary too.
Looks like an editor accident on my part.
> + * virDomainListCheckpoints:
> + * 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
s/, and/ and/
> + * 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.
Not clear what a group is within this context and how one defines that,
uses that, and of course the CYA condition of impossible combination ;-).
Of course groups start to be clearer the following description... Not
that I completely understand them. Seems to add levels of complexity.
A lot of this text is copy-and-paste from the snapshot counterpart;
except that snapshots have two additional filter groups
(_INACTIVE/_ACTIVE, _DISK_ONLY/_INTERNAL/_EXTERNAL) that aren't
applicable to filter groups for checkpoints. There may indeed be ways
to clean this text up, but it stems from the fact that all of libvirt's
objects that can be listed with various filter groups have to define
which filter groups exist based on subsets of the overall flags.
> +virDomainListCheckpoints(virDomainPtr domain,
> + virDomainCheckpointPtr **checkpoints,
> + unsigned int flags)
Why not virDomainCheckpointListAll (beyond the difference w/ Snapshots)
As mentioned above, the snapshot version was virDomainListAllSnapshots,
but only because it was a newer API designed to replace the older
virDomainSnapshotListNames that was inherently racy. Since the
operation is on a virDomain object (and not a virDomainCheckpoint
object), the naming makes sense to me, but I'm still open to suggestions
if my explanation is not convincing, or if you like the name 'All' in
the description to match all the other APIs, even if I don't want to
match the non-modern listing interfaces.
> + * By default, this command covers only direct children; it is
also possible
> + * to expand things to cover all descendants, when @flags includes
> + * VIR_DOMAIN_CHECKPOINT_LIST_DESCENDANTS. Also, some filters are provided in
> + * groups, where each group contains bits that describe mutually exclusive
> + * attributes of a snapshot, and where all bits within a group describe
s/, and/ and/
> + * all possible snapshots. 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.
Can we just say similar to ListAll - that way we don't need to update both.
If these really are similar API's then perhaps they could be combined
such that if @checkpoint == NULL, then we're listing from root;
otherwise, we're listing from @checkpoint. Assuming checkpoint == NULL
is ListAll right?
Well, we could, except that one API takes a virDomainPtr, the other
takes only a virDomainCheckpointPtr (where the associated virDomainPtr
is grabbed implicitly from the checkpoint object), and you CAN'T pass a
NULL virDomainCheckpointPtr (because you'd then be lacking a
virDomainPtr object to use).
Could be easier to not have ListAll and ListChildren...
Snapshots have both (list a filtered set of all snapshots from a domain,
and list a filtered set of descendent snapshots from a parent); having
both was pretty easy to do just by copying concepts.
> +int
> +virDomainHasCurrentCheckpoint(virDomainPtr domain, unsigned int flags)
Use one line for each argument.
Why not virDomainCheckpointHasCurrent to keep the virDomainCheckpoint*
namespace consistent?
Mentioned above - because virDomainHasCurrentSnapshot() is likewise
operating on a virDomainPtr.
> +virDomainCheckpointPtr
> +virDomainCheckpointCurrent(virDomainPtr domain,
> + unsigned int flags)
Why isn't this virDomainCheckpointGetCurrent for consistency?
Again, because of existing practice with virDomainSnapshotCurrent. (I
don't necessarily mind using smarter names, but it makes for that many
more special cases in the language binding code, and we already have
enough inconsistent oddities in the public API).
> +
> +/**
> + * virDomainCheckpointDelete:
> + * @checkpoint: the checkpoint to remove
> + * @flags: not used yet, pass 0
Remove this one^^
> + * @flags: bitwise-OR of supported virDomainCheckpointDeleteFlags
Oops - yeah.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org