[libvirt] [PATCH v6 8/9] backup: Introduce virDomainBackup APIs

Tue Mar 26 18:13:17 UTC 2019

On 3/26/19 12:08 PM, Daniel P. Berrangé wrote:
> On Tue, Mar 26, 2019 at 01:13:52AM -0500, Eric Blake wrote:
>> Introduce a few more new public APIs related to incremental backups.
>> This builds on the previous notion of a checkpoint (without an
>> existing checkpoint, the new API is a full backup, differing from
>> virDomainBlockCopy in the point of time chosen and in operation on
>> multiple disks at once); and also allows creation of a new checkpoint
>> at the same time as starting the backup (after all, an incremental
>> backup is only useful if it covers the state since the previous
>> backup).  Snapshot creation is also a point in time at which creating
>> a checkpoint atomically can be useful. A backup job also affects
>> filtering a listing of domains, as well as adding event reporting for
>> signaling when a push model backup completes (where the hypervisor
>> creates the backup); note that the pull model does not have an event
>> (starting the backup lets a third party access the data, and only the
>> third party knows when it is finished).
>>
>> Since multiple backup jobs can be run in parallel in the future (well,
>> qemu doesn't support it yet, but we don't want to preclude the idea),
>> virDomainBackupBegin() returns a job id, which can also be queried by
>> virDomainListJobIds(), and this job id must be used for
>> virDomainBackupGetXMLDesc() and virDomainBackupEnd(). In the future,
>> we may also extend other jobs (migration as the default global job
>> impacting virDomainJobStats(), and the various block jobs) to all have
>> ids, where the existing APIs act like thin wrappers around more
>> powerful APIs that support a job id everywhere.

This is the part that has me most worried about 5.2 - anything that we
find a future libvirt supplying about job management APIs will not be
backported without a .so bump if it didn't make it into 5.2. So the hope
is that whatever lands now is usable (even if only one job at a time) by
whatever downstreams pick 5.2 as their starting point, without wishing
we had additional Job apis available.

>>
>> The full list of new API:
>>         virDomainBackupBegin;
>>         virDomainBackupEnd;
>>         virDomainBackupGetXMLDesc;
>>         virDomainListJobIds;
>>         virDomainSnapshotCreateXML2;
>>

>>  10 files changed, 463 insertions(+), 17 deletions(-)
> 
> 
>> +virDomainSnapshotPtr virDomainSnapshotCreateXML2(virDomainPtr domain,
>> +                                                 const char *xmlDesc,
>> +                                                 const char *snapshotXml,
>> +                                                 unsigned int flags);
> 
> s/snapshotXml/checkpointXml/ based on later docs.

Correct.

> 
> On IRC, you had said an alternative would be to put the checkpointXml
> as a <domaincheckpoint> child of the main <domainsnapshot> or <domainbackup>
> XML document.
> 
> IIUC, the <domaincheckpoint> XML is merely forwarded on the checkpoint
> APIs.  IOW, if you later call virDomainSnapshotGetXMLDesc, you would
> *not* expect to see the <domaincheckpoint> child again ? If that is
> correct, then having it via the separate API parameter makes more sense
> than as a XML child element.  I'd only want it as an XML child if that
> where the canonical representation & storage location. So the separate
> API looks ok to me.

All right, that answers a question I've had for a while. You are correct
that the <domaincheckpoint> is independent from the snapshot, and you
would NOT expect to see it under the listing of a <domainsnapshot> after
the fact. Embedding things also makes it conceptually harder for
<incremental>name</incremental> to refer back to a particular checkpoint
if checkpoints are not independent objects, but if they ARE independent
objects, then embedding them just to avoid a new API seems fishy.

So my only remaining question is if there is any better name than
virDomainSnapshotCreateXML2(), but since I modeled it after
virDomainMigrate[23](), I think we're okay.

>> @@ -3231,6 +3234,18 @@ int virDomainGetJobStats(virDomainPtr domain,
>>                           unsigned int flags);
>>  int virDomainAbortJob(virDomainPtr dom);
>>
>> +typedef struct _virDomainJobId virDomainJobId;
>> +typedef virDomainJobId *virDomainJobIdPtr;
>> +struct _virDomainJobId {
> 
> Shouldn't this be called just "virDomainJob" ? Id is just
> one piece of info inside the struct.
> 
> Should we be making this struct opaque, and adding
> virDomainJobGetID and virDomainJobGetType accessors,
> and thne passing a virDomainJobPtr to the other APIs
> instead of just an id ?

It's a late request on a late API addition, but it sounds reasonable. We
have the benefit that it is NOT a type that has to be backed by on-disk
XML, so while it requires more patching to datatypes.h and figuring out
how to express the type over RPC, it should be a lot less grunt-work
than what I've been doing with CheckpointXML and to a lesser extent
Backup XML.

> 
> It feels safer if virDomainBackupGetXMLDesc were
> given the full virDomainJobPtr, as then it can
> validate that the "type" field represents an
> backup job. This could detect the case where a
> stale job ID was passed in that now points to a
> completely different job type.

Interesting idea; and not that much harder (s/int/virDomainJobPtr/).

> 
>> +    /* One of virDomainJobType */
>> +    int type;
>> +
>> +    /* The job id */
>> +    int id;
>> +};
>> +int virDomainListJobIds(virDomainPtr dom, virDomainJobId **ids,
>> +                        unsigned int flags);

And this would be virDomainJobIdPtr **ids, where the caller then has to
free both each id pointer and the overall list (but we have plenty of
other opaque types to copy that practice from, including how the
generator would work with it).

Naming wise, I guess the most consistent name would be virDomainListAllJobs?

>> +/**
>> + * virDomainListJobIds:
>> + * @domain: a domain object
>> + * @ids: Pointer to a variable to store the array containing job ids or NULL
>> + *       if the list is not required (just returns number of jobs).
>> + * @flags: extra flags; not used yet, so callers should always pass 0
>> + *
>> + * Collect a list of all background jobs, and return an allocated
>> + * array of information about the type and id of each.
>> + *
>> + * The default background job (id 0, which is typically migration)
>> + * might not be included in the list; for that, use
> 
> This line makes me a little uncomfortable ?  Why would we exclude
> the default background job (sometimes) ? I feel it is preferrable
> to always return all jobs.

Okay, I'll drop the line. As I have not yet done a full implementation
of job listing, it shouldn't be too hard to get that right (and the fact
that right now, we only support 1 [migration], soon to be 2 [backup],
orthogonal domain jobs that can't be run simultaneously, plus an
additional maximum of 1 block job per domain disk, means it's still not
hard to get an initial version correct).

I'll go ahead and push the patches for Checkpoint APIs in time for the
5.2 freeze tomorrow, as well as post another round of the Backup APIs
later tonight to see how far I can get with the idea of virDomainJobPtr
being an opaque type.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190326/335d410a/attachment-0001.sig>