GNU bug report logs - #54215
[PATCH Shepherd] service: Add #:rlimits parameter to 'exec-command' & co.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 54215 in the body.
You can then email your comments to 54215 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Attila Lendvai <attila <at> lendvai.name>
To: guix-patches <at> gnu.org
Cc: Attila Lendvai <attila <at> lendvai.name>
Subject: [PATCH Shepherd] service: Add #:rlimits parameter to 'exec-command' &
 co.
Date: Tue,  1 Mar 2022 19:12:43 +0100

* modules/shepherd/service.scm (exec-command, fork+exec-command,
make-forkexec-constructor): Add #:rlimits and honor it.  Reorder keyword args
where needed to be uniform.
---

this patch supersedes my previous CALL-IN-FORK proposal:

https://issues.guix.gnu.org/54205

i will either close that, or maybe do the internal refactor. we'll see.

 modules/shepherd/service.scm | 26 ++++++++++++++++++--------
 tests/forking-service.sh     | 15 +++++++++++++--
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm
index ad8608b..c6f0f4e 100644
--- a/modules/shepherd/service.scm
+++ b/modules/shepherd/service.scm
@@ -787,7 +787,8 @@ daemon writing FILE is running in a separate PID namespace."
                        (directory (default-service-directory))
                        (file-creation-mask #f)
                        (create-session? #t)
-                       (environment-variables (default-environment-variables)))
+                       (environment-variables (default-environment-variables))
+                       (rlimits '()))
   "Run COMMAND as the current process from DIRECTORY, with FILE-CREATION-MASK
 if it's true, and with ENVIRONMENT-VARIABLES (a list of strings like
 \"PATH=/bin\").  File descriptors 1 and 2 are kept as is or redirected to
@@ -795,6 +796,9 @@ LOG-FILE if it's true, whereas file descriptor 0 (standard input) points to
 /dev/null; all other file descriptors are closed prior to yielding control to
 COMMAND.  When CREATE-SESSION? is true, call 'setsid' first.
 
+Guile's SETRLIMIT function will be applied on the entries in RLIMITS.  For
+example a valid value would be '((nproc 10 100) (nofile 4096 4096)).
+
 By default, COMMAND is run as the current user.  If the USER keyword
 argument is present and not false, change to USER immediately before
 invoking COMMAND.  USER may be a string, indicating a user name, or a
@@ -808,6 +812,8 @@ false."
        ;; Programs such as 'mingetty' expect this.
        (setsid))
 
+     (for-each (cut apply setrlimit <>) rlimits)
+
      (chdir directory)
      (environ environment-variables)
 
@@ -893,7 +899,8 @@ false."
                             (file-creation-mask #f)
                             (create-session? #t)
                             (environment-variables
-                             (default-environment-variables)))
+                             (default-environment-variables))
+                            (rlimits '()))
   "Spawn a process that executed COMMAND as per 'exec-command', and return
 its PID."
   ;; Install the SIGCHLD handler if this is the first fork+exec-command call.
@@ -924,7 +931,8 @@ its PID."
                           #:directory directory
                           #:file-creation-mask file-creation-mask
                           #:create-session? create-session?
-                          #:environment-variables environment-variables))
+                          #:environment-variables environment-variables
+                          #:rlimits rlimits))
           pid))))
 
 (define* (make-forkexec-constructor command
@@ -932,15 +940,16 @@ its PID."
                                     (user #f)
                                     (group #f)
                                     (supplementary-groups '())
+                                    (log-file #f)
                                     (directory (default-service-directory))
-                                    (environment-variables
-                                     (default-environment-variables))
                                     (file-creation-mask #f)
                                     (create-session? #t)
+                                    (environment-variables
+                                     (default-environment-variables))
+                                    (rlimits '())
                                     (pid-file #f)
                                     (pid-file-timeout
-                                     (default-pid-file-timeout))
-                                    (log-file #f))
+                                     (default-pid-file-timeout)))
   "Return a procedure that forks a child process, closes all file
 descriptors except the standard output and standard error descriptors, sets
 the current directory to @var{directory}, sets the umask to
@@ -978,7 +987,8 @@ start."
                                   #:file-creation-mask file-creation-mask
                                   #:create-session? create-session?
                                   #:environment-variables
-                                  environment-variables)))
+                                  environment-variables
+                                  #:rlimits rlimits)))
       (if pid-file
           (match (read-pid-file pid-file
                                 #:max-delay pid-file-timeout
diff --git a/tests/forking-service.sh b/tests/forking-service.sh
index bd9aac9..a745bf4 100644
--- a/tests/forking-service.sh
+++ b/tests/forking-service.sh
@@ -25,6 +25,7 @@ conf="t-conf-$$"
 log="t-log-$$"
 pid="t-pid-$$"
 service_pid="t-service-pid-$$"
+service_nofiles="t-service-nofiles-$$"
 service2_pid="t-service2-pid-$$"
 service2_started="t-service2-starts-$$"
 
@@ -49,14 +50,15 @@ cat > "$conf"<<EOF
 (default-pid-file-timeout 10)
 
 (define %command
-  '("$SHELL" "-c" "sleep 600 & echo \$! > $PWD/$service_pid"))
+  '("$SHELL" "-c" "ulimit -n >$PWD/$service_nofiles; sleep 600 & echo \$! > $PWD/$service_pid"))
 
 (register-services
  (make <service>
    ;; A service that forks into a different process.
    #:provides '(test)
    #:start (make-forkexec-constructor %command
-                                      #:pid-file "$PWD/$service_pid")
+                                      #:pid-file "$PWD/$service_pid"
+                                      #:rlimits '((nofile 1567 1567)))
    #:stop  (make-kill-destructor)
    #:respawn? #f))
 
@@ -125,6 +127,15 @@ $herd status test2 | grep started
 test "`cat $PWD/$service2_started`" = "started
 started"
 
+
+
+# test if nofiles was set properly
+test -f "$service_nofiles"
+nofiles_value="`cat $service_nofiles`"
+test 1567 -eq $nofiles_value
+
+
+
 # Try to trigger eventual race conditions, when killing a process between fork
 # and execv calls.
 for i in `seq 1 50`
-- 
2.34.0

Message #8 received at 54215 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Attila Lendvai <attila <at> lendvai.name>, 54215 <at> debbugs.gnu.org
Subject: Re: [bug#54215] [PATCH Shepherd] service: Add #:rlimits parameter
 to 'exec-command' & co.
Date: Tue, 01 Mar 2022 19:26:05 +0100

[Message part 1 (text/plain, inline)]

Attila Lendvai schreef op di 01-03-2022 om 19:12 [+0100]:
>  (define* (make-forkexec-constructor command
> [...]
> +                                  #:rlimits rlimits)))

I think it would be better to verify if rlimits is well-formed
before forking, to let exception reporting work better.  WYDT?

Greetings,
Maxime.

[signature.asc (application/pgp-signature, inline)]

Message #11 received at 54215 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Attila Lendvai <attila <at> lendvai.name>, 54215 <at> debbugs.gnu.org
Subject: Re: [bug#54215] [PATCH Shepherd] service: Add #:rlimits parameter
 to 'exec-command' & co.
Date: Tue, 01 Mar 2022 19:32:52 +0100

[Message part 1 (text/plain, inline)]

Maxime Devos schreef op di 01-03-2022 om 19:26 [+0100]:
> before forking, to let exception reporting work better.  WYDT?

Also, if the

  (begin
    (for-each ...)
    (unblock-signals ...)
    (exec-command ...))

throws an exception, then it probably it should ignore exception
handlers and ignore dynamic-wind, otherwise the listening socket
might be deleted (in call-with-server-socket) (*).  This can be
done by catching all exceptions and calling 'primitive-_exit'
in case of an exception.

(*) unverified

Greetings,
Maxime.

[signature.asc (application/pgp-signature, inline)]

Message #14 received at 54215 <at> debbugs.gnu.org (full text, mbox):

From: Attila Lendvai <attila <at> lendvai.name>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 54215 <at> debbugs.gnu.org
Subject: Re: [bug#54215] [PATCH Shepherd] service: Add #:rlimits parameter to
 'exec-command' & co.
Date: Tue, 01 Mar 2022 18:35:47 +0000

> I think it would be better to verify if rlimits is well-formed
> before forking, to let exception reporting work better. WYDT?

now that i have a reaconable edit-build-test cycle for shepherd, i'm planning to
clean up shepherd error reporting and logging, so that when an error occur then
there's a proper backtrace in the shepherd logs.

i'd rather work on that instead. does that sound reasonable?

but feel free to tailor this as you see fit!

--
• attila lendvai
• PGP: 963F 5D5F 45C7 DFCD 0A39
--
“Don’t be a slave to your own ignorance. Know where your opinions, especially the strongly held ones, came from and be brave enough to question them.”
	— Dean van Drasek

Message #17 received at 54215 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Attila Lendvai <attila <at> lendvai.name>
Cc: 54215 <at> debbugs.gnu.org
Subject: Re: [bug#54215] [PATCH Shepherd] service: Add #:rlimits parameter
 to 'exec-command' & co.
Date: Tue, 01 Mar 2022 19:38:08 +0100

[Message part 1 (text/plain, inline)]

Attila Lendvai schreef op di 01-03-2022 om 18:35 [+0000]:
> now that i have a reaconable edit-build-test cycle for shepherd, i'm planning to
> clean up shepherd error reporting and logging, so that when an error occur then
> there's a proper backtrace in the shepherd logs.
> 
> i'd rather work on that instead. does that sound reasonable?

Sure!  Better error reporting and rlimit support are orthogonal
concerns.

Greetings,
Maxime.

[signature.asc (application/pgp-signature, inline)]

Message #20 received at 54215 <at> debbugs.gnu.org (full text, mbox):

From: Attila Lendvai <attila <at> lendvai.name>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 54215 <at> debbugs.gnu.org
Subject: Re: [bug#54215] [PATCH Shepherd] service: Add #:rlimits parameter to
 'exec-command' & co.
Date: Tue, 01 Mar 2022 19:17:21 +0000

> > now that i have a reaconable edit-build-test cycle for shepherd, i'm planning to
> > clean up shepherd error reporting and logging, so that when an error occur then
> > there's a proper backtrace in the shepherd logs.
> > i'd rather work on that instead. does that sound reasonable?
>
> Sure! Better error reporting and rlimit support are orthogonal
> concerns.

well, it's not orthogonal in the sense that i can only work on one of them in
the same unit of time, and this is already a side-project of a side-project for
me.

let me know if sanity checking the rlimit arg is a precondition for applying
this patch, and then i'll look into it.

otherwise APPLY and SETRLIMIT both signal any errors they encounter, and i think
better logging and backtraces will take us much farther than numerous sanity
checks and error messages, let alone the not-so-apparent costs of the extra LoC
that they introduce into the code.

--
• attila lendvai
• PGP: 963F 5D5F 45C7 DFCD 0A39
--
“The only way to have a friend is to be one.”
	— Ralph Waldo Emerson (1803–1882)

Message #23 received at 54215 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Attila Lendvai <attila <at> lendvai.name>
Cc: 54215 <at> debbugs.gnu.org
Subject: Re: [bug#54215] [PATCH Shepherd] service: Add #:rlimits parameter
 to 'exec-command' & co.
Date: Tue, 01 Mar 2022 20:46:39 +0100

[Message part 1 (text/plain, inline)]

Attila Lendvai schreef op di 01-03-2022 om 19:17 [+0000]:
> > Sure! Better error reporting and rlimit support are orthogonal
> > concerns.
> 
> well, it's not orthogonal in the sense that i can only work on one of
> them in the same unit of time, and this is already a side-project of
> a side-project for me.
> 
> let me know if sanity checking the rlimit arg is a precondition for
> applying this patch, and then i'll look into it.

Sanity-checking the rlimits (and environment-variables, file-umask,
etc.) can be left for later I believe.

Greetings,
Maxime.

[signature.asc (application/pgp-signature, inline)]

Message #26 received at 54215 <at> debbugs.gnu.org (full text, mbox):

From: Attila Lendvai <attila <at> lendvai.name>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 54215 <at> debbugs.gnu.org
Subject: Re: [bug#54215] [PATCH Shepherd] service: Add #:rlimits parameter to
 'exec-command' & co.
Date: Fri, 04 Mar 2022 08:29:46 +0000

if there aren't any obstacles left, then i'd appreciate if merging this weren't delayed too long.

once the shepherd-for-guix commits also get merged, i can send or update that patch to also include this #:rlimits shepherd commit, and then publish the service code on my channel for a wider audience.

i won't be available on the weekend, but let me know if there's any way i can help the process, and i'll look to it when i'm back at the machine.

--
• attila lendvai
• PGP: 963F 5D5F 45C7 DFCD 0A39
--
“A private central bank issuing the public currency is a greater menace to the liberties of the people than a standing army. […] We must not let our rulers load us with perpetual debt.”
	— Thomas Jefferson (1743–1826)

Message #31 received at 54215-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Attila Lendvai <attila <at> lendvai.name>
Cc: 54215-done <at> debbugs.gnu.org, Maxime Devos <maximedevos <at> telenet.be>
Subject: Re: bug#54215: [PATCH Shepherd] service: Add #:rlimits parameter to
 'exec-command' & co.
Date: Mon, 21 Mar 2022 13:48:44 +0100

Hi Attila,

Attila Lendvai <attila <at> lendvai.name> skribis:

> * modules/shepherd/service.scm (exec-command, fork+exec-command,
> make-forkexec-constructor): Add #:rlimits and honor it.  Reorder keyword args
> where needed to be uniform.

Pushed, at last!

  https://git.savannah.gnu.org/cgit/shepherd.git/commit/?id=3ee9a7193d73821d6f1dd76a745ed5e4bb1a78c8

I took the liberty to change #:rlimits to #:resource-limits, to be
consistent with the naming style used for other keyword arguments.

I also updated ‘doc/shepherd.texi’ and made sure the commit log mentions
all the changes.

Thank you for this welcome addition, and apologies for the delay!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.