qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] Jobs 2.0 QAPI [RFC]


From: John Snow
Subject: Re: [Qemu-devel] Jobs 2.0 QAPI [RFC]
Date: Mon, 21 Dec 2015 12:45:37 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0


On 12/21/2015 07:31 AM, Kevin Wolf wrote:
> Am 17.12.2015 um 01:50 hat John Snow geschrieben:
>> In working through a prototype to enable multiple block jobs. A few
>> problem spots in our API compatibility become apparent.
>>
>> In a nutshell, old Blockjobs rely on the "device" to identify the job,
>> which implies:
>>
>> 1) A job is always attached to a device / the root node of a device
>> 2) There can only be one job per device
>> 3) A job can only affect one device at a time
>> 4) Once created, a job will always be associated with that device.
>>
>> All four of these are wrong and something we need to change, so
>> principally the "device" addressing scheme for Jobs needs to go and we
>> need a new ID addressing scheme instead.
>>
>> While we're at it, though, Jeff Cody is cooking up a new permissions
>> system for the block layer and we are considering the idea of a generic
>> job system for QEMU.
>>
>> So we need to retool our QMP commands. Now's the time to design a good
>> clean break.
> 
> I'lll reply here without having read Eric's comments yet, because I'm
> afraid if I were to read the whole thread first, I would forget half of
> what I wanted to comment on here...
> 
> So I may be repeating some things or make suggestions where Eric has
> already posted a good solution, sorry for that.
> 
>> ==== qmp_query_block_jobs ====
>>
>> Currently, this loops through every BDS we have and sees if it has a job
>> attached or not. If yes, it appends its info into a flat list and
>> reports the list back.
>>
>> This is easily extendable to an arbitrary number of jobs; we just append
>> more jobs into the list.
>>
>> Instead of identifying a job by its device, we will now need to identify
>> a job based on its ID.
>>
>> e.g.
>>
>> [{"device": "drive0", ...}, {"device": "drive1", ...}, ...]
>>
>> becomes:
>>
>> [{"device": "drive0", "id": "#job102", ...},
>>  {"device": "drive1", "id": "#job529", ...}, ...]
>>
>> However, if Jeff Cody's work on Block Job permissions progresses, device
>> may not always be reportable anymore. This job may not be attached to
>> any one device, not attached to a root node, or associated with multiple
>> graphs entirely.
>>
>> Let's consider using a new command. I'm not sure if this is strictly
>> possible with current QAPI, but Eric will yell at me if it isn't --
>> forgive the wiggly pseudo-specification.
>>
>> query-jobs
>>
>> Will return a list of all jobs.
>>
>> query-jobs sys="block"
>>
>> Will return a list of all block jobs. This will be the only valid
>> subsystem to start with -- we don't have non-block jobs yet and it may
>> be some time before we do.
> 
> In the past we avoided filtering functionality in query-* commands and
> instead required the client to request the full list and do its own
> filtering. I'm not strictly opposed to such functionality if it's useful
> enough, but we should be aware that we're doing something new here.
> 

I don't intend to add more complex filtering systems, but it seemed a
natural fit in this instance. In particular, allowing `query-jobs
sys=block` gives us the 2.0 equivalent to the block job query command.

Should make scripts easy to upgrade and doesn't cost us anything.

Why have we resisted filtering in the past?

>> Each subsystem will have a sys= enumeration, and the jobs for that
>> particular subsystem can subclass the default return type. The QAPI
>> commands can be defined to accept a union of the subclasses so we don't
>> have to specify in advance which type each command accepts, but we can
>> allow the responsible subsystem to interpret it on demand dynamically.
>>
>> The base type can be something along the lines of:
>>
>> { 'struct': 'JobInfo',
>>   'data': {
>>     'type': JobTypeEnum,
>>     'sys': JobSysEnum,
>>     'id': str,
>>     'busy': bool,
>>     'paused': bool,
>>     'ready': bool
>>   }
>> }
> 
> 'ready' optional and missing for jobs which don't require manual
> completion?
> 

Good add, yes.

>> And the BlockJobInfo can inherit/extend, looking like this:
>>
>> { 'struct': 'BlockJobInfo',
>>   'base': JobInfo
>>   'data': {
>>     'len': int,
>>     'offset': int,
> 
> len/offset are misnomers and really represent the progress of the job. I
> think we want to have these in JobInfo, with better names.
> 

We could add a generic "progress" field that goes from [0.0,100.0].
Block Jobs could still report things like bytes transferred, estimated
total bytes, etc.

>>     'speed': 'int',
> 
> Eventually, we'll deprecate this and require insertion of a throttling
> filter in the right place instead. But for now, I'm afraid we'll need
> this.
> 
> I'm not completely sure if it makes sense for every thinkable block job,
> though. A block job might in theory have no background operation or more
> than one.
> 

We could make it optional. I went for "minimal changes" at first, but we
can be more drastic if we desire.

>>     'io-status': BlockDeviceIoStatus,
> 
> This has always been a weird error reporting interface because it can't
> tell you whether the I/O error happened on the source or the target.
> It's similar to attach the job to a single BDS: It kind of works in
> practice, but conceptually it doesn't make a lot of sense.
> 

Let's nix it then; we can include it per-node in the tree-view below.

>>     'devices': <see below>,
>>   }
>> }
>>
>> 'devices' will now report a more nuanced view of this Job's associated
>> nodes in our graph, as in the following:
>>
>> { "devices": [
>>   { "name": "drive0",
>>     "nodes": [
>>       { "id": "#node123",
>>         "permissions": "+RW-X"
>>       },
>>       { "id": "#node456",
>>         "permissions": "+R"
>>       }
>>     ]
>>   }
>> }
>>
>> This lets us show exactly what nodes we are touching, what BlockBackends
>> they belong to, and what permissions are involved. Unambiguously. You
>> can use your own imagination for a permissions string that will make
>> sense -- This depends on Jeff Cody's work primarily.
> 
> There is one important information missing: What role that node takes
> for the job - that is, which is the source and which is the target. I'm
> not completely sure if the op blocker information should be visible to
> users, I consider this mostly internal node management.
> 

The thought was that if the permissions were visible, clients would be
able to pre-determine if they could launch another job or not from the
query data. I was also thinking the exact "role" of that node was not
really important as long as we knew the permissions associated with that
node.

Adding role information wouldn't hurt, though. We can add a string like:

"role": "target"
or
"role": "source"
or
"role": "deleting" (For nodes that get deleted via block commit, for
instance. Have a better name? "middle" to indicate it is not the base or
top? "interior" ?)

or any others that we need to grab permissions on.

> I would suggest that we don't introduce a BlockJobInfo, but let the
> individual job types directly inherit JobInfo, and also replace the list
> of devices by named keys:
> 
>     { 'struct': 'JobInfo',
>       'data': {
>         'type': JobTypeEnum,
>         'sys': JobSysEnum,
>         'id': str,
>         'busy': bool,
>         'paused': bool,
>         'ready': bool,
>         'work_total': 'int',
>         'work_completed': 'int'
>       }
>     }
> 
>     { 'struct': 'BlockJobNode',
>       'data': {
>         'node-name': 'str',
>         'io-status': 'BlockDeviceIoStatus',
>         'permissions': 'whatever'
>       }
>     }
> 
>     { 'struct': 'MirrorJobInfo',
>       'base': JobInfo
>       'data': {
>           'source': 'BlockJobNode',
>           'target': 'BlockJobNode',
>           'speed': 'int',
>       }
>     }
> 

Perhaps. I guess it depends on what information we wish to expose to
management applications. This method is very specific to each job, which
increases the parsing effort required by each management application for
each job we add.

My approach might reveal slightly more than we care for, but it will
definitely apply to the broad range of block jobs sufficiently.

We can always subclass the generic BlockJobInfo to include even more
specific information per-job if we need to, as well -- but I believe
we'll need job-specific discriminators instead of subsystem-specific
discriminators.

>> The new command gives a chance to make a clean break and any users of
>> this new command won't be confused about the information they receive.
>>
>> The legacy qmp-query-block-jobs command can continue to operate
>> providing a best-effort approximation of the data it finds. The legacy
>> command will *abort* and return error if it detects any job that was
>> launched by a new-style command, e.g. if it detects there is more than
>> one job attached to any node under a single BlockBackend -- clearly the
>> user has been using new features -- and should abort announcing this
>> command is deprecated.
> 
> I agree with that. As soon as you use the new API once, you should be
> required to use it consistently.
> 
>> If the graph looks reasonably plain, it can report back the information
>> it always has, except that it will now also report the "id" field in
>> addition to be perfectly unambiguous about how to issue commands to this
>> job, like I outlined in the first paragraph of this section.
>>
>>
>> ==== block-job-cancel ====
>>
>> This command can remain as a legacy command. Provided the "device"
>> provided has precisely one job attached, we can allow this command to
>> cancel it. If more complicated configurations are found, abort and
>> encourage the user to use a new API.
>>
>> We can refuse to augment block-job-cancel; forcing users who want to
>> specify IDs to cancel to use the new API.
>>
>> We can add a new "job-cancel" command which removes the block specificity.
>>
>> We can introduce sub-classed arguments as needed for e.g. BlockJob
>> cancellations:
>>
>> { 'command': 'job-cancel',
>>   'data': { 'id': str,
>>             '*force': bool,
>>             'options': JobCancelOpts
>>   }
>> }
>>
>> The idea is that JobCancelOpts would be a subclassable type that can be
>> passed directly to the subsystem's implementation of the cancel
>> operation for further interpretation, so different subsystems can get
>> relevant arguments as needed. We don't currently have any such need for
>> BlockJobCancelOpts, but it can't hurt to be prepared for it.
>>
>> The type can be interpreted through the lens of whichever type the job
>> we've identified expects to see. (Block Jobs expect to see
>> BlockJobCancelOpts, etc.)
> 
> Sounds good, possibly with s/subsystem/individual job type/.
> 
> 'options' should be optional.
> 

Yes.

>> ==== block-job-pause, block-job-resume, block-job-complete ====
>>
>> Same story as above. Introduce job-pause, job-resume and job-complete
>> with subclassible job structures we can use for the block subsystem.
>>
>> Legacy commands continue to operate only as long as the "device"
>> argument is unambiguous to decipher.
> 
> Ack.
> 
>> ==== block-job-set-speed ====
>>
>> This one is interesting since it definitely does apply only to Block Jobs.
> 
> Why? The one non-block job we're envisioning so far, live migration,
> does have its own speed option.
> 
> I think it highly dependent on the job type whether it makes sense.
> 

Fair enough -- It just seemed likely that "speed" was rather
block-specific in our current usage of it, but it /could/ become useful
to other systems.

We can just opt for the generic property setting API anyway.

>> We can augment it and limp it along, allowing e.g.
>>
>> { 'data':
>>   { '*device': 'str',
>>     '*id': 'str',
>>     'speed': 'int'
>>   }
>> }
>>
>> Where we follow the "One of ID or Device but not both nor either"
>> pattern that we've applied in the past.
>>
>> Or, we can say "If there isn't one job per device, reject the command
>> and use the new API"
> 
> The latter would be more consistent, in my opinion.
> 
>> where the new API is:
>>
>> job-set-option :
>>   { 'id': str,
>>     'options': {
>>       '*etc': ...,
>>       '*etc2': ...,
>>     }
>>   }
>>
>> Similar to the other commands, the idea would be to take the subclassed
>> options structure that belongs to that Job's Subclass (e.g.
>> BlockJobOptions) but allow any field inside to be absent.
>>
>> Then we could have commands like this:
>>
>> job-set-option \
>> arguments={ 'id': '#job789',
>>             'options': {
>>               'speed': 10000
>>             }
>> }
>>
>> And so on. These would remain a flexible way of setting any various
>> post-launch options a job would need, and the relevant job-subsystem can
>> be responsible for rejecting certain options, etc.
> 
> That's an interesting thought. I kind of like it, but haven't thought
> through the consequences yet.
> 
>> I believe that covers all the existing jobs interface in the QMP, and
>> that should be sufficiently vague and open-ended enough to behave well
>> with a generic jobs system if we roll one out in the future.
>>
>> Eric, Markus: Please inform me of all the myriad ways my fever dreams
>> for QAPI are wrong. :~)
> 
> Kevin
> 

Thanks for taking a look. You didn't duplicate anything by Eric afaict!
--js



reply via email to

[Prev in Thread] Current Thread [Next in Thread]