[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#32408] [PATCH shepherd] Allow replacement of services
From: |
Ludovic Courtès |
Subject: |
[bug#32408] [PATCH shepherd] Allow replacement of services |
Date: |
Mon, 20 Aug 2018 22:33:42 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
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’.