[PATCH 00/10] resolve hangs/crashes on libvirtd shutdown
This series follows [1] but address the issue slightly differently. Instead of polling for RPC thread pool termination it waits for thread pool drain in distinct thread and then signal the main loop to exit. The series introduces new driver's methods stateShutdown/stateShutdownWait to finish all driver's background threads. The methods however are only implemented for qemu driver and only partially. There are other drivers that have background threads and I don't check every one of them in terms of how they manage their background threads. For example node driver creates 2 threads. One of them is supposed to live a for a short amount of time and thus not tracked. This thread can cause issues on shutdown. The second thread is tracked and finished synchronously on driver cleanup. So this thread can not cause crashes but can cause hangs theoretically speaking so we may want to move the thread finishing to stateShutdownWait method so that possible hang will be handled by shutdown timeout. The qemu driver also has untracked threads and they can cause crashes on shutdown. For example reconnect threads or reboot thread. These need to be tracked. I'm going to address these issues in qemu and other drivers once the overall approach will be approved. I added 2 new driver's methods so that thread finishing will be done in parallel. If we have only one method then the shutdown is done one by one effectively. I've added clean shutdown timeout in event loop as suggested by Daniel in [2]. Now I think why we can't just go with systemd unit management? Systemd will eventually send SIGKILL and we can tune the timeout using TimeoutStopUSec parameter. This way we even don't need to introduce new driver's methods. Driver's background threads can be finished in stateCleanup method. AFAIU as drivers are cleaned up in reverse order it is safe in the sense that already cleaned up driver can not be used by background threads of not yet cleaned up driver. Of course this way the cleanup is not done in parallel. Well to turn it into parallel we can introduce just stateShutdown which we don't need to call in netdaemon code and thus don't introduce undesired dependency of netdaemon on drivers concept. [1] Resolve libvirtd hang on termination with connected long running client https://www.redhat.com/archives/libvir-list/2018-July/msg00382.html [2] Races / crashes in shutdown of libvirtd daemon https://www.redhat.com/archives/libvir-list/2020-April/msg01328.html Nikolay Shirokovskiy (10): libvirt: add stateShutdown/stateShutdownWait to drivers util: always initialize priority condition util: add stop/drain functions to thread pool rpc: don't unref service ref on socket behalf twice rpc: finish all threads before exiting main loop vireventthread: add virEventThreadClose qemu: exit thread synchronously in qemuDomainObjStopWorker qemu: implement driver's shutdown/shutdown wait methods rpc: cleanup virNetDaemonClose method util: remove unused virThreadPoolNew macro scripts/check-drivername.py | 2 + src/driver-state.h | 8 ++++ src/libvirt.c | 42 +++++++++++++++++++ src/libvirt_internal.h | 2 + src/libvirt_private.syms | 3 ++ src/libvirt_remote.syms | 1 - src/qemu/qemu_domain.c | 1 + src/qemu/qemu_driver.c | 32 +++++++++++++++ src/remote/remote_daemon.c | 3 -- src/rpc/virnetdaemon.c | 95 ++++++++++++++++++++++++++++++++++++------- src/rpc/virnetdaemon.h | 2 - src/rpc/virnetserver.c | 8 ++++ src/rpc/virnetserver.h | 1 + src/rpc/virnetserverservice.c | 1 - src/util/vireventthread.c | 9 ++++ src/util/vireventthread.h | 1 + src/util/virthreadpool.c | 65 ++++++++++++++++++++--------- src/util/virthreadpool.h | 6 +-- 18 files changed, 238 insertions(+), 44 deletions(-) -- 1.8.3.1
stateShutdown is supposed to inform driver that it will be closed soon so that the driver can prepare and finish all background threads quickly on stateShutdownWait call. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- scripts/check-drivername.py | 2 ++ src/driver-state.h | 8 ++++++++ src/libvirt.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_internal.h | 2 ++ 4 files changed, 54 insertions(+) diff --git a/scripts/check-drivername.py b/scripts/check-drivername.py index 39eff83..19d1cd1 100644 --- a/scripts/check-drivername.py +++ b/scripts/check-drivername.py @@ -50,6 +50,8 @@ for drvfile in drvfiles: "virDrvStateCleanup", "virDrvStateReload", "virDrvStateStop", + "virDrvStateShutdown", + "virDrvStateShutdownWait", "virDrvConnectSupportsFeature", "virDrvConnectURIProbe", "virDrvDomainMigratePrepare", diff --git a/src/driver-state.h b/src/driver-state.h index 6b3f501..1f664f3 100644 --- a/src/driver-state.h +++ b/src/driver-state.h @@ -45,6 +45,12 @@ typedef int typedef int (*virDrvStateStop)(void); +typedef int +(*virDrvStateShutdown)(void); + +typedef int +(*virDrvStateShutdownWait)(void); + typedef struct _virStateDriver virStateDriver; typedef virStateDriver *virStateDriverPtr; @@ -55,4 +61,6 @@ struct _virStateDriver { virDrvStateCleanup stateCleanup; virDrvStateReload stateReload; virDrvStateStop stateStop; + virDrvStateShutdown stateShutdown; + virDrvStateShutdownWait stateShutdownWait; }; diff --git a/src/libvirt.c b/src/libvirt.c index b2d0ba3..28f9332 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -673,6 +673,48 @@ virStateInitialize(bool privileged, /** + * virStateShutdown: + * + * Run each virtualization driver's shutdown method. + * + * Returns 0 if all succeed, -1 upon any failure. + */ +int +virStateShutdown(void) +{ + size_t i; + + for (i = 0; i < virStateDriverTabCount; i++) { + if (virStateDriverTab[i]->stateShutdown && + virStateDriverTab[i]->stateShutdown() < 0) + return -1; + } + return 0; +} + + +/** + * virStateShutdownWait: + * + * Run each virtualization driver's shutdown wait method. + * + * Returns 0 if all succeed, -1 upon any failure. + */ +int +virStateShutdownWait(void) +{ + size_t i; + + for (i = 0; i < virStateDriverTabCount; i++) { + if (virStateDriverTab[i]->stateShutdownWait && + virStateDriverTab[i]->stateShutdownWait() < 0) + return -1; + } + return 0; +} + + +/** * virStateCleanup: * * Run each virtualization driver's cleanup method. diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h index 72c6127..5b6035f 100644 --- a/src/libvirt_internal.h +++ b/src/libvirt_internal.h @@ -34,6 +34,8 @@ int virStateInitialize(bool privileged, const char *root, virStateInhibitCallback inhibit, void *opaque); +int virStateShutdown(void); +int virStateShutdownWait(void); int virStateCleanup(void); int virStateReload(void); int virStateStop(void); -- 1.8.3.1
On Tue, Jul 14, 2020 at 12:32:52PM +0300, Nikolay Shirokovskiy wrote:
stateShutdown is supposed to inform driver that it will be closed soon so that the driver can prepare and finish all background threads quickly on stateShutdownWait call.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- scripts/check-drivername.py | 2 ++ src/driver-state.h | 8 ++++++++ src/libvirt.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_internal.h | 2 ++ 4 files changed, 54 insertions(+)
diff --git a/scripts/check-drivername.py b/scripts/check-drivername.py index 39eff83..19d1cd1 100644 --- a/scripts/check-drivername.py +++ b/scripts/check-drivername.py @@ -50,6 +50,8 @@ for drvfile in drvfiles: "virDrvStateCleanup", "virDrvStateReload", "virDrvStateStop", + "virDrvStateShutdown", + "virDrvStateShutdownWait", "virDrvConnectSupportsFeature", "virDrvConnectURIProbe", "virDrvDomainMigratePrepare", diff --git a/src/driver-state.h b/src/driver-state.h index 6b3f501..1f664f3 100644 --- a/src/driver-state.h +++ b/src/driver-state.h @@ -45,6 +45,12 @@ typedef int typedef int (*virDrvStateStop)(void);
+typedef int +(*virDrvStateShutdown)(void); + +typedef int +(*virDrvStateShutdownWait)(void);
This is a bit of a bikeshedding comment, but I would have called them "StateShutdownPrepare" and "StateShutdownComplete" (or Wait) just to make it slightly clearer that the first method is just intended to kick off a shutdown asynchronously. The design overall looks fine though, so regardless of the name Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Even if we have no priority threads on pool creation we can add them thru virThreadPoolSetParameters later. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/util/virthreadpool.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index 379d236..10a44de 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -245,6 +245,8 @@ virThreadPoolNewFull(size_t minWorkers, goto error; if (virCondInit(&pool->cond) < 0) goto error; + if (virCondInit(&pool->prioCond) < 0) + goto error; if (virCondInit(&pool->quit_cond) < 0) goto error; @@ -255,13 +257,8 @@ virThreadPoolNewFull(size_t minWorkers, if (virThreadPoolExpand(pool, minWorkers, false) < 0) goto error; - if (prioWorkers) { - if (virCondInit(&pool->prioCond) < 0) - goto error; - - if (virThreadPoolExpand(pool, prioWorkers, true) < 0) - goto error; - } + if (virThreadPoolExpand(pool, prioWorkers, true) < 0) + goto error; return pool; @@ -274,7 +271,6 @@ virThreadPoolNewFull(size_t minWorkers, void virThreadPoolFree(virThreadPoolPtr pool) { virThreadPoolJobPtr job; - bool priority = false; if (!pool) return; @@ -283,10 +279,8 @@ void virThreadPoolFree(virThreadPoolPtr pool) pool->quit = true; if (pool->nWorkers > 0) virCondBroadcast(&pool->cond); - if (pool->nPrioWorkers > 0) { - priority = true; + if (pool->nPrioWorkers > 0) virCondBroadcast(&pool->prioCond); - } while (pool->nWorkers > 0 || pool->nPrioWorkers > 0) ignore_value(virCondWait(&pool->quit_cond, &pool->mutex)); @@ -301,10 +295,8 @@ void virThreadPoolFree(virThreadPoolPtr pool) virMutexDestroy(&pool->mutex); virCondDestroy(&pool->quit_cond); virCondDestroy(&pool->cond); - if (priority) { - VIR_FREE(pool->prioWorkers); - virCondDestroy(&pool->prioCond); - } + VIR_FREE(pool->prioWorkers); + virCondDestroy(&pool->prioCond); VIR_FREE(pool); } -- 1.8.3.1
On Tue, Jul 14, 2020 at 12:32:53PM +0300, Nikolay Shirokovskiy wrote:
Even if we have no priority threads on pool creation we can add them thru virThreadPoolSetParameters later.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/util/virthreadpool.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Stop just send signal for threads to exit when they finish with current task. Drain waits when all threads will finish. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt_private.syms | 2 ++ src/util/virthreadpool.c | 43 ++++++++++++++++++++++++++++++++++++++----- src/util/virthreadpool.h | 3 +++ 3 files changed, 43 insertions(+), 5 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 73b72c9..f64b1de 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3326,6 +3326,7 @@ virThreadJobSetWorker; # util/virthreadpool.h +virThreadPoolDrain; virThreadPoolFree; virThreadPoolGetCurrentWorkers; virThreadPoolGetFreeWorkers; @@ -3336,6 +3337,7 @@ virThreadPoolGetPriorityWorkers; virThreadPoolNewFull; virThreadPoolSendJob; virThreadPoolSetParameters; +virThreadPoolStop; # util/virtime.h diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index 10a44de..ca44f55 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -268,19 +268,27 @@ virThreadPoolNewFull(size_t minWorkers, } -void virThreadPoolFree(virThreadPoolPtr pool) -{ - virThreadPoolJobPtr job; - if (!pool) +static void +virThreadPoolStopLocked(virThreadPoolPtr pool) +{ + if (pool->quit) return; - virMutexLock(&pool->mutex); pool->quit = true; if (pool->nWorkers > 0) virCondBroadcast(&pool->cond); if (pool->nPrioWorkers > 0) virCondBroadcast(&pool->prioCond); +} + + +static void +virThreadPoolDrainLocked(virThreadPoolPtr pool) +{ + virThreadPoolJobPtr job; + + virThreadPoolStopLocked(pool); while (pool->nWorkers > 0 || pool->nPrioWorkers > 0) ignore_value(virCondWait(&pool->quit_cond, &pool->mutex)); @@ -289,6 +297,15 @@ void virThreadPoolFree(virThreadPoolPtr pool) pool->jobList.head = pool->jobList.head->next; VIR_FREE(job); } +} + +void virThreadPoolFree(virThreadPoolPtr pool) +{ + if (!pool) + return; + + virMutexLock(&pool->mutex); + virThreadPoolDrainLocked(pool); VIR_FREE(pool->workers); virMutexUnlock(&pool->mutex); @@ -475,3 +492,19 @@ virThreadPoolSetParameters(virThreadPoolPtr pool, virMutexUnlock(&pool->mutex); return -1; } + +void +virThreadPoolStop(virThreadPoolPtr pool) +{ + virMutexLock(&pool->mutex); + virThreadPoolStopLocked(pool); + virMutexUnlock(&pool->mutex); +} + +void +virThreadPoolDrain(virThreadPoolPtr pool) +{ + virMutexLock(&pool->mutex); + virThreadPoolDrainLocked(pool); + virMutexUnlock(&pool->mutex); +} diff --git a/src/util/virthreadpool.h b/src/util/virthreadpool.h index c97d9b3..dd1aaf3 100644 --- a/src/util/virthreadpool.h +++ b/src/util/virthreadpool.h @@ -56,3 +56,6 @@ int virThreadPoolSetParameters(virThreadPoolPtr pool, long long int minWorkers, long long int maxWorkers, long long int prioWorkers); + +void virThreadPoolStop(virThreadPoolPtr pool); +void virThreadPoolDrain(virThreadPoolPtr pool); -- 1.8.3.1
On Tue, Jul 14, 2020 at 12:32:54PM +0300, Nikolay Shirokovskiy wrote:
Stop just send signal for threads to exit when they finish with current task. Drain waits when all threads will finish.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt_private.syms | 2 ++ src/util/virthreadpool.c | 43 ++++++++++++++++++++++++++++++++++++++----- src/util/virthreadpool.h | 3 +++ 3 files changed, 43 insertions(+), 5 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Second unref was added in [1]. We don't need it actually as we pass free callback to virNetSocketAddIOCallback thus when we call virNetSocketRemoveIOCallback the extra ref for callback will be dropped without extra efforts. [1] 355d8f470f9: virNetServerServiceClose: Don't leak sockets --- src/rpc/virnetserverservice.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 9d5df45..e4165ea 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -449,6 +449,5 @@ void virNetServerServiceClose(virNetServerServicePtr svc) for (i = 0; i < svc->nsocks; i++) { virNetSocketRemoveIOCallback(svc->socks[i]); virNetSocketClose(svc->socks[i]); - virObjectUnref(svc); } } -- 1.8.3.1
On Tue, Jul 14, 2020 at 12:32:55PM +0300, Nikolay Shirokovskiy wrote:
Second unref was added in [1]. We don't need it actually as we pass free callback to virNetSocketAddIOCallback thus when we call virNetSocketRemoveIOCallback the extra ref for callback will be dropped without extra efforts.
Oh, so this is actually fixing unref of free'd memory. Surprised we don't crash in this already.
[1] 355d8f470f9: virNetServerServiceClose: Don't leak sockets --- src/rpc/virnetserverservice.c | 1 - 1 file changed, 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 9d5df45..e4165ea 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -449,6 +449,5 @@ void virNetServerServiceClose(virNetServerServicePtr svc) for (i = 0; i < svc->nsocks; i++) { virNetSocketRemoveIOCallback(svc->socks[i]); virNetSocketClose(svc->socks[i]); - virObjectUnref(svc); } } -- 1.8.3.1
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 :|
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@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) + 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; + + if ((dmn->finishTimer = virEventAddTimeout(15 * 1000, + virNetDaemonFinishTimer, + dmn, NULL)) < 0) { + VIR_WARN("Failed to register finish timer."); + break; + } + + if (virThreadCreateFull(&shutdownThread, true, daemonShutdownWait, + "daemon-shutdown", false, dmn) < 0) { + VIR_WARN("Failed to register join thread."); + break; + } + } + } + + if (dmn->graceful) { + virThreadJoin(&shutdownThread); + } else { + VIR_WARN("Make forcefull daemon shutdown"); + exit(EXIT_FAILURE); } cleanup: diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index e0a2386..79ea9f6 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -942,9 +942,17 @@ void virNetServerClose(virNetServerPtr srv) for (i = 0; i < srv->nclients; i++) virNetServerClientClose(srv->clients[i]); + virThreadPoolStop(srv->workers); + virObjectUnlock(srv); } +void +virNetServerShutdownWait(virNetServerPtr srv) +{ + virThreadPoolDrain(srv->workers); +} + static inline size_t virNetServerTrackPendingAuthLocked(virNetServerPtr srv) { diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h index 1c6a2ef..112a51d 100644 --- a/src/rpc/virnetserver.h +++ b/src/rpc/virnetserver.h @@ -56,6 +56,7 @@ virNetServerPtr virNetServerNewPostExecRestart(virJSONValuePtr object, ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6); void virNetServerClose(virNetServerPtr srv); +void virNetServerShutdownWait(virNetServerPtr srv); virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv); -- 1.8.3.1
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@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 :|
virEventThreadClose is used when event loop thread should be exited synchronously (which is not the case when event loop is just unreferenced). Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt_private.syms | 1 + src/util/vireventthread.c | 9 +++++++++ src/util/vireventthread.h | 1 + 3 files changed, 11 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f64b1de..c85ec43 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2023,6 +2023,7 @@ virEventGLibRunOnce; # util/vireventthread.h +virEventThreadClose; virEventThreadGetContext; virEventThreadNew; diff --git a/src/util/vireventthread.c b/src/util/vireventthread.c index cf86592..0a7c436 100644 --- a/src/util/vireventthread.c +++ b/src/util/vireventthread.c @@ -183,6 +183,15 @@ virEventThreadNew(const char *name) } +void +virEventThreadClose(virEventThread *evt) +{ + g_main_loop_quit(evt->loop); + g_thread_join(evt->thread); + evt->thread = NULL; +} + + GMainContext * virEventThreadGetContext(virEventThread *evt) { diff --git a/src/util/vireventthread.h b/src/util/vireventthread.h index 5826c25..6f01629 100644 --- a/src/util/vireventthread.h +++ b/src/util/vireventthread.h @@ -27,5 +27,6 @@ G_DECLARE_FINAL_TYPE(virEventThread, vir_event_thread, VIR, EVENT_THREAD, GObject); virEventThread *virEventThreadNew(const char *name); +void virEventThreadClose(virEventThread *evt); GMainContext *virEventThreadGetContext(virEventThread *evt); -- 1.8.3.1
On Tue, Jul 14, 2020 at 12:32:57PM +0300, Nikolay Shirokovskiy wrote:
virEventThreadClose is used when event loop thread should be exited synchronously (which is not the case when event loop is just unreferenced).
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt_private.syms | 1 + src/util/vireventthread.c | 9 +++++++++ src/util/vireventthread.h | 1 + 3 files changed, 11 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f64b1de..c85ec43 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2023,6 +2023,7 @@ virEventGLibRunOnce;
# util/vireventthread.h +virEventThreadClose; virEventThreadGetContext; virEventThreadNew;
diff --git a/src/util/vireventthread.c b/src/util/vireventthread.c index cf86592..0a7c436 100644 --- a/src/util/vireventthread.c +++ b/src/util/vireventthread.c @@ -183,6 +183,15 @@ virEventThreadNew(const char *name) }
+void +virEventThreadClose(virEventThread *evt) +{ + g_main_loop_quit(evt->loop); + g_thread_join(evt->thread); + evt->thread = NULL; +}
Lets invoke this from vir_event_thread_finalize too. Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
The change won't hurt much current callers performance I guess and now we can use the function when we need to be sure of synchronous thread exit as well. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2d9d822..18651d0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1571,6 +1571,7 @@ qemuDomainObjStopWorker(virDomainObjPtr dom) qemuDomainObjPrivatePtr priv = dom->privateData; if (priv->eventThread) { + virEventThreadClose(priv->eventThread); g_object_unref(priv->eventThread); priv->eventThread = NULL; } -- 1.8.3.1
On Tue, Jul 14, 2020 at 12:32:58PM +0300, Nikolay Shirokovskiy wrote:
The change won't hurt much current callers performance I guess and now we can use the function when we need to be sure of synchronous thread exit as well.
I can't remember the exact scenario, but I'm reasonably sure i tried this approach when adding the event thread support and hit scenario where this would deadlock.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2d9d822..18651d0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1571,6 +1571,7 @@ qemuDomainObjStopWorker(virDomainObjPtr dom) qemuDomainObjPrivatePtr priv = dom->privateData;
if (priv->eventThread) { + virEventThreadClose(priv->eventThread); g_object_unref(priv->eventThread);
IIRC, it was something like this unref does not always release the last reference.
priv->eventThread = NULL;
IOW, despite setting evnetThread to NULL, the thread may linger for a short while longer in the background until any operations have completed. 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 :|
On 21.07.2020 19:09, Daniel P. Berrangé wrote:
On Tue, Jul 14, 2020 at 12:32:58PM +0300, Nikolay Shirokovskiy wrote:
The change won't hurt much current callers performance I guess and now we can use the function when we need to be sure of synchronous thread exit as well.
I can't remember the exact scenario, but I'm reasonably sure i tried this approach when adding the event thread support and hit scenario where this would deadlock.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2d9d822..18651d0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1571,6 +1571,7 @@ qemuDomainObjStopWorker(virDomainObjPtr dom) qemuDomainObjPrivatePtr priv = dom->privateData;
if (priv->eventThread) { + virEventThreadClose(priv->eventThread); g_object_unref(priv->eventThread);
IIRC, it was something like this unref does not always release the last reference.
priv->eventThread = NULL;
IOW, despite setting evnetThread to NULL, the thread may linger for a short while longer in the background until any operations have completed.
Yeah, we can not stop event loop thread synchronously when call qemuDomainObjStopWorker from qemuProcessHandleMonitorEOF as the latter is called from event loop. Hmm, pthread_join will fail in this case: EDEADLK A deadlock was detected (e.g., two threads tried to join with each other); or thread specifies the calling thread. Anyway, make sense calling virEventThreadClose optional. Nikolay
On shutdown we just stop accepting new jobs for worker thread so that on shutdown wait we can exit worker thread faster. Yes we basically stop processing of events for VMs but we are going to do so anyway in case of daemon shutdown. At the same time synchronous event processing that some API calls may require are still possible as per VM event loop is still running and we don't need worker thread for synchronous event processing. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d185666..f7ff0fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1098,6 +1098,36 @@ qemuStateStop(void) return ret; } + +static int +qemuStateShutdown(void) +{ + virThreadPoolStop(qemu_driver->workerPool); + return 0; +} + + +static int +qemuDomainObjStopWorkerIter(virDomainObjPtr vm, + void *opaque G_GNUC_UNUSED) +{ + virObjectLock(vm); + qemuDomainObjStopWorker(vm); + virObjectUnlock(vm); + return 0; +} + + +static int +qemuStateShutdownWait(void) +{ + virDomainObjListForEach(qemu_driver->domains, false, + qemuDomainObjStopWorkerIter, NULL); + virThreadPoolDrain(qemu_driver->workerPool); + return 0; +} + + /** * qemuStateCleanup: * @@ -23413,6 +23443,8 @@ static virStateDriver qemuStateDriver = { .stateCleanup = qemuStateCleanup, .stateReload = qemuStateReload, .stateStop = qemuStateStop, + .stateShutdown = qemuStateShutdown, + .stateShutdownWait = qemuStateShutdownWait, }; int qemuRegister(void) -- 1.8.3.1
On Tue, Jul 14, 2020 at 12:32:59PM +0300, Nikolay Shirokovskiy wrote:
On shutdown we just stop accepting new jobs for worker thread so that on shutdown wait we can exit worker thread faster. Yes we basically stop processing of events for VMs but we are going to do so anyway in case of daemon shutdown.
At the same time synchronous event processing that some API calls may require are still possible as per VM event loop is still running and we don't need worker thread for synchronous event processing.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d185666..f7ff0fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1098,6 +1098,36 @@ qemuStateStop(void) return ret; }
+ +static int +qemuStateShutdown(void) +{ + virThreadPoolStop(qemu_driver->workerPool); + return 0; +} + + +static int +qemuDomainObjStopWorkerIter(virDomainObjPtr vm, + void *opaque G_GNUC_UNUSED) +{ + virObjectLock(vm); + qemuDomainObjStopWorker(vm);
My comment on the previous patch makes me slightly concerned about this too but Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt_remote.syms | 1 - src/rpc/virnetdaemon.c | 13 ------------- src/rpc/virnetdaemon.h | 2 -- 3 files changed, 16 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index 0018a0c..2cd1b76 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -75,7 +75,6 @@ virNetDaemonAddServer; virNetDaemonAddShutdownInhibition; virNetDaemonAddSignalHandler; virNetDaemonAutoShutdown; -virNetDaemonClose; virNetDaemonGetServer; virNetDaemonGetServers; virNetDaemonHasClients; diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index c4b31c6..87d669c 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -966,19 +966,6 @@ daemonServerClose(void *payload, return 0; } -void -virNetDaemonClose(virNetDaemonPtr dmn) -{ - if (!dmn) - return; - - virObjectLock(dmn); - - virHashForEach(dmn->servers, daemonServerClose, NULL); - - virObjectUnlock(dmn); -} - static int daemonServerHasClients(void *payload, const void *key G_GNUC_UNUSED, diff --git a/src/rpc/virnetdaemon.h b/src/rpc/virnetdaemon.h index c2c7767..e708967 100644 --- a/src/rpc/virnetdaemon.h +++ b/src/rpc/virnetdaemon.h @@ -73,8 +73,6 @@ void virNetDaemonRun(virNetDaemonPtr dmn); void virNetDaemonQuit(virNetDaemonPtr dmn); -void virNetDaemonClose(virNetDaemonPtr dmn); - bool virNetDaemonHasClients(virNetDaemonPtr dmn); virNetServerPtr virNetDaemonGetServer(virNetDaemonPtr dmn, -- 1.8.3.1
On Tue, Jul 14, 2020 at 12:33:00PM +0300, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt_remote.syms | 1 - src/rpc/virnetdaemon.c | 13 ------------- src/rpc/virnetdaemon.h | 2 -- 3 files changed, 16 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/util/virthreadpool.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/util/virthreadpool.h b/src/util/virthreadpool.h index dd1aaf3..cb800dc 100644 --- a/src/util/virthreadpool.h +++ b/src/util/virthreadpool.h @@ -28,9 +28,6 @@ typedef virThreadPool *virThreadPoolPtr; typedef void (*virThreadPoolJobFunc)(void *jobdata, void *opaque); -#define virThreadPoolNew(min, max, prio, func, opaque) \ - virThreadPoolNewFull(min, max, prio, func, #func, opaque) - virThreadPoolPtr virThreadPoolNewFull(size_t minWorkers, size_t maxWorkers, size_t prioWorkers, -- 1.8.3.1
On Tue, Jul 14, 2020 at 12:33:01PM +0300, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/util/virthreadpool.h | 3 --- 1 file changed, 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
As far as code goes: Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> About the design I have a question about the timeout. Patch 5/10 is setting a 15 second timeout. How did you reach this value? Reading the bug, specially this comment from Daniel: https://bugzilla.redhat.com/show_bug.cgi?id=1828207#c6 He mentions "give it 5 seconds of running before shutting it down". 5 seconds before shutdown is something that most users can be slightly annoyed but in the end don't mind that much, but 15 seconds is something that will cause bugs to be opened because "Libvirt is taking too long to shutdown". Besides, it's a fair assumption that a transaction that takes more than 5 or so seconds to finish is already compromised* - might as well shutdown the daemon and deal with the errors. * assuming user discretion to avoid shutting down the daemon in the middle of a long duration transaction, of course. Thanks, DHB On 7/14/20 6:32 AM, Nikolay Shirokovskiy wrote:
This series follows [1] but address the issue slightly differently. Instead of polling for RPC thread pool termination it waits for thread pool drain in distinct thread and then signal the main loop to exit.
The series introduces new driver's methods stateShutdown/stateShutdownWait to finish all driver's background threads. The methods however are only implemented for qemu driver and only partially. There are other drivers that have background threads and I don't check every one of them in terms of how they manage their background threads.
For example node driver creates 2 threads. One of them is supposed to live a for a short amount of time and thus not tracked. This thread can cause issues on shutdown. The second thread is tracked and finished synchronously on driver cleanup. So this thread can not cause crashes but can cause hangs theoretically speaking so we may want to move the thread finishing to stateShutdownWait method so that possible hang will be handled by shutdown timeout.
The qemu driver also has untracked threads and they can cause crashes on shutdown. For example reconnect threads or reboot thread. These need to be tracked.
I'm going to address these issues in qemu and other drivers once the overall approach will be approved.
I added 2 new driver's methods so that thread finishing will be done in parallel. If we have only one method then the shutdown is done one by one effectively.
I've added clean shutdown timeout in event loop as suggested by Daniel in [2]. Now I think why we can't just go with systemd unit management? Systemd will eventually send SIGKILL and we can tune the timeout using TimeoutStopUSec parameter. This way we even don't need to introduce new driver's methods. Driver's background threads can be finished in stateCleanup method. AFAIU as drivers are cleaned up in reverse order it is safe in the sense that already cleaned up driver can not be used by background threads of not yet cleaned up driver. Of course this way the cleanup is not done in parallel. Well to turn it into parallel we can introduce just stateShutdown which we don't need to call in netdaemon code and thus don't introduce undesired dependency of netdaemon on drivers concept.
[1] Resolve libvirtd hang on termination with connected long running client https://www.redhat.com/archives/libvir-list/2018-July/msg00382.html [2] Races / crashes in shutdown of libvirtd daemon https://www.redhat.com/archives/libvir-list/2020-April/msg01328.html
Nikolay Shirokovskiy (10): libvirt: add stateShutdown/stateShutdownWait to drivers util: always initialize priority condition util: add stop/drain functions to thread pool rpc: don't unref service ref on socket behalf twice rpc: finish all threads before exiting main loop vireventthread: add virEventThreadClose qemu: exit thread synchronously in qemuDomainObjStopWorker qemu: implement driver's shutdown/shutdown wait methods rpc: cleanup virNetDaemonClose method util: remove unused virThreadPoolNew macro
scripts/check-drivername.py | 2 + src/driver-state.h | 8 ++++ src/libvirt.c | 42 +++++++++++++++++++ src/libvirt_internal.h | 2 + src/libvirt_private.syms | 3 ++ src/libvirt_remote.syms | 1 - src/qemu/qemu_domain.c | 1 + src/qemu/qemu_driver.c | 32 +++++++++++++++ src/remote/remote_daemon.c | 3 -- src/rpc/virnetdaemon.c | 95 ++++++++++++++++++++++++++++++++++++------- src/rpc/virnetdaemon.h | 2 - src/rpc/virnetserver.c | 8 ++++ src/rpc/virnetserver.h | 1 + src/rpc/virnetserverservice.c | 1 - src/util/vireventthread.c | 9 ++++ src/util/vireventthread.h | 1 + src/util/virthreadpool.c | 65 ++++++++++++++++++++--------- src/util/virthreadpool.h | 6 +-- 18 files changed, 238 insertions(+), 44 deletions(-)
On 14.07.2020 17:53, Daniel Henrique Barboza wrote:
As far as code goes:
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
About the design I have a question about the timeout. Patch 5/10 is setting a 15 second timeout. How did you reach this value? Reading the bug, specially this comment from Daniel:
https://bugzilla.redhat.com/show_bug.cgi?id=1828207#c6
He mentions "give it 5 seconds of running before shutting it down".
I guess 5 seconds is time for libvirtd to finish startup. This time has different nature than time for libvirtd to finish it's work on shutdown so it can be different.
5 seconds before shutdown is something that most users can be slightly annoyed but in the end don't mind that much, but 15 seconds is something that will cause bugs to be opened because "Libvirt is taking too long to shutdown". Besides, it's a fair assumption that a transaction that takes more than 5 or so seconds to finish is already compromised* - might as well shutdown the daemon and deal with the errors.
15 seconds was mentioned by Daniel in [1] when he first proposed the approach so I used this value without any extra thought. However I missed that in the last John's series [2] the default for waiting time is 0s. May be this is the current decision on waiting time. Let's wait for others to join the review. [1] https://www.redhat.com/archives/libvir-list/2018-January/msg00942.html [2] https://www.redhat.com/archives/libvir-list/2018-July/msg00382.html Nikolay
* assuming user discretion to avoid shutting down the daemon in the middle of a long duration transaction, of course.
Thanks,
DHB
On 7/14/20 6:32 AM, Nikolay Shirokovskiy wrote:
This series follows [1] but address the issue slightly differently. Instead of polling for RPC thread pool termination it waits for thread pool drain in distinct thread and then signal the main loop to exit.
The series introduces new driver's methods stateShutdown/stateShutdownWait to finish all driver's background threads. The methods however are only implemented for qemu driver and only partially. There are other drivers that have background threads and I don't check every one of them in terms of how they manage their background threads.
For example node driver creates 2 threads. One of them is supposed to live a for a short amount of time and thus not tracked. This thread can cause issues on shutdown. The second thread is tracked and finished synchronously on driver cleanup. So this thread can not cause crashes but can cause hangs theoretically speaking so we may want to move the thread finishing to stateShutdownWait method so that possible hang will be handled by shutdown timeout.
The qemu driver also has untracked threads and they can cause crashes on shutdown. For example reconnect threads or reboot thread. These need to be tracked.
I'm going to address these issues in qemu and other drivers once the overall approach will be approved.
I added 2 new driver's methods so that thread finishing will be done in parallel. If we have only one method then the shutdown is done one by one effectively.
I've added clean shutdown timeout in event loop as suggested by Daniel in [2]. Now I think why we can't just go with systemd unit management? Systemd will eventually send SIGKILL and we can tune the timeout using TimeoutStopUSec parameter. This way we even don't need to introduce new driver's methods. Driver's background threads can be finished in stateCleanup method. AFAIU as drivers are cleaned up in reverse order it is safe in the sense that already cleaned up driver can not be used by background threads of not yet cleaned up driver. Of course this way the cleanup is not done in parallel. Well to turn it into parallel we can introduce just stateShutdown which we don't need to call in netdaemon code and thus don't introduce undesired dependency of netdaemon on drivers concept.
[1] Resolve libvirtd hang on termination with connected long running client https://www.redhat.com/archives/libvir-list/2018-July/msg00382.html [2] Races / crashes in shutdown of libvirtd daemon https://www.redhat.com/archives/libvir-list/2020-April/msg01328.html
Nikolay Shirokovskiy (10): libvirt: add stateShutdown/stateShutdownWait to drivers util: always initialize priority condition util: add stop/drain functions to thread pool rpc: don't unref service ref on socket behalf twice rpc: finish all threads before exiting main loop vireventthread: add virEventThreadClose qemu: exit thread synchronously in qemuDomainObjStopWorker qemu: implement driver's shutdown/shutdown wait methods rpc: cleanup virNetDaemonClose method util: remove unused virThreadPoolNew macro
scripts/check-drivername.py | 2 + src/driver-state.h | 8 ++++ src/libvirt.c | 42 +++++++++++++++++++ src/libvirt_internal.h | 2 + src/libvirt_private.syms | 3 ++ src/libvirt_remote.syms | 1 - src/qemu/qemu_domain.c | 1 + src/qemu/qemu_driver.c | 32 +++++++++++++++ src/remote/remote_daemon.c | 3 -- src/rpc/virnetdaemon.c | 95 ++++++++++++++++++++++++++++++++++++------- src/rpc/virnetdaemon.h | 2 - src/rpc/virnetserver.c | 8 ++++ src/rpc/virnetserver.h | 1 + src/rpc/virnetserverservice.c | 1 - src/util/vireventthread.c | 9 ++++ src/util/vireventthread.h | 1 + src/util/virthreadpool.c | 65 ++++++++++++++++++++--------- src/util/virthreadpool.h | 6 +-- 18 files changed, 238 insertions(+), 44 deletions(-)
On Wed, Jul 15, 2020 at 08:51:03AM +0300, Nikolay Shirokovskiy wrote:
On 14.07.2020 17:53, Daniel Henrique Barboza wrote:
As far as code goes:
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
About the design I have a question about the timeout. Patch 5/10 is setting a 15 second timeout. How did you reach this value? Reading the bug, specially this comment from Daniel:
https://bugzilla.redhat.com/show_bug.cgi?id=1828207#c6
He mentions "give it 5 seconds of running before shutting it down".
I guess 5 seconds is time for libvirtd to finish startup. This time has different nature than time for libvirtd to finish it's work on shutdown so it can be different.
5 seconds before shutdown is something that most users can be slightly annoyed but in the end don't mind that much, but 15 seconds is something that will cause bugs to be opened because "Libvirt is taking too long to shutdown". Besides, it's a fair assumption that a transaction that takes more than 5 or so seconds to finish is already compromised* - might as well shutdown the daemon and deal with the errors.
15 seconds was mentioned by Daniel in [1] when he first proposed the approach so I used this value without any extra thought. However I missed that in the last John's series [2] the default for waiting time is 0s. May be this is the current decision on waiting time. Let's wait for others to join the review.
Don't read too much into the precise numbers I mentioned, they would just be plucked out of the air :-) If there is some job taking place wrt a VM that is taking a long time to complete and thus blocking shutdown, I think it is important to give it a fair opportunity to finish gracefully. systemd itself gives services something like 90 seconds to exit before it gives up on them. On a heavily loaded host, 5 seconds is almost certainly too short. 15 seconds is not bad, but I wouldn't object to 30 seconds either, as long as we're emitting some log message warning that we're delayed. In the "normal" case these timeouts won't be hit, so we're only delayed in the scenarios where we're likely to be doing something important for a VM. 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 :|
participants (3)
-
Daniel Henrique Barboza -
Daniel P. Berrangé -
Nikolay Shirokovskiy