Re: [Xen-devel] [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On 06/29/2016 02:44 PM, Juergen Gross wrote:
> On 29/06/16 15:31, Ross Lagerwall wrote:
> > On 06/29/2016 02:00 PM, Juergen Gross wrote:
> > > On 29/06/16 14:52, Andrew Cooper wrote:
> > > > On 29/06/16 13:44, Juergen Gross wrote:
> > > > > @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
> > > > >      /* Tell the kernel we're up and running. */
> > > > >      xenbus_notify_running();
> > > > >
> > > > > -#if defined(XEN_SYSTEMD_ENABLED)
> > > > > -    if (systemd) {
> > > > > -        sd_notify(1, "READY=1");
> > > > > -        fprintf(stderr, SD_NOTICE "xenstored is ready\n");
> > > > > -    }
> > > > > -#endif
> > > > Getting rid of the socket configuration for systemd is ok, but we should
> > > > keep the sd_notify() calls for when the daemon is started by systemd.
> > > >
> > > > Socket activiation and sd_notify() are orthogonal, and sd_notify() is
> > > > still required if we don't want systemd to treat xenstored as a legacy
> > > > unix daemon.
> > > So what is the downside of xenstored being treated as a legacy daemon?
> > > This question is especially interesting for the case of patch 2 being
> > > considered: xenstored is no longer started by systemd, but by a wrapper
> > > script which might decide to start the xenstore domain instead.
> > >
> > > Another problem: today xenstored decides whether to call sd_notify()
> > > by testing the xenstore sockets being specified via systemd. This will
> > > no longer work. So how to do it now?
> > One problem with the patch as it currently is implemented is that the
> > service type is not correct for when xenstored is a daemon. This makes
> > it difficult to manage with systemd and difficult for other services to
> > depend on it in a sensible fashion. The end result is subtle races and
> > occasional failures.
> Could you please educate me what's wrong? I'm no systemd expert.
Sorry for the late response.

With Type=oneshot, systemd considers the service to be "started" as soon 
as the ExecStart command completes. After re-reading the patches, I 
realize that timeout_xenstore() should ensure that xenstored is actually 
ready before systemd starts dependent services. This should prevent the 
races I was mentioned previously.
Nevertheless, I feel that this patch series makes the implementation 
worse for users of xenstored as a daemon:
- Because Type=oneshot is not intended to be used for daemon processes, 
systemctl status does not show the service as "running", instead it 
shows "exited". This is surprising, at the very least. I haven't tested, 
but I believe it would also prevent us someday using the systemd service 
management features like Restart=on-failure
- Removes socket activation. Services would have to explicitly depend on 
xenstored.service rather than having an implicit dependency on simply by 
using xenstored.socket. This means configuring explicit ordering, etc., 
which is easier to get wrong. Socket activation is also generally the 
most efficient way of starting a service.
- Uses a poll loop to determine whether the daemon is ready or not 
rather than explicit notification from the daemon itself.
- Uses a shell script wrapper...

> > How about:
> > * Maintain the existing service for xenstored
> > * Have a separate service (with different a service type) for starting
> > the xenstore domain
> > * Switch between the two services
> How could I specify e.g. in xendomains.service the dependency to
> xenstore?
Hmm. I'm not sure of the best way, but it should be possible to define 
something like xenstored.target:
[Unit]
Description=...
Wants=xenstored.service
Wants=xenstore-domain.service

and then have services depend on xenstored.target. With the ExecStartPre 
lines I mentioned previously, only one of the xenstore*.service will 
actually start.
> > Personally I think it is better and more uniform for the administrator
> > to enable and disable services in the normal fashion, but if you want to
> The two services would be mutually exclusive. Can I tell systemd this
> is the case?
It is possible to set Conflicts=... on a service. I'm not sure if that 
would suit your needs.
> > make it configurable with /etc/sysconfig/xencommons, then you can add
> > something like this to xenstored.service:
> >
> > ExecStartPre=/bin/grep -q XENSTORETYPE=daemon /etc/sysconfig/xencommons
> >
> > and to xenstore-domain.service:
> >
> > ExecStartPre=/bin/grep -q XENSTORETYPE=domain /etc/sysconfig/xencommons
> That's no good idea. Someone commenting out the old line and adding the
> other option would end in both variants to be tried. This would have to
> be a little bit more sophisticated. :-)
Yeah, indeed...


I would suggest asking on the systemd-devel mailing list or the #systemd 
IRC channel on freenode. They should be able to suggest the best way of 
solving this.
Thanks,
--
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel