-
Notifications
You must be signed in to change notification settings - Fork 18.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add "local" log driver #37092
Add "local" log driver #37092
Conversation
@@ -81,9 +85,13 @@ func decodeFunc(rdr io.Reader) func() (*logger.Message, error) { | |||
if err == io.ErrUnexpectedEOF { | |||
reader := io.MultiReader(dec.Buffered(), rdr) | |||
dec = json.NewDecoder(reader) | |||
retries++ | |||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is retries not incremented? Both here and L73?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were erroneous. The for loop already increments retries.
func TestTailFiles(t *testing.T) { | ||
s1 := strings.NewReader("Hello.\nMy name is Inigo Montoya.\n") | ||
s2 := strings.NewReader("I'm serious.\nDon't call me Shirley!\n") | ||
s3 := strings.NewReader("Roads?\nWhere we're going we don't need roads.\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add s4 with Why does it say paper jam when there is no paper jam?
.
Overall LGTM. Needs a followup doc PR |
@@ -107,7 +108,10 @@ func BenchmarkJSONFileLoggerLog(b *testing.B) { | |||
ContainerID: "a7317399f3f857173c6179d44823594f8294678dea9999662e5c625b5a1c7657", | |||
LogPath: tmp.Join("container.log"), | |||
Config: map[string]string{ | |||
"labels": "first,second", | |||
"labels": "first,second", | |||
"max-file": "10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, the intent of this logging driver is to be an "optimised, general-purpose logging driver" (which could become the default).
The storage mechanisms for this driver (currently, file-based, file-format-x) is an implementation detail. Because of that, should we abstract these options? i.e., have options for:
- "maximum amount of logs to keep" (the driver can decide to use multiple files to optimize)
- "maximum age of logs to keep" (in light of GDPR [EU GDPR] Time-based log rotation for JSON file log driver #36853)
- compression; good one: is there a significant overhead? are there situations where you want to disable this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this on the maintainers meeting and decided to keep using the same options that are available for jsonfile. Others can be added later.
@@ -0,0 +1,9 @@ | |||
// Package local provides a logger implementation that stores logs on disk. | |||
// | |||
// Log messages are encoded as protobufs with a header and footer for each message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: If I understand correctly, we want the storage-format to be an implementation detail; if we ever decide to switch to/add a new storage format, would there be a need to store the format in the container's configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is covered a bit in the propsoal issue.
Basically we'd store the format details in the header of the log file.
I started to go ahead and add support in logfile
for setting headers, but it's a significant change, and technically not needed for the first format since we can detect that no format is specified and therefore it's the first format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically not needed for the first format since we can detect that no format is specified and therefore it's the first format.
Makes sense, and yes, that would work
Moved this to code review per discussion with @thaJeztah on #maintainers in Slack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM 🐫
if err := os.MkdirAll(logDir, 0700); err != nil { | ||
return nil, errdefs.System(errors.Wrap(err, "error creating local logs dir")) | ||
} | ||
info.LogPath = filepath.Join(logDir, "container.log") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't we set container.LogPath
here too ? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to expose this to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment about that? Just in case someone decides to add it, and doesn't know about this history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally able to give this another look; found some issues in tests, and some possible issues around max-file
validation/checks
pkg/tailfile/tailfile_test.go
Outdated
|
||
"bufio" | ||
|
||
"github.com/gotestyourself/gotestyourself/assert" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be updated to gotest.tools
@@ -309,33 +321,45 @@ func (w *LogFile) ReadLogs(config logger.ReadConfig, watcher *logger.LogWatcher) | |||
} | |||
|
|||
if config.Tail != 0 { | |||
// TODO(@cpuguy83): Instead of opening every file, only get the files which |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make a tracking-issue for this after this is merged
|
||
"github.com/docker/docker/daemon/logger" | ||
"github.com/docker/docker/pkg/tailfile" | ||
"github.com/gotestyourself/gotestyourself/assert" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should now use gotest.tools
if err := os.MkdirAll(logDir, 0700); err != nil { | ||
return nil, errdefs.System(errors.Wrap(err, "error creating local logs dir")) | ||
} | ||
info.LogPath = filepath.Join(logDir, "container.log") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment about that? Just in case someone decides to add it, and doesn't know about this history.
return errors.New("max size should be a positive number") | ||
} | ||
if cfg.MaxFileCount < 0 { | ||
return errors.New("max file count cannot be less than 0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be less than one?
Ok; testing; --log-opt max-file=0
is equivalent to --log-opt max-file=1
;
CID=$(docker run -dit --rm --log-driver=local --log-opt max-file=0 --log-opt max-size=100k busybox /bin/sh -c 's="aasdasdad"; while true; do echo $s; done')
watch ls -lah /var/lib/docker/containers/${CID}/local-logs/
Shows that it's maxing out at 100k, then resets the file;
Every 2.0s: ls -lah /var/lib/docker/containers/57656d42e81d529cf5c2ddd097460d1bbd64530ea0690b529fbbf5b65dccfbb7/local-logs/
drwx------ 2 root root 4.0K Jul 11 12:42 .
drwx------ 5 root root 4.0K Jul 11 12:42 ..
-rw-r----- 1 root root 0 Jul 11 12:44 container.log
Every 2.0s: ls -lah /var/lib/docker/containers/57656d42e81d529cf5c2ddd097460d1bbd64530ea0690b529fbbf5b65dccfbb7/local-logs/
drwx------ 2 root root 4.0K Jul 11 12:42 .
drwx------ 5 root root 4.0K Jul 11 12:42 ..
-rw-r----- 1 root root 90K Jul 11 12:44 container.log
Every 2.0s: ls -lah /var/lib/docker/containers/57656d42e81d529cf5c2ddd097460d1bbd64530ea0690b529fbbf5b65dccfbb7/local-logs/
drwx------ 2 root root 4.0K Jul 11 12:42 .
drwx------ 5 root root 4.0K Jul 11 12:42 ..
-rw-r----- 1 root root 0 Jul 11 12:44 container.log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some further testing; if no max-file
is provided, the default is 4 (5?);
Every 2.0s: ls -lah /var/lib/docker/containers/dfc64e17d8fa30ea9d5328d35a28cbe38b3852d69719f12af182a1418c5ce746/local-logs/ 117a4e8e575a: Wed Jul 11 12:56:21 2018
total 112K
drwx------ 2 root root 4.0K Jul 11 12:56 .
drwx------ 5 root root 4.0K Jul 11 12:55 ..
-rw-r----- 1 root root 54K Jul 11 12:56 container.log
-rw-r----- 1 root root 9.0K Jul 11 12:56 container.log.1.gz
-rw-r----- 1 root root 9.0K Jul 11 12:56 container.log.2.gz
-rw-r----- 1 root root 8.8K Jul 11 12:56 container.log.3.gz
-rw-r----- 1 root root 9.1K Jul 11 12:56 container.log.4.gz
But inspecting the container won't reflect that; should we show the default?
docker inspect $CID --format '{{json .HostConfig.LogConfig}}'
{"Type":"local","Config":{"max-size":"100k"}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could... maybe adding a DefaultConfig()
function to the logger interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think it would be better to not do this for now at least.
I'd prefer to not change the hostconfig on the container.
daemon/logger/local/config.go
Outdated
} | ||
|
||
if !cfg.DisableCompression { | ||
if cfg.MaxFileCount == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the above; MaxFileCount == 1
looks to do exactly the same as MaxFileCount == 0
, so this should probably be;
if cfg.MaxFileCount <= 1 {
SGTM |
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This makes it so consumers of `LogFile` should pass in how to get an io.Reader to the requested number of lines to tail. This is also much more efficient when tailing a large number of lines. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Previous z build failure seems infrastructure network related:
I kicked the z builds again, but if it can't get a decent run, I propose we ignore z results. We've been having intermittent z infrastructure issues the last few days. |
Agreed; z failure is definitely not related; everything else is green, so let's get this merged |
Just to be clear for users that read the description in #33475 and think that more is done than is currently complete. It is correct that this PR only addresses the adding of the Maybe add a checklist over there to show when things are merged and which docker release is targeted for changing the default from jsonfile? |
@yosifkit That is correct, this is only the logger. |
How do we enable this driver? |
@mterron it's not in a release yet, but it would be "--log-driver=local" |
Ref: moby/moby#37092 Also adds log-opt `compress` to json-file log driver because this was also added in the referenced PR. Signed-off-by: Harald Albers <github@albersweb.de>
Ref: moby/moby#37092 Also adds log-opt `compress` to json-file log driver because this was also added in the referenced PR. Signed-off-by: Harald Albers <github@albersweb.de> (cherry picked from commit c59038b) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Ref: moby/moby#37092 Also adds log-opt `compress` to json-file log driver because this was also added in the referenced PR. Signed-off-by: Harald Albers <github@albersweb.de> Upstream-commit: c59038b15c4fd2a796665925dec1cbb5993d4501 Component: cli
Ref: moby/moby#37092 Also adds log-opt `compress` to json-file log driver because this was also added in the referenced PR. Signed-off-by: Harald Albers <github@albersweb.de> (cherry picked from commit c59038b15c4fd2a796665925dec1cbb5993d4501) Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: e05745b4a55bb9a7d5e5d01eb614949ab1d8e897 Component: cli
Ref: moby/moby#37092 Also adds log-opt `compress` to json-file log driver because this was also added in the referenced PR. Signed-off-by: Harald Albers <github@albersweb.de> Upstream-commit: c59038b15c4fd2a796665925dec1cbb5993d4501 Component: cli Upstream-commit: 0e20f85 Component: cli
This driver shipped with Docker CE 18.09 and up, for those that were still waiting for this to arrive 👍 (I think it was already included in Docker Enterprise (EE) 18.03) |
This adds the "local" log driver described in #33475.