diff mbox series

[bug#55080,shepherd] service: Gracefully handle non-existing log directories.

Message ID 2e5292c2d45e525aa1b8e4c495704104d4121291.camel@gmail.com
State New
Headers
Series [bug#55080,shepherd] service: Gracefully handle non-existing log directories. |

Commit Message

Liliana Marie Prikler April 23, 2022, 1:11 p.m. UTC 
  * gnu/packages/services.scm (%service-file-logger): New variable,
implementing...
(service-file-logger): ... the old behaviour of this variable.  Catch system
errors from %service-file-logger and handle them.
---
 modules/shepherd/service.scm | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Comments

Ludovic Courtès April 30, 2022, 2:15 p.m. UTC | #1 
Hi,

Liliana Marie Prikler <liliana.prikler@gmail.com> skribis:

> * gnu/packages/services.scm (%service-file-logger): New variable,
> implementing...
> (service-file-logger): ... the old behaviour of this variable.  Catch system
> errors from %service-file-logger and handle them.

[...]

> +(define (service-file-logger file input)
> +  "Return a thunk meant to run as a fiber that reads from INPUT and logs it to
> +FILE."
> +  (catch 'system-error
> +    (lambda ()
> +      (%service-file-logger file input))
> +    (lambda args
> +      (if (= ENOENT (system-error-errno args))
> +          (begin
> +            (mkdir-p (dirname file))
> +            (%service-file-logger file input))
> +          (apply throw args)))))

I wonder to what extent automatically creating log directories is a good
idea.  A potential drawback is if shepherd creates them with unexpected
ownership or permissions.

Did you encounter this issue while working on services?

Am I right that the Shepherd 0.8 had the same problem?

Thanks,
Ludo’.
Liliana Marie Prikler April 30, 2022, 2:26 p.m. UTC | #2 
Am Samstag, dem 30.04.2022 um 16:15 +0200 schrieb Ludovic Courtès:
> 
> > +(define (service-file-logger file input)
> > +  "Return a thunk meant to run as a fiber that reads from INPUT
> > and logs it to
> > +FILE."
> > +  (catch 'system-error
> > +    (lambda ()
> > +      (%service-file-logger file input))
> > +    (lambda args
> > +      (if (= ENOENT (system-error-errno args))
> > +          (begin
> > +            (mkdir-p (dirname file))
> > +            (%service-file-logger file input))
> > +          (apply throw args)))))
> 
> I wonder to what extent automatically creating log directories is a
> good idea.  A potential drawback is if shepherd creates them with
> unexpected ownership or permissions.
As far as I know, those logs should be managed by shepherd, no?  It
just redirects stdout/stderr there, or is there something special going
on?

> Did you encounter this issue while working on services?
> 
> Am I right that the Shepherd 0.8 had the same problem?
It might be, I don't know.  I've encountered this for non-existing log
directory, so a reproducer would be setting #:log-file to $test-tmp-
directory/does-not-exist/log and check for each service.

Cheers
Ludovic Courtès May 1, 2022, 1:32 p.m. UTC | #3 
Liliana Marie Prikler <liliana.prikler@gmail.com> skribis:

> Am Samstag, dem 30.04.2022 um 16:15 +0200 schrieb Ludovic Courtès:

[...]

>> Did you encounter this issue while working on services?
>> 
>> Am I right that the Shepherd 0.8 had the same problem?
> It might be, I don't know.  I've encountered this for non-existing log
> directory, so a reproducer would be setting #:log-file to $test-tmp-
> directory/does-not-exist/log and check for each service.

Usually /var/log and similar directories are created not by shepherd but
by Guix System, the distro being used, or whatever.  That’s why I wonder
if it’s shepherd’s job to do that.

I was asking how you encountered it to better understand in which
circumstances the problem can occur in practice and what the failure
more is like.

Thanks,
Ludo’.
Liliana Marie Prikler May 1, 2022, 1:50 p.m. UTC | #4 
Am Sonntag, dem 01.05.2022 um 15:32 +0200 schrieb Ludovic Courtès:
> > 
> > > Did you encounter this issue while working on services?
> > > 
> > > Am I right that the Shepherd 0.8 had the same problem?
> > It might be, I don't know.  I've encountered this for non-existing
> > log directory, so a reproducer would be setting #:log-file to
> > $test-tmp-directory/does-not-exist/log and check for each service.
> 
> Usually /var/log and similar directories are created not by shepherd
> but by Guix System, the distro being used, or whatever.  That’s why I
> wonder if it’s shepherd’s job to do that.
Hmm, it might not be.  Still, I wouldn't like shepherd to fail in such
a weird manner if the log file can't be created.  Should we write a
warning to shepherd's log and redirect to /dev/null instead?  Should we
just kill the service?
Ludovic Courtès Aug. 29, 2022, 3:18 p.m. UTC | #5 
Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> skribis:

> Am Sonntag, dem 01.05.2022 um 15:32 +0200 schrieb Ludovic Courtès:
>> > 
>> > > Did you encounter this issue while working on services?
>> > > 
>> > > Am I right that the Shepherd 0.8 had the same problem?
>> > It might be, I don't know.  I've encountered this for non-existing
>> > log directory, so a reproducer would be setting #:log-file to
>> > $test-tmp-directory/does-not-exist/log and check for each service.
>> 
>> Usually /var/log and similar directories are created not by shepherd
>> but by Guix System, the distro being used, or whatever.  That’s why I
>> wonder if it’s shepherd’s job to do that.
> Hmm, it might not be.  Still, I wouldn't like shepherd to fail in such
> a weird manner if the log file can't be created.

I reread this thread and I concur.  Patch finally pushed as
b0d3f625543bcb32e94167c27cba153f9fc03acd.

Thanks,
Ludo’.
diff mbox series

Patch

diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm
index 013347b..567a08b 100644
--- a/modules/shepherd/service.scm
+++ b/modules/shepherd/service.scm
@@ -873,9 +873,9 @@  daemon writing FILE is running in a separate PID namespace."
               (try-again)
               (apply throw args)))))))
 
-(define (service-file-logger file input)
-  "Return a thunk meant to run as a fiber that reads from INPUT and logs it to
-FILE."
+(define (%service-file-logger file input)
+  "Like 'service-file-logger', but doesn't handle the case in which FILE does
+not exist."
   (let* ((fd     (open-fdes file (logior O_CREAT O_WRONLY O_APPEND) #o640))
          (output (fdopen fd "al")))
     (set-port-encoding! output "UTF-8")
@@ -894,6 +894,19 @@  FILE."
                  (format output "~a~a~%" prefix line)
                  (loop))))))))))
 
+(define (service-file-logger file input)
+  "Return a thunk meant to run as a fiber that reads from INPUT and logs it to
+FILE."
+  (catch 'system-error
+    (lambda ()
+      (%service-file-logger file input))
+    (lambda args
+      (if (= ENOENT (system-error-errno args))
+          (begin
+            (mkdir-p (dirname file))
+            (%service-file-logger file input))
+          (apply throw args)))))
+
 (define (service-builtin-logger command input)
   "Return a thunk meant to run as a fiber that reads from INPUT and logs to
 'log-output-port'."