Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Wei Liu <wei.liu2@citrix.com>]

On Wed, Aug 05, 2015 at 05:30:34PM +0100, George Dunlap wrote:
> On Wed, Aug 5, 2015 at 2:17 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Wed, Aug 05, 2015 at 11:56:44AM +0100, George Dunlap wrote:
> >> On Wed, Aug 5, 2015 at 11:17 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> >> > On Wed, 2015-08-05 at 11:06 +0100, George Dunlap wrote:
> >> >> On Fri, Jul 18, 2014 at 12:28 AM, Luis R. Rodriguez
> >> >> <mcgrof@do-not-panic.com> wrote:
> >> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >> >> >
> >> >> > This adds systemd socket activation support for the C xenstored.
> >> >> > Active sockets enable xenstored to be loaded only if required by a
> >> >> > system
> >> >> > onto which Xen is installed on. Socket activation is handled by
> >> >> > systemd, once a port for a service which claims a socket is used
> >> >> > systemd will start the required services for it, on demand. For more
> >> >> > details on socket activation refer to Lennart's socket-activation
> >> >> > post regarding this [0].
> >> >> >
> >> >> > Right now this code adds a no-op for this functionality, leaving the
> >> >> > enablement to be done later once systemd is properly hooked into
> >> >> > the build system. The socket activation is ordered in aligment with
> >> >> > the socket activation order passed on to systemd.
> >> >> >
> >> >> > [0] http://0pointer.de/blog/projects/socket-activation2.html
> >> >>
> >> >> So with this patch in place, xenstored will not start on a system that
> >> >> has systemd, *even if it wasn't started from systemd*.
> >> >
> >> > But where systemd is /sbin/init, right?
> >> >
> >> > The case where xenstored was compiled with systemd support but systemd is
> >> > not /sbin/init should still be expected to work, and isn't what I think you
> >> > are complaining about here.
> >> >
> >> >> Lots of systems (e.g., CentOS 7) have legacy systems in place to allow
> >> >> you to do things like "chkconfig --add xencommons" even on a systemd
> >> >> system.  I think we still want to work with those, right?
> >> >
> >> > Isn't chkconfig --add still arranging for the thing to be started by
> >> > systemd under the hood? If not systemd on a host where /sbin/init==systemd
> >> > then what does else would start it?
> >> >
> >> > If you are asking "should the sysvinit initscripts still be us(ed|able)
> >> > even though systemd is being used as /sbin/init on the host and a unit file
> >> > is present" then AIUI the systemd answer is "no". (We may choose to
> >> > disagree with systemd on this I suppose)
> >>
> >> Well that's not (apparently) the RHEL answer; doing "chkconfig --add
> >> [foo]" Just Works on CentOS 7 for all the sysvinit scripts I've used
> >> (including the Xen 4.4 Xen4CentOS packages).
> >>
> >> I think we want to still *enable* people to use that mode if they want
> >> to.  But I won't argue if people feel strongly otherwise.
> >>
> >> > On the other hand, does this mean I can no longer start xenstored by hand
> >> > from the CLI? _That_ would seem to be worth preserving, for debugging etc
> >> > if nothing else.
> >>
> >> So what happens at the moment is that xenstored, run either from the
> >> command-line says, "Oh, look!  I'm running on a systemd system.  I'll
> >> check for my systemd sockets.  Oh no, no sockets!  *dies*".
> >>
> >> If run from xencommons, it doesn't even get that far: it says, "Oh,
> >> look! I'm running on a systemd system.  But wait! You asked me to use
> >> a pidfile! BAD USER! NO PIDFILE ON SYSTEMD! *dies*".
> >>
> >> Modifying xenstored to try to open the systemd sockets, and fall back
> >> to normal sockets if it doesn't find any, works when started from the
> >> command-line.
> >
> > I have always thought this is the expected behaviour. Just that the code
> > has a bug.
> >
> > Here is a patch that is not even compile test. :-)
> 
> sd_booted() checks to see *if systemd is running on the system*; it
> doesn't actually tell you *if the program was started from systemd*
> (and thus has the appropriate sockets &c).
> 
> So although checking for non-negative rather than non-zero is more
> correct, this will have the same basic behavior -- if xenstored is
> started *on a system with systemd running*, even if it wasn't started
> *by* systemd, it will try to use the systemd sockets and fail (and
> also ignore the pidfile).
> 
> Something like the attached (compile-tested only).
> 

Right. I misinterpreted sd_boot.

You patch, however, has the undesirable effect that it fails to report
error if xenstored is started by systemd but couldn't claim the
socket. I don't think this is the correct behaviour.

After consulting with systemd manual [0], I think we should check
sd_listen_fds return value to determine if it is started by systemd.
Currently it only checks for <= 0, which covers 1) not started by
systemd 2) an error occurs.

Hopefully I interpret the doc correctly this time. I will prepare a
patch shortly.

Wei.

[0] http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html

>  -George