On 27.04.2020 18:54, Daniel P. Berrangé wrote:

We got a new BZ filed about a libvirtd crash in shutdown

https://bugzilla.redhat.com/show_bug.cgi?id=1828207

We can see from the stack trace that the "interface" driver is in the middle of servicing an RPC call for virConnectListAllInterfaces()

Meanwhile the libvirtd daemon is doing virObjectUnref(dmn) on the virNetDaemonPtr object.

The fact that it is doing this unref, means that it must have already call virStateCleanup(), given the code sequence:

/* Run event loop. */ virNetDaemonRun(dmn);

ret = 0;

virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_SHUTDOWN, 0, "shutdown", NULL, NULL);

cleanup: /* Keep cleanup order in inverse order of startup */ virNetDaemonClose(dmn);

virNetlinkEventServiceStopAll();

if (driversInitialized) { /* NB: Possible issue with timing window between driversInitialized * setting if virNetlinkEventServerStart fails */ driversInitialized = false; virStateCleanup(); }

virObjectUnref(adminProgram); virObjectUnref(srvAdm); virObjectUnref(qemuProgram); virObjectUnref(lxcProgram); virObjectUnref(remoteProgram); virObjectUnref(srv); virObjectUnref(dmn);

Unless I'm missing something non-obvious, this cleanup code path is inherantly broken & racy. When virNetDaemonRun() returns the RPC worker threads are all still active. They are all liable to still be executing RPC calls, which means any of the drivers may be in use. So calling virStateCleanup() is an inherantly dangerous thing to do. There is the further complication that once we have exitted the main loop we may prevent the RPC calls from ever completing, as they may be waiting on an event to be dispatched.

I know we're had various patch proposals in the past to improve the robustness of shutdown cleanup but I can't remember the outcome of the reviews. Hopefully people involved in those threads can jump in here...

Hi, all! Yeah I and John Ferlan attempted to address the issue in the past. The last reference I found is [1]. One can dig down the history of the issue thru the links inside. IIRC the latest approach was similar to what you suggested below namely run event loop for a while after quit is seen in order to let RPC worker threads waiting for event to finish. [1] https://www.redhat.com/archives/libvir-list/2018-July/msg00382.html

IMHO the key problem here is the virNetDeamonRun() method which just looks at the "quit" flag and immediately returns if it is set. This needs to be changed so that when it sees quit == true, it takes the following actions

1. Call virNetDaemonClose() to drop all RPC clients and thus prevent new RPC calls arriving 2. Flush any RPC calls which are queued but not yet assigned to a worker thread 3. Tell worker threads to exit after finishing their current job 4. Wait for all worker threads to exit 5. Now virNetDaemonRun may return

At this point we can call virStateCleanup and the various other things, as we know no drivers are still active in RPC calls.

Having said that, there could be background threads in the the drivers which are doing work that uses the event loop thread.

So we probably need a virStateClose() method that we call from virNetDaemonRun, *after* all worker threads are gone, which would cleanup any background threads while the event loop is still running.

I guess RPC worker threads may need some signal to finish in time too. For example in case of migration. Nikolay

The issue is that step 4 above ("Wait for all worker threads to exit") may take too long, or indeed never complete. To deal with this, it will need a timeout. In the remote_daemon.c cleanup code path, if there are still worker threads present, then we need to skip all cleanup and simply call _exit(0) to terminate the process with no attempt at cleanup, since it would be unsafe to try anything else.

Regards, Daniel