[libvirt] [PATCH 0/4] libvirtd: fix hang on termination in qemu driver
Libvirtd termination can hang. For example if some API call in qemu driver awaiting monitor response it will never finish because event loop does not functional during termination. As a result we hang in virNetDaemonClose call during termination as this call finishes RPC threads. Let's ask hypervisor drivers to finish all API calls by calling introduced state driver shutdown function before call to virNetDaemonClose. Nikolay Shirokovskiy (4): libvirt: introduce hypervisor driver shutdown function qemu: implement state driver shutdown function qemu: agent: fix monitor close during first sync qemu: monitor: check monitor not closed upon send daemon/libvirtd.c | 2 ++ src/driver-state.h | 4 ++++ src/libvirt.c | 18 ++++++++++++++++++ src/libvirt_internal.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_agent.c | 14 +++++++------- src/qemu/qemu_driver.c | 39 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 27 +++++++++++++-------------- 8 files changed, 85 insertions(+), 21 deletions(-) -- 1.8.3.1
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. --- daemon/libvirtd.c | 2 ++ src/driver-state.h | 4 ++++ src/libvirt.c | 18 ++++++++++++++++++ src/libvirt_internal.h | 1 + src/libvirt_private.syms | 1 + 5 files changed, 26 insertions(+) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 589b321..d2bbe1e 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1504,6 +1504,8 @@ int main(int argc, char **argv) { virObjectUnref(lxcProgram); virObjectUnref(qemuProgram); virObjectUnref(adminProgram); + if (driversInitialized) + virStateShutdown(); virNetDaemonClose(dmn); virObjectUnref(srv); virObjectUnref(srvAdm); diff --git a/src/driver-state.h b/src/driver-state.h index 1cb3e4f..ea549a7 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 536d56f..330c5ce 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 62f490a..9863b07 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 448d962..84af751 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1194,6 +1194,7 @@ virSetSharedStorageDriver; virStateCleanup; virStateInitialize; virStateReload; +virStateShutdown; virStateStop; virStreamInData; -- 1.8.3.1
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. --- 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 32a416f..c9adb58 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1067,6 +1067,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: * @@ -21286,6 +21324,7 @@ static virStateDriver qemuStateDriver = { .stateCleanup = qemuStateCleanup, .stateReload = qemuStateReload, .stateStop = qemuStateStop, + .stateShutdown = qemuStateShutdown, }; int qemuRegister(void) -- 1.8.3.1
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. --- 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 5d125c4..e1440ec 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; } } -- 1.8.3.1
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. --- 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 64efb89..63e6f74 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1000,22 +1000,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); } @@ -1047,6 +1034,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", @@ -1070,6 +1063,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)); -- 1.8.3.1
On 10/25/2017 05:05 AM, Nikolay Shirokovskiy wrote:
Libvirtd termination can hang. For example if some API call in qemu driver awaiting monitor response it will never finish because event loop does not functional during termination. As a result we hang in virNetDaemonClose call during termination as this call finishes RPC threads.
Let's ask hypervisor drivers to finish all API calls by calling introduced state driver shutdown function before call to virNetDaemonClose.
Nikolay Shirokovskiy (4): libvirt: introduce hypervisor driver shutdown function qemu: implement state driver shutdown function qemu: agent: fix monitor close during first sync qemu: monitor: check monitor not closed upon send
daemon/libvirtd.c | 2 ++ src/driver-state.h | 4 ++++ src/libvirt.c | 18 ++++++++++++++++++ src/libvirt_internal.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_agent.c | 14 +++++++------- src/qemu/qemu_driver.c | 39 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 27 +++++++++++++-------------- 8 files changed, 85 insertions(+), 21 deletions(-)
Moving the discussion started in the refcount thread to here via just simple cut-n-paste... I didn't CC Erik or Martin directly because I'm fairly confident they read libvir-list posts anyway ;-) John [jferlan comment] https://www.redhat.com/archives/libvir-list/2017-November/msg00236.html The other series seems to be adding a state shutdown step to be run before state cleanup; however, if we alter the cleanup code in libvirtd does that make the state shutdown step unnecessary? I don't know and really didn't want to think about it until we got through thinking about the cleanup processing order. Still if one considers that state initialization is really two steps (init and auto start), then splitting cleanup into shutdown and cleanup seems reasonable, but I don't think affects whether we have a crash or latent usage of dmn->servers. I could be wrong, but it's so hard to tell. [eskultety comment/followup] https://www.redhat.com/archives/libvir-list/2017-November/msg00249.html Actually, no, the stateShutdown was addressing the issue with running <driver>StateCleanup while there still was an active worker thread that could potentially access the @driver object, in which case - SIGSEGV, but as I wrote in my response, I don't like the idea of having another state driver callback, as I feel it introduces a bit of ambiguity, since by just looking at the callback names, you can't have an idea, what the difference between stateCleanup and stateShutdown is. I think this should be handled as part of the final stage of leaving the event loop and simply register an appropriate callback to the handle, that way, you don't need to define a new state callback which would most likely used by a couple of drivers. [mkletzan comment/followup] https://www.redhat.com/archives/libvir-list/2017-November/msg00450.html If stateShutdown is what makes everything go away, then why not? The names are pretty descriptive, shutdown is called when the daemon is shutting down and cleanup when you need to clean up after yourself. Used by only few drivers? Well, we can implement it everywhere and not make it make optional, but rather mandatory. If that is checked on the registration then we'll know right away when we missed something (aat runtime, but right after someone tries running it). -----
On 13.11.2017 18:46, John Ferlan wrote:
On 10/25/2017 05:05 AM, Nikolay Shirokovskiy wrote:
Libvirtd termination can hang. For example if some API call in qemu driver awaiting monitor response it will never finish because event loop does not functional during termination. As a result we hang in virNetDaemonClose call during termination as this call finishes RPC threads.
Let's ask hypervisor drivers to finish all API calls by calling introduced state driver shutdown function before call to virNetDaemonClose.
Nikolay Shirokovskiy (4): libvirt: introduce hypervisor driver shutdown function qemu: implement state driver shutdown function qemu: agent: fix monitor close during first sync qemu: monitor: check monitor not closed upon send
daemon/libvirtd.c | 2 ++ src/driver-state.h | 4 ++++ src/libvirt.c | 18 ++++++++++++++++++ src/libvirt_internal.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_agent.c | 14 +++++++------- src/qemu/qemu_driver.c | 39 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 27 +++++++++++++-------------- 8 files changed, 85 insertions(+), 21 deletions(-)
Moving the discussion started in the refcount thread to here via just simple cut-n-paste... I didn't CC Erik or Martin directly because I'm fairly confident they read libvir-list posts anyway ;-)
John
[jferlan comment] https://www.redhat.com/archives/libvir-list/2017-November/msg00236.html
The other series seems to be adding a state shutdown step to be run before state cleanup; however, if we alter the cleanup code in libvirtd does that make the state shutdown step unnecessary? I don't know and really didn't want to think about it until we got through thinking about the cleanup processing order. Still if one considers that state initialization is really two steps (init and auto start), then splitting cleanup into shutdown and cleanup seems reasonable, but I don't think affects whether we have a crash or latent usage of dmn->servers. I could be wrong, but it's so hard to tell.
[eskultety comment/followup] https://www.redhat.com/archives/libvir-list/2017-November/msg00249.html
Actually, no, the stateShutdown was addressing the issue with running <driver>StateCleanup while there still was an active worker thread that could potentially access the @driver object, in which case - SIGSEGV, but as I wrote in my response, I don't like the idea of having another state driver callback, as I feel it introduces a bit of ambiguity, since by just looking at the callback names, you can't have an idea, what the difference between stateCleanup and stateShutdown is. I think this should be handled as part of the final stage of leaving the event loop and simply register an appropriate callback to the handle, that way, you don't need to define a new state callback which would most likely used by a couple of drivers.
This way we will indroduce some function in event API to notify event loop clients of shutdown, something like virEventNotifyShutdown. And we will probably introduce another function for clients to register for shutdown event. Thus in terms of implementation and API this new piece of functionality is fully apart from the current event loop function. So I'm not for this functional mixup. Nikolay
[mkletzan comment/followup] https://www.redhat.com/archives/libvir-list/2017-November/msg00450.html
If stateShutdown is what makes everything go away, then why not? The names are pretty descriptive, shutdown is called when the daemon is shutting down and cleanup when you need to clean up after yourself. Used by only few drivers? Well, we can implement it everywhere and not make it make optional, but rather mandatory. If that is checked on the registration then we'll know right away when we missed something (aat runtime, but right after someone tries running it).
-----
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy