[bug#32408] [PATCH shepherd] Allow replacement of services

[Ludovic Courtès]

Hi Carlo,

Carlo Zancanaro <address@hidden> skribis:

> This is a relatively simple patch adding a replacement slot to
> services in the Shepherd. When stopping a service, the replacement
> slot is checked and, if it has a value, is used to upgrade the current
> service.
>
> I've chosen to modify the existing service, rather than creating a new
> one, but that was mostly because it was easier for me to implement
> quickly, and I didn't have a huge amount of time.
>
> I'm hopeful that this, or something like it, can be used by GuixSD to
> allow people to restart services after reconfiguring without rebooting
> (or remembering to stop, reconfigure, start).

Awesome!  This is exactly what we need to address the problem.

We’ll still want to be able to special-case things like nginx that can
be “hot-replaced”, though.  So perhaps, in addition to this patch on the
Shepherd side, we’ll need extra stuff in (gnu services shepherd).

For instance, the ‘actions’ field of <shepherd-service> could, by
default, include an “upgrade” action that simply sets the ‘replacement’
slot.  For nginx, we’d provide a custom “upgrade” action that does
“nginx -s restart” or whatever it is that needs to be done.

‘guix system reconfigure’ would automatically invoke the ‘upgrade’
action for each new service.

WDYT?

> From e03290041c91813f1a301c7e9c4dbb9ee768b400 Mon Sep 17 00:00:00 2001
> From: Carlo Zancanaro <address@hidden>
> Date: Thu, 9 Aug 2018 22:30:38 +1000
> Subject: [PATCH] service: Add a replacement slot for delayed service
>  replacement.
>
> * modules/shepherd/service.scm (<service>): Add replacement slot
> (replace-service): New procedure.
> (stop): Call replace-service after stopping a service.
> * tests/replacement.sh: Add a test for it.
> * Makefile.am (TESTS): Add the new test.
> * doc/shepherd.texi (Slots of services): Document it.

Overall LGTM.  Some comments:

> +(define (replace-service service)

Please add a docstring.

> +  (let ((replacement (slot-ref service 'replacement)))
> +    (define (copy-slot! slot)
> +      (slot-set! service slot (slot-ref replacement slot)))
> +    (when replacement
> +      (copy-slot! 'provides)
> +      (copy-slot! 'requires)
> +      (copy-slot! 'respawn?)
> +      (copy-slot! 'start)
> +      (copy-slot! 'stop)
> +      (copy-slot! 'actions)
> +      (copy-slot! 'running)
> +      (copy-slot! 'docstring))
> +    service))

Having a hardcoded list of slots sounds error-prone—surely we’ll forget
to update it down the road.  I wonder what else could be done.

One option would be to grab the block asyncs and atomically replace the
service in the ‘%services’ hash table.  Then we only need to copy the
‘last-respawns’ slot to the new service, I believe.  (This changes the
object identity of the service but I think its OK.)

Another option would be to use GOOPS tricks to iterate over the list of
slots and have a list of slots *not* to copy.  I’m not a big fan of this
option, though.

> +cat - > "$rconf"<<EOF
       ^
You can remove the dash.

> +(let ((service (lookup-running 'test)))
> +  (slot-set! service 'replacement
> +             (make <service>

I wonder if we should let users fiddle with ‘replacement’ directly, or
if we should provide a higher-level construct.

For instance, ‘register-services’ could transparently set the
‘replacement’ field for services already registered instead of doing:

    (assert (null? (lookup-services (canonical-name new))))

Not sure if there are cases where this behavior would be undesirable,
though.

Thoughts?

> +if test `cat $stamp` != "Goodbye"; then

Nitpick: "`cat $stamp`".

Thanks!

Ludo’.