Re: [Qemu-devel] Dropping the MONITOR_CMD_ASYNC

[Alon Levy]

On Mon, Dec 12, 2011 at 10:24:53AM -0600, Anthony Liguori wrote:
> On 12/12/2011 10:00 AM, Alon Levy wrote:
> >On Mon, Dec 12, 2011 at 03:23:35PM +0000, Stefan Hajnoczi wrote:
> >>On Mon, Dec 12, 2011 at 12:39 PM, Gerd Hoffmann<address@hidden>  wrote:
> >>>On 12/12/11 13:10, Stefan Hajnoczi wrote:
> >>>>On Mon, Dec 12, 2011 at 11:29 AM, Gerd Hoffmann<address@hidden>  wrote:
> >>>>>On 12/12/11 11:18, Stefan Hajnoczi wrote:
> >>>>>>On Sun, Dec 11, 2011 at 10:29 AM, Alon Levy<address@hidden>  wrote:
> >>>>>>>On Thu, Dec 08, 2011 at 05:45:44PM -0200, Luiz Capitulino wrote:
> >>>>>>>>Hi there,
> >>>>>>>>
> >>>>>>>>I'm about to completely drop the MONITOR_CMD_ASYNC API, but it turns 
> >>>>>>>>out that
> >>>>>>>>the command client_migrate_info uses it. That's a legacy interface 
> >>>>>>>>and has to
> >>>>>>>>be dropped, no command should be using it...
> >>>>>>>>
> >>>>>>>>Something tells me that if I just drop it (and change the command to 
> >>>>>>>>use the
> >>>>>>>>regular interface), bad things will happen. Am I right? :)
> >>>>>>>>
> >>>>>>>
> >>>>>>>The monitor command client_migrate_info needs to complete after getting
> >>>>>>>an ACK message from the currently connected spice client (this is the
> >>>>>>>only case where this is required - if there is no client then the
> >>>>>>>MONITOR_CMD_ASYNC API won't be used). This in turn requires the main
> >>>>>>>thread to perform select and call the callback that will process this
> >>>>>>>ACK. That's why the MONITOR_CMD_ASYNC API was used.
> >>>>>>>
> >>>>>>>I'm not aware of any other way to do this, I'll be glad for any help
> >>>>>>>here. Also, I understand this is not what is not true async, since one
> >>>>>>>would expect a true async interface to support multiple in flight
> >>>>>>>monitor commands. If there is any ETA or existing way to do this we
> >>>>>>>could change the implementation of client_migrate_info.
> >>>>>>
> >>>>>>Is it possible to use a QMP event to signal completion instead of a
> >>>>>>MONITOR_CMD_ASYNC command?
> >>>>>
> >>>>>Problem is this breaks the qemu<->  libvirt interface.
> >>>>
> >>>>I had the same issue with the block_job_cancel command, which Adam
> >>>>Litke and Eric Blake helped us fix and find a backward-compatible
> >>>>libvirt solution for:
> >>>>
> >>>>http://www.redhat.com/archives/libvir-list/2011-November/msg01351.html
> >>>
> >>>Isn't going to fly as waiting for completion isn't optional in that
> >>>case.  Workflow is this:
> >>>
> >>>(1) libvirt issues client_migrate_info command.
> >>>(2) qemu forwards it to spice-server, which in turn forwards it to
> >>>    the spice client (if connected).
> >>>(3) spice client connects to the migration target machine.
> >>>(4) spice client signals completion to spice-server, which in turn
> >>>    notifies qemu.
> >>>(5) qemu calls the monitor completion callback, libvirt gets
> >>>    client_migrate_info result.
> >>>(6) libvirt issues migrate command.
> >>>
> >>>The problem is that (3) must be finished before (6) because qemu on the
> >>>target side doesn't accept incoming tcp connections any more once the
> >>>migration started.
> >>>
> >>>I don't see a way to switch this to qmp events without breaking old
> >>>libvirt versions managing new qemu.
> >>
> >>I don't see a solution in this case either.  Looks like libvirt
> >>supports this command since 0.9.2 so it's not a good idea to just yank
> >>it.
> >>
> >>It might be possible for the QEMU client_migrate_info handler to run a
> >>nested event loop in the legacy libvirt case.  This would suck since
> >>the VM is unresponsive while waiting for spice migration to complete.
> >>New libvirt would call the async version of the command which is
> >>well-behaved and uses a QMP event to signal completion.
> >
> >I agree that a nested event loop would be a bad solution, the point is
> >to let the guest continue to work while waiting, otherwise you destroy
> >the live migration experience, might as well disconnect the client from
> >the source and have it connect to the target.
> >
> >I thought the whole point of MONITOR_CMD_ASYNC was to allow this
> >scenario. So iiuc QMP is the alternative but it would require a rewrite,
> >i.e. break existing users like libvirt. Hence my suggestion as a reply
> >to Luiz's initial email that we just deprecate MONITOR_CMD_ASYNC right
> >now instead of dropping it,
> 
> It *has* to be dropped.  Any command using it is fundamentally broken.
> 

Care to explain what is fundamental about it?

> The command should have never been introduced in the first place to use async.

Gerd gave a very good explanation of the requirement, I don't think I
can add anything to it. What you suggest is to rewrite it, I just don't
understand why we can't leave the MONITOR_CMD_ASYNC around until there
is both an alternative and libvirt learns to use it.

> 
> Regards,
> 
> Anthony Liguori
>