[libvirt] qemu: Add timeout for monitor to avoid virsh getting stuck when monitor gets die.

[Daniel P. Berrange]

On Wed, Apr 06, 2011 at 12:13:10PM +0800, Mark Wu wrote:
> Hello Guys,
> 
> When the qemu process becomes hung,  virsh will get stuck on the
> hung guest and can't move forward.  It can be reproduced by the
> following steps:
> 
> 1. setup a virt guest with qemu-kvm, and start it
> 2. stop qemu process with following:
>  kill -STOP  `ps aux | grep qemu | grep -v grep | awk '{print $2}'`
> 3. run the following command:
> virsh list
> 
> I think we can add a timeout for qemu monitor to resolve this
> problem: using virCondWaitUntil instead of virCondWait in
> qemuMonitorSend. What's your opinions?
> 
> Thanks!
> 

> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index fca8590..65d8de9 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -25,6 +25,7 @@
>  
>  #include <poll.h>
>  #include <sys/un.h>
> +#include <sys/time.h>
>  #include <unistd.h>
>  #include <fcntl.h>
>  
> @@ -691,11 +692,14 @@ int qemuMonitorClose(qemuMonitorPtr mon)
>      return refs;
>  }
>  
> +#define QEMU_JOB_WAIT_TIME (1000ull * 30)
>  
>  int qemuMonitorSend(qemuMonitorPtr mon,
>                      qemuMonitorMessagePtr msg)
>  {
>      int ret = -1;
> +    struct timeval now;
> +    unsigned long long then;
>  
>      if (mon->eofcb) {
>          msg->lastErrno = EIO;
> @@ -706,7 +710,14 @@ int qemuMonitorSend(qemuMonitorPtr mon,
>      qemuMonitorUpdateWatch(mon);
>  
>      while (!mon->msg->finished) {
> -        if (virCondWait(&mon->notify, &mon->lock) < 0)
> +       if (gettimeofday(&now, NULL) < 0) {
> +               virReportSystemError(errno, "%s",
> +                             _("cannot get time of day"));
> +               return -1;
> +       }
> +       then = (now.tv_sec * 1000ull) + (now.tv_usec / 1000);
> +       then += QEMU_JOB_WAIT_TIME;
> +        if (virCondWaitUntil(&mon->notify, &mon->lock, then) < 0)
>              goto cleanup;
>      }

This may seem simple, but it has a lot of nasty consequences.

Adding the timeout causes the thread to stop waiting for the
monitor command reply, and returns an error for that API call.

If QEMU should recover though, and more API calls are made
which issue monitor commands, all those future commands will
receive the wrong reply data.

If we are going to allow a timeout when waiting for a reply
to a monitor command, then we need to mark the monitor as
'broken' and forbid all future use of it until the VM is
restarted.

If it was a simple 'info' command, then we could potentially
add code to read+discard the delayed reply later.

If it was an action command though, we can't simply discard
the delayed reply, because that will result in libvirt's
view of the world becoming out of sync with QEMU. In certain
cases we may be able to cope with this, by listening for
event notifications.

eg, if 'stop' command times out, and libvirt will thing the
VM is still running, but if QEMU later completes it, then
it will in fact be paused. We will see a 'PAUSED' event from
QEMU that lets us re-sync our state.

If something like a 'device_add' commands time out though, we
have no way to gracefully recover.


NB, I'm not saying your patch is wrong. In fact I think it is
potentially a good idea, but we need to make sure we are able
to safely deal with the consequences of timing out first.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|