[PATCH] Shepherd: Terminate all services upon SIGTERM or SIGHUP

Done

Details

3 participants

Carlo Zancanaro
Leo Famulari
Ludovic Courtès

Owner: unassigned

Submitted by: Carlo Zancanaro

Severity: normal

Carlo Zancanaro wrote on 19 Feb 2018 18:11

Recipients:(address . guix-patches@gnu.org)

Message-ID:87sh9wnbu6.fsf@zancanaro.id.au

Hey,

I use Shepherd to manage my user session, and if I log out then

Shepherd leaves all my services running. This patch handles

SIGTERM and SIGHUP to prevent that.

I hope the guix-patches mailing list was the right place to send

it. From what I understand Shepherd development is done through

the guix lists, so it seemed to make the most sense to me.

It's also worth noting that I had to run `gettextize` to get

Shepherd to build on current master, but I haven't included those

changes in this commit (because I don't know what should be

committed and what shouldn't).

Carlo

Attachment: 0001-Terminate-all-services-upon-SIGTERM-or-SIGHUP.patch

Ludovic Courtès wrote on 27 Feb 2018 10:45

Recipients:(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)(address . 30532@debbugs.gnu.org)

Message-ID:87lgfe4vft.fsf@gnu.org

Hi Carlo,

Carlo Zancanaro <carlo@zancanaro.id.au> skribis:

Toggle quote (4 lines)

> I use Shepherd to manage my user session, and if I log out then

> Shepherd leaves all my services running. This patch handles SIGTERM

> and SIGHUP to prevent that.

Good catch!

Toggle quote (4 lines)

> I hope the guix-patches mailing list was the right place to send

> it. From what I understand Shepherd development is done through the

> guix lists, so it seemed to make the most sense to me.

Yes, that’s perfect.

Toggle quote (5 lines)

> It's also worth noting that I had to run `gettextize` to get Shepherd

> to build on current master, but I haven't included those changes in

> this commit (because I don't know what should be committed and what

> shouldn't).

Right, I’m not sure what’s wrong with that. I’ve committed the same

files under po/ as I did for Guix:

Toggle snippet (5 lines)

$ git ls-files po/

po/Makevars

po/POTFILES.in

Toggle quote (10 lines)> From aabb9c6b1b52189d20339531de0b8b96bcace69f Mon Sep 17 00:00:00 2001
> From: Carlo Zancanaro <carlo@zancanaro.id.au>
> Date: Tue, 20 Feb 2018 02:52:47 +1100
> Subject: [PATCH] Terminate all services upon SIGTERM or SIGHUP
>
> * modules/shepherd.scm (main): Add SIGTERM and SIGHUP handlers which stop
>   root-service.
> * tests/sigterm.sh, tests/sighup.sh: New files.
> * Makefile.am (TESTS): Add tests/sigterm.sh and tests/sighup.sh.

Excellent!

I have one request: since the three tests differ only in the signal

name, could you make it a single test file and have a loop like:

for signal in SIGTERM SIGHUP SIGINT

TIA,

Ludo’.

Leo Famulari wrote on 27 Feb 2018 18:22

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:20180227172243.GA19017@jasmine.lan

On Tue, Feb 27, 2018 at 10:45:58AM +0100, Ludovic Courtès wrote:

Toggle quote (8 lines)

> Carlo Zancanaro <carlo@zancanaro.id.au> skribis:

> > I use Shepherd to manage my user session, and if I log out then

> > Shepherd leaves all my services running. This patch handles SIGTERM

> > and SIGHUP to prevent that.

> Good catch!

"This update broke my workflow" https://xkcd.com/1172/

Joking aside, I think this change is correct, but it would be great to
be able to have long-running unprivileged processes, as on systemd.
There, the administrator can use `loginctl enable-linger $USER`. We'd
want to do it in the system configuration. From loginctl(1):

------
Enable/disable user lingering for one or more users. If enabled for a
specific user, a user manager is spawned for the user at boot and kept
around after logouts. This allows users who are not logged in to run
long-running services. Takes one or more user names or numeric UIDs as
argument. If no argument is specified, enables/disables lingering for
the user of the session of the caller.
------

Ludovic Courtès wrote on 27 Feb 2018 22:03

Recipients:(name . Leo Famulari)(address . leo@famulari.name)

Message-ID:877eqyxhz9.fsf@gnu.org

Leo Famulari <leo@famulari.name> skribis:

Toggle quote (25 lines)> On Tue, Feb 27, 2018 at 10:45:58AM +0100, Ludovic Courtès wrote:
>> Carlo Zancanaro <carlo@zancanaro.id.au> skribis:
>> 
>> > I use Shepherd to manage my user session, and if I log out then
>> > Shepherd leaves all my services running. This patch handles SIGTERM
>> > and SIGHUP to prevent that.
>> 
>> Good catch!
>
> "This update broke my workflow" <https://xkcd.com/1172/>
>
> Joking aside, I think this change is correct, but it would be great to
> be able to have long-running unprivileged processes, as on systemd.
> There, the administrator can use `loginctl enable-linger $USER`. We'd
> want to do it in the system configuration. From loginctl(1):
>
> ------
> Enable/disable user lingering for one or more users. If enabled for a
> specific user, a user manager is spawned for the user at boot and kept
> around after logouts. This allows users who are not logged in to run
> long-running services. Takes one or more user names or numeric UIDs as
> argument. If no argument is specified, enables/disables lingering for
> the user of the session of the caller.
> ------

Indeed, that sounds useful.

I suppose on GuixSD PID�1 could start subprocesses (with an initially

empty config file?) for selected users at boot time. Would that make

sense?

Ludo’.

Carlo Zancanaro wrote on 27 Feb 2018 22:19

Recipients:(name . Leo Famulari)(address . leo@famulari.name)

Message-ID:87bmga9lm4.fsf@zancanaro.id.au

On Tue, Feb 27 2018, Leo Famulari wrote:

Toggle quote (2 lines)

> "This update broke my workflow" https://xkcd.com/1172/

How seriously should I take this? I understand that it's a joke, 
but are you currently relying on this behaviour of shepherd? I 
could add an action to the root service to tell shepherd to leave 
processes behind when it is killed, if we wanted to expose that 
behaviour.

Carlo

Carlo Zancanaro wrote on 27 Feb 2018 22:29

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 30532@debbugs.gnu.org)

Message-ID:87a7vu9l4h.fsf@zancanaro.id.au

Hey Ludo,

On Tue, Feb 27 2018, Ludovic Courtès wrote:

Toggle quote (20 lines)>> It's also worth noting that I had to run `gettextize` to get 
>> Shepherd
>> to build on current master, but I haven't included those 
>> changes in
>> this commit (because I don't know what should be committed and 
>> what
>> shouldn't).
>
> Right, I’m not sure what’s wrong with that.  I’ve committed the 
> same
> files under po/ as I did for Guix:
>
> --8<---------------cut 
> here---------------start------------->8---
> $ git ls-files po/
> po/Makevars
> po/POTFILES.in
> --8<---------------cut 
> here---------------end--------------->8---

If you clone shepherd from the repository, can you build it?

Comparing the guix repository to the shepherd one I can't easily

work out what needs to be done for this, but hopefully you can at

least reproduce my problem.

Toggle quote (6 lines)

> I have one request: since the three tests differ only in the

> signal

> name, could you make it a single test file and have a loop like:

> for signal in SIGTERM SIGHUP SIGINT

Updated patch is attached.

Carlo

From 717456edd92ba753daf5dd40e2f8d51eab3e7a73 Mon Sep 17 00:00:00 2001
From: Carlo Zancanaro <carlo@zancanaro.id.au>
Date: Tue, 20 Feb 2018 02:52:47 +1100
Subject: [PATCH] Terminate all services upon SIGTERM or SIGHUP

* modules/shepherd.scm (main): Add SIGTERM and SIGHUP handlers which stop
  root-service.
* tests/sigint.sh: Rename to...
* tests/signals.sh: ... this, and add tests for SIGTERM and SIGUP.
---
 Makefile.am                     |  3 ++-
 modules/shepherd.scm            | 11 +++++++++++
 tests/{sigint.sh => signals.sh} | 23 ++++++++++++++---------
 3 files changed, 27 insertions(+), 10 deletions(-)
 rename tests/{sigint.sh => signals.sh} (72%)

Toggle diff (94 lines)diff --git a/Makefile.am b/Makefile.am
index a30b11d..1c394e1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,6 +1,7 @@
 # Makefile.am -- How to build and install the Shepherd.
 # Copyright © 2002, 2003 Wolfgang Jährling <wolfgang@pro-linux.de>
 # Copyright © 2013, 2014, 2015, 2016, 2018 Ludovic Courtès <ludo@gnu.org>
+# Copyright © 2018 Carlo Zancanaro <carlo@zancanaro.id.au>
 #
 # This file is part of the GNU Shepherd.
 #
@@ -188,7 +189,7 @@ TESTS =						\
   tests/no-home.sh				\
   tests/pid-file.sh				\
   tests/status-sexp.sh				\
-  tests/sigint.sh
+  tests/signals.sh
 
 TEST_EXTENSIONS = .sh
 EXTRA_DIST += $(TESTS)
diff --git a/modules/shepherd.scm b/modules/shepherd.scm
index 5334657..650ba63 100644
--- a/modules/shepherd.scm
+++ b/modules/shepherd.scm
@@ -1,6 +1,7 @@
 ;; shepherd.scm -- The daemon shepherd.
 ;; Copyright (C) 2013, 2014, 2016, 2018 Ludovic Courtès <ludo@gnu.org>
 ;; Copyright (C) 2002, 2003 Wolfgang Jährling <wolfgang@pro-linux.de>
+;; Copyright (C) 2018 Carlo Zancanaro <carlo@zancanaro.id.au>
 ;;
 ;; This file is part of the GNU Shepherd.
 ;;
@@ -182,6 +183,16 @@
       (lambda _
         (stop root-service)))
 
+    ;; Stop everything when we get SIGTERM.
+    (sigaction SIGTERM
+      (lambda _
+        (stop root-service)))
+
+    ;; Stop everything when we get SIGHUP.
+    (sigaction SIGHUP
+      (lambda _
+        (stop root-service)))
+
     ;; Ignore SIGPIPE so that we don't die if a client closes the connection
     ;; prematurely.
     (sigaction SIGPIPE SIG_IGN)
diff --git a/tests/sigint.sh b/tests/signals.sh
similarity index 72%
rename from tests/sigint.sh
rename to tests/signals.sh
index 7354848..acb254a 100644
--- a/tests/sigint.sh
+++ b/tests/signals.sh
@@ -1,5 +1,6 @@
-# GNU Shepherd --- Make sure SIGINT is correctly handled.
+# GNU Shepherd --- Make sure SIGINT, SIGTERM, and SIGHUP are correctly handled.
 # Copyright © 2014, 2016 Ludovic Courtès <ludo@gnu.org>
+# Copyright © 2018 Carlo Zancanaro <carlo@zancanaro.id.au>
 #
 # This file is part of the GNU Shepherd.
 #
@@ -44,14 +45,18 @@ cat > "$conf"<<EOF
  (start 'test)
 EOF
 
-rm -f "$pid" "$stamp"
-shepherd -I -s "$socket" -c "$conf" --pid="$pid" --log="$log" &
+for signal in INT TERM HUP; do
 
-while [ ! -f "$pid" ] ; do sleep 0.5 ; done
+  rm -f "$pid" "$stamp" "$socket"
+  shepherd -I -s "$socket" -c "$conf" --pid="$pid" --log="$log" &
 
-# Send SIGINT to shepherd.
-kill -INT "`cat "$pid"`"
-while kill -0 "`cat "$pid"`" ; do sleep 0.5 ; done
+  while [ ! -f "$pid" ] ; do sleep 0.5 ; done
 
-# Make sure the service's 'stop' method was called.
-test -f "$stamp"
+  # Send signal to shepherd.
+  kill -$signal "`cat "$pid"`"
+  while kill -0 "`cat "$pid"`" ; do sleep 0.5 ; done
+
+  # Make sure the service's 'stop' method was called.
+  test -f "$stamp"
+
+done
-- 
2.16.1

Ludovic Courtès wrote on 27 Feb 2018 22:30

Recipients:(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)

Message-ID:87efl6w26t.fsf@gnu.org

Carlo Zancanaro <carlo@zancanaro.id.au> skribis:

Toggle quote (8 lines)

> On Tue, Feb 27 2018, Leo Famulari wrote:

>> "This update broke my workflow" <https://xkcd.com/1172/>

> How seriously should I take this? I understand that it's a joke, but

> are you currently relying on this behaviour of shepherd? I could add

> an action to the root service to tell shepherd to leave processes

> behind when it is killed, if we wanted to expose that behaviour.

Maybe with an option: --signal-handling-style=leo's?

:-)

Ludo’.

Ludovic Courtès wrote on 1 Mar 2018 10:55

Recipients:(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)(address . 30532@debbugs.gnu.org)

Message-ID:878tbc86hm.fsf@gnu.org

Carlo Zancanaro <carlo@zancanaro.id.au> skribis:

Toggle quote (10 lines)> From 717456edd92ba753daf5dd40e2f8d51eab3e7a73 Mon Sep 17 00:00:00 2001
> From: Carlo Zancanaro <carlo@zancanaro.id.au>
> Date: Tue, 20 Feb 2018 02:52:47 +1100
> Subject: [PATCH] Terminate all services upon SIGTERM or SIGHUP
>
> * modules/shepherd.scm (main): Add SIGTERM and SIGHUP handlers which stop
>   root-service.
> * tests/sigint.sh: Rename to...
> * tests/signals.sh: ... this, and add tests for SIGTERM and SIGUP.

Perfect. Applied, thanks!

Ludo’.

Leo Famulari wrote on 2 Mar 2018 01:30

Recipients:(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)

Message-ID:20180302003009.GA8305@jasmine.lan

On Wed, Feb 28, 2018 at 08:19:15AM +1100, Carlo Zancanaro wrote:

Toggle quote (8 lines)

> On Tue, Feb 27 2018, Leo Famulari wrote:

> > "This update broke my workflow" <https://xkcd.com/1172/>

> How seriously should I take this? I understand that it's a joke, but are you

> currently relying on this behaviour of shepherd? I could add an action to

> the root service to tell shepherd to leave processes behind when it is

> killed, if we wanted to expose that behaviour.

I am using it, but it's not critical. I can stay logged in with tmux or
something after this change. I'd rather we address this use case as
described previously, with something like 'enable-linger'.

Carlo Zancanaro wrote on 2 Mar 2018 08:25

Recipients:(name . Leo Famulari)(address . leo@famulari.name)

Message-ID:874llzgcrd.fsf@zancanaro.id.au

Hey Leo,

On Fri, Mar 02 2018, Leo Famulari wrote:

Toggle quote (3 lines)

> I'd rather we address this use case as described previously,

> with something like 'enable-linger'.

I've not used the enable-linger functionality of systemd, but 
after doing some reading, maybe it could work like this?

At boot-time we could start an instance of shepherd for each 
permitted user (maybe with a `lingering-user-service-type` which 
adds a shepherd service?). It loads ~/.config/shepherd/init.scm, 
and starts any services which are enabled. It also adds a 
pseudo-service "login", which allows a user to specify a 
dependency on there being a current user session (so shepherd can 
be told to start processes on login, and to terminate them on 
logout).

For users who don't have a `lingering-user-service-type` running, 
pid 1 will listen until they log-in and will start a shepherd 
instance for them on login, loading ~/.config/shepherd/init.scm. 
When they logout pid 1 will terminate their user shepherd instance 
(along with any services it started).

This will require a few more changes to shepherd before it can 
work, but does that sound like the sort of behaviour you want? 
Could you open a new bug about it?

Carlo

Ludovic Courtès wrote on 2 Mar 2018 13:57

control message for bug #30532

Recipients:(address . control@debbugs.gnu.org)

Message-ID:87fu5ihbye.fsf@gnu.org

tags 30532 fixed

close 30532

Your comment

This issue is archived.

To comment on this conversation send an email to 30532@debbugs.gnu.org

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date