When virNetDaemonQuit is called from libvirtd's shutdown
handler (daemonShutdownHandler) we need to perform the quit
in multiple steps. The first part is to "request" the quit
and notify the NetServer's of the impending quit which causes
the NetServers to inform their workers that a quit was requested.

Still because we cannot guarantee a quit will happen or it's
possible there's no workers pending, use a virNetDaemonQuitTimer
to not only break the event loop but keep track of how long we're
waiting and we've waited too long, force an ungraceful exit so
that we don't hang waiting forever or cause some sort of SEGV
because something is still pending and we Unref things.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/libvirt_remote.syms    |  1 +
 src/remote/remote_daemon.c |  1 +
 src/rpc/virnetdaemon.c     | 68 +++++++++++++++++++++++++++++++++++++-
 src/rpc/virnetdaemon.h     |  4 +++
 4 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index a8f937f32e..e416cfec44 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -87,6 +87,7 @@ virNetDaemonPreExecRestart;
 virNetDaemonQuit;
 virNetDaemonRemoveShutdownInhibition;
 virNetDaemonRun;
+virNetDaemonSetQuitTimeout;
 virNetDaemonUpdateServices;
 
 
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index dee634d7e1..d11adf422f 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -1273,6 +1273,7 @@ int main(int argc, char **argv) {
         ret = VIR_DAEMON_ERR_DRIVER;
         goto cleanup;
     }
+    virNetDaemonSetQuitTimeout(dmn, config->quit_timeout);
 
     if (!(srv = virNetServerNew("libvirtd", 1,
                                 config->min_workers,
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 329f116a6c..c6ed65c8c3 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -73,6 +73,8 @@ struct _virNetDaemon {
     virHashTablePtr servers;
     virJSONValuePtr srvObject;
 
+    unsigned int quitTimeout;
+    bool quitRequested;
     bool quit;
 
     unsigned int autoShutdownTimeout;
@@ -153,6 +155,14 @@ virNetDaemonNew(void)
 }
 
 
+void
+virNetDaemonSetQuitTimeout(virNetDaemonPtr dmn,
+                           unsigned int quitTimeout)
+{
+    dmn->quitTimeout = quitTimeout;
+}
+
+
 int
 virNetDaemonAddServer(virNetDaemonPtr dmn,
                       virNetServerPtr srv)
@@ -791,11 +801,50 @@ daemonServerProcessClients(void *payload,
     return 0;
 }
 
+
+static int
+daemonServerWorkerCount(void *payload,
+                        const void *key ATTRIBUTE_UNUSED,
+                        void *opaque)
+{
+    size_t *workerCount = opaque;
+    virNetServerPtr srv = payload;
+
+    *workerCount += virNetServerWorkerCount(srv);
+
+    return 0;
+}
+
+
+static bool
+daemonServerWorkersDone(virNetDaemonPtr dmn)
+{
+    size_t workerCount = 0;
+
+    virHashForEach(dmn->servers, daemonServerWorkerCount, &workerCount);
+
+    return workerCount == 0;
+}
+
+
+static void
+virNetDaemonQuitTimer(int timer ATTRIBUTE_UNUSED,
+                      void *opaque)
+{
+    int *quitCount = opaque;
+
+    (*quitCount)++;
+    VIR_DEBUG("quitCount=%d", *quitCount);
+}
+
+
 void
 virNetDaemonRun(virNetDaemonPtr dmn)
 {
     int timerid = -1;
     bool timerActive = false;
+    int quitTimer = -1;
+    int quitCount = 0;
 
     virObjectLock(dmn);
 
@@ -855,10 +904,27 @@ virNetDaemonRun(virNetDaemonPtr dmn)
         virObjectLock(dmn);
 
         virHashForEach(dmn->servers, daemonServerProcessClients, NULL);
+
+        /* HACK: Add a dummy timeout to break event loop */
+        if (dmn->quitRequested && quitTimer == -1)
+            quitTimer = virEventAddTimeout(500, virNetDaemonQuitTimer,
+                                           &quitCount, NULL);
+
+        if (dmn->quitRequested && daemonServerWorkersDone(dmn)) {
+            dmn->quit = true;
+        } else {
+            /* Firing every 1/2 second and quitTimeout in seconds, force
+             * an exit when there are still worker threads running and we
+             * have waited long enough */
+            if (quitCount > dmn->quitTimeout * 2)
+                _exit(EXIT_FAILURE);
+        }
     }
 
  cleanup:
     virObjectUnlock(dmn);
+    if (quitTimer != -1)
+        virEventRemoveTimeout(quitTimer);
 }
 
 
@@ -880,7 +946,7 @@ virNetDaemonQuit(virNetDaemonPtr dmn)
     virObjectLock(dmn);
 
     VIR_DEBUG("Quit requested %p", dmn);
-    dmn->quit = true;
+    dmn->quitRequested = true;
     virHashForEach(dmn->servers, daemonServerQuitRequested, NULL);
 
     virObjectUnlock(dmn);
diff --git a/src/rpc/virnetdaemon.h b/src/rpc/virnetdaemon.h
index 09ed5adf36..8433d6a09d 100644
--- a/src/rpc/virnetdaemon.h
+++ b/src/rpc/virnetdaemon.h
@@ -35,6 +35,10 @@
 
 virNetDaemonPtr virNetDaemonNew(void);
 
+void
+virNetDaemonSetQuitTimeout(virNetDaemonPtr dmn,
+                           unsigned int quitTimeout);
+
 int virNetDaemonAddServer(virNetDaemonPtr dmn,
                           virNetServerPtr srv);
 
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Sat, Jul 07, 2018 at 08:11:05 -0400, John Ferlan wrote:
> When virNetDaemonQuit is called from libvirtd's shutdown
> handler (daemonShutdownHandler) we need to perform the quit
> in multiple steps. The first part is to "request" the quit
> and notify the NetServer's of the impending quit which causes
> the NetServers to inform their workers that a quit was requested.
> 
> Still because we cannot guarantee a quit will happen or it's
> possible there's no workers pending, use a virNetDaemonQuitTimer
> to not only break the event loop but keep track of how long we're
> waiting and we've waited too long, force an ungraceful exit so
> that we don't hang waiting forever or cause some sort of SEGV
> because something is still pending and we Unref things.
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/libvirt_remote.syms    |  1 +
>  src/remote/remote_daemon.c |  1 +
>  src/rpc/virnetdaemon.c     | 68 +++++++++++++++++++++++++++++++++++++-
>  src/rpc/virnetdaemon.h     |  4 +++
>  4 files changed, 73 insertions(+), 1 deletion(-)

[...]

> @@ -855,10 +904,27 @@ virNetDaemonRun(virNetDaemonPtr dmn)
>          virObjectLock(dmn);
>  
>          virHashForEach(dmn->servers, daemonServerProcessClients, NULL);
> +
> +        /* HACK: Add a dummy timeout to break event loop */
> +        if (dmn->quitRequested && quitTimer == -1)
> +            quitTimer = virEventAddTimeout(500, virNetDaemonQuitTimer,
> +                                           &quitCount, NULL);
> +
> +        if (dmn->quitRequested && daemonServerWorkersDone(dmn)) {
> +            dmn->quit = true;
> +        } else {
> +            /* Firing every 1/2 second and quitTimeout in seconds, force
> +             * an exit when there are still worker threads running and we
> +             * have waited long enough */
> +            if (quitCount > dmn->quitTimeout * 2)
> +                _exit(EXIT_FAILURE);

If you have a legitimate long-running job which would finish eventually
and e.g. write an updated status XML this will break things. I'm not
persuaded that this is a systematic solution to some API getting stuck.

The commit message also does not help persuading me that this is a good
idea.

NACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 07/09/2018 03:11 AM, Peter Krempa wrote:
> On Sat, Jul 07, 2018 at 08:11:05 -0400, John Ferlan wrote:
>> When virNetDaemonQuit is called from libvirtd's shutdown
>> handler (daemonShutdownHandler) we need to perform the quit
>> in multiple steps. The first part is to "request" the quit
>> and notify the NetServer's of the impending quit which causes
>> the NetServers to inform their workers that a quit was requested.
>>
>> Still because we cannot guarantee a quit will happen or it's
>> possible there's no workers pending, use a virNetDaemonQuitTimer
>> to not only break the event loop but keep track of how long we're
>> waiting and we've waited too long, force an ungraceful exit so
>> that we don't hang waiting forever or cause some sort of SEGV
>> because something is still pending and we Unref things.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>  src/libvirt_remote.syms    |  1 +
>>  src/remote/remote_daemon.c |  1 +
>>  src/rpc/virnetdaemon.c     | 68 +++++++++++++++++++++++++++++++++++++-
>>  src/rpc/virnetdaemon.h     |  4 +++
>>  4 files changed, 73 insertions(+), 1 deletion(-)
> 
> [...]
> 
>> @@ -855,10 +904,27 @@ virNetDaemonRun(virNetDaemonPtr dmn)
>>          virObjectLock(dmn);
>>  
>>          virHashForEach(dmn->servers, daemonServerProcessClients, NULL);
>> +
>> +        /* HACK: Add a dummy timeout to break event loop */
>> +        if (dmn->quitRequested && quitTimer == -1)
>> +            quitTimer = virEventAddTimeout(500, virNetDaemonQuitTimer,
>> +                                           &quitCount, NULL);
>> +
>> +        if (dmn->quitRequested && daemonServerWorkersDone(dmn)) {
>> +            dmn->quit = true;
>> +        } else {
>> +            /* Firing every 1/2 second and quitTimeout in seconds, force
>> +             * an exit when there are still worker threads running and we
>> +             * have waited long enough */
>> +            if (quitCount > dmn->quitTimeout * 2)
>> +                _exit(EXIT_FAILURE);
> 
> If you have a legitimate long-running job which would finish eventually
> and e.g. write an updated status XML this will break things. I'm not
> persuaded that this is a systematic solution to some API getting stuck.
> 
> The commit message also does not help persuading me that this is a good
> idea.
> 
> NACK
> 

I understand the sentiment and this hasn't been a "top of the list" item
for me to think about, but to a degree the purpose of re-posting the
series was to try to come to a consensus over some solution. A naked
NACK almost makes it appear as if you would prefer to not do anything in
this situation, letting nature takes it's course (so to speak).

Do you have any thoughts or suggestions related to what processing you
believe is appropriate if someone issues a SIG{INT|QUIT|TERM} to a
daemon (libvirtd|lockd|logd}? That is what to do if we reach the
cleanup: label in main() of src/remote/remote_daemon.c and
virNetDaemonClose() gets run? Right now IIRC it's either going to be a
SEGV or possible long wait for some worker thread to complete. The
latter is the don't apply this example to cause a wait in block stats
fetch. Naturally the long term wait only matters up through the point
while someone is patient before issuing a SIGKILL.

Do you favor waiting forever for some worker thread to finish? To some
degree if some signal is sent to libvirtd, then I would think the
expectation is to not wait for some indeterminate time. And most
definitely trying to avoid some sort of defunct state resolvable by a
host reboot. Knowing exactly what a worker thread is doing may help, but
that'd take a bit (;-)) of code to trace the stack. Perhaps providing a
list of client/workers still active or even some indication that we're
waiting on some worker rather than no feedback? How much is "too much"?

Another option that was proposed, but didn't really gain any support was
introduction of a stateShutdown in virStateDriver that would be run
during libvirtd's cleanup: processing prior to virNetDaemonClose. That
would be before virStateCleanup. For qemu, the priv->mon and priv->agent
would be closed. That would seemingly generate an error and cause the
worker to exit thus theoretically not needing any forced quit of the
thread "if" the reason was some sort of hang as a result of
monitor/agent processing. Doesn't mean there wouldn't be some other
issue causing a hang. Refreshing my memory on this - there was some
discussion or investigation related to using virStateStop, but I since
that's gated by "WITH_DBUS" being defined, it wasn't clear if/how it
could be used (daemonStop is only called/setup if daemonRunStateInit has
set it up).

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Jul 12, 2018 at 03:34:00PM -0400, John Ferlan wrote:
> 
> 
> On 07/09/2018 03:11 AM, Peter Krempa wrote:
> > On Sat, Jul 07, 2018 at 08:11:05 -0400, John Ferlan wrote:
> >> When virNetDaemonQuit is called from libvirtd's shutdown
> >> handler (daemonShutdownHandler) we need to perform the quit
> >> in multiple steps. The first part is to "request" the quit
> >> and notify the NetServer's of the impending quit which causes
> >> the NetServers to inform their workers that a quit was requested.
> >>
> >> Still because we cannot guarantee a quit will happen or it's
> >> possible there's no workers pending, use a virNetDaemonQuitTimer
> >> to not only break the event loop but keep track of how long we're
> >> waiting and we've waited too long, force an ungraceful exit so
> >> that we don't hang waiting forever or cause some sort of SEGV
> >> because something is still pending and we Unref things.
> >>
> >> Signed-off-by: John Ferlan <jferlan@redhat.com>
> >> ---
> >>  src/libvirt_remote.syms    |  1 +
> >>  src/remote/remote_daemon.c |  1 +
> >>  src/rpc/virnetdaemon.c     | 68 +++++++++++++++++++++++++++++++++++++-
> >>  src/rpc/virnetdaemon.h     |  4 +++
> >>  4 files changed, 73 insertions(+), 1 deletion(-)
> > 
> > [...]
> > 
> >> @@ -855,10 +904,27 @@ virNetDaemonRun(virNetDaemonPtr dmn)
> >>          virObjectLock(dmn);
> >>  
> >>          virHashForEach(dmn->servers, daemonServerProcessClients, NULL);
> >> +
> >> +        /* HACK: Add a dummy timeout to break event loop */
> >> +        if (dmn->quitRequested && quitTimer == -1)
> >> +            quitTimer = virEventAddTimeout(500, virNetDaemonQuitTimer,
> >> +                                           &quitCount, NULL);
> >> +
> >> +        if (dmn->quitRequested && daemonServerWorkersDone(dmn)) {
> >> +            dmn->quit = true;
> >> +        } else {
> >> +            /* Firing every 1/2 second and quitTimeout in seconds, force
> >> +             * an exit when there are still worker threads running and we
> >> +             * have waited long enough */
> >> +            if (quitCount > dmn->quitTimeout * 2)
> >> +                _exit(EXIT_FAILURE);
> > 
> > If you have a legitimate long-running job which would finish eventually
> > and e.g. write an updated status XML this will break things. I'm not
> > persuaded that this is a systematic solution to some API getting stuck.
> > 
> > The commit message also does not help persuading me that this is a good
> > idea.
> > 
> > NACK
> > 
> 
> I understand the sentiment and this hasn't been a "top of the list" item
> for me to think about, but to a degree the purpose of re-posting the
> series was to try to come to a consensus over some solution. A naked
> NACK almost makes it appear as if you would prefer to not do anything in
> this situation, letting nature takes it's course (so to speak).
> 
> Do you have any thoughts or suggestions related to what processing you
> believe is appropriate if someone issues a SIG{INT|QUIT|TERM} to a
> daemon (libvirtd|lockd|logd}? That is what to do if we reach the
> cleanup: label in main() of src/remote/remote_daemon.c and
> virNetDaemonClose() gets run? Right now IIRC it's either going to be a
> SEGV or possible long wait for some worker thread to complete. The
> latter is the don't apply this example to cause a wait in block stats
> fetch. Naturally the long term wait only matters up through the point
> while someone is patient before issuing a SIGKILL.
> 
> Do you favor waiting forever for some worker thread to finish? To some
> degree if some signal is sent to libvirtd, then I would think the
> expectation is to not wait for some indeterminate time. And most
> definitely trying to avoid some sort of defunct state resolvable by a
> host reboot. Knowing exactly what a worker thread is doing may help, but
> that'd take a bit (;-)) of code to trace the stack. Perhaps providing a
> list of client/workers still active or even some indication that we're
> waiting on some worker rather than no feedback? How much is "too much"?
> 
> Another option that was proposed, but didn't really gain any support was
> introduction of a stateShutdown in virStateDriver that would be run
> during libvirtd's cleanup: processing prior to virNetDaemonClose. That
> would be before virStateCleanup. For qemu, the priv->mon and priv->agent
> would be closed. That would seemingly generate an error and cause the
> worker to exit thus theoretically not needing any forced quit of the
> thread "if" the reason was some sort of hang as a result of
> monitor/agent processing. Doesn't mean there wouldn't be some other
> issue causing a hang. Refreshing my memory on this - there was some
> discussion or investigation related to using virStateStop, but I since
> that's gated by "WITH_DBUS" being defined, it wasn't clear if/how it
> could be used (daemonStop is only called/setup if daemonRunStateInit has
> set it up).

I think the key thing to bear in mind is what is the benefit of what we
are trying todo. ie why do we need todo a clean shutdown instead of just
immediately exiting. The kernel will clean up all resources associated
with the process when it exits. So we only need care about a clean shutdown
if there is something needing cleanup that the kernel won't handle, or if
we are trying to debug with valgrind & similar tools.

I tend to think we put way too much time into worrying about getting perfect
clean shutdown, for little real world benefit.

IMHO, we should make a moderate effort to shutdown cleanly, but if there's
something stuck after 15 seconds, we should just uncoditionally call exit().

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Jul 16, 2018 at 10:18:59 +0100, Daniel Berrange wrote:
> On Thu, Jul 12, 2018 at 03:34:00PM -0400, John Ferlan wrote:
> > On 07/09/2018 03:11 AM, Peter Krempa wrote:
> > > On Sat, Jul 07, 2018 at 08:11:05 -0400, John Ferlan wrote:
> 
> I think the key thing to bear in mind is what is the benefit of what we
> are trying todo. ie why do we need todo a clean shutdown instead of just
> immediately exiting. The kernel will clean up all resources associated
> with the process when it exits. So we only need care about a clean shutdown
> if there is something needing cleanup that the kernel won't handle, or if
> we are trying to debug with valgrind & similar tools.
> 
> I tend to think we put way too much time into worrying about getting perfect
> clean shutdown, for little real world benefit.
> 
> IMHO, we should make a moderate effort to shutdown cleanly, but if there's
> something stuck after 15 seconds, we should just uncoditionally call exit().

I'm not really worried about the uncleannes of the shutdown but rather
desynchronisation of qemu-libvirt state. If we exit during a modify job
prior to writing the status XML to disk we might end up in an
intermediate state which will not be recognized by libvirt afterwards.

Just exit()-ing is not a systematic solution here because of the above.

In my opinion a better solution is to close the monitor connections
after some time and only in cases where we know it's safe. E.g. in QUERY
type jobs. Some modify-type jobs may get stuck as well, but only a very
few commands create this risk so annotating them and not allowing to
break the monitor there should be way better.

Currently I'm mostly worried about the 'transaction' command which is
used for snapshots, as losing track of the new files is not recoverable.

Other block jobs which modify the backing graph would be a problem
normally but since we don't track the bakcing chain fully yet it's okay
except for finishing step of the active block commit.

For the blockjobs it will be possible to work this around by the new
APIs in qemu which allow the jobs to linger until we retrieve the state.

That means that the only problematic job probably will be
'transaction'/snapshot creation.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list