[libvirt] [RFC] New domain job control and stat APIs

[Daniel P. Berrangé]

On Wed, Jul 10, 2019 at 02:27:21PM +0200, Peter Krempa wrote:
> Currently we don't have a consolidated approach for managing
> asynchronous long-running domain jobs. Historically there were
> long-running jobs which interlocked with each other and thus there was
> only one such job possible at given time (migration, save, restore, dump)
> 
> These jobs have a not very flexible set of APIs:
> virDomainGetJobInfo, virDomainGetJobStats, virDomainAbortJob.

Yeah the person who designed virDomainGetJobInfo was not doing
a very good job of it ;-P

> With this series I want to introduce a set of APIs for managing the jobs
> which are designed to be universal enough and a new event so that noting
> will try to retrofit onto existing infrastructure.
> 
> An example of the job XML would be:
> 
> <job type='block-commit-active' state='ready'>

I'd expect to see a unique identifier of some kind here.

>   <config>
>     <disk>vda</disk>
>     <top>vda[1]</top>
>     <base>vda[5]</base>
>   </config>
>   <stats>
>    <current>12345</current>
>    <end>12345</current>
>   </stats>
> </job>
> 
> but this will be mostly a topic for the second part of this excercise
> after we discuss the APIs.

> +typedef enum {
> +    VIR_DOMAIN_JOB_TYPE_NONE = 0,
> +    VIR_DOMAIN_JOB_TYPE_MIGRATION = 1,
> +    VIR_DOMAIN_JOB_TYPE_BLOCK_PULL = 2,
> +    [...]
> +
> +# ifdef VIR_ENUM_SENTINELS
> +    VIR_DOMAIN_JOB_TYPE_LAST
> +# endif
> +} virDomainJobType;
> +
> +
> +typedef enum {
> +    VIR_DOMAIN_JOB_STATE_NONE = 0, /* unknown job state */
> +    VIR_DOMAIN_JOB_STATE_RUNNING = 1, /* job is currently running */
> +    VIR_DOMAIN_JOB_STATE_READY = 2, /* job reached a synchronized state and may be finalized */
> +    VIR_DOMAIN_JOB_STATE_FAILED = 3, /* job has failed */
> +    VIR_DOMAIN_JOB_STATE_COMPLETED = 4, /* job has completed successfully */
> +    VIR_DOMAIN_JOB_STATE_ABORTED = 5, /* job has been aborted */
> +    [...]
> +
> +# ifdef VIR_ENUM_SENTINELS
> +    VIR_DOMAIN_JOB_STATE_LAST
> +# endif
> +} virDomainJobState;
> +
> +
> +typedef struct _virDomainJob virDomainJob;
> +typedef virDomainJob *virDomainJobPtr;
> +struct _virDomainJob {
> +    char *name;
> +    virDomainJobType type;
> +    virDomainJobState state;
> +
> +    /* possibly overkill? - currently empty*/
> +    virTypedParameterPtr data;
> +    size_t ndata;
> +};

I'd have a preference for keeping this as an opaque
struct not exposed in the public header, and making
it a first class object. So instead have accessors

  char * virDomainJobGetName(virDomainJobPtr job);
  virDomainJobType virDOmainJobGetType(virDomainJobPtr job);
  virDomainJobState virDOmainJobGetState(virDomainJobPtr job);

which fetch from the local struct. The local struct would
also need to contain a reference to the virDomainPtrpass over

This avoids the query you have about virTypedParameterPtr
inclusion, as we can ignore it now, and if we get a compelling
need, we then just add a remotable method:

   virDomainJobGetStats(virDomainJobPtr job,
                        virTypedParameterPtr **params,
			unsigned int *nparams);

> +void virDomainJobFree(virDomainJobPtr job);
> +
> +int virDomainJobList(virDomainPtr domain,
> +                     virDomainJobPtr **jobs,
> +                     unsigned int flags);
> +
> +int virDomainJobGetXMLDesc(virDomainPtr domain,
> +                           const char *jobname,
> +                           unsigned int flags);

This would become

 int virDomainJobGetXMLDesc(virDomainJobPtr job,
                            unsigned int flags);


> +
> +typedef enum {
> +    VIR_DOMAIN_JOB_CONTROL_OPERATION_NONE = 0,
> +    VIR_DOMAIN_JOB_CONTROL_OPERATION_ABORT = 1,
> +    VIR_DOMAIN_JOB_CONTROL_OPERATION_FINALIZE = 2,
> +    VIR_DOMAIN_JOB_CONTROL_OPERATION_PAUSE = 3,
> +    VIR_DOMAIN_JOB_CONTROL_OPERATION_RESUME = 4,
> +    VIR_DOMAIN_JOB_CONTROL_OPERATION_DISMISS = 5,
> +
> +# ifdef VIR_ENUM_SENTINELS
> +    VIR_DOMAIN_JOB_CONTROL_OPERATION_LAST
> +# endif
> +} virDomainJobControlOperation;
> +
> +int virDomainJobControl(virDomainPtr domain,
> +                        const char *jobname,
> +                        virDomainJobControlOperation op,
> +                        unsigned int flags);

And again change to

 int virDomainJobControl(virDomainJobPtr job,
                         virDomainJobControlOperation op,
                         unsigned int flags);


I think I'd also have a preference to identify a job based on a UUID too,
as that gives stronger uniqueness. eg when debugging there's no risk of
confusing two jobs which are against different domains but happened to get
the same name. 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|