[libvirt] [PATCH v4 08/20] backup: Introduce virDomainBackup APIs

[Eric Blake]

On 2/11/19 3:21 PM, John Ferlan wrote:
> 
> 
> On 2/6/19 2:18 PM, 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 only
>> from virDomainCopy in the point of time chosen); 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).  It also enhances 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).
>>
>> The full list of new API:
>>         virDomainBackupBegin;
>>         virDomainBackupEnd;
>>         virDomainBackupGetXMLDesc;
>>

>> +typedef enum {
>> +    VIR_DOMAIN_BACKUP_BEGIN_NO_METADATA = (1 << 0), /* Make checkpoint without
>> +                                                       remembering it */
>> +    /* TODO: VIR_DOMAIN_BACKUP_BEGIN_QUIESCE */
> 
> Implement or drop TODO:

Indeed. And if I get it working for virDomainCheckpointCreateXML, it's
trivially supported here as well.


>> +++ b/src/libvirt-domain-checkpoint.c
>> @@ -721,3 +721,205 @@ virDomainCheckpointFree(virDomainCheckpointPtr checkpoint)
>>      virObjectUnref(checkpoint);
>>      return 0;
>>  }
> 
> Dirtying the namespace of libvirt-domain-checkpoint w/ Backup. Why not a
> separate libvirt-domain-backup - beyond the obvious need to alter more
> stuff of course.  If kept here would seem to need to alter line #2 way
> back up at the top to include virDomainBackupPtr API's too.

Or even place the backup APIs directly in libvirt-domain.c, as they are
domain-based operations with no separate object being created, and are
more similar to other job-based APIs like virDomainMigrate().


>> + * This operation returns quickly, such that a user can choose to
>> + * start a backup job between virDomainFSFreeze() and
>> + * virDomainFSThaw() in order to create the backup while guest I/O is
>> + * quiesced.
>> + */
>> +/* FIXME: Do we need a specific API for listing all current backup
>> + * jobs (which, at the moment, is at most one job), or is it better to
>> + * refactor other existing job APIs in libvirt-domain.c to have job-id
>> + * counterparts along with a generic listing of all jobs (with flags
>> + * for filtering to specific job types)?
>> + */
> 
> Or do we wait until some consumer asks for this? Do you mean
> GetBlockJobInfo? GetJobStats? GetJobInfo?
> 
> Is it required for initial implementation?  Not sure I'm expert enough
> to answer that questions!

I've already had Nir asking for something; he also requested an ability
to name jobs (he may use a UUID, but any name would work). My initial
implementation was hard-coded to always create job id '1' (and what's
the point of listing something, if the answer is either 'no job running'
or 'job 1 is running').  But where things get tricky is that if we do
NOT have an API for listing all jobs (even if there is just one possible
job in the initial implementation), then it gets much harder to backport
that API later. So yes, I _do_ need to get this implemented. I haven't
got it up yet (my v5 posting will probably still have it lacking), but
if I've missed 5.1, then I have enough time before 5.2 to definitely get
that API tackled as part of everything else related to incremental backups.


>> +/**
>> + * virDomainBackupGetXMLDesc:
>> + * @domain: a domain object
>> + * @id: the id of an active backup job previously started with
>> + *      virDomainBackupBegin()
>> + * @flags: extra flags; not used yet, so callers should always pass 0
>> + *
>> + * In some cases, a user can start a backup job without supplying all
>> + * details, and rely on libvirt to fill in the rest (for example,
> 
> s/, and/ and/
> 
>> + * selecting the port used for an NBD export). This API can then be
>> + * used to learn what default values were chosen.
>> + *
>> + * Returns a NUL-terminated UTF-8 encoded XML instance, or NULL in
> 
> s/, or/ or/
> 
>> + * case of error.  The caller must free() the returned value.
>> + */
> 
> Do we have the same security concerns as Checkpoint?  IOW: One doesn't
> necessarily have to use a conn that was used for virDomainBackupBegin or
> do they?  Should we just check for a non read-only connection (which I
> assume has implications later in remote.x defs).

The security concern for CheckpointGetXMLDesc is that the checkpoint XML
includes a <domain> sub-element, which must NOT expose any passwords in
the <domain> except on a read-write connection where the _SECURE flag is
passed.

In the patches as posted so far, I separated the Checkpoint XML (with
its <domain> subelement) to be completely distinct from the Backup XML
(which is very minimal, and just describes the job and perhaps a
<checkpoint> name but not full XML); thus, there is nothing
security-related in the output.

But I also asked the question in the cover letter if I should make
<domaincheckpoint> be a subelement of <domainsnapshot>, in which case it
should also be a subelement of a backup job (since both a backup job and
a snapshot are logical places to atomically create a checkpoint at the
same time).  And depending on how we answer that, then yes, anything
that might expose a <domain> sub-subelement (via a <domaincheckpoint>
sub-element of the backup job) would need the same _SECURE flag.


>> + * Returns 1 if the backup job completed successfully (the backup
>> + * destination file in a push model is consistent), 0 if the job was
>> + * aborted successfully (only when VIR_DOMAIN_BACKUP_END_ABORT is
>> + * passed; the destination file is unusable), and -1 on failure.
>> + */
> 
> So 0 can only be returned if/when ABORT is used (just checking my
> reading...)
> 

Correct.

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