On Mon, Jul 16, 2018 at 07:18:28PM +0200, Markus Armbruster wrote: > Peter Xu <pet...@redhat.com> writes: > > > Based-on: <20180703085358.13941-1-arm...@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
