Add support for logging to a file #6429
Conversation
This commit introduces a rotating file logger for Nomad Agent Logs. The logger implementation itself is a lift and shift from Consul, with tests updated to fit with the Nomad pattern of using require, and not having a testutil for creating tempdirs cleanly.
There was a problem hiding this comment.
Should we drop logfile.go into the logutils package for sharing with Consul? On the one hand it's only a couple hundred lines of code with tests and shouldn't change often. On the other hand it's not up to our code standards and would be nice to fixup in a way that way easy for Consul to share.
I'm fine merging and shipping as-is since it seems to be working well for Consul.
Don't forget the changelog entry and docs.
| ) | ||
|
|
||
| // logFile is used to setup a file based logger that also performs log rotation | ||
| type logFile struct { |
There was a problem hiding this comment.
Let's link to https://github.com/hashicorp/consul/blob/53f3962cab68ec3590cbbe16af4e54b500b0570f/logger/logfile.go
in a comment so when people wonder why this file looks so funny they know it's shared.
| } | ||
| // Prune if there are more files stored than the configured max | ||
| stale := len(matches) - l.MaxFiles | ||
| for i := 0; i < stale; i++ { |
There was a problem hiding this comment.
I don't see that Glob guarantees ordered results so we should probably sort.Strings first. Might want to mention this to Consul to?
There was a problem hiding this comment.
Glob internally guarantees (in filepath.glob) that files are returned in lexographical order by doing a sort.Strings after it performs a Readdirnames, but this doesn't get surfaced in the Glob public documentation so I'll add a sort.Strings to be defensive.
command/agent/log_file.go
Outdated
| return 0, err | ||
| } | ||
| l.BytesWritten += int64(len(b)) | ||
| return l.FileInfo.Write(b) |
There was a problem hiding this comment.
This should capture n, err and update BytesWritten with n not with len(b), although I guess if you get an error writing your logs you have way bigger problems than your rotation being off a bit.
Let's not worry about fixing it I guess.
| } | ||
| } | ||
| // Check for the last contact and rotate if necessary | ||
| if err := l.rotate(); err != nil { |
There was a problem hiding this comment.
This approach will create files larger than the size limit which isn't correct but also shouldn't hurt much.
Let's not worry about fixing it I guess.
| ) | ||
|
|
||
| var ( | ||
| now = time.Now |
There was a problem hiding this comment.
This would be easier to override in tests if it were on the logFile struct, but we never touch it in tests anyway so I guess it's not worth fixing.
There was a problem hiding this comment.
yeah there's a bunch of stuff I'm not a fan of in here, but the incremental benefit of adding support and improving later felt worthwhile to me. (and reduces the surface area for scary bugs bc we know this ~works)
Currently this logging implementation is dependent on the order of files as returned by filepath.Glob, which although internal methods are documented to be lexographical, does not publicly document this. Here we defensively resort.
Currently this assumes that a short write will never happen. While these are improbable in a case where rotation being off a few bytes would matter, this now correctly tracks the number of written bytes.
* guide for installing as a windows service. * configuration for logging to file from PR #6429
* guide for installing as a windows service. * configuration for logging to file from PR #6429
* guide for installing as a windows service. * configuration for logging to file from PR #6429
* guide for installing as a windows service. * configuration for logging to file from PR #6429
This is the basic code to add the Windows Service Manager hooks to Nomad. Includes vendoring golang.org/x/sys/windows/svc and added Docs: * guide for installing as a windows service. * configuration for logging to file from PR #6429
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR introduces support for file logging in Nomad.
The implementation of
logFileis a lift and shift from Consul'slogger/LogFile.It introduces a few new configuration options:
It also updates the command/agent log setup to more easily allow extra logging backends in the future, alongside wiring up configuration for logFile.
required for #6372