Re: QMP and the 'id' parameter

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 11/20/20 5:25 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>> On 11/11/20 3:27 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>> On 11/10/20 1:22 AM, Markus Armbruster wrote:
>>>>>> John Snow <jsnow@redhat.com> writes:
>
>> 
>> If you find yourself in a situation where an error class would
>> definitely be useful, we should talk about providing you one.
>> 
>
> OK, thanks. I just wanted to check there wasn't a stronger opposition 
> than I was aware of. I only vaguely knew they were "on the way out."
>
>>>> QGA has a special case, but you probably don't want to know.
>>>
>>> I do want to know, unfortunately. A good QMP client should likely
>>> support both targets.
>> 
>> QGA has a few commands that send *nothing* on success.
>> 
>> qapi-code-gen.txt explains:
>> 
>>      Normally, the QAPI schema is used to describe synchronous exchanges,
>>      where a response is expected.  But in some cases, the action of a
>>      command is expected to change state in a way that a successful
>>      response is not possible (although the command will still return an
>>      error object on failure).  When a successful reply is not possible,
>>      the command definition includes the optional member 'success-response'
>>      with boolean value false.  So far, only QGA makes use of this member.
>> 
>> qmp-spec.txt neglects to cover this special case.
>> 
>
> Oh, I see. That's fun!
>
> (Was it not possible to have the client send an ACK response that 
> doesn't indicate success, but just signals receipt? Semantically, it 
> would be the difference between do-x and start-x as a command.)

Feels quite possible to me.

If I read git-log correctly, the commands' design ignored the race with
shutdown / suspend, and 'success-response': false was a quick fix.

I think changing the commands from 'do-x' to 'start-x' would have been
better.  A bit more work, though, and back then there weren't any
'start-x' examples to follow, I guess.

I wish success-response didn't exist, but is getting rid of it worth
changing the interface?  I honestly don't know.

>>>> Even though counting responses is a workable way to map responses back
>>>> to requests, I recommend to either have at most one request in flight
>>>> (so no counting is necessary), or to add IDs to all requests (so
>>>> counting isn't necessary as long as you get the syntax right enough to
>>>> avoid the ID-less errors discussed above).
>>>
>>> I am taking this approach. I still need some error handling for the
>>> case that an incoming response cannot be routed to the correct pending
>>> queue.
>>>
>>> The QMP spec states that if I get an ID and I don't recognize it, I am
>>> free to drop the response on the floor (So I have done so),
>> 
>> Should not happen.  When it does, something is wrong, possibly seriously
>> wrong.
>
> Yep, I agree.
>
> (In my client library: I have decided to enforce string IDs and not 
> allow the caller to specify their own. Though we allow arbitrary objects 
> to be used as an ID, being able to compare more complex objects seems 
> more prone to failure than a simple string.)

Makes sense.

>> One possible scenario: ID got corrupted on the way from client via
>> server back to the client.  The response with the uncorrupted ID will
>> never come.  Other responses and events may still arrive fine.
>> 
>> Two recovery strategies:
>> 
>> 1. Drop the unexpected response, carry on.  If commands are in flight,
>>     be prepared for missing responses.
>> 
>> 2. Give up, close connection.
>> 
>> Pick the strategy that best fits your client's needs.
>
> Dropping it on the floor seems fine for now, the spec says I can! :)
>
> The most realistic scenario for this outside of catastrophic error is 
> that a client sends a command, but then times out waiting for a 
> response. Later, the response arrives -- but that context has already 
> come and gone, so there's nowhere to route the message to.
>
> Effectively, an orphaned reply. I suppose I could always stash them in 
> an orphaned queue and a very, very rigorous client could check the 
> orphaned queue if something winds up in there, but I suspect most 
> clients would actually be just as happy to let it fall on the floor.

Sounds okay to me.

>>>                                                              but I need
>>> to handle the case that I see a response with no ID at all.
>> 
>> Should not happen as long as the client sends syntactically sane input.
>> When it does, something is wrong, possibly seriously wrong.
>> 
>>> Perhaps the easiest, and most sensible thing to do, is to declare this
>>> a catastrophic failure and move the QMP connection into an error
>>> state, invalidate the whole queue, and disconnect.
>>>
>>> (And then maybe some of the commands you issued got processed, maybe
>>> they didn't. I guess it's up to the application driving the library to
>>> query around to find out what happened.)
>> 
>> Again, two recovery strategies:
>> 
>> 1. Drop the unexpected response, carry on.  If commands are in flight,
>>     be prepared for missing responses.  The server's JSON parser may now
>>     be in a state where it can't parse additional commands.  You may want
>>     to provoke a lexical error to force it into known-good state, as
>>     explained below.
>> 
>> 2. Give up, close connection.
>> 
>
> 2 is easier for now. 1 involves some more advanced recovery logic, but 
> as you say: something is wrong, possibly seriously wrong.

Taking the easy way out sounds okay to me.  Build more advanced recovery
when you *know* you need it.

>>> I see. So OOB commands don't execute in parallel, and when they "jump
>>> the queue", further executions are not received. Right?
>> 
>> Right.  It's "out of band", not "in parallel".
>
> OK. Naively I thought that new OOB commands would continue to be 
> processed "out of band", but it's fine if that capacity is finite.

The out-of-band feature is not meant to compete with jobs.  It's a
narrow solution to a specific problem: robust postcopy migration
recovery needs to get in certain commands reliably and quickly, even
previously sent commands hog the monitor.  A second use has since come
up: a 'yank' command to recover from hung network connections by
shutting them down.

>>> Conceptually, then: There's one input queue and two pending execution
>>> slots. One slot is for OOB commands, and the other slot is for
>>> traditional commands.
>>>
>>> The input queue is FIFO, and when we pull a request from the queue, it
>>> occupies one of the conceptual slots (normal/oob). If the slot for
>>> this command type is already occupied, we block until it's free.
>>>
>>> Is that the correct mental model? (Even if it's not really even
>>> vaguely correct, details-wise.)
>> 
>> Almost.
>> 
>> Consider
>> 
>>      - Send execute A
>>      - Send execute B
>>      - Send exec-oob C
>>      - Send execute D
>> 
>
> [...]
>
>> The I/O thread receives in-band A, and puts it into the empty in-band
>> queue.
>> 
>> The I/O thread receives in-band B, and puts it into the non-full in-band
>> queue.
>> 
>> The I/O thread receives out-of-band C, and exectutes it right away.
>> 
>
> Ah, I see where I went wrong. OK, it's crystal clear now.
>
>> The I/O thread receives in-band C, and puts it into the non-full in-band
>> queue.
>> 
>> Meanwhile, the main thread takes in-band A out of the queue, and starts
>> executing it.  A takes its own sweet time.
>> 
>> Only if the client has more in-band commands in flight than the in-band
>> queue can hold, the I/O thread encounters a full queue and suspends
>> reading.
>> 
>> qmp-spec.txt:
>> 
>>      If the client sends in-band commands faster than the server can
>>      execute them, the server will stop reading the requests from the QMP
>>      channel until the request queue length is reduced to an acceptable
>>      range.
>> 
>> The queue length should probably be specified here.  It's 8.
>> 
>
> Oh! That's very helpful to know, actually. If you write a patch to amend 
> the language of the doc, I will give you my R-B.

Added to my queue.

>> The server limits the in-band request queue length for the same
>> robustness reasons it limits JSON token length, token count and nesting
>> depth: to not let a malfunctioning client make it consume unlimited
>> amounts of memory.  Hole (kind of): it doesn't limit output queue
>> length.
>> 
>
> Understood!
>
>>> Is there a reason to not always use \xFF ?
>> 
>> It's invalid UTF-8.
>> 
>> "Older versions" should probably read "ancient versions" by now.  I
>> don't remember offhand which commit fixed the JSON parser to do the
>> right thing in all states.
>> 
>
> I suppose right now I don't have any code that tries to unjam the 
> parser, so I suppose it doesn't matter.
>
> If it did, I guess I would just try to assume that I was allowed to use 
> the newer method and too-bad to folks trying to talk to ancient stuff.

Right.

>>>>>> Questions?
>>>>> A few :)
>>>> More?
>>> None of tremendous import, I think, by now. Maybe the GQA stuff, but
>>> that can be later.
>> Still more?
>
> Where does matter come from? If energy cannot be created or destroyed, 
> how did it come to be?

"D'où venons-nous?  Que sommes-nous?  Où allons-nous?"

;)