From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48908) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gRGEI-0006wP-2g for guix-patches@gnu.org; Mon, 26 Nov 2018 07:43:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gRGEB-0006Dd-Rq for guix-patches@gnu.org; Mon, 26 Nov 2018 07:43:08 -0500 Received: from debbugs.gnu.org ([208.118.235.43]:43658) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gRGE9-0006CD-Qr for guix-patches@gnu.org; Mon, 26 Nov 2018 07:43:03 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1gRGE9-0008Nr-Lc for guix-patches@gnu.org; Mon, 26 Nov 2018 07:43:01 -0500 Subject: [bug#33508] [PATCH] gnu: Add ability to restart services on system reconfigure Resent-Message-ID: References: <87efb8m5gy.fsf@zancanaro.id.au> From: =?UTF-8?Q?Cl=C3=A9ment?= Lassieur In-reply-to: <87efb8m5gy.fsf@zancanaro.id.au> Date: Mon, 26 Nov 2018 13:42:02 +0100 Message-ID: <871s78ypr9.fsf@lassieur.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Carlo Zancanaro Cc: 33508@debbugs.gnu.org Hi Carlo, It might be safer to 'reload' some services, rather than 'restarting' them. E.g. for nginx and prosody. Do you think it would be possible add a 'custom' value that would point to a custom Shepherd action? Thank you for your work on this! Cl=C3=A9ment Carlo Zancanaro writes: > Hey Guix! > > A few months ago I mentioned the idea of adding the ability to=20 > have services automatically restarted when running "guix system=20 > reconfigure". These patches are a start on making that happen.=20 > They're incomplete (in particular, documentation is missing), but=20 > I'm offering them up for comment. > > The broad idea is to add a new field to our guix shepherd=20 > services: restart-strategy. There are three valid values: > > - always: this service is always safe to restart when running=20 > reconfigure >=20=20 > - manual: this service may not be safe to restart when running=20 > reconfigure - a message will be printed telling the user to=20 > restart the service manually, or they can provide the=20 > --restart-services flag to reconfigure to automatically restart=20 > them > > - never: this service is never safe to restart when running=20 > reconfigure (eg. udev) > > I have added the flag to the guix daemon's shepherd service to=20 > show how it works. I tested this by changing my substitute servers=20 > in config.scm, and after running "reconfigure" I saw my updated=20 > substitute servers in ps without having to run "sudo herd restart=20 > guix-daemon". > > If nobody has any feedback in the next few days then I'll update=20 > the manual and send through another patch. > > Carlo > > From 8b92ebac4fa13a2a89f279b249be152051f31d94 Mon Sep 17 00:00:00 2001 > From: Carlo Zancanaro > Date: Mon, 26 Nov 2018 22:38:08 +1100 > Subject: [PATCH 1/3] gnu: Add ability to restart services on system > reconfigure > > * gnu/services/herd.scm (restart-service): New procedure. > * gnu/services/shepherd.scm ()[restart-strategy]: New > field. > (shepherd-service-upgrade): Return lists of services to automatically a= nd > manually restart. > * guix/scripts/system.scm (call-with-service-upgrade-info): Pass through > services to be automatically and manually restarted. > (upgrade-shepherd-services): Automatically restart services that should= be > automatically restarted, and print a message about manually restarting > services that should be manually restarted. > --- > gnu/services/herd.scm | 5 +++++ > gnu/services/shepherd.scm | 35 ++++++++++++++++++++++-------- > guix/scripts/system.scm | 45 ++++++++++++++++++++++++--------------- > 3 files changed, 59 insertions(+), 26 deletions(-) > > diff --git a/gnu/services/herd.scm b/gnu/services/herd.scm > index 8ff817759..c8d6eb04e 100644 > --- a/gnu/services/herd.scm > +++ b/gnu/services/herd.scm > @@ -52,6 +52,7 @@ > load-services > load-services/safe > start-service > + restart-service > stop-service)) >=20=20 > ;;; Commentary: > @@ -256,6 +257,10 @@ when passed a service with an already-registered nam= e." > (with-shepherd-action name ('start) result > result)) >=20=20 > +(define (restart-service name) > + (with-shepherd-action name ('restart) result > + result)) > + > (define (stop-service name) > (with-shepherd-action name ('stop) result > result)) > diff --git a/gnu/services/shepherd.scm b/gnu/services/shepherd.scm > index 49d08cc30..0c80e44f2 100644 > --- a/gnu/services/shepherd.scm > +++ b/gnu/services/shepherd.scm > @@ -159,7 +159,9 @@ DEFAULT is given, use it as the service's default val= ue." > (auto-start? shepherd-service-auto-start? ;Boolean > (default #t)) > (modules shepherd-service-modules ;list of module n= ames > - (default %default-modules))) > + (default %default-modules)) > + (restart-strategy shepherd-service-restart-strategy > + (default 'manual))) >=20=20 > (define-record-type* > shepherd-action make-shepherd-action > @@ -344,9 +346,10 @@ symbols provided/required by a service." > #t)))))) >=20=20 > (define (shepherd-service-upgrade live target) > - "Return two values: the subset of LIVE (a list of ) that= needs > -to be unloaded, and the subset of TARGET (a list of ) = that > -need to be restarted to complete their upgrade." > + "Return three values: (a) the subset of LIVE (a list of = ) that > +needs to be unloaded, (b) the subset of TARGET (a list of ) > +that can be restarted automatically, and (c) the subset of TARGET that m= ust be > +restarted manually." > (define (essential? service) > (memq (first (live-service-provision service)) > '(root shepherd))) > @@ -373,14 +376,28 @@ need to be restarted to complete their upgrade." > (#f (every obsolete? (live-service-dependents service))) > (_ #f))) >=20=20 > - (define to-restart > - ;; Restart services that are currently running. > - (filter running? target)) > - > (define to-unload > ;; Unload services that are no longer required. > (remove essential? (filter obsolete? live))) >=20=20 > - (values to-unload to-restart)) > + (define to-automatically-restart > + ;; Automatically restart services that are currently running and can > + ;; always be restarted. > + (filter (lambda (service) > + (and (running? service) > + (eq? (shepherd-service-restart-strategy service) > + 'always))) > + target)) > + > + (define to-manually-restart > + ;; Manually restart services that are currently running and must be > + ;; manually restarted. > + (filter (lambda (service) > + (and (running? service) > + (eq? (shepherd-service-restart-strategy service) > + 'manual))) > + target)) > + > + (values to-unload to-automatically-restart to-manually-restart)) >=20=20 > ;;; shepherd.scm ends here > diff --git a/guix/scripts/system.scm b/guix/scripts/system.scm > index d92ec7d5a..6f14b1395 100644 > --- a/guix/scripts/system.scm > +++ b/guix/scripts/system.scm > @@ -322,11 +322,12 @@ names of services to load (upgrade), and the list o= f names of services to > unload." > (match (current-services) > ((services ...) > - (let-values (((to-unload to-restart) > + (let-values (((to-unload to-automatically-restart to-manually-resta= rt) > (shepherd-service-upgrade services new-services))) > - (mproc to-restart > - (map (compose first live-service-provision) > - to-unload)))) > + (mproc (map (compose first live-service-provision) > + to-unload) > + to-automatically-restart > + to-manually-restart))) > (#f > (with-monad %store-monad > (warning (G_ "failed to obtain list of shepherd services~%")) > @@ -347,7 +348,7 @@ bring the system down." > ;; Arrange to simply emit a warning if the service upgrade fails. > (with-shepherd-error-handling > (call-with-service-upgrade-info new-services > - (lambda (to-restart to-unload) > + (lambda (to-unload to-automatically-restart to-manually-restart) > (for-each (lambda (unload) > (info (G_ "unloading service '~a'...~%") unload) > (unload-service unload)) > @@ -355,27 +356,37 @@ bring the system down." >=20=20 > (with-monad %store-monad > (munless (null? new-services) > - (let ((new-service-names (map shepherd-service-canonical-na= me new-services)) > - (to-restart-names (map shepherd-service-canonical-na= me to-restart)) > - (to-start (filter shepherd-service-auto-star= t? new-services))) > - (info (G_ "loading new services:~{ ~a~}...~%") new-service= -names) > - (unless (null? to-restart-names) > - ;; Listing TO-RESTART-NAMES in the message below wouldn'= t help > - ;; because many essential services cannot be meaningfully > - ;; restarted. See . > - (format #t (G_ "To complete the upgrade, run 'herd resta= rt SERVICE' to stop, > -upgrade, and restart each service that was not automatically restarted.\= n"))) > + (let ((new-service-names (map shepherd-service-= canonical-name new-services)) > + (to-start-names (map shepherd-service-= canonical-name (filter shepherd-service-auto-start? new-services))) > + (to-automatically-restart-names (map shepherd-service-= canonical-name to-automatically-restart)) > + (to-manually-restart-names (map shepherd-service-= canonical-name to-manually-restart))) > + (set! to-start-names > + (remove (lambda (name) > + (or (member name to-automatically-restart-name= s) > + (member name to-manually-restart-names))) > + to-start-names)) > + > (mlet %store-monad ((files (mapm %store-monad > (compose lower-object > shepherd-service= -file) > new-services))) > + (for-each restart-service to-automatically-restart-names) > + > ;; Here we assume that FILES are exactly those that were= computed > ;; as part of the derivation that built OS, which is nor= mally the > ;; case. > + (info (G_ "loading new services:~{ ~a~}~%") new-service-= names) > (load-services/safe (map derivation->output-path files)) > - > + (info (G_ "starting services:~{ ~a~}~%") to-start-names) > (for-each start-service > - (map shepherd-service-canonical-name to-start)) > + to-start-names) > + (info (G_ "restarting services:~{ ~a~}~%") to-automatica= lly-restart-names) > + (for-each restart-service > + to-automatically-restart-names) > + > + (unless (null? to-manually-restart-names) > + (format #t (G_ "To complete the upgrade, the following= services need to be manually restarted:~{ ~a~}~%") > + to-manually-restart-names)) > (return #t))))))))) >=20=20 > (define* (switch-to-system os > --=20 > 2.19.1 > > From 3fdef27c8f11b6a0f013afa9b6e619659ce78dec Mon Sep 17 00:00:00 2001 > From: Carlo Zancanaro > Date: Mon, 26 Nov 2018 22:38:18 +1100 > Subject: [PATCH 2/3] system: Add --restart-services flag for reconfigure > > * guix/scripts/system.scm (upgrade-shepherd-services): Add parameter to > automatically restart services marked as needing manual restart. > (switch-to-system): Pass through restart-services? flag. > (perform-action): Pass through restart-services? flag. > (%options): Add --restart-services flag. > (%default-options): Add #f as default value for --restart-services flag. > (process-action): Pass through restart-services? flag. > --- > guix/scripts/system.scm | 24 +++++++++++++++++------- > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/guix/scripts/system.scm b/guix/scripts/system.scm > index 6f14b1395..bf632c534 100644 > --- a/guix/scripts/system.scm > +++ b/guix/scripts/system.scm > @@ -333,7 +333,7 @@ unload." > (warning (G_ "failed to obtain list of shepherd services~%")) > (return #f))))) >=20=20 > -(define (upgrade-shepherd-services os) > +(define (upgrade-shepherd-services os restart-services?) > "Upgrade the Shepherd (PID 1) by unloading obsolete services and loadi= ng new > services specified in OS and not currently running. >=20=20 > @@ -360,6 +360,10 @@ bring the system down." > (to-start-names (map shepherd-service-= canonical-name (filter shepherd-service-auto-start? new-services))) > (to-automatically-restart-names (map shepherd-service-= canonical-name to-automatically-restart)) > (to-manually-restart-names (map shepherd-service-= canonical-name to-manually-restart))) > + (when restart-services? > + (set! to-automatically-restart-names (append to-automati= cally-restart-names > + to-manually= -restart-names)) > + (set! to-manually-restart-names '())) > (set! to-start-names > (remove (lambda (name) > (or (member name to-automatically-restart-name= s) > @@ -389,7 +393,7 @@ bring the system down." > to-manually-restart-names)) > (return #t))))))))) >=20=20 > -(define* (switch-to-system os > +(define* (switch-to-system os restart-services? > #:optional (profile %system-profile)) > "Make a new generation of PROFILE pointing to the directory of OS, swi= tch to > it atomically, and then run OS's activation script." > @@ -417,7 +421,7 @@ it atomically, and then run OS's activation script." > (primitive-load (derivation->output-path script)))) >=20=20 > ;; Finally, try to update system services. > - (upgrade-shepherd-services os)))) > + (upgrade-shepherd-services os restart-services?)))) >=20=20 > (define-syntax-rule (unless-file-not-found exp) > (catch 'system-error > @@ -825,7 +829,8 @@ and TARGET arguments." > use-substitutes? bootloader-target target > image-size file-system-type full-boot? > (mappings '()) > - (gc-root #f)) > + (gc-root #f) > + (restart-services? #f)) > "Perform ACTION for OS. INSTALL-BOOTLOADER? specifies whether to inst= all > bootloader; BOOTLOADER-TAGET is the target for the bootloader; TARGET is= the > target root directory; IMAGE-SIZE is the size of the image to be built, = for > @@ -907,7 +912,7 @@ static checks." > (case action > ((reconfigure) > (mbegin %store-monad > - (switch-to-system os) > + (switch-to-system os restart-services?) > (mwhen install-bootloader? > (install-bootloader bootloader-script > #:bootcfg bootcfg > @@ -1090,6 +1095,9 @@ Some ACTIONS support additional ARGS.\n")) > (option '(#\r "root") #t #f > (lambda (opt name arg result) > (alist-cons 'gc-root arg result))) > + (option '("restart-services") #f #f > + (lambda (opt name arg result) > + (alist-cons 'restart-services? #t result))) > %standard-build-options)) >=20=20 > (define %default-options > @@ -1104,7 +1112,8 @@ Some ACTIONS support additional ARGS.\n")) > (verbosity . 0) > (file-system-type . "ext4") > (image-size . guess) > - (install-bootloader? . #t))) > + (install-bootloader? . #t) > + (restart-services? . #f))) >=20=20 > > ;;; > @@ -1177,7 +1186,8 @@ resulting from command-line parsing." > #:install-bootloader? bootloader? > #:target target > #:bootloader-target bootloader-target > - #:gc-root (assoc-ref opts 'gc-root))))) > + #:gc-root (assoc-ref opts 'gc-root) > + #:restart-services? (assoc-ref opts 'restar= t-services?))))) > #:system system)) > (warn-about-disk-space))) >=20=20 > --=20 > 2.19.1 > > From 099a8e2e6e28b38816ed1ba895c407f1d9efe62e Mon Sep 17 00:00:00 2001 > From: Carlo Zancanaro > Date: Mon, 26 Nov 2018 22:38:26 +1100 > Subject: [PATCH 3/3] services: Always restart guix daemon > > * gnu/services/base.scm (guix-shepherd-service): Add restart-strategy of > 'always. > --- > gnu/services/base.scm | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/gnu/services/base.scm b/gnu/services/base.scm > index 228d3c592..7e0fdcb3e 100644 > --- a/gnu/services/base.scm > +++ b/gnu/services/base.scm > @@ -1573,6 +1573,7 @@ failed to register hydra.gnu.org public key: ~a~%" = status)))))))) > (documentation "Run the Guix daemon.") > (provision '(guix-daemon)) > (requirement '(user-processes)) > + (restart-strategy 'always) > (modules '((srfi srfi-1))) > (start > #~(make-forkexec-constructor