[PATCH Shepherd] service: Add #:rlimits parameter to 'exec-command' & co.

  • Done
  • quality assurance status badge
Details
3 participants
  • Attila Lendvai
  • Ludovic Courtès
  • Maxime Devos
Owner
unassigned
Submitted by
Attila Lendvai
Severity
normal
A
A
Attila Lendvai wrote on 1 Mar 2022 19:12
(address . guix-patches@gnu.org)(name . Attila Lendvai)(address . attila@lendvai.name)
20220301181242.18384-1-attila@lendvai.name
* 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:


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(-)

Toggle diff (132 lines)
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
M
M
Maxime Devos wrote on 1 Mar 2022 19:26
48526d32a684a531940b59edcfb016ab81c7d3bb.camel@telenet.be
Attila Lendvai schreef op di 01-03-2022 om 19:12 [+0100]:
Toggle quote (4 lines)
>  (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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYh5lPRccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7lsgAQCRYf15n4X8MlvrQoVOlQxUlIwb
tRe0peTxCvsHPncy1wD/Sb8/PM1anV98sXgV+iuIRSy0cjZOTda/RRaMxOv30wY=
=n/l0
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 1 Mar 2022 19:32
a230c56c7202d6ea11f5cd97baf748606b40b4fb.camel@telenet.be
Maxime Devos schreef op di 01-03-2022 om 19:26 [+0100]:
Toggle quote (2 lines)
> 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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYh5m1BccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7ukhAP9mOdpmBAn/s6acdI/CXa85Hwza
IXSDQUJ5dWjDs70VzwEAwgxK8UJt1yi+WtNp+y6XLjB4o+QR8gTTqGkedkRfIAo=
=/7az
-----END PGP SIGNATURE-----


A
A
Attila Lendvai wrote on 1 Mar 2022 19:35
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 54215@debbugs.gnu.org)
ZOcTGAkar68OcuV6P3Mo-h9KAM5drKW7GlswHK9EPoVlse-MbWIp0IdmwQ41BDxi8yLY9SdNrY0qeoPpP14fe8ALj5PnVRnExsqUYXwBmDE=@lendvai.name
Toggle quote (3 lines)
> 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
M
M
Maxime Devos wrote on 1 Mar 2022 19:38
(name . Attila Lendvai)(address . attila@lendvai.name)(address . 54215@debbugs.gnu.org)
9dc56d38b66bcdecf7eb6143f873f07c6cf54582.camel@telenet.be
Attila Lendvai schreef op di 01-03-2022 om 18:35 [+0000]:
Toggle quote (6 lines)
> 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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYh5oEBccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7mbuAQCPAza4OTx0VN3rZxCL0CVPQpQO
fAeaEy3SD9gfgFsh7wEA0oszpj6F/Wq6btpfBovrTklrKRQH4LAmysdvMpi+KgE=
=MFv/
-----END PGP SIGNATURE-----


A
A
Attila Lendvai wrote on 1 Mar 2022 20:17
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 54215@debbugs.gnu.org)
CH236W_O1EHlQ2glonl5hirMhi1nIPw0r8xWMSQwcnWfXUTfx-9LMOxXZS2djx1RafhyQUCOTemS9sb1lgXrViu0hoWscAX7PTlZnkPz6po=@lendvai.name
Toggle quote (8 lines)
> > 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)
M
M
Maxime Devos wrote on 1 Mar 2022 20:46
(name . Attila Lendvai)(address . attila@lendvai.name)(address . 54215@debbugs.gnu.org)
29ee0164622feb55d52a5e806df9abd5d617467e.camel@telenet.be
Attila Lendvai schreef op di 01-03-2022 om 19:17 [+0000]:
Toggle quote (10 lines)
> > 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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYh54HxccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7nHkAQDfvcbjH7XQSg8c6AXlCmKdVz0b
d/gaJ4LoB/umMaVFSQD7BMsZIH8HrrQ4NCUaX4h/tkRGp4TVE5qGmC+h8M5s5wo=
=pEV/
-----END PGP SIGNATURE-----


A
A
Attila Lendvai wrote on 4 Mar 2022 09:29
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 54215@debbugs.gnu.org)
lh9HQQJc6k8cA03kmLPhMRnraUTq1Z8yRk855akSoJGvJS1f5OVJ-BE-Fw5Ne5DdJnCP77zYLo5sk9Gtx4XggHKWluMKRTabgajIEau_kjE=@lendvai.name
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)
L
L
Ludovic Courtès wrote on 21 Mar 2022 13:48
Re: bug#54215: [PATCH Shepherd] service: Add #:rlimits parameter to 'exec-command' & co.
(name . Attila Lendvai)(address . attila@lendvai.name)
878rt3lbdf.fsf@gnu.org
Hi Attila,

Attila Lendvai <attila@lendvai.name> skribis:

Toggle quote (4 lines)
> * 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!


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’.
Closed
?