[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
?
Your comment

This issue is archived.

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

To respond to this issue using the mumi CLI, first switch to it
mumi current 54215
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch