[libvirt] [PATCH v4 07/20] backup: Introduce virDomainCheckpoint APIs

[John Ferlan]

On 2/25/19 2:15 PM, Eric Blake wrote:
> 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.
> 

Makes sense to me to have BackupBegin live in libvirt-domain.h

Seems like merging Snapshot and Checkpoint code can be a "wishlist" type
task.

[...]

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

More recent changes to the hacking guide have some fairly specific
guidelines about API naming. That's where my thoughts were since this is
to a degree a new API set. But you made good points about consistency
between Snapshots and Checkpoints as children of the Domain. In a way I
see this as similar to how Volumes are a child of StoragePool objects.

When in doubt I fall back to Andrea's epiphany "Naming is hard".
Everyone seems to have an opinion or tweak to your chosen name. In the
long run, for me it's about consistency.

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

virDomainSnapshotCurrent probably should have been
virDomainSnapshotGetCurrent...

The virDomainHasCurrentSnapshot is more "magical" since it's something
that was handled/saved more so internally and not exposed (prior to your
other recent series). In any case, the naming does fit the current model
it's just where the API is defined that throws me off. It should be in
libvirt-domain.c at least "theoretically logically speaking" (how's that
for wishy-washy!).

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

OK fair enough. I guess that's essentially consistent with how Volumes
API's are named.  The question then becomes does it belong in
libvirt-domain.c or libvirt-domain-checkpoint.c...

[...]

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

Suffice to say it's all very confusing with all the various options and
gotcha.  Simplicity is so much easier.

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

Your explanation above makes (more) sense now... Using
virDomainListCheckpoints that is.

John

[...]  The rest would be covered under the - I see more clearly now how
you were thinking about this...