Add "local" log driver by cpuguy83 · Pull Request #37092 · moby/moby

cpuguy83 · 2018-05-17T14:57:46Z

This adds the "local" log driver described in #33475.

anusha-ragunathan · 2018-05-17T17:10:59Z

daemon/logger/jsonfilelog/read.go

Why is retries not incremented? Both here and L73?

These were erroneous. The for loop already increments retries.

anusha-ragunathan · 2018-05-17T17:18:51Z

daemon/logger/loggerutils/logfile_test.go

We should add s4 with Why does it say paper jam when there is no paper jam?.

anusha-ragunathan · 2018-05-17T17:30:34Z

Overall LGTM.

Needs a followup doc PR

thaJeztah · 2018-05-26T10:21:40Z

daemon/logger/jsonfilelog/jsonfilelog_test.go

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?

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.

thaJeztah · 2018-05-26T10:36:37Z

daemon/logger/local/doc.go

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?

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.

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

cpuguy83 · 2018-07-06T17:58:44Z

Moved this to code review per discussion with @thaJeztah on #maintainers in Slack.

vdemeester

SGTM 🐫

vdemeester · 2018-07-09T08:11:58Z

container/container.go

why don't we set container.LogPath here too ? 🤔

We don't want to expose this to users.

Can you add a comment about that? Just in case someone decides to add it, and doesn't know about this history.

thaJeztah

Finally able to give this another look; found some issues in tests, and some possible issues around max-file validation/checks

thaJeztah · 2018-07-11T11:18:00Z

pkg/tailfile/tailfile_test.go

This needs to be updated to gotest.tools

thaJeztah · 2018-07-11T11:20:17Z

daemon/logger/loggerutils/logfile.go

We can make a tracking-issue for this after this is merged

thaJeztah · 2018-07-11T11:20:46Z

daemon/logger/loggerutils/logfile_test.go

This should now use gotest.tools

thaJeztah · 2018-07-11T11:22:25Z

container/container.go

Can you add a comment about that? Just in case someone decides to add it, and doesn't know about this history.

thaJeztah · 2018-07-11T11:24:21Z

daemon/logger/local/config.go

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

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"}}

We could... maybe adding a DefaultConfig() function to the logger interface?

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.

thaJeztah · 2018-07-11T12:55:06Z

daemon/logger/local/config.go

Per the above; MaxFileCount == 1 looks to do exactly the same as MaxFileCount == 0, so this should probably be;

if cfg.MaxFileCount <= 1 {

kolyshkin · 2018-07-26T08:32:15Z

SGTM

Signed-off-by: Brian Goff <[email protected]>

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 <[email protected]>

codecov · 2018-08-11T04:02:47Z

Codecov Report

Merging #37092 into master will decrease coverage by 0.17%.
The diff coverage is 64.03%.

@@            Coverage Diff             @@
##           master   #37092      +/-   ##
==========================================
- Coverage   35.85%   35.67%   -0.18%     
==========================================
  Files         606      618      +12     
  Lines       44704    46150    +1446     
==========================================
+ Hits        16027    16465     +438     
- Misses      26469    27501    +1032     
+ Partials     2208     2184      -24

andrewhsu · 2018-08-19T20:56:09Z

Previous z build failure seems infrastructure network related:

00:00:51.155 docker: Error response from daemon: Get https://registry-1.docker.io/v2/library/docker-s390x/manifests/0e10011: net/http: TLS handshake timeout.

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.

thaJeztah · 2018-08-20T06:01:33Z

Agreed; z failure is definitely not related; everything else is green, so let's get this merged

yosifkit · 2018-08-21T20:07:47Z

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 local log driver and does not yet add the "dual-logging" described there?

Maybe add a checklist over there to show when things are merged and which docker release is targeted for changing the default from jsonfile?

cpuguy83 · 2018-08-21T20:30:45Z

@yosifkit That is correct, this is only the logger.

mterron · 2018-08-30T21:18:28Z

How do we enable this driver?

cpuguy83 · 2018-08-31T00:30:44Z

@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 <[email protected]>

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 <[email protected]> (cherry picked from commit c59038b) Signed-off-by: Sebastiaan van Stijn <[email protected]>

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 <[email protected]> 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 <[email protected]> (cherry picked from commit c59038b15c4fd2a796665925dec1cbb5993d4501) Signed-off-by: Sebastiaan van Stijn <[email protected]> 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 <[email protected]> Upstream-commit: c59038b15c4fd2a796665925dec1cbb5993d4501 Component: cli Upstream-commit: 0e20f85 Component: cli

thaJeztah · 2019-02-20T14:36:16Z

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)

GordonTheTurtle added the status/0-triage label May 17, 2018

cpuguy83 requested a review from anusha-ragunathan May 17, 2018 14:58

cpuguy83 added the rebuild/experimental label May 17, 2018

GordonTheTurtle removed the rebuild/experimental label May 17, 2018

cpuguy83 added status/1-design-review rebuild/experimental and removed status/0-triage labels May 17, 2018

GordonTheTurtle removed the rebuild/experimental label May 17, 2018

anusha-ragunathan reviewed May 17, 2018

View reviewed changes

cpuguy83 mentioned this pull request May 17, 2018

Add docs for local logging driver. docker/docs#6717

Merged

thaJeztah requested changes May 26, 2018

View reviewed changes

GordonTheTurtle assigned dnephin Jun 1, 2018

cpuguy83 added status/2-code-review and removed status/1-design-review labels Jul 6, 2018

vdemeester approved these changes Jul 9, 2018

View reviewed changes

thaJeztah requested changes Jul 11, 2018

View reviewed changes

thaJeztah added area/logging kind/feature Functionality or other elements that the project doesn't currently have. Features are new and shiny impact/changelog impact/documentation labels Jul 11, 2018

thaJeztah mentioned this pull request Jul 16, 2018

Docker logs -f is inefficient (high CPU decoding json) #37471

Open

cpuguy83 added 3 commits August 10, 2018 20:44

Add partial log metadata to log driver proto

e78ec2a

Signed-off-by: Brian Goff <[email protected]>

Add func to get an io.Reader for tail operations

874867d

Signed-off-by: Brian Goff <[email protected]>

Decouple logfile from tailfile.

94a1015

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 <[email protected]>

cpuguy83 force-pushed the local_logger branch from 877ca64 to 1d1c205 Compare August 11, 2018 04:02

GordonTheTurtle removed the rebuild/z label Aug 17, 2018

cpuguy83 added the rebuild/z label Aug 17, 2018

GordonTheTurtle removed the rebuild/z label Aug 17, 2018

AkihiroSuda added the rebuild/z label Aug 18, 2018

GordonTheTurtle removed the rebuild/z label Aug 18, 2018

yongtang added the rebuild/z label Aug 19, 2018

GordonTheTurtle removed the rebuild/z label Aug 19, 2018

thaJeztah merged commit e0ad6d0 into moby:master Aug 20, 2018

cpuguy83 deleted the local_logger branch August 20, 2018 14:24

cpuguy83 mentioned this pull request Aug 21, 2018

slow response from docker daemon for log request with --tail parameter for jsonfile log driver #31968

Closed

cpuguy83 mentioned this pull request Aug 21, 2018

[Proposal] Enhance local logging / remote logging #33475

Closed

2 tasks

crazy-max mentioned this pull request Nov 12, 2018

Traefik crazy-max/docker-fail2ban#8

Closed

albers mentioned this pull request Dec 2, 2018

Add bash completion for "local" log driver docker/cli#1547

Merged

thaJeztah mentioned this pull request Dec 7, 2018

[18.09 backport] Add bash completion for "local" log driver docker/cli#1553

Merged

thaJeztah mentioned this pull request Jan 28, 2020

Question on logdriver entry.proto #40420

Closed

thaJeztah mentioned this pull request Nov 3, 2020

Use the "local" logging driver as a default for Docker Desktop docker/roadmap#163

Open

kzys mentioned this pull request Dec 9, 2021

daemon/logger: read the length header correctly #43043

Merged

thaJeztah mentioned this pull request Dec 8, 2023

Inconsistent behavior for logs timestamps timezone depending on logging driver used #46910

Closed

Conversation

cpuguy83 commented May 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anusha-ragunathan commented May 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Jul 6, 2018

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Jul 26, 2018

Uh oh!

codecov bot commented Aug 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

andrewhsu commented Aug 19, 2018

Uh oh!

thaJeztah commented Aug 20, 2018

Uh oh!

yosifkit commented Aug 21, 2018

Uh oh!

cpuguy83 commented Aug 21, 2018

Uh oh!

mterron commented Aug 30, 2018

Uh oh!

cpuguy83 commented Aug 31, 2018

Uh oh!

thaJeztah commented Feb 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecov bot commented Aug 11, 2018 •

edited

Loading