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

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.

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

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