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

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.