Re: [Qemu-devel] [PATCH v2 00/25] qmp: add async command type

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Thu, May 4, 2017 at 11:02 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> I figure everyone interested has had his say.  Let me try to summarize
>> and draw my conclusions.
>>
>> Commands + events can replace asynchronous commands (they did).
>>
>> Asynchronous commands cannot replace events, because "interesting" side
>> effects of commands need to be broadcast.  Doesn't mean asynchronous
>> commands can't make sense for us.  Does mean that we need to address any
>> serious flaws in the existing commands + event mechanism even if we
>> adopt asynchronous commands.  The cover letter lists multiple flaws,
>> which I'll discuss inline.
>>
>> If an operation satisfies certain conditions, then implementing it as
>> asynchronous command provides certain benefits.  If the operation
>>
>> (C1) does not need to broadcast state changes, including start and end
>>      of operation, and
>>
>> (C2) even the initiating client needs no state change notifications
>>      except for end of operation, and
>>
>> (C3) even that isn't needed when the client loses the connection and
>>      reconnects, and
>>
>> (C4) the operation doesn't have to be examined or controlled while it
>>      runs,
>>
>> then making it an asynchronous command
>>
>> (B1) saves you the trouble of defining an event together with its link
>>      to the command, and
>>
>> (B2) saves you the trouble of sending the event, and
>>
>> (B3) saves a bit of QMP traffic, namely the event broadcast.
>>
>> Letting clients opt into the asynchronousness as this series does
>> provides an additional benefit:
>>
>> (B4) when you find one of your synchronous commands is at times taking
>>      too long, you can compatibly evolve it.
>>
>> We can remove (C3) and (C4) by providing suitable ways to examine and
>> control asynchronous commands while they run.
>>
>> We could remove (C1) and (C2) by sending events, but that would also
>> lose pretty much all benefits.
>>
>> The operation chosen to showcase asynchronous commands is taking
>> screenshots.  It obviously satisfies (C1).  I guess for (C2) and (C4) we
>> have to assume that it completes "quickly".  I'm willing to buy that.
>> (C3) seems pretty dubious to me, however.
>>
>> The tradeoff to judge is desirability of the benefits vs. cost to get
>> them.
>>
>> Desirability depends on how many commands satisfy these conditions, and
>> how much trouble exactly is saved.
>>
>> A list of commands that is more complete than just "screendump" would
>> help with the former.  query- commands that need to gather information
>> in ways that could block are among the candidates.
>>
>> On the latter, I figure (B1) dwarves (B2), (B3) is a relatively minor
>> optimization unless proven otherwise, and (B4) is real, but asynchronous
>> commands are not the only way to get it; we can equally well evolve
>> synchronous commands compatibly into synchronous + event.
>>
>> The cost is interface and implementation complexity.
>>
>> Interface complexity increases because we add the concept "asynchronous
>> command" and the concept "synchronous or asynchronous command, client's
>> choice".  This is my main concern.
>>
>
> Since we already have client dispatch of unrelated reply and hidden-async
> pattern, this is less of a concern in practice, most of the complexity has
> to be already handled.

Yes, we already handle the abstract concept of kicking off operations
and getting notified when they change state.  But we handle it in *one*
abstract way: command + event(s).  Adding a second way (asynchronous
commands) can only increase complexity.

> Note it is a bit simpler for a client to handle a generic async return
> rather than various events.

I posit that the complexitity difference between zero and one instances
of command + event far exceeds the one between one and multiple.

Same for asynchronous commands.

The increase in conceptual complexity might be offset by a decrease in
individual operations' complexity.  That's (B1).

Your best estimate of the overall complexity change is more favourable
than mine.  That's okay, you're as entitled to your opinion as I am.
But my duty is to come up with my own best estimate, and act on it.

>> Implementation complexity isn't prohibitive according to diffstat, but
>> it's not neglible, either: with the last six patches peeled off, we get
>> some 500 additional lines plus some 300 in tests/.  The last six patches
>> put the new infrastructure to work, which takes another 200.  Ways to
>> examine and control asynchronous commands while they run would add some
>> more, but we don't know how much exactly.
>>
>> Now let me take a step back: we need to crack the "jobs" problem anyway.
>> Merging this series now would in a way add another special case of the
>> general case "jobs" before adding the general case.  In other words,
>> create more stuff to unify.  I'm afraid that's a bad idea, not least
>> since it's an external interface.  I think we should do "jobs" first.
>> Once we got at least a prototype, we can
>>
>> * judge how onerous defining and implementing jobs really is, i.e. have
>>   a clearer idea of the potential benefits of asynchronous commands, and
>>
>> * explore asynchronous commands as a special case of "jobs" optimized
>>   for operations that satisfy the conditions above, i.e. get a clearer
>>   idea of the *actual* additional implementation complexity.
>>
>
> That's what I have been told for over 1.5y :) I would like to know the list
> of requirements / features "jobs" would have. I am willing to help
> implementing it.

We have multiple use cases for generic jobs infrastructure in various
subsystems, but the ones driving the work are the block subsystem's,
simply because the existing block jobs need a major revamp anyway.  The
people most familiar with the requirements are John Snow, Kevin Wolf,
Stefan Hajnoczi, and to a lesser degree myself.  Recommend to meet John
in front of a big whiteboard, or else the next best way to talk things
over you two can find.

>> Bottom line:
>>
>> 1. I still don't want to merge this.
>>
>> 2. I want us to tackle jobs sooner rather than later.
>>
>> 3. Once we got at least a jobs prototype, I'm willing to reconsider
>>    asynchronous commands implemented as special case of jobs.
>>
>>
> Who is going to work on it?

I hope John can work on jobs.

>> One of the most important maintainer duties is saying "no".  It's also
>> one of the least fun duties.
>>
>>
> It doesn't sound like a "no" :)

Well, it's as close to "no" as you can get in a situation that's going
to change!  It's a straight "no" for the current situation, and a
"maybe" for a future situation that will be different in ways we don't
yet understand.

I'm *not* encouraging you to pursue asynchronous QMP commands.  In fact,
I never did, at least not knowingly.  It's your decision.  All I offer
is what I offer to all patch submitters: my best effort at a fair
hearing.