Re: [libvirt] [PATCH RFC 30/40] backup: Introduce virDomainBackup APIs

On Fri, Oct 18, 2019 at 06:11:15PM +0200, Peter Krempa wrote: > From: Eric Blake <eblake(a)redhat.com&gt; > > Introduce a few 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). > > 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 positive job id, and the id is also > visible in the backup XML. But until a future libvirt release adds a > bunch of APIs related to parallel job management where job ids will > actually matter, the documentation is also clear that job id 0 means > the 'currently running backup job' (provided one exists), for use in > virDomainBackupGetXMLDesc() and virDomainBackupEnd(). > > The full list of new APIs: > virDomainBackupBegin; > virDomainBackupEnd; > virDomainBackupGetXMLDesc; > > Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; > Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; > --- > include/libvirt/libvirt-domain.h | 26 ++++- > src/driver-hypervisor.h | 20 ++++ > src/libvirt-domain-checkpoint.c | 7 +- > src/libvirt-domain.c | 191 +++++++++++++++++++++++++++++++ > src/libvirt_public.syms | 8 ++ > tools/virsh-domain.c | 4 +- > 6 files changed, 252 insertions(+), 4 deletions(-) > > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h > index 22277b0a84..2d9f69f7d4 100644 > --- a/include/libvirt/libvirt-domain.h > +++ b/include/libvirt/libvirt-domain.h > @@ -3267,6 +3267,7 @@ typedef enum { > > +/** > + * VIR_DOMAIN_JOB_ID: > + * > + * virDomainGetJobStats field: the id of the job (so far, only for jobs > + * started by virDomainBackupBegin()), as VIR_TYPED_PARAM_INT. > + */ > +# define VIR_DOMAIN_JOB_ID "id" > + > /** > * VIR_DOMAIN_JOB_TIME_ELAPSED: > * > @@ -4106,7 +4115,8 @@ typedef void (*virConnectDomainEventMigrationIterationCallback)(virConnectPtr co > * @nparams: size of the params array > * @opaque: application specific data > * > - * This callback occurs when a job (such as migration) running on the domain > + * This callback occurs when a job (such as migration or push-model > + * virDomainBackupBegin()) running on the domain > * is completed. The params array will contain statistics of the just completed > * job as virDomainGetJobStats would return. The callback must not free @params > * (the array will be freed once the callback finishes). > @@ -4916,4 +4926,18 @@ int virDomainGetGuestInfo(virDomainPtr domain, > int *nparams, > unsigned int flags); > > + > +int virDomainBackupBegin(virDomainPtr domain, > + const char *backupXML, > + const char *checkpointXML, > + unsigned int flags); > + > +char *virDomainBackupGetXMLDesc(virDomainPtr domain, > + int id, > + unsigned int flags); > + > +int virDomainBackupEnd(virDomainPtr domain, > + int id, > + unsigned int flags); So this is still using a plain integer job ID, which is a concern wrt future extensibility.

Earlier in the year I queried whether we should turn the "job" into a fully fledged object, using either a string or a UUID to identify it uniquely. https://www.redhat.com/archives/libvir-list/2019-March/msg01695.html IOW having something like this:

typedef struct _virDomainJob virDomainJob; typedef virDomainJob *virDomainJobPtr;

void virDomainJobFree(virDomainJobPtr job); virDomainJobLookupByUUID(virDomainPtr job, unsigned char *uuid); int virDomainJobGetType(virDomainJobPtr job); int virDomainJobGetUUID(virDomainJobPtr job, unsigned char *uuid); int virDomainJobGetUUIDString(virDomainJobPtr job, char *uuidstr); virDomainJobPtr virDomainBackupBegin(virDomainPtr domain, const char *backupXML, const char *checkpointXML, unsigned int flags);

char *virDomainBackupGetXMLDesc(virDomainPtr domain, virDomainJobPtr job, unsigned int flags); int virDomainBackupEnd(virDomainPtr domain, virDomainJobPtr job, unsigned int flags); Privately we'd define struct _virDomainJob { unsigned char uuid[VIR_UUID_BUFLEN]; int type; }; So we don't do a RPC call for virDomainJobGet{Type,UUID,UUIDString}, and we'd just serialize the uuid over the wire for the virDomainBackup APIs I presume. Currently we have a single domain block job that can be active at any time and it would be desirable to fold that into the new API in some way.

We can either create new v2 APIs for existing methods making them return a virDomainJobPtr, or we could reserve a well-known UUID exclusively to refer to the single default domain block job, which the user can grab via virDomainJobLookupByUUID().