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

[John Ferlan]

[...]

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

Yes, makes more sense I think - operating on a domain or domain obj,
then use libvirt-domain.c

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

How much of that can be left for the future beyond 5.2. It seems more
requirements keep getting added and we cannot at least get something
going/pushed.

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

OMG the cover letter - I wasn't even thinking about that at this point
let alone remember what I may have read ;-)

Not sure what the best answer would be. I can see overlap between the
two, but I really do "fear" there's some pieces of heavy weight in
snapshots that just would be so difficult to link directly as a parent
to a checkpoint.  As a lightweight piece, a checkpoint would be
something that both backup and snapshot could conceivably use I suppose
though. Of course you've thought about this much longer than I have.

I'd be concerned over come cyclical dependency showing its ugly face as
well as not knowing whether there is some depth to which it would not be
supportable in XML to make checkpoint a subelement. Something about how
"<backingStore>" ran into problems with depth comes to mind.

John

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