Re-open the log file by filesystem notify #43359
Conversation
github-actions
bot
requested review from
camscale,
espadolini,
mmcallister,
ptgott and
r0mant
This PR implements an handler for the CONT signal. The handler will close and re-open the log file. If the log destination is not a file, this is a no-op. Useful for logrotate postrotate hook.
marcoandredinis
force-pushed
the
marco/logrotate
branch
from
34069d3
to
bdfa269
Compare
|
🤖 Vercel preview here: https://docs-rjalvj2mc-goteleport.vercel.app/docs/ver/preview |
|
🤖 Vercel preview here: https://docs-fvl6lzscr-goteleport.vercel.app/docs/ver/preview |
|
@marcoandredinis I'm going to change the logic a bit, to test it within SIGHUP signal instead, it might be useful in case we have some active sessions that produce the logs and in the case with graceful restart, we should guarantee that the logs from still active sessions not going to split into two log files, so process keep writing in the same file until we can restart |
|
@marcoandredinis I've checked the implementation with SIGHUP, SIGCONT you cover both cases so everything looks good |
|
Does that imply that any logrotate hook has to send SIGCONT to all the teleport processes that might've spawned by HUP? Is that true of all the exec and port forward processes too? |
There was a problem hiding this comment.
@vapopov @marcoandredinis @espadolini I just had a thought regarding this - instead of hogging another signal here (which are a scarce resource for us), what do you think about adding a new "post logrotate" command to the new debug service that @gabrielcorado recently implemented?
It is enabled by default and already has the logic for changing log level for a running process so having ability to reopen log files seems like a natural addition. We could add another API handler to it and a corresponding CLI command, say teleport debug post-logrotate that would be set as a post-rotate hook for logrotate.
WDYGT?
|
The debug service doesn't have a way to make all running Teleport processes reopen their log file, which is possible instead with a |
That's fair. I'm slightly concerned about us just picking random signals for purposes they're not originally intended for meaning e.g. CONT being supposed to resume a suspended process (not saying we have or will have a use-case for it in teleport but still feels a bit off) but I don't see any other signals that would be more appropriate. Interestingly enough, it appears you can actually send payload with the signal using |
|
🤖 Vercel preview here: https://docs-kkdhvly56-goteleport.vercel.app/docs/ver/preview |
| s.lock.Lock() | ||
| defer s.lock.Unlock() |
There was a problem hiding this comment.
Should we have a separate lock for the write synchronization and for the rest of the FileSharedWriter machinery?
There was a problem hiding this comment.
Not sure if we really need it, because we can't write when in goroutine reopen is triggered, same for closing the file. Only RunWatcherReopen kind of independent of the writing process, but we call it before file shared writer is assigned to the logger (it might be even moved to constructor)
lib/utils/log/file_writer.go
Outdated
| if !ok { | ||
| return | ||
| } | ||
| slog.ErrorContext(context.Background(), "Error received on logger watcher", "error", err) |
There was a problem hiding this comment.
Nit: can we pass the context to all slog functions from the caller?
There was a problem hiding this comment.
unfortunately in configuration process (where we init FileSharedWriter) we don't have any context, I wanted to use slog.Error, but it blacklisted in linter test. One way is add it in general configuration function
func Configure(clf *CommandLineFlags, cfg *servicecfg.Config, legacyAppFlags bool) error
func ApplyFileConfig(fc *FileConfig, cfg *servicecfg.Config) error
func applyLogConfig(loggerConfig Log, cfg *servicecfg.Config) errorThere was a problem hiding this comment.
I just meant you can pass it here from RunWatcherReopen, and RunWatcherReopen can in turn accept the context itself from its caller in applyLogConfig - there you can pass context.Background(), but then if we need to plumb a proper context here later if would be easier to do.
There was a problem hiding this comment.
got you, it was initially like that, then I removed within watcher closing by context cancelation
@r0mant I've made watcher always enabled, and removed this config flag
|
🤖 Vercel preview here: https://docs-55x5lzgjq-goteleport.vercel.app/docs/ver/preview |
public-teleport-github-review-bot
bot
removed request for
camscale,
zmb3,
gabrielcorado,
hugoShaka,
mmcallister and
xinding33
|
@marcoandredinis See the table below for backport results.
|
* Re-open the log file on OS Signal trigger This PR implements an handler for the CONT signal. The handler will close and re-open the log file. If the log destination is not a file, this is a no-op. Useful for logrotate postrotate hook. * signal replaced with the fsnotify * code review changes: - refactored to launch watcher with process context - make file shared writer as singleton - add test for logrotate case * add locking for write, reopen * - configuration for log section update - test simplification * code review changes: - add closing for file shared writer - make runWatcherFunc not exported * linter fixes * change default file mode for log file simplify setting global watcher * terminate watcher after override global one added test for validating global override * make log file name static for shared writer * replace logic to use finalizer instead * fix possible locking write while reopen is triggered * wrap internal error, fix incorrect unlock * drop configuration flag, make watcher always enabled * tune context passing --------- Co-authored-by: Vadym Popov <vadym.popov@goteleport.com>
This PR implements a handler for logfile re-open by subscription on the filesystem notifications, once file renamed (which usually happen with logrotate default configuration), log file must be reopened. Added configuration flag to enable or disable the watcher, might be useful if graceful restart should be used instead.
changelog: Re-open log file by filesystem notify rename/remove (useful for logrotate).
Related:
#6589
#9468
https://github.com/gravitational/customer-sensitive-requests/issues/155
Co-authored-by: @vapopov