xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Ross Lagerwall <ross.lagerwall@citrix.com>
To: Juergen Gross <jgross@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	dave@recoil.org, ian.jackson@eu.citrix.com, wei.liu2@citrix.com,
	xen-devel@lists.xen.org
Subject: Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions
Date: Wed, 20 Jul 2016 10:02:57 +0100	[thread overview]
Message-ID: <0f91255f-d6c1-d0da-5722-be0b7732f758@citrix.com> (raw)
In-Reply-To: <5773D0B1.40709@suse.com>

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@lists.xen.org
https://lists.xen.org/xen-devel

  parent reply	other threads:[~2016-07-20  9:02 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-29 12:44 [PATCH 0/2] tools: make xenstore domain/daemon configurable Juergen Gross
2016-06-29 12:44 ` [PATCH 1/2] tools: remove systemd xenstore socket definitions Juergen Gross
2016-06-29 12:52   ` Andrew Cooper
2016-06-29 13:00     ` Juergen Gross
2016-06-29 13:31       ` Ross Lagerwall
2016-06-29 13:44         ` Juergen Gross
2016-07-13  7:05           ` Juergen Gross
2016-07-20  9:02           ` Ross Lagerwall [this message]
2016-07-20  9:58             ` Juergen Gross
2016-07-20 10:52               ` George Dunlap
2016-07-20 11:12                 ` Juergen Gross
2016-07-20 11:21                   ` Andrew Cooper
2016-07-20 11:59                     ` Juergen Gross
2016-07-20 12:08                     ` Ian Jackson
2016-07-20 12:21                       ` Juergen Gross
2016-07-20 12:32                       ` Andrew Cooper
2016-07-20 13:02                         ` Juergen Gross
2016-07-20 13:23                         ` Ian Jackson
2016-07-20 13:29                           ` George Dunlap
2016-07-20 14:09                             ` Andrew Cooper
2016-07-20 12:11             ` Ian Jackson
2016-07-20 12:42               ` Juergen Gross
2016-07-08 12:15       ` Wei Liu
2016-07-08 12:32         ` Juergen Gross
2016-07-08 13:02           ` Wei Liu
2016-06-29 12:44 ` [PATCH 2/2] tools: make xenstore domain easy configurable Juergen Gross

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0f91255f-d6c1-d0da-5722-be0b7732f758@citrix.com \
    --to=ross.lagerwall@citrix.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=dave@recoil.org \
    --cc=ian.jackson@eu.citrix.com \
    --cc=jgross@suse.com \
    --cc=wei.liu2@citrix.com \
    --cc=xen-devel@lists.xen.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).