[libvirt] [PATCH 3/3] Speed up waiting for the session daemon

[Daniel P. Berrange]

On Thu, Apr 18, 2013 at 12:47:46PM +0200, Martin Kletzander wrote:
> On 04/18/2013 12:43 PM, Daniel P. Berrange wrote:
> > On Thu, Apr 18, 2013 at 12:30:56PM +0200, Martin Kletzander wrote:
> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369
> >>
> >> When launching the session daemon, we were waiting for around 20
> >> seconds even it died before creating any sockets etc.
> >>
> >> The following modification pre-creates the pidfile which will be used
> >> by the forked daemon and cleans it up in case the daemon doesn't start
> >> successfully.  This makes it possible to check whether the daemon
> >> started and died immediately.
> >>
> >> The pidfile is needed due to the process being daemonized.  Neither
> >> creation nor locking the pidfile will fail in the daemon thanks to the
> >> same PID keeping the lock.
> >>
> >> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> >> ---
> >>  src/rpc/virnetsocket.c | 41 +++++++++++++++++++++++++++++++++++------
> >>  1 file changed, 35 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> >> index c4fd9ee..835e11f 100644
> >> --- a/src/rpc/virnetsocket.c
> >> +++ b/src/rpc/virnetsocket.c
> >> @@ -52,6 +52,7 @@
> >>  #include "virfile.h"
> >>  #include "virthread.h"
> >>  #include "virprocess.h"
> >> +#include "virpidfile.h"
> >>
> >>  #include "passfd.h"
> >>
> >> @@ -119,20 +120,29 @@ VIR_ONCE_GLOBAL_INIT(virNetSocket)
> >>
> >>
> >>  #ifndef WIN32
> >> -static int virNetSocketForkDaemon(const char *binary)
> >> +static int virNetSocketForkDaemon(const char *binary, char **pidfile)
> >>  {
> >> -    int ret;
> >> +    int ret = -1;
> >>      virCommandPtr cmd = virCommandNewArgList(binary,
> >>                                               "--timeout=30",
> >>                                               NULL);
> >>
> >> +    if (pidfile &&
> >> +        virGetDaemonPidFilePath(false, pidfile) < 0) {
> >> +        goto cleanup;
> >> +    }
> > 
> > And here, you're hardcoding use of the libvirtd pid file in a
> > method that is used to spawn any type of daemon.
> > 
> > The much better way to deal with this is to follow the approach that
> > systemd uses for race-free activation of daemons. Namely pass in the
> > pre-opened listen socket to the daemon. We already support this for
> > the virtlockd daemon and it would be easy to support it for libvirtd
> > too. This leaves the daemons in charge of their own pidfiles.
> > 
> 
> I completely missed the fact that this could be used for something else
> than libvirtd, thanks for pointing that out.  Could you point me to the
> place where we use the approach you described?

Look at the method

   virLockDaemonSetupNetworkingSystemD

in src/locking/lock_daemon.c This allows virtlockd to inherit a pre-opened
UNIX domain socket from systemd when it starts. We can easily do the same
in libvirtd (don't need to worry about TCP sockets). Then the method
virNetSocketForkDaemon() could be made to pass in such a pre-opened socket,
so the client has the ability to connect. Basically youd want to

  - Create the listen socket in the client
  - Connect to the listen socket in the client
  - Spawn the daemon, passing in in the listen socket

So you have no race there, but if the daemon fails to complete its startup
for whatever reason the listen socket will be closed & the client will see
EOF as normal.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|