Re: [PATCH RFC 0/7] RFC: Asynchronous QMP Draft

[Stefan Hajnoczi]

On Wed, Apr 14, 2021 at 03:17:48PM -0400, John Snow wrote:
> First and foremost, thank you for reviewing this! It is very helpful to me
> to see what others think of this pet project I've been growing in the
> privacy of my own mind.
> 
> On 4/14/21 2:38 AM, Stefan Hajnoczi wrote:
> > Below are the API docs that I found helpful for understanding the big
> > picture.
> > 
> > The QMP.execute() API is nice.
> > 
> 
> Yes. It mimics (sync) qmp.command(), which I believe Eduardo Habkost wrote.
> I think it's the correct idea for a generic (QAPI-schema ignorant) QMP
> client library meant to be "used".
> 
> I think raising RPC in-band execution errors as exceptions is a nice
> "pythonic" way to do it.
> 
> (And, if desired, it is possible to use the QAPI generator to generate
> wrappers around this interface using type-safe arguments in a low-level SDK
> layer. I think that would be pretty swell. We are not there yet, though, and
> I'll focus on this layer first.)
> 
> > Regarding QMP events, I can think of two approaches:
> > 1. Callbacks
> > 2. An async get_event(name=Optional[str]) -> object API
> >     (plus get_event_nowait(name=Optional[str]) -> object)
> > 
> > (There's probably a third approach using async iterators but it's
> > similar to get_event().)
> > 
> > Both approaches are useful. The first is good in larger asynchronous
> > applications that perform many tasks concurrently. The second is good
> > when there is just one specific thing to do, like waiting for a block
> > job to complete.
> > 
> (1) On callbacks:
> 
> Callbacks are what I meagerly mocked up; discord.py has a "cute" little hack
> that works like this:
> 
> bot = commands.bot(...)
> 
> @bot.event
> async def on_ready():
>     print("Logged in as")
>     print(bot.user.name)
>     ...
> 
> (See https://github.com/Rapptz/discord.py/blob/master/examples/basic_bot.py
> )
> 
> I find this to be extremely cute: the framework uses the name of the
> callback to determine which event you are registering, and uses the
> decorator to merely register the callback.
> 
> This makes a nice, uncomplicated way to plug coroutines into the state
> machine of the client loop in the most basic cases.
> 
> I thought it might be nice to try and mimic that design, by perhaps using
> the names of QMP events as their own 'queues', and then dispatching user
> callbacks as desired. (Possibly with one mega-queue that exists for ALL
> callbacks.)
> 
> For instance, something like this:
> 
> @qmp.event
> async def job_status_block_job_ready(qmp, event):
>     ...
> 
> or more generally,
> 
> @qmp.event_handler
> async def my_own_event_handler(qmp, event):
>     ...
> 
> I didn't spend much time on the actual queue or dispatch mechanism in my
> draft, though, but it could be "bolstered" into a more full-fledged API if
> desired.
> 
> One nice thing about this design is that events aren't "consumed" by a
> caller, they are just dispatched to anyone waiting on an event of that type.
> 
> As I recall, events getting "eaten" at the wrong time was a major burden
> when writing iotests that exercised multiple jobs, transactions, etc.
> 
> (A side note: a higher-level VM class that uses QMP may wish to capture
> certain events to record state changes, such that the state can be queried
> at an arbitrary point by any number of callers without needing to have
> witnessed the state change event personally. That's not really important
> here in the protocol library, though, which will pretend not to know which
> events exist -- but it's a consideration for making sure the design that IS
> chosen can be extensible to support that kind of thing.)
> 
> 
> (2) On get_event or async iterators:
> 
> This is likely a good ad-hoc feature. Should it only work for events that
> are delivered from that moment in time, or should there be a "backlog" of
> events to deliver?
> 
> Should waiting on events in this manner "consume" the event from the
> backlog, if we have one?
> 
> My concern::
> 
>   await qmp.execute('blockdev-backup', {...etc...})
>   async for event in qmp.get_events():
>       ...
> 
> 
> It's possible that an event we'd like to see has already occurred by the
> time we get around to invoking the async iterator. You'd really want to
> start checking for events *before* you issue the job request, but that
> involves tasks, and the code doesn't "flow" well anymore.
> 
> I don't have ideas, at-present, for how to make things like iotests "flow"
> well in a linear co-routine sense...
> 
> ...although, maybe it's worth creating something like an Event Listener
> object that, from its creation, stashes events from that point forward. How
> about::
> 
>   async with qmp.event_listener() as events:
>       await qmp.execute('blockdev-backup', {...})
>       async for event in events:
>           ...
> 
> Actually, that seems pretty cool. What do you think? I think it's fairly
> elegant for ad-hoc use. We could even extend the constructor to accept
> filtering criteria if we wanted to, later.

Yeah, it seems very nice for allowing multiple event listeners that
don't steal each other's events. I like it.

qmp.event_listener() could take a sequence of QMP event names to trigger
on. If the sequence is empty then all QMP events will be reported.

> 
> Possibly we could also augment the Event Listener object to support a few
> methods to facilitate blocking until a certain event occurs, like::
> 
>   async with qmp.event_listener() as events:
>       await qmp.execute('blockdev-backup', {...})
>       await events.event('JOB_STATUS_CHANGE', status="pending")
>       await qmp.execute('job-finalize', {...})
>       ...
> 
> 
> I think that's pretty workable, actually! And it could co-exist perfectly
> well alongside event callback handlers.

Callbacks and async iterators are equivalent since a callback is
basically a Task with an event_listener() loop that invokes the callback
function. If the boilerplate for setting that up is minimal then there
might be no need to provide both interfaces.

> Pydantic models are definitely optional at this stage, but I am floating
> them here to prepare people for the idea that I might try to get more
> mileage out of them in the future to offer a type-safe, QAPI-aware SDK
> layer.
> 
> They're definitely only a mild benefit here, for now, as the strict typing
> they help provide is not floated upwards or exposed to the user.

Yes, I can see the benefits for programs that need a lot of data
validation and have complex schemas.

Since this library is oblivious to the QMP schema it's probably not
needed.

An example of why I suggested dropping pydantic: I was trying to figure
out what happens if QMP is extended with new response fields. Will
pydantic raise an exception when it encounters an unexpected field? It's
not obvious from the code so I needed to go study pydantic to find the
answer.

> (4) On combining protocol and qmp_protocol:
> 
> Maybe. Do you want to look at the qtest implementation? It's somewhat
> ancillary to this project, but felt it would make a nice companion library.
> It doesn't benefit as strongly as QMP (As it does not offer anything like
> OOB), but it does have async messages it can send, so it can re-use the same
> infrastructure.
> 
> (Fully admit that the first draft, of course, did feature a combined
> protocol/qmp_protocol class. It was split out later.)

Sure, it would be interesting to see the qtest code.

> > Things that might be worth adding:
> > 1. File descriptor passing support.
> 
> Do you have an example workflow that I can use to test this? This is a weak
> spot in my knowledge.

The add-fd QMP command. I guess this patch series cannot execute that
command successfully since it doesn't support fd passing.

It should be easy to do:

  qmp.execute('add-fd', pass_fds=[fobj])

where pass_fds is an optional sequence of file descriptors. The file
descriptors can either be int or file-like objects that support the
fileno() method.

I'm not sure if QMP commands also send file descriptors back to the
client in responses.

> > 2. Introspection support to easily check if a command/feature is
> >     available. Users can do this manually by sending QMP commands and
> >     interpreting the response, but this may be common enough to warrant a
> >     friendly API.
> > 
> 
> I think this treads into QAPI-specific domain knowledge, and I might leave
> such features to a higher-level object.
> 
> The QMP spec itself does not define a mechanism by which the QMP protocol
> itself will reveal the valid commands, and so it might be up to a
> machine.py-based extension/capsulation of qmp_protocol to provide that.
> 
> (Though, I do agree; I want this feature somewhere. We do have such a thing
> coded into the existing qmp-shell tool, using the query-commands command.
> Maybe I can offer a subclass that offers some of these convenience features
> using a best-effort guess-and-check style introspection. Please forgive me
> if I focus on shoring up the design of the core implementation first.)

Okay.

> In general, do you feel this design is roughly serviceable and worth
> pursuing cleanups for? I realize it's a bit "much" but as the audience
> extends beyond our castle walls, I wanted to be quite thorough. It's a

I see the complexity mostly as accidental complexity, not essential
complexity. IMO it's not that the current approach is overkill now but
could be necessary later. I think it will always be unnecessarily
complex because there are simpler ways to do it :D.

Stefan

signature.asc
Description: PGP signature