[libvirt] [PATCH 0/4] libvirtd: fix hang on termination in qemu driver

[John Ferlan]

On 10/25/2017 05:05 AM, Nikolay Shirokovskiy wrote:
> Libvirtd termination can hang. For example if some API call in qemu
> driver awaiting monitor response it will never finish because event
> loop does not functional during termination. As a result we hang
> in virNetDaemonClose call during termination as this call finishes RPC
> threads.
> 
> Let's ask hypervisor drivers to finish all API calls by calling
> introduced state driver shutdown function before call to virNetDaemonClose.
> 
> Nikolay Shirokovskiy (4):
>   libvirt: introduce hypervisor driver shutdown function
>   qemu: implement state driver shutdown function
>   qemu: agent: fix monitor close during first sync
>   qemu: monitor: check monitor not closed upon send
> 
>  daemon/libvirtd.c        |  2 ++
>  src/driver-state.h       |  4 ++++
>  src/libvirt.c            | 18 ++++++++++++++++++
>  src/libvirt_internal.h   |  1 +
>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_agent.c    | 14 +++++++-------
>  src/qemu/qemu_driver.c   | 39 +++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor.c  | 27 +++++++++++++--------------
>  8 files changed, 85 insertions(+), 21 deletions(-)
> 

Moving the discussion started in the refcount thread to here via just
simple cut-n-paste... I didn't CC Erik or Martin directly because I'm
fairly confident they read libvir-list posts anyway ;-)

John

[jferlan comment]
https://www.redhat.com/archives/libvir-list/2017-November/msg00236.html

The other series seems to be adding a state shutdown step to be run
before state cleanup; however, if we alter the cleanup code in libvirtd
does that make the state shutdown step unnecessary? I don't know and
really didn't want to think about it until we got through thinking about
the cleanup processing order. Still if one considers that state
initialization is really two steps (init and auto start), then splitting
cleanup into shutdown and cleanup seems reasonable, but I don't think
affects whether we have a crash or latent usage of dmn->servers. I could
be wrong, but it's so hard to tell.


[eskultety comment/followup]
https://www.redhat.com/archives/libvir-list/2017-November/msg00249.html

Actually, no, the stateShutdown was addressing the issue with running
<driver>StateCleanup while there still was an active worker thread that
could potentially access the @driver object, in which case - SIGSEGV,
but as I wrote in my response, I don't like the idea of having another
state driver callback, as I feel it introduces a bit of ambiguity, since
by just looking at the callback names, you can't have an idea, what the
difference between stateCleanup and stateShutdown is. I think this
should be handled as part of the final stage of leaving the event loop
and simply register an appropriate callback to the handle, that way, you
don't need to define a new state callback which would most likely used
by a couple of drivers.



[mkletzan comment/followup]
https://www.redhat.com/archives/libvir-list/2017-November/msg00450.html

If stateShutdown is what makes everything go away, then why not?  The
names are pretty descriptive, shutdown is called when the daemon is
shutting down and cleanup when you need to clean up after yourself.
Used by only few drivers? Well, we can implement it everywhere and not
make it make optional, but rather mandatory.  If that is checked on the
registration then we'll know right away when we missed something (aat
runtime, but right after someone tries running it).

-----