Message ID | 20220301181242.18384-1-attila@lendvai.name |
---|---|
State | Accepted |
Headers | show |
Series | [bug#54215,Shepherd] service: Add #:rlimits parameter to 'exec-command' & co. | expand |
Context | Check | Description |
---|---|---|
cbaines/applying patch | fail | View Laminar job |
cbaines/issue | success | View issue |
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.
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.
> 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
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.
> > 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)
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.
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)
Hi Attila, Attila Lendvai <attila@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’.
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`