[libvirt] [RFC PATCH 00/10] Resolve libvirtd hang on termination with connected long running client
This RFC is a combination of a couple of different patch postings that I've combined into one central "stream" of patches that can be discussed for their relative importance or need to fix the problem. Although a bit long winded, I think I've captured enough history for anyone so inclined to walk through the history to understand the maze of twisty patches that it takes to hopefully resolve the issue. The first two patches were presented previously, but not accepted: https://www.redhat.com/archives/libvir-list/2017-November/msg00296.html https://www.redhat.com/archives/libvir-list/2017-November/msg00297.html However since that time, it seems "some form" of the patches is necessary. Most importantly making sure the virObjectUnref for @srv and @srvAdm occurs *prior to* the virNetDaemonClose(dmn); at cleanup (IOW: out of order for a reason). Doing that also requires any program started on the servers also has the virObjectUnref prior to daemon close. The 3rd and 4th patches are a result of discussions held in mid December related to libvirtd crashes/hangs and some possible adjustments to help. Discussion starts here: https://www.redhat.com/archives/libvir-list/2017-December/msg00515.html This led to suggestions to move the toggling of services from Dispose to Close *and* to split the virThreadPoolFree into a Drain function that could also be called during the Close function rather than waiting for the Dispose to occur. Still testing showed that just those 4 patches it still wasn't enough as libvirtd ended up just "hung" because of some patches Nikolay posted that add a new shutdown state, see: https://www.redhat.com/archives/libvir-list/2017-October/msg01134.html Those patches languished mainly because it wasn't clear (at the time) the relationship between them and another series dealing with libvirtd crashes that was partially accepted and pushed: https://www.redhat.com/archives/libvir-list/2017-October/msg01347.html and followup discussion starting here: https://www.redhat.com/archives/libvir-list/2017-November/msg00023.html The 9th patch can be used to test that the first 8 do the job. The details on how I set up the test environment is in the patch. If the sequence is run before the first 8 patches, you will end up with a couple of different hang scenarios. So if you're compelled to see what the big deal is, then apply this one alone and have fun playing. The 10th patch is the one patch from the partially pushed series that wasn't pushed as it was not deemed necessary. It's presented here mainly for completeness. John Ferlan (5): libvirtd: Alter refcnt processing for domain server objects libvirtd: Alter refcnt processing for server program objects netserver: Toggle service off during close qemu: Introduce virTheadPoolDrain APPLY ONLY FOR TESTING PURPOSES Nikolay Shirokovskiy (5): 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 libvirtd: fix crash on termination daemon/libvirtd.c | 46 ++++++++++++++++++++++++++++++++----------- src/driver-state.h | 4 ++++ src/libvirt.c | 18 +++++++++++++++++ src/libvirt_internal.h | 1 + src/libvirt_private.syms | 2 ++ src/qemu/qemu_agent.c | 14 ++++++------- src/qemu/qemu_driver.c | 44 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 27 ++++++++++++------------- src/rpc/virnetdaemon.c | 1 + src/rpc/virnetserver.c | 5 ++--- src/rpc/virnetserverservice.c | 2 ++ src/util/virthreadpool.c | 19 ++++++++++++------ src/util/virthreadpool.h | 2 ++ 13 files changed, 143 insertions(+), 42 deletions(-) -- 2.13.6
Once virNetDaemonAddServer returns, the @srv or @srvAdm have either been added to the @dmn list increasing the refcnt or there was an error. In either case, we can lower the refcnt from virNetServerNew, but not set to NULL. Thus "using" the hash table reference when adding programs later or allowing the immediate free of the object on error. The @dmn dispose function (virNetDaemonDispose) will handle the Unref of each object during the virHashFree when the object is removed from the hash table. Signed-off-by: John Ferlan <jferlan@redhat.com> --- daemon/libvirtd.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 6d3b83355..0afd1dd82 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1065,6 +1065,7 @@ int main(int argc, char **argv) { char *remote_config_file = NULL; int statuswrite = -1; int ret = 1; + int rc; int pid_file_fd = -1; char *pid_file = NULL; char *sock_file = NULL; @@ -1319,7 +1320,11 @@ int main(int argc, char **argv) { goto cleanup; } - if (virNetDaemonAddServer(dmn, srv) < 0) { + /* Add @srv to @dmn servers hash table and Unref to allow removal from + * hash table at @dmn disposal to decide when to free @srv */ + rc = virNetDaemonAddServer(dmn, srv); + virObjectUnref(srv); + if (rc < 0) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; } @@ -1393,7 +1398,11 @@ int main(int argc, char **argv) { goto cleanup; } - if (virNetDaemonAddServer(dmn, srvAdm) < 0) { + /* Add @srvAdm to @dmn servers hash table and Unref to allow removal from + * hash table at @dmn disposal to decide when to free @srvAdm */ + rc = virNetDaemonAddServer(dmn, srvAdm); + virObjectUnref(srvAdm); + if (rc < 0) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; } @@ -1516,11 +1525,9 @@ int main(int argc, char **argv) { } virObjectUnref(adminProgram); - virObjectUnref(srvAdm); virObjectUnref(qemuProgram); virObjectUnref(lxcProgram); virObjectUnref(remoteProgram); - virObjectUnref(srv); virObjectUnref(dmn); virNetlinkShutdown(); -- 2.13.6
On Wed, Jan 10, 2018 at 12:23:26PM -0500, John Ferlan wrote:
Once virNetDaemonAddServer returns, the @srv or @srvAdm have either been added to the @dmn list increasing the refcnt or there was an error. In either case, we can lower the refcnt from virNetServerNew, but not set to NULL. Thus "using" the hash table reference when adding programs later or allowing the immediate free of the object on error.
The @dmn dispose function (virNetDaemonDispose) will handle the Unref of each object during the virHashFree when the object is removed from the hash table.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
Once virNetServerProgramNew returns, the program objects have either been added to the @srv or @srvAdm list increasing the refcnt or there was an error. In either case, we can lower the refcnt from virNetServerProgramNew and set the object to NULL since it won't be used any more. The @srv or @srvAdm dispose function (virNetServerDispose) will handle the Unref of each object during the virHashFree when the object is removed from the hash table. Signed-off-by: John Ferlan <jferlan@redhat.com> --- daemon/libvirtd.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 0afd1dd82..b47f875d9 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1352,7 +1352,11 @@ int main(int argc, char **argv) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; } - if (virNetServerAddProgram(srv, remoteProgram) < 0) { + + rc = virNetServerAddProgram(srv, remoteProgram); + virObjectUnref(remoteProgram); + remoteProgram = NULL; + if (rc < 0) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; } @@ -1364,7 +1368,11 @@ int main(int argc, char **argv) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; } - if (virNetServerAddProgram(srv, lxcProgram) < 0) { + + rc = virNetServerAddProgram(srv, lxcProgram); + virObjectUnref(lxcProgram); + lxcProgram = NULL; + if (rc < 0) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; } @@ -1376,7 +1384,11 @@ int main(int argc, char **argv) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; } - if (virNetServerAddProgram(srv, qemuProgram) < 0) { + + rc = virNetServerAddProgram(srv, qemuProgram); + virObjectUnref(qemuProgram); + qemuProgram = NULL; + if (rc < 0) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; } @@ -1414,7 +1426,11 @@ int main(int argc, char **argv) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; } - if (virNetServerAddProgram(srvAdm, adminProgram) < 0) { + + rc = virNetServerAddProgram(srvAdm, adminProgram); + virObjectUnref(adminProgram); + adminProgram = NULL; + if (rc < 0) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; } @@ -1524,10 +1540,6 @@ int main(int argc, char **argv) { virStateCleanup(); } - virObjectUnref(adminProgram); - virObjectUnref(qemuProgram); - virObjectUnref(lxcProgram); - virObjectUnref(remoteProgram); virObjectUnref(dmn); virNetlinkShutdown(); -- 2.13.6
On Wed, Jan 10, 2018 at 12:23:27PM -0500, John Ferlan wrote:
Once virNetServerProgramNew returns, the program objects have either been added to the @srv or @srvAdm list increasing the refcnt or there was an error. In either case, we can lower the refcnt from virNetServerProgramNew and set the object to NULL since it won't be used any more.
The @srv or @srvAdm dispose function (virNetServerDispose) will handle the Unref of each object during the virHashFree when the object is removed from the hash table.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
Rather than waiting until virNetServerDispose to toggle the service to off, let's do that when virNetServerServiceClose is called such as during virNetServerClose. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/rpc/virnetserver.c | 3 --- src/rpc/virnetserverservice.c | 2 ++ 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 77a4c0b8d..7bab11efb 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -805,9 +805,6 @@ void virNetServerDispose(void *obj) VIR_FREE(srv->name); - for (i = 0; i < srv->nservices; i++) - virNetServerServiceToggle(srv->services[i], false); - virThreadPoolFree(srv->workers); for (i = 0; i < srv->nservices; i++) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 4e5426ffe..636c5be4e 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -525,4 +525,6 @@ void virNetServerServiceClose(virNetServerServicePtr svc) virNetSocketClose(svc->socks[i]); virObjectUnref(svc); } + + virNetServerServiceToggle(svc, false); } -- 2.13.6
On Wed, Jan 10, 2018 at 12:23:28PM -0500, John Ferlan wrote:
Rather than waiting until virNetServerDispose to toggle the service to off, let's do that when virNetServerServiceClose is called such as during virNetServerClose.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/rpc/virnetserver.c | 3 --- src/rpc/virnetserverservice.c | 2 ++ 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 77a4c0b8d..7bab11efb 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -805,9 +805,6 @@ void virNetServerDispose(void *obj)
VIR_FREE(srv->name);
- for (i = 0; i < srv->nservices; i++) - virNetServerServiceToggle(srv->services[i], false); -
^This hunk would suffice.
virThreadPoolFree(srv->workers);
for (i = 0; i < srv->nservices; i++) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 4e5426ffe..636c5be4e 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -525,4 +525,6 @@ void virNetServerServiceClose(virNetServerServicePtr svc) virNetSocketClose(svc->socks[i]); virObjectUnref(svc); } + + virNetServerServiceToggle(svc, false);
^This is a NOP, since all the sockets have been closed already (in the loop which precedes the call) and the IO callback handle removed with watch reset to -1. Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 01/15/2018 11:35 AM, Erik Skultety wrote:
On Wed, Jan 10, 2018 at 12:23:28PM -0500, John Ferlan wrote:
Rather than waiting until virNetServerDispose to toggle the service to off, let's do that when virNetServerServiceClose is called such as during virNetServerClose.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/rpc/virnetserver.c | 3 --- src/rpc/virnetserverservice.c | 2 ++ 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 77a4c0b8d..7bab11efb 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -805,9 +805,6 @@ void virNetServerDispose(void *obj)
VIR_FREE(srv->name);
- for (i = 0; i < srv->nservices; i++) - virNetServerServiceToggle(srv->services[i], false); -
^This hunk would suffice.
virThreadPoolFree(srv->workers);
for (i = 0; i < srv->nservices; i++) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 4e5426ffe..636c5be4e 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -525,4 +525,6 @@ void virNetServerServiceClose(virNetServerServicePtr svc) virNetSocketClose(svc->socks[i]); virObjectUnref(svc); } + + virNetServerServiceToggle(svc, false);
^This is a NOP, since all the sockets have been closed already (in the loop which precedes the call) and the IO callback handle removed with watch reset to -1.
oh right <facepalm>... and it was discussed multiple times in various threads in the "other" series that I pulled this from... I was more focused on trying to put together 3 or 4 disjoint series and discussions into one pile and really wasn't thinking beyond the take existing code or words and generate patches. So, I'll drop the second hunk and change the commit message to: netserver: Remove ServiceToggle during ServerDispose No sense in calling ServiceToggle for all nservices during ServiceDispose since ServerClose calls ServiceClose which removes the IOCallback that's being toggled via ServiceToggle. Tks - John
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Split up virThreadPoolFree to create a Drain function which will be called from virNetServerClose in order to ensure the various worker threads are removed during the close rather than waiting for the dispose function. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/rpc/virnetserver.c | 2 ++ src/util/virthreadpool.c | 19 +++++++++++++------ src/util/virthreadpool.h | 2 ++ 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a705fa846..f1e31ffcb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2864,6 +2864,7 @@ virThreadJobSetWorker; # util/virthreadpool.h +virThreadPoolDrain; virThreadPoolFree; virThreadPoolGetCurrentWorkers; virThreadPoolGetFreeWorkers; diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 7bab11efb..1ae98c244 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -835,6 +835,8 @@ void virNetServerClose(virNetServerPtr srv) for (i = 0; i < srv->nservices; i++) virNetServerServiceClose(srv->services[i]); + virThreadPoolDrain(srv->workers); + for (i = 0; i < srv->nclients; i++) virNetServerClientClose(srv->clients[i]); diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index 10f2bd2c3..f4ac88ddc 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -269,7 +269,8 @@ virThreadPoolNewFull(size_t minWorkers, } -void virThreadPoolFree(virThreadPoolPtr pool) +void +virThreadPoolDrain(virThreadPoolPtr pool) { virThreadPoolJobPtr job; bool priority = false; @@ -294,15 +295,21 @@ void virThreadPoolFree(virThreadPoolPtr pool) VIR_FREE(job); } - VIR_FREE(pool->workers); - virMutexUnlock(&pool->mutex); - virMutexDestroy(&pool->mutex); - virCondDestroy(&pool->quit_cond); - virCondDestroy(&pool->cond); if (priority) { VIR_FREE(pool->prioWorkers); virCondDestroy(&pool->prioCond); } + + virMutexUnlock(&pool->mutex); +} + + +void virThreadPoolFree(virThreadPoolPtr pool) +{ + VIR_FREE(pool->workers); + virMutexDestroy(&pool->mutex); + virCondDestroy(&pool->quit_cond); + virCondDestroy(&pool->cond); VIR_FREE(pool); } diff --git a/src/util/virthreadpool.h b/src/util/virthreadpool.h index e1f362f5b..1b897e1fd 100644 --- a/src/util/virthreadpool.h +++ b/src/util/virthreadpool.h @@ -50,6 +50,8 @@ size_t virThreadPoolGetCurrentWorkers(virThreadPoolPtr pool); size_t virThreadPoolGetFreeWorkers(virThreadPoolPtr pool); size_t virThreadPoolGetJobQueueDepth(virThreadPoolPtr pool); +void virThreadPoolDrain(virThreadPoolPtr pool); + void virThreadPoolFree(virThreadPoolPtr pool); int virThreadPoolSendJob(virThreadPoolPtr pool, -- 2.13.6
On Wed, Jan 10, 2018 at 12:23:29PM -0500, John Ferlan wrote:
Split up virThreadPoolFree to create a Drain function which will be called from virNetServerClose in order to ensure the various worker threads are removed during the close rather than waiting for the dispose function.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
I think that Dan had a bit different idea about the virThreadPoolDrain (I think that the name should have been something like virThreadPoolJobsPurge) function. https://www.redhat.com/archives/libvir-list/2017-December/msg00802.html Erik
On Mon, Jan 15, 2018 at 05:51:28PM +0100, Erik Skultety wrote:
On Wed, Jan 10, 2018 at 12:23:29PM -0500, John Ferlan wrote:
Split up virThreadPoolFree to create a Drain function which will be called from virNetServerClose in order to ensure the various worker threads are removed during the close rather than waiting for the dispose function.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
I think that Dan had a bit different idea about the virThreadPoolDrain (I think that the name should have been something like virThreadPoolJobsPurge) function. https://www.redhat.com/archives/libvir-list/2017-December/msg00802.html
Yep, this impl in John's patch is more akin to a StopWorkers() method, than a Drain() method. To me "Drain" implies the workers are still available to process further jobs 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 01/15/2018 11:57 AM, Daniel P. Berrange wrote:
On Mon, Jan 15, 2018 at 05:51:28PM +0100, Erik Skultety wrote:
On Wed, Jan 10, 2018 at 12:23:29PM -0500, John Ferlan wrote:
Split up virThreadPoolFree to create a Drain function which will be called from virNetServerClose in order to ensure the various worker threads are removed during the close rather than waiting for the dispose function.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
I think that Dan had a bit different idea about the virThreadPoolDrain (I think that the name should have been something like virThreadPoolJobsPurge) function. https://www.redhat.com/archives/libvir-list/2017-December/msg00802.html
Yep, this impl in John's patch is more akin to a StopWorkers() method, than a Drain() method. To me "Drain" implies the workers are still available to process further jobs
Regards, Daniel
OK - right... there's so many links and thoughts with various terminology in the former series... I of course read one path about splitting up PoolFree and just ran with that, but had the more recent stuff in the front of the brain so the Drain name stuck, but the concept of Drain certainly wasn't what Dan described. Let's see what I can come up with for a Drain function. Still trying to wrap my head around the totality of this code - so many nooks and crannies. Still not 100% clear how to handle the what happens if some worker/job is truly stuck and the TERM is attempted. John
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> This function is called by daemon before shutting down netdaemon threads that serves client requests to make sure all these threads will be able to shutdown. Signed-off-by: John Ferlan <jferlan@redhat.com> --- daemon/libvirtd.c | 3 +++ src/driver-state.h | 4 ++++ src/libvirt.c | 18 ++++++++++++++++++ src/libvirt_internal.h | 1 + src/libvirt_private.syms | 1 + 5 files changed, 27 insertions(+) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index b47f875d9..37d66b3e9 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1529,6 +1529,9 @@ int main(int argc, char **argv) { cleanup: /* Keep cleanup order in inverse order of startup */ + if (driversInitialized) + virStateShutdown(); + virNetDaemonClose(dmn); virNetlinkEventServiceStopAll(); diff --git a/src/driver-state.h b/src/driver-state.h index 1cb3e4faf..ea549a7db 100644 --- a/src/driver-state.h +++ b/src/driver-state.h @@ -42,6 +42,9 @@ typedef int typedef int (*virDrvStateStop)(void); +typedef void +(*virDrvStateShutdown)(void); + typedef struct _virStateDriver virStateDriver; typedef virStateDriver *virStateDriverPtr; @@ -52,6 +55,7 @@ struct _virStateDriver { virDrvStateCleanup stateCleanup; virDrvStateReload stateReload; virDrvStateStop stateStop; + virDrvStateShutdown stateShutdown; }; diff --git a/src/libvirt.c b/src/libvirt.c index 536d56f0a..330c5ce8c 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -812,6 +812,24 @@ virStateCleanup(void) /** + * virStateShutdown: + * + * Run each virtualization driver's shutdown method. + * + */ +void +virStateShutdown(void) +{ + int r; + + for (r = virStateDriverTabCount - 1; r >= 0; r--) { + if (virStateDriverTab[r]->stateShutdown) + virStateDriverTab[r]->stateShutdown(); + } +} + + +/** * virStateReload: * * Run each virtualization driver's reload method. diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h index 62f490a7d..9863b0781 100644 --- a/src/libvirt_internal.h +++ b/src/libvirt_internal.h @@ -36,6 +36,7 @@ int virStateInitialize(bool privileged, int virStateCleanup(void); int virStateReload(void); int virStateStop(void); +void virStateShutdown(void); /* Feature detection. This is a libvirt-private interface for determining * what features are supported by the driver. diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f1e31ffcb..48223f8ed 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1211,6 +1211,7 @@ virSetSharedStorageDriver; virStateCleanup; virStateInitialize; virStateReload; +virStateShutdown; virStateStop; virStreamInData; -- 2.13.6
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> Shutdown function should help API calls to finish when event loop is not running anymore. For this reason let's close agent and qemu monitors. These function will unblock API calls wating for response from qemu process or qemu agent. Closing agent monitor and setting priv->agent to NULL when waiting for response is normal usecase (handling EOF from agent is handled the same way for example). However we can not do the same for qemu monitor. This monitor is normally closed and unrefed during qemuProcessStop under destroy job so other threads do not deal with priv->mon. But if we take extra reference to monitor we are good. This can lead to double close but this function looks like to be idempotent. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a203c9297..1de236cb5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1093,6 +1093,44 @@ qemuStateStop(void) return ret; } + +static int +qemuDomainDisconnect(virDomainObjPtr vm, void *opaque ATTRIBUTE_UNUSED) +{ + + qemuDomainObjPrivatePtr priv; + + virObjectLock(vm); + priv = vm->privateData; + + if (priv->mon) { + /* Take extra reference to monitor so it won't be disposed + * by qemuMonitorClose last unref. */ + virObjectRef(priv->mon); + qemuMonitorClose(priv->mon); + } + + if (priv->agent) { + /* Other threads are ready for priv->agent to became NULL meanwhile */ + qemuAgentClose(priv->agent); + priv->agent = NULL; + } + + virObjectUnlock(vm); + return 0; +} + + +static void +qemuStateShutdown(void) +{ + if (!qemu_driver) + return; + + virDomainObjListForEach(qemu_driver->domains, qemuDomainDisconnect, NULL); +} + + /** * qemuStateCleanup: * @@ -21357,6 +21395,7 @@ static virStateDriver qemuStateDriver = { .stateCleanup = qemuStateCleanup, .stateReload = qemuStateReload, .stateStop = qemuStateStop, + .stateShutdown = qemuStateShutdown, }; int qemuRegister(void) -- 2.13.6
On Wed, Jan 10, 2018 at 12:23:31PM -0500, John Ferlan wrote:
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Shutdown function should help API calls to finish when event loop is not running anymore. For this reason let's close agent and qemu monitors. These function will unblock API calls wating for response from qemu process or qemu agent.
Closing agent monitor and setting priv->agent to NULL when waiting for response is normal usecase (handling EOF from agent is handled the same way for example).
However we can not do the same for qemu monitor. This monitor is normally closed and unrefed during qemuProcessStop under destroy job so other threads do not deal with priv->mon. But if we take extra reference to monitor we are good. This can lead to double close but this function looks like to be idempotent.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a203c9297..1de236cb5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1093,6 +1093,44 @@ qemuStateStop(void) return ret; }
+ +static int +qemuDomainDisconnect(virDomainObjPtr vm, void *opaque ATTRIBUTE_UNUSED) +{ + + qemuDomainObjPrivatePtr priv; + + virObjectLock(vm); + priv = vm->privateData; + + if (priv->mon) { + /* Take extra reference to monitor so it won't be disposed + * by qemuMonitorClose last unref. */ + virObjectRef(priv->mon); + qemuMonitorClose(priv->mon); + } + + if (priv->agent) { + /* Other threads are ready for priv->agent to became NULL meanwhile */ + qemuAgentClose(priv->agent); + priv->agent = NULL; + } + + virObjectUnlock(vm); + return 0; +} + + +static void +qemuStateShutdown(void) +{ + if (!qemu_driver) + return; + + virDomainObjListForEach(qemu_driver->domains, qemuDomainDisconnect, NULL); +} + + /** * qemuStateCleanup: * @@ -21357,6 +21395,7 @@ static virStateDriver qemuStateDriver = { .stateCleanup = qemuStateCleanup, .stateReload = qemuStateReload, .stateStop = qemuStateStop, + .stateShutdown = qemuStateShutdown,
I'm curious why we need this separate .stateShutdown hook. It feels like this code could just be placed at the start of qemuStateStop and achieve the same result. 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 01/10/2018 01:05 PM, Daniel P. Berrange wrote:
On Wed, Jan 10, 2018 at 12:23:31PM -0500, John Ferlan wrote:
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Shutdown function should help API calls to finish when event loop is not running anymore. For this reason let's close agent and qemu monitors. These function will unblock API calls wating for response from qemu process or qemu agent.
Closing agent monitor and setting priv->agent to NULL when waiting for response is normal usecase (handling EOF from agent is handled the same way for example).
However we can not do the same for qemu monitor. This monitor is normally closed and unrefed during qemuProcessStop under destroy job so other threads do not deal with priv->mon. But if we take extra reference to monitor we are good. This can lead to double close but this function looks like to be idempotent.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a203c9297..1de236cb5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1093,6 +1093,44 @@ qemuStateStop(void) return ret; }
+ +static int +qemuDomainDisconnect(virDomainObjPtr vm, void *opaque ATTRIBUTE_UNUSED) +{ + + qemuDomainObjPrivatePtr priv; + + virObjectLock(vm); + priv = vm->privateData; + + if (priv->mon) { + /* Take extra reference to monitor so it won't be disposed + * by qemuMonitorClose last unref. */ + virObjectRef(priv->mon); + qemuMonitorClose(priv->mon); + } + + if (priv->agent) { + /* Other threads are ready for priv->agent to became NULL meanwhile */ + qemuAgentClose(priv->agent); + priv->agent = NULL; + } + + virObjectUnlock(vm); + return 0; +} + + +static void +qemuStateShutdown(void) +{ + if (!qemu_driver) + return; + + virDomainObjListForEach(qemu_driver->domains, qemuDomainDisconnect, NULL); +} + + /** * qemuStateCleanup: * @@ -21357,6 +21395,7 @@ static virStateDriver qemuStateDriver = { .stateCleanup = qemuStateCleanup, .stateReload = qemuStateReload, .stateStop = qemuStateStop, + .stateShutdown = qemuStateShutdown,
I'm curious why we need this separate .stateShutdown hook. It feels like this code could just be placed at the start of qemuStateStop and achieve the same result.
This path is only run when compiled "WITH_DBUS" and it only matters when (!virNetDaemonIsPrivileged(dmn)). Also since qemuStateStop will suspend and save to disk all active domains, it doesn't seem that's what may be desired in this case where libvirtd is stopped. John
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> Normally if first agent sync is failed we retry. First sync can also be failed due to agent was closed. In this case we should fail sync otherwise second attempt will hang. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_agent.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 5d125c413..e1440ec27 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -987,17 +987,17 @@ qemuAgentGuestSync(qemuAgentPtr mon) goto cleanup; if (!sync_msg.rxObject) { - if (sync_msg.first) { + if (!mon->running) { + virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s", + _("Guest agent disappeared while executing command")); + goto cleanup; + } else if (sync_msg.first) { VIR_FREE(sync_msg.txBuffer); memset(&sync_msg, 0, sizeof(sync_msg)); goto retry; } else { - if (mon->running) - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing monitor reply object")); - else - virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s", - _("Guest agent disappeared while executing command")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing monitor reply object")); goto cleanup; } } -- 2.13.6
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> Close monitor sets monitor error if another thread is awating the response to propagate error condition to that thread. However if there is no such thread error will not be set. Now if API thread try to send a message it will hang. This can easily happen for example if API thread does not reach the point when it take domain lock and qemu driver is shutdowned. Let's add checks for whether monitor is closed to send routine and remove passing of this condition thru setting monitor error. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 046caf001..d97506e11 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1001,22 +1001,9 @@ qemuMonitorClose(qemuMonitorPtr mon) } /* In case another thread is waiting for its monitor command to be - * processed, we need to wake it up with appropriate error set. + * processed, we need to wake it up. */ if (mon->msg) { - if (mon->lastError.code == VIR_ERR_OK) { - virErrorPtr err = virSaveLastError(); - - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("QEMU monitor was closed")); - virCopyLastError(&mon->lastError); - if (err) { - virSetError(err); - virFreeError(err); - } else { - virResetLastError(); - } - } mon->msg->finished = 1; virCondSignal(&mon->notify); } @@ -1048,6 +1035,12 @@ qemuMonitorSend(qemuMonitorPtr mon, { int ret = -1; + if (mon->fd < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("QEMU monitor was closed")); + return -1; + } + /* Check whether qemu quit unexpectedly */ if (mon->lastError.code != VIR_ERR_OK) { VIR_DEBUG("Attempt to send command while error is set %s", @@ -1071,6 +1064,12 @@ qemuMonitorSend(qemuMonitorPtr mon, } } + if (mon->fd < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("QEMU monitor was closed")); + goto cleanup; + } + if (mon->lastError.code != VIR_ERR_OK) { VIR_DEBUG("Send command resulted in error %s", NULLSTR(mon->lastError.message)); -- 2.13.6
Modify GetAllStats to generate a long enough pause in order to send a SIGTERM to libvirtd while a client connection is processing. In order to "set things up": 1. In one terminal window from a local branch built using these patches using a root account run libvirtd debug, e.g.: # ./run gdb daemon/libvirtd once running, type a 'c' (e.g. continue) and <return> 2. Start a domain (or have one running with the current libvirtd) virsh start $domain 3. Prepare a domstats command for that domain (but don't yet hit <return> in order run it): virsh domstats $domain 4. Prepare a kill command for the running libvirtd, e.g.: jferlan 4143 4107 0 09:51 pts/1 00:00:00 vim +1396 daemon/libvirtd.c root 30054 21195 6 11:17 pts/8 00:00:01 gdb /home/jferlan/git/libvirt.work/daemon/.libs/lt-libvirtd root 30087 30054 7 11:17 pts/8 00:00:01 /home/jferlan/git/libvirt.work/daemon/.libs/lt-libvirtd root 30385 19861 0 11:17 pts/17 00:00:00 grep --color=auto libvirtd but again don't hit <return> yet. 5. Align your stars perfectly now... a. Hit <return> on your domstats command b. Swap to the kill command window and hit <return> This should cause the libvirtd debug window to stop, but since you already typed 'c' it'll continue at least briefly, for example: ... [Thread 0x7fffc3231700 (LWP 30374) exited] Detaching after fork from child process 30376. Detaching after fork from child process 30377. Detaching after fork from child process 30378. [Thread 0x7fffc4445700 (LWP 30106) exited] c 2018-01-10 16:18:12.962+0000: 30094: info : libvirt version: 4.0.0 2018-01-10 16:18:12.962+0000: 30094: info : hostname: unknown4ceb42c824f4.attlocal.net 2018-01-10 16:18:12.962+0000: 30094: warning : qemuConnectGetAllDomainStats:20265 : k = -5340232226128654848 Thread 1 "lt-libvirtd" received signal SIGTERM, Terminated. 0x00007ffff3ae6d2d in poll () from /lib64/libc.so.6 ... (gdb) c Continuing. [Thread 0x7fffc5c48700 (LWP 30103) exited] [Thread 0x7fffc5447700 (LWP 30104) exited] [Thread 0x7fffc4c46700 (LWP 30105) exited] [Thread 0x7fffc6449700 (LWP 30102) exited] [Thread 0x7fffc6c4a700 (LWP 30101) exited] [Thread 0x7fffe3b57700 (LWP 30097) exited] [Thread 0x7fffe4358700 (LWP 30096) exited] [Thread 0x7fffe2354700 (LWP 30100) exited] [Thread 0x7fffe3356700 (LWP 30098) exited] [Thread 0x7fffe2b55700 (LWP 30099) exited] [Thread 0x7fffe535a700 (LWP 30094) exited] [Thread 0x7fffe5b5b700 (LWP 30093) exited] [Thread 0x7fffe635c700 (LWP 30092) exited] [Thread 0x7fffe6b5d700 (LWP 30091) exited] 2018-01-10 16:18:25.451+0000: 30095: warning : qemuConnectGetAllDomainStats:20265 : k = -5340232226128654848 [Thread 0x7fffe4b59700 (LWP 30095) exited] [Thread 0x7fffc3a32700 (LWP 30187) exited] [Inferior 1 (process 30087) exited normally] (gdb) c The program is not being run. (gdb) quit The virsh domstats window will "close" as follows: error: Disconnected from qemu:///system due to end of file error: End of file while reading data: Input/output error If something's wrong, then the libvirtd window may not exit those final two threads in which case you could interrupt it (^c) and check the threads (thread apply all bt) which will probably show some sort of hang... My testing shows that the hang no longer occurs with all the previous patches applied. The subsequent patch calling virHashRemoveAll from virNetDaemonClose does not seem to be necessary, although I suppose it cannot hurt as the same essential functionality occurs during the Dispose function Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1de236cb5..0d0b03d86 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20222,6 +20222,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, bool enforce = !!(flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS); int nstats = 0; size_t i; + size_t j, k = 0; int ret = -1; unsigned int privflags = 0; unsigned int domflags = 0; @@ -20259,6 +20260,10 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, if (qemuDomainGetStatsNeedMonitor(stats)) privflags |= QEMU_DOMAIN_STATS_HAVE_JOB; + for (j = 0; j < 10000000000; j++) // Add one more zero for longer... + k = j + k; + VIR_WARN("k = %zd", k); + for (i = 0; i < nvms; i++) { virDomainStatsRecordPtr tmp = NULL; domflags = 0; -- 2.13.6
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> [NB: Taken from earlier patch, although I'm not sure this patch would be necessary any more...] The problem is incorrect order of qemu driver shutdown and shutdown of netserver threads that serve client requests (thru qemu driver particularly). Net server threads are shutdown upon dispose which is triggered by last daemon object unref at the end of main function. At the same time qemu driver is shutdown earlier in virStateCleanup. As a result netserver threads see invalid driver object in the middle of request processing. Let's move shutting down netserver threads earlier to virNetDaemonClose. Note: order of last daemon unref and virStateCleanup is introduced in 85c3a182 for a valid reason. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/rpc/virnetdaemon.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index 8c2141489..33bd8e3b0 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -881,6 +881,7 @@ virNetDaemonClose(virNetDaemonPtr dmn) virObjectLock(dmn); virHashForEach(dmn->servers, daemonServerClose, NULL); + virHashRemoveAll(dmn->servers); virObjectUnlock(dmn); } -- 2.13.6
participants (3)
-
Daniel P. Berrange -
Erik Skultety -
John Ferlan