diff mbox series

[bug#54205,Shepherd] Factor out a public CALL-IN-FORK.

Message ID 20220301070615.21028-1-attila@lendvai.name
State Accepted
Headers show
Series [bug#54205,Shepherd] Factor out a public CALL-IN-FORK. | expand

Checks

Context Check Description
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue

Commit Message

Attila Lendvai March 1, 2022, 7:06 a.m. UTC 
This enables service implementations to easily inject code that is run before
their service is started.  One such example is calling setrlimit from a start
action to set NOFILE (the open files limit), before the service is exec'ed and
thus inherits this value from the parent process, i.e. from Shepherd.

* modules/shepherd/service.scm (fork-and-call): New function.
(fork+exec-command): Use the above.
---
 modules/shepherd/service.scm | 51 ++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 22 deletions(-)

Comments

M March 1, 2022, 12:47 p.m. UTC | #1 
Attila Lendvai schreef op di 01-03-2022 om 08:06 [+0100]:
> their service is started.  One such example is calling setrlimit from a start
> action to set NOFILE (the open files limit), before the service is exec'ed and
> thus inherits this value from the parent process, i.e. from Shepherd.

'fork+exec-command' already accepts a 'environment-variables' and
'file-creation-mask', how about adding an 'open-file-limit' argument?
To me, that seems more declarative and less fragile than having
to call 'call-in-fork' manually in a 'start' procedure (*).

Support for other rlimits can be added on an as-needed basis.
Alternatively, the argument could be generalised to a more general
'rlimit' argument:

  #:rlimits
  `((,RLIMIT_AS ,SOFT ,HARD)
    (,RLIMIT_NPROC ,SOFT ,HARD)
    (,RLIMIT_NOFILE ,SOFT ,HARD))

WDYT?

Greetings,
Maxime.

(*) E.g., one of the ideas for making shepherd faster, was using some
kind of multi-threading.  Forking when multi-threading is ill-defined
(see POSIX) though, so some kind of zygote process + IPC might be
necessary
(http://neugierig.org/software/chromium/notes/2011/08/zygote.html has a
nice explanation on zygote processes, the bits about software updates
can be ignored here).
Ludovic Courtès March 2, 2022, 4:05 p.m. UTC | #2 
Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

> Attila Lendvai schreef op di 01-03-2022 om 08:06 [+0100]:
>> their service is started.  One such example is calling setrlimit from a start
>> action to set NOFILE (the open files limit), before the service is exec'ed and
>> thus inherits this value from the parent process, i.e. from Shepherd.
>
> 'fork+exec-command' already accepts a 'environment-variables' and
> 'file-creation-mask', how about adding an 'open-file-limit' argument?
> To me, that seems more declarative and less fragile than having
> to call 'call-in-fork' manually in a 'start' procedure (*).

Seconded.

> Support for other rlimits can be added on an as-needed basis.
> Alternatively, the argument could be generalised to a more general
> 'rlimit' argument:
>
>   #:rlimits
>   `((,RLIMIT_AS ,SOFT ,HARD)
>     (,RLIMIT_NPROC ,SOFT ,HARD)
>     (,RLIMIT_NOFILE ,SOFT ,HARD))
>
> WDYT?

This interface brings more flexibility, I’m all for it.

> (*) E.g., one of the ideas for making shepherd faster, was using some
> kind of multi-threading.  Forking when multi-threading is ill-defined

I think what we need is concurrency, not POSIX threads.  IOW, we can
achieve the concurrency we need without resorting to POSIX threads, for
example using Fibers on a single POSIX thread.

Thanks,
Ludo’.
M March 2, 2022, 6:21 p.m. UTC | #3 
Ludovic Courtès schreef op wo 02-03-2022 om 17:05 [+0100]:
> I think what we need is concurrency, not POSIX threads.  IOW, we can
> achieve the concurrency we need without resorting to POSIX threads, for
> example using Fibers on a single POSIX thread.

guile-fibers uses threads internally, e.g. in (fibers interrupts).
Interrupts can theoretically be avoided, but that has a downside
that if a start procedure goes into infinite loop (while forgetting to
sleep), the whole shepherd would hang.

I'm not saying that we need POSIX threads per-se -- I find
'choice-operation', 'perform-operation', the channel operations and
Fibers conditions much more convenient than the (lack of) POSIX
equivalents, but I'd prefer avoiding the assumption of single-threading
where feasible, to make it ourselves not harder than necessary in the
future, in case it turns out we need POSIX threading somewhere (even if
only as an implementation detail).

Greetings,
Maxime.
Attila Lendvai March 3, 2022, 8:04 a.m. UTC | #4 
> > Support for other rlimits can be added on an as-needed basis.
> >
> > Alternatively, the argument could be generalised to a more general
> > 'rlimit' argument:
> >
> > #:rlimits
> > `((,RLIMIT_AS ,SOFT ,HARD)
> > (,RLIMIT_NPROC ,SOFT ,HARD)
> > (,RLIMIT_NOFILE ,SOFT ,HARD))
> >
> > WDYT?
>
> This interface brings more flexibility, I’m all for it.


FTR, i've filed this as a patch (with a test!).

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

--
• attila lendvai
• PGP: 963F 5D5F 45C7 DFCD 0A39
--
“It is a miracle that curiosity survives formal education. It is a very grave mistake to think that the enjoyment of seeing and searching can be promoted by means of coercion and a sense of duty.”
	— Albert Einstein (1879–1955), 'Autobiographical Notes' (1949), slightly paraphrased
Ludovic Courtès March 21, 2022, 1:03 p.m. UTC | #5 
Attila Lendvai <attila@lendvai.name> skribis:

> FTR, i've filed this as a patch (with a test!).
>
> https://issues.guix.gnu.org/54215

Awesome, closing this one!

Ludo’.
diff mbox series

Patch

diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm
index ad8608b..8d5e30f 100644
--- a/modules/shepherd/service.scm
+++ b/modules/shepherd/service.scm
@@ -79,6 +79,7 @@ 
             make-forkexec-constructor
             make-kill-destructor
             exec-command
+            fork-and-call
             fork+exec-command
             default-pid-file-timeout
             read-pid-file
@@ -883,19 +884,8 @@  false."
   ;; Signals that the shepherd process handles.
   (list SIGCHLD SIGINT SIGHUP SIGTERM))
 
-(define* (fork+exec-command command
-                            #:key
-                            (user #f)
-                            (group #f)
-                            (supplementary-groups '())
-                            (log-file #f)
-                            (directory (default-service-directory))
-                            (file-creation-mask #f)
-                            (create-session? #t)
-                            (environment-variables
-                             (default-environment-variables)))
-  "Spawn a process that executed COMMAND as per 'exec-command', and return
-its PID."
+(define* (fork-and-call thunk)
+  "Call THUNK in a fork."
   ;; Install the SIGCHLD handler if this is the first fork+exec-command call.
   (unless %sigchld-handler-installed?
     (sigaction SIGCHLD handle-SIGCHLD SA_NOCLDSTOP)
@@ -916,17 +906,34 @@  its PID."
             ;; process.
             (unblock-signals %precious-signals)
 
-            (exec-command command
-                          #:user user
-                          #:group group
-                          #:supplementary-groups supplementary-groups
-                          #:log-file log-file
-                          #:directory directory
-                          #:file-creation-mask file-creation-mask
-                          #:create-session? create-session?
-                          #:environment-variables environment-variables))
+            (thunk))
           pid))))
 
+(define* (fork+exec-command command
+                            #:key
+                            (user #f)
+                            (group #f)
+                            (supplementary-groups '())
+                            (log-file #f)
+                            (directory (default-service-directory))
+                            (file-creation-mask #f)
+                            (create-session? #t)
+                            (environment-variables
+                             (default-environment-variables)))
+  "Spawn a process that executed COMMAND as per 'exec-command', and return
+its PID."
+  (fork-and-call
+   (lambda ()
+     (exec-command command
+                   #:user user
+                   #:group group
+                   #:supplementary-groups supplementary-groups
+                   #:log-file log-file
+                   #:directory directory
+                   #:file-creation-mask file-creation-mask
+                   #:create-session? create-session?
+                   #:environment-variables environment-variables))))
+
 (define* (make-forkexec-constructor command
                                     #:key
                                     (user #f)