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

[Juergen Gross <jgross@xxxxxxxx>]

On 20/07/16 11:02, Ross Lagerwall wrote:
> 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.

So you are not taking xenstore domain into account at all.

There is no socket for xenstore domain so wanting to rely on socket
activation is the wrong way.

> - Uses a poll loop to determine whether the daemon is ready or not
> rather than explicit notification from the daemon itself.

How to do this in the domain case?

> - Uses a shell script wrapper...

That's on purpose: this enables me to switch between both setups
(daemon or domain) by modifying only _one_ configuration file.

>>> 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.

And the other will be "failed". How is this better than the current
"exited" state?

>>> 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.

On a first glance I don't think so. This would be interesting in case I
could switch between both setups during runtime, but it is no easy way
to select one of two mutual exclusive services at boot time.

>>> 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.

I'm sure I could tweak the whole setup more in direction of systemd.
I'm not sure the final picture would be the preferred one.

All of your reasoning seems to boil down to rejecting the "exited" state
of the xenstore service. The only way to avoid this (but only for the
daemon case, the domain would still suffer from it) would be to accept
a "failed" state for the complementary service. In case you want to
avoid removing the systemd socket handling you'd add those to be
"failed" in the domain case, too.

I think the maintainers should decide. Ian, Wei?


Juergen

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