Re: [Qemu-devel] [PATCH v5 00/21] blockjobs: add explicit job management

[John Snow]

On 04/18/2018 03:25 AM, Markus Armbruster wrote:
> John Snow <address@hidden> writes:
> 
>> On 04/17/2018 09:44 AM, Markus Armbruster wrote:
>>> John Snow <address@hidden> writes:
>>>
>>>> This series seeks to address two distinct but closely related issues
>>>> concerning the job management API.
>>>>
>>>> (1) For jobs that complete when a monitor is not attached and receiving
>>>>     events or notifications, there's no way to discern the job's final
>>>>     return code. Jobs must remain in the query list until dismissed
>>>>     for reliable management.
>>>>
>>>> (2) Jobs that change the block graph structure at an indeterminate point
>>>>     after the job starts compete with the management layer that relies
>>>>     on that graph structure to issue meaningful commands.
>>>>
>>>>     This structure should change only at the behest of the management
>>>>     API, and not asynchronously at unknown points in time. Before a job
>>>>     issues such changes, it must rely on explicit and synchronous
>>>>     confirmation from the management API.
>>>>
>>>> These changes are implemented by formalizing a State Transition Machine
>>>> for the BlockJob subsystem.
>>>>
>>>> Job States:
>>>>
>>>> UNDEFINED       Default state. Internal state only.
>>>> CREATED         Job has been created
>>>> RUNNING         Job has been started and is running
>>>> PAUSED          Job is not ready and has been paused
>>>> READY           Job is ready and is running
>>>> STANDBY         Job is ready and is paused
>>>>
>>>> WAITING         Job is waiting on peers in transaction
>>>> PENDING         Job is waiting on ACK from QMP
>>>> ABORTING        Job is aborting or has been cancelled
>>>> CONCLUDED       Job has finished and has a retcode available
>>>> NULL            Job is being dismantled. Internal state only.
>>>>
>>>> Job Verbs:
>>>>
>>
>> Backporting your quote up here:
>>
>>> For each job verb and job state: what's the new job state?
>>>
>>
>> That's not always 1:1, though I tried to address it in the commit messages.
> 
> Let me rephrase my question then.  For each job verb and job state: what
> are the possible new job states?  If there's more than one, what's the
> condition for each?
> 

Is my answer below not sufficient? Maybe you're asking "Can you write
this up in a formal document" instead, or did I miss explaining something?

> I appreciate commit messages explaining that, but having complete state
> machine documentation in one place (a comment or in docs/) would be
> nice, wouldn't it?
> 
>>>> CANCEL          Instructs a running job to terminate with error,
>>>>                 (Except when that job is READY, which produces no error.)
>>
>> CANCEL will take a job to either NULL... (this is the early abort
>> pathway, prior to the job being fully realized.)
>>
>> ...or to ABORTING (from CREATED once it has fully realized the job, or
>> from RUNNING, READY, WAITING, or PENDING.)
>>
>>>> PAUSE           Request a job to pause.
>>
>> issued to RUNNING or READY, transitions to PAUSED or STANDBY respectively.
>>
>>>> RESUME          Request a job to resume from a pause.
>>
>> issued to PAUSED or STANDBY, transitions to RUNNING or READY respectively.
>>
>>>> SET-SPEED       Change the speed limiting parameter of a job.
>>
>> No run state change.
>>
>>>> COMPLETE        Ask a READY job to finish and exit.
>>>>
>>
>> Issued to a READY job, transitions to WAITING.
>>
>>>> FINALIZE        Ask a PENDING job to perform its graph finalization.
>>
>> Issued to a PENDING job, transitions to CONCLUDED.
>>
>>>> DISMISS         Finish cleaning up an empty job.
>>>
>>
>> Issued to a CONCLUDED job, transitions to NULL.
>>
>>
>>>> And here's my stab at a diagram:
>>>>
>>>>                  +---------+
>>>>                  |UNDEFINED|
>>>>                  +--+------+
>>>>                     |
>>>>                  +--v----+
>>>>        +---------+CREATED+-----------------+
>>>>        |         +--+----+                 |
>>>>        |            |                      |
>>>>        |         +--+----+     +------+    |
>>>>        +---------+RUNNING<----->PAUSED|    |
>>>>        |         +--+-+--+     +------+    |
>>>>        |            | |                    |
>>>>        |            | +------------------+ |
>>>>        |            |                    | |
>>>>        |         +--v--+       +-------+ | |
>>>>        +---------+READY<------->STANDBY| | |
>>>>        |         +--+--+       +-------+ | |
>>>>        |            |                    | |
>>>>        |         +--v----+               | |
>>>>        +---------+WAITING<---------------+ |
>>>>        |         +--+----+                 |
>>>>        |            |                      |
>>>>        |         +--v----+                 |
>>>>        +---------+PENDING|                 |
>>>>        |         +--+----+                 |
>>>>        |            |                      |
>>>>     +--v-----+   +--v------+               |
>>>>     |ABORTING+--->CONCLUDED|               |
>>>>     +--------+   +--+------+               |
>>>>                     |                      |
>>>>                  +--v-+                    |
>>>>                  |NULL<--------------------+
>>>>                  +----+
>>>
>>> Is this diagram missing a few arrowheads?  E.g. on the edge between
>>> RUNNING and WAITING.
>>>
>>
>> Apparently yes. :\
>>
>> (Secretly fixed up in my reply.)
>>
>>> Might push the limits of ASCII art, but here goes anyway: can we label
>>> the arrows with job verbs?
>>>
>>
>> Can you recommend a tool to help me do that? I've been using asciiflow
>> infinity (http://asciiflow.com) and it's not very good, but I don't have
>> anything better.
> 
> I do my ASCII art in Emacs picture-mode.
> 
>>> Can you briefly explain how this state machine addresses (1) and (2)?
>>>
>>
>> (1) The CONCLUDED state allows jobs to persist in the job query list
>> after they would have disappeared in 2.11-era QEMU. This lets us query
>> for completion codes and to dismiss the job at our own leisure.
> 
> Got it.
> 
>> (2) The PENDING state allows jobs to wait in a nearly-completed state,
>> pending authorization from the QMP client to make graph changes.
>> Otherwise, the job has to asynchronously perform this cleanup and the
>> exact point in time is unknowable to the QMP client. By making a PENDING
>> state and a finalize callback (.prepare), we can make this portion of a
>> job's task synchronous.
> 
> This provides for jobs modifying the graph on job completion.  It
> doesn't provide for jobs modifying the graph while they run.  Fine with
> me; we're not aware of a use for messing with the graph in the middle of
> a job.
> 

I didn't consider this possibility. The concept could in theory be
expanded to arbitrary sync points, but I'm not going to worry about that
until the need arises.

>> "John, you added more than two states..."
>>
>> Yup, this was to help simplify the existing state machine, believe it or
>> not. I modeled all jobs as transactions to eliminate different cleanup
>> routing and added two new interim states;
>>
>> - WAITING
>> - ABORTING
>>
>> to help make assertions about the valid transitions jobs can make. The
>> ABORTING state helps make it clear when a job is allowed to fail (and
>> emit QMP events related to such).
>>
>> The WAITING state is simply advisory to help a client know that a job is
>> "finished" but cannot yet receive further instruction because of peers
>> in a transaction. This helps me to add nice QMP errors for any verbs
>> issued to such jobs. "Sorry pal, this job is waiting and can't hear you
>> right now!"
>>
>> This kept the code cleaner than adding a bunch of very fragile boolean
>> error-checking pathways in dozens of helper functions to help avoid
>> illegal instructions on jobs not prepared to receive those instructions.
>>
>> So these two new states don't help accomplish (1) or (2) strictly, but
>> they do facilitate the code additions that _do_ a lot less ugly.
> 

I really bungled that sentence.

> Thanks!
> 
> Looks like a fine starting point for in-tree state machine documentation
> :)
>