[PATCH 05/10] rpc: finish all threads before exiting main loop

[Daniel P. Berrangé]

On Tue, Jul 14, 2020 at 12:32:56PM +0300, Nikolay Shirokovskiy wrote:
> Currently we have issues like [1] on libvirtd shutdown as we cleanup while RPC
> and other threads are still running. Let's finish all threads other then main
> before cleanup.
> 
> The approach to finish threads is suggested in [2]. In order to finish RPC
> threads serving API calls we let the event loop run but stop accepting new API
> calls and block processing any pending API calls. We also inform all drivers of
> shutdown so they can prepare for shutdown too. Then we wait for all RPC threads
> and driver's background thread to finish. If finishing takes more then 15s we
> just exit as we can't safely cleanup in time.
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1828207
> [2] https://www.redhat.com/archives/libvir-list/2020-April/msg01328.html
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> ---
>  src/remote/remote_daemon.c |  3 --
>  src/rpc/virnetdaemon.c     | 82 +++++++++++++++++++++++++++++++++++++++++++++-
>  src/rpc/virnetserver.c     |  8 +++++
>  src/rpc/virnetserver.h     |  1 +
>  4 files changed, 90 insertions(+), 4 deletions(-)
> 
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index 1aa9bfc..222bb5f 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -1201,9 +1201,6 @@ int main(int argc, char **argv) {
>                  0, "shutdown", NULL, NULL);
>  
>   cleanup:
> -    /* Keep cleanup order in inverse order of startup */
> -    virNetDaemonClose(dmn);
> -
>      virNetlinkEventServiceStopAll();
>  
>      if (driversInitialized) {
> diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
> index bb81a43..c4b31c6 100644
> --- a/src/rpc/virnetdaemon.c
> +++ b/src/rpc/virnetdaemon.c
> @@ -23,6 +23,7 @@
>  #include <unistd.h>
>  #include <fcntl.h>
>  
> +#include "libvirt_internal.h"
>  #include "virnetdaemon.h"
>  #include "virlog.h"
>  #include "viralloc.h"
> @@ -69,7 +70,10 @@ struct _virNetDaemon {
>      virHashTablePtr servers;
>      virJSONValuePtr srvObject;
>  
> +    int finishTimer;
>      bool quit;
> +    bool finished;
> +    bool graceful;
>  
>      unsigned int autoShutdownTimeout;
>      size_t autoShutdownInhibitions;
> @@ -80,6 +84,11 @@ struct _virNetDaemon {
>  
>  static virClassPtr virNetDaemonClass;
>  
> +static int
> +daemonServerClose(void *payload,
> +                  const void *key G_GNUC_UNUSED,
> +                  void *opaque G_GNUC_UNUSED);
> +
>  static void
>  virNetDaemonDispose(void *obj)
>  {
> @@ -796,11 +805,53 @@ daemonServerProcessClients(void *payload,
>      return 0;
>  }
>  
> +static int
> +daemonServerShutdownWait(void *payload,
> +                         const void *key G_GNUC_UNUSED,
> +                         void *opaque G_GNUC_UNUSED)
> +{
> +    virNetServerPtr srv = payload;
> +
> +    virNetServerShutdownWait(srv);
> +    return 0;
> +}
> +
> +static void
> +daemonShutdownWait(void *opaque)
> +{
> +    virNetDaemonPtr dmn = opaque;
> +    bool graceful = false;
> +
> +    virHashForEach(dmn->servers, daemonServerShutdownWait, NULL);
> +    if (virStateShutdownWait() < 0)

This code can't have any dependancy on virStateShutdownWait because
it is used in virtlockd and virtlogd, neither of which use the
virState infrastructure.

> +        goto finish;
> +
> +    graceful = true;
> +
> + finish:
> +    virObjectLock(dmn);
> +    dmn->graceful = graceful;
> +    virEventUpdateTimeout(dmn->finishTimer, 0);
> +    virObjectUnlock(dmn);
> +}
> +
> +static void
> +virNetDaemonFinishTimer(int timerid G_GNUC_UNUSED,
> +                        void *opaque)
> +{
> +    virNetDaemonPtr dmn = opaque;
> +
> +    virObjectLock(dmn);
> +    dmn->finished = true;
> +    virObjectUnlock(dmn);
> +}
> +
>  void
>  virNetDaemonRun(virNetDaemonPtr dmn)
>  {
>      int timerid = -1;
>      bool timerActive = false;
> +    virThread shutdownThread;
>  
>      virObjectLock(dmn);
>  
> @@ -811,6 +862,9 @@ virNetDaemonRun(virNetDaemonPtr dmn)
>      }
>  
>      dmn->quit = false;
> +    dmn->finishTimer = -1;
> +    dmn->finished = false;
> +    dmn->graceful = false;
>  
>      if (dmn->autoShutdownTimeout &&
>          (timerid = virEventAddTimeout(-1,
> @@ -826,7 +880,7 @@ virNetDaemonRun(virNetDaemonPtr dmn)
>      virSystemdNotifyStartup();
>  
>      VIR_DEBUG("dmn=%p quit=%d", dmn, dmn->quit);
> -    while (!dmn->quit) {
> +    while (!dmn->finished) {
>          /* A shutdown timeout is specified, so check
>           * if any drivers have active state, if not
>           * shutdown after timeout seconds
> @@ -857,6 +911,32 @@ virNetDaemonRun(virNetDaemonPtr dmn)
>          virObjectLock(dmn);
>  
>          virHashForEach(dmn->servers, daemonServerProcessClients, NULL);
> +
> +        if (dmn->quit && dmn->finishTimer == -1) {
> +            virHashForEach(dmn->servers, daemonServerClose, NULL);
> +            if (virStateShutdown() < 0)
> +                break;

Again, we cna't have this direct call here.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|