| Message ID | 2e5292c2d45e525aa1b8e4c495704104d4121291.camel@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#55080,shepherd] service: Gracefully handle non-existing log directories. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/applying patch | fail | View Laminar job |
| cbaines/issue | success | View issue |
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’.
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
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’.
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?
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 --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'."