[sheepdog] [PATCH v2] fix systemd service to run sheep in foreground and with defaults

[Alexander Guy]

On Tue, Apr 14, 2015 at 10:59:51AM +0800, Liu Yuan wrote:
> 
> Hi Alexander,
> 
>    Could you please give this patch a review?
> 

Hey Yuan / Vasiliy,
    Sorry for the slow reply.

> On Mon, Apr 13, 2015 at 01:32:56PM +0300, Vasiliy Tolstov wrote:
> >  [Service]
> > +Environment="SHEEPDOG_ARGS=--cluster local --log dst=syslog -f --upgrade" "SHEEPDOG_PATH=@LOCALSTATEDIR@/lib/sheepdog"

[snip]

> > -ExecStart=/bin/sh -c 'ulimit -n 32768; @SBINDIR@/sheep --pidfile @LOCALSTATEDIR@/run/sheep.pid $(if [ -z "$SHEEP_OPTS" ]; then echo "--cluster local --log dst=syslog --upgrade @LOCALSTATEDIR@/lib/sheepdog"; else echo $SHEEP_OPTS; fi)'

Removing the shell script from the ExecStart and using Environment is a good idea.

Changing SHEEP_OPTS to SHEEPDOG_ARGS and SHEEPDOG_PATH is going to break existing installs that use this service file.  What's the functional benefit to doing this?  We already have a separate service file for Debian packages (i.e. debian/sheepdog.service), that uses another naming scheme.

> > -PIDFile=@LOCALSTATEDIR@/run/sheep.pid
> > -Type=forking
> > +ExecStart=@SBINDIR@/sheep $SHEEPDOG_ARGS $SHEEPDOG_PATH

I don't think this is a good idea.  I went and double-checked the systemd documentation:


http://www.freedesktop.org/software/systemd/man/systemd.service.html

If set to simple (the default if neither Type= nor BusName=, but ExecStart= are specified), it is expected that the process configured with ExecStart= is the main process of the service. In this mode, if the process offers functionality to other processes on the system, its communication channels should be installed before the daemon is started up (e.g. sockets set up by systemd, via socket activation), as systemd will immediately proceed starting follow-up units.


... so if I'm understanding this right:

   1) By removing 'Type=forking', it defaults to 'Type=simple'
   2) 'Type=simple' expects to have communication setup before the daemon is started (e.g. by systemd managing the sockets via socket activation).

#2 means that (unless we implement socket activation) we're going to have an even bigger start-up race condition than we already have, for services that depend on sheepdog (e.g. qemu guests that start at bootup) because systemd will have no way of knowing when sheep is actually ready for work. 

So overall, I NACK these changes.

Thanks.


Alexander