[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.