bug#26830: Allow services to implement a 'reload' action

["Clément Lassieur" <clement@lassieur.org>]

Hi Ludovic,

Thanks for commenting on this :)

Ludovic Courtès <ludo@gnu.org> writes:

> Hello!
>
> Mathieu Othacehe <m.othacehe@gmail.com> skribis:
>
>>> Services do not have to implement 'reload' and if, say, foo-daemon
>>> doesn't implement it, 'herd reload foo-daemon' will return 1 and display
>>> a message saying that foo-deamon does not have an action 'reload'.
>>> That's the reason of the #f default value.
>>>
>>> WDYT?
>>
>> Your whole serie LGTM for me !
>
> Same here, really happy to see this addressed!
>
>> I have just one small concern, there is a already a "reload" action on
>> shepherd root service.
>
> Right, but that’s just for ‘root’, not for the other services.
>
> However, I think ‘reload’ might be confusing since in fact it doesn’t
> load Scheme code, contrary to what “herd load root foo.scm” does (maybe
> that’s what you meant?).  In fact it’s closer to what “herd restart
> foo” does.
>
> What about changing the name to ‘reconfigure’ or ‘upgrade’ to avoid the
> confusion?

I think it's going to be even more confusing because the other init
systems (systemd, sysvinit) all call it 'reload'.  And, well, people are
probably more familiar with Systemd's 'reload' than with Shepherd's
'reload root' :)  WDYT?

> The logical next step of this series will be to have the service upgrade
> code in ‘guix system reconfigure’ invoke this action when it is defined.
> That will be awesome.

Indeed!

> Some comments:
>
> +                      #:actions (make-actions
> +                                 (reload
> +                                  "Reload the service's configuration files."
> +                                  #$(shepherd-service-reload service))))))))
>
> Here I think we should only define the action when it has a non-#f
> value.  That way we can distinguish between services that have a useful
> reload/reconfigure/upgrade action and those that don’t; in the latter
> case, we simply use ‘restart’ when upgrading.

Ok.  But right now IIRC we don't even use restart after a system
reconfigure, probably because some services can't be restarted safely.
Would that be why we need a 'reload/reconfigure/upgrade' for them?

> Regarding nginx:
>
> +              (stop (nginx-action "-s" "stop"))
> +              (reload (nginx-action "-s" "reload"))))))))
>
> Is this of any use in practice?  The nginx command line is something
> like:
>
>   /gnu/store/74kz9m850ycxpzkg6dvn9wbd3xjkwwrb-nginx-1.12.0/sbin/nginx -c /gnu/store/5w11ahw113fndvab3xmwcjzs2rw56sbh-nginx-config/bayfront.conf -p /var/run/nginx
>
> and the configuration file in /gnu/store is immutable, so “nginx -s
> reload” does nothing.

Actually, my goal was to use this after a certificate renewal.  There
was going to be other patches on the certbot service as well :)  And
after a certificate renewal, the names of the certificates don't change
(AFAIK they are still in /etc).  So I think it would work.  But indeed,
I didn't think about the main configuration file :/

> If the action took an argument, we could do:
>
>   herd reconfigure nginx /gnu/store/…-new-config.conf
>
> which would translate to:
>
>   nginx -s reload -c /gnu/store/…-new-config.conf
>
> Probably our best option.

I don't see the point.  If the service has already been reloaded by the
'guix system reconfigure' command (let's assume it does, but I know it
doesn't currently reload nor restart sevices...), why would a user want
to reload it again with the 'herd' command?  Or maybe you want this
feature as a workaround while the 'guix system reconfigure' that reloads
services isn't implemented?

Anyway, I think the argument should be optional, so that if there are
none, the current configuration file is used.  That will be useful for
certificates anyway, or for other kinds of configuration files that
aren't in the store.

> Otherwise, I think we’d have to move the config to a fixed location, say
> /etc/nginx, for “nginx -s reload” to have any effect.  However I don’t
> quite like the use of /etc.  Thoughts?

I don't like it either :)  'reload' definitely has to support
configuration files that are in the store!

> Does Dovecot have the same problem?

Yes.  (But Prosody doesn't.)

Clément