On Mon, Jul 16, 2018 at 07:18:28PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > Based-on: <20180703085358.13941-1-armbru@redhat.com>
>
> Now in master.
>
> > This work is based on Markus's latest out-of-band fixes:
> > "[PATCH v2 00/32] ] qmp: Fixes and cleanups around OOB commands"
> >
> > Major stuff: some cleanups that were previously suggested by Markus or
> > Eric. Meanwhile fix up the flow control issue. Since my proposal
> > here is to drop COMMAND_DROPPED event, then we don't need to introduce
> > per-monitor event emission API any more. Though Markus told me that
> > he might use that code somewhere else, so I'll post that per-monitor
> > event code out separately as RFC later.
> >
> > Patch 1-3: cleanups.
>
> I expect these to be ready in the next version.
>
> Since it's just cleanup, there's no need to rush these into 3.0 unless
> they enable something we want in 3.0.
>
> > Patch 4-7: solve the flow control issue reported by Markus that
> > response queue might overflow even if we have limitation on the
> > request queue. Firstly we drop the COMMAND_DROP event since that
> > won't work for response queue (since queuing an event will need to
> > append to the response queue itself), then we use monitor suspend and
> > resume to control the flow. Last, we apply that to response queue
> > too.
>
> These need work. We need to agree on how exactly flow control should
> work. Moreover, we need to reconcile your work with Marc-André's
> "[PATCH 00/12] RFC: monitor: various code simplification and fixes"
> (which I haven't fully reviewed yet).
Sure.
>
> > Patch 8-9: the original patches to enable out-of-band by default.
>
> I figure these patches are good; we just need to decide whether we're
> ready to enable OOB. Let's review the known issues.
>
> * We might want to make "id" mandatory with exec-oob
>
> Best to do that right in the first release that fully supports OOB.
Yeah, I'd say I would prefer it that way, but after all we're having
that optional now in master, so it's fine for me in either way.
>
> * OOB offered but rejected by client is not obviously the same as
> OOB not offered
>
> I still need to digest and reply to your
> Message-ID: <20180629090115.GH2455@xz-mi>
As a summary of the reply - I think the only difference is where we
run the chardev IOs for the monitor:
- When OOB not offered: we run chardev IOs in main thread
- When OOB offered but client rejected: we run chardev IOs in iothread
The rest of the things should be all the same.
>
> * COMMAND_DROPPED is broken by design, and
> * Flow control limits only request queue; response buffer can still
> grow without bounds
>
> You proposed to drop COMMAND_DROPPED, and do flow control by corking
> input, see above.
>
> Getting rid of broken COMMAND_DROPPED is a blocker. Fully functional
> flow control isn't, since the QMP client is trusted.
>
> * We lack test coverage for flow control
> * test-qmp-cmds neglects to cover the OOB additions to qmp-dispatch.c
>
> I'm inclined to treating lack of test coverage as a blocker.
I will look at this one before repost after 3.0.
>
> * scripts/qmp/ doesn't support OOB, yet. qmp-shell.py in particular.
>
> Not a blocker.
I'll possibly temporarily put this one aside. If not, I'll leave a
FIXME there (or I'll just do).
>
> Whatever we don't address right away we should at least mark FIXME in
> the source code.
>
> Assuming my list is complete, and my assessments correct, then we're
> quite close to the point where we can enable OOB. But since rc1 is
> tomorrow already, I feel enabling it in 3.0 has become unrealistic. I
> understand we need it sooner rather than later for postcopy recovery.
> However, the current state should be servicable for teaching OOB to
> libvirt: just follow the recommendation to supply "id" (libvirt always
> does anyway), pretend COMMAND_DROPPED doesn't exist, and pretend flow
> control isn't an issue.
I guess the thing is not about COMMAND_DROPPED; it's about we're going
to drop the x-oob parameter. Now we can only use that parameter to
enable out-of-band, and after we enable it by default we possibly want
to remove that x-oob parameter. So we'd better provide a constant way
for libvirt to enable out-of-band first (and now I'll assume the
"exec-oob" interface won't change again). But yes I think we can do
that after 3.0.
Thanks,
--
Peter Xu