Update dockerd to support JSON logging format by pkwarren · Pull Request #45737 · moby/moby

pkwarren · 2023-06-13T22:12:11Z

- What I did
Update docker to support a --log-format option, which accepts either text (default) or json. Propagate the log format to containerd as well, to ensure that everything will be logged consistently.

Fixes #44940.

- How I did it
Added a new CLI flag --log-format which accepts text (default) or json. Propagate log format to containerd so that all daemon output uses consistent formatting.

- How to verify it
Run dockerd with --log-format=json and --log-format=text (or leaving the option unset) to verify both encoding formats. This should set consistent log formats for both dockerd and containerd.

- Description for the changelog
Add a --log-format option to dockerd to allow logging in text (default) or JSON format.

- A picture of a cute animal (not mandatory but encouraged)

corhere

Looks good! My only request is to use constants for "json" and "text" instead of spreading more string literals everywhere. I'm even tempted to reuse the log.TextFormat and log.JSONFormat constants from containerd to really emphasize that the log format options have to align with the options containerd supports so that supervised containerd can be successfully configured.

cmd/dockerd/options.go

thaJeztah · 2023-06-13T22:42:44Z

Bikeshedding: perhaps we should use plain (instead of text) as name for the default format. I know we use (e.g.) --progress=plain on docker build, so mostly to try to be a bit consistent in the terminology 😅

thaJeztah · 2023-06-13T22:48:29Z

cmd/dockerd/daemon.go

+	if logFormat := cli.Config.LogFormat; logFormat != "" && logFormat != "text" {
+		opts = append(opts, supervisor.WithLogFormat(logFormat))
+	}
+


I'm a bit on the fence whether we should punch this through to containerd (related: #43946 and #43950)

Without punching it through, I got inconsistent formatted log lines (some JSON, some text) when testing (since the dockerd logs also include the containerd logs).

corhere · 2023-06-13T22:50:24Z

Bikeshedding: perhaps we should use plain (instead of text) as name for the default format.

If we're punching through to containerd we would either have to use the same names as containerd, or map plain -> text

corhere

LGTM! Please squash down to a single commit and force-push so it's ready to merge.

cmd/dockerd/options.go

corhere

LGTM, thanks!

cmd/dockerd/daemon.go

thaJeztah · 2023-06-15T10:47:57Z

#45737 (comment)

Without punching it through, I got inconsistent formatted log lines (some JSON, some text) when testing (since the dockerd logs also include the containerd logs).

Yup, I understand the downside/inconvenience. My concern here is that that's what one would get in the "normal" situation where dockerd and containerd run as separate systemd units (services). With the containerd integration work, there may be more configuration needed on containerd in future, and for those configuration-changes, we should probably handle those in the way containerd expects them (config file). containerd running as a child process is mostly for debugging purposes, or for exceptional cases where no systemd is available (e.g. the docker-in-docker image). For those, I'd like to align them more (and only deal with "running the process" and the bare-minimum of other options to be managed by dockerd).

pkwarren · 2023-06-15T19:24:24Z

@thaJeztah - thank you for the context. As a new contributor this is helpful.

With the containerd integration work, there may be more configuration needed on containerd in future, and for those configuration-changes, we should probably handle those in the way containerd expects them (config file).

One thing that may not be clear from the changes - https://github.com/moby/moby/pull/45737/files#diff-4c6f83cd169941ac3f98bf9b46a442581492bad0318d756599e65d8a60bfed27R18-R23 is setting a variable which will be written out to the containerd config file (this doesn't result in passing a different command line option like --log-level).

corhere · 2023-06-19T17:59:15Z

My concern here is that that's what one would get in the "normal" situation where dockerd and containerd run as separate systemd units (services).

When running a separate systemd units, dockerd logs will be segregated from the containerd logs in the journal so it would not be a big deal if the format (or verbosity) of logging was inconsistent between them. The only time it would be an issue for the log formats to diverge is when both daemons log to the same stream: when dockerd runs a managed containerd subprocess. Even if we were to remove propagation of the log level to the managed containerd config, I strongly believe that aligning the log format for all processes sharing the log stream should be included in the "bare minimum" set of of options managed by dockerd.

pkwarren · 2023-06-22T23:51:20Z

This has two reviews and appears to be passing all CI checks. I'm happy to fix any follow up issues, but if it is ready to go what are the next steps to close this PR?

cmd/dockerd/daemon_test.go

neersighted · 2023-06-28T12:15:38Z

We discussed this during the maintainer's call again; @thaJeztah has reservations about adding more to the managed-containerd path, but I think in this case it is justified (especially with the merge of #45799). @thaJeztah, are you still -1 on this one?

Looks like this needs a rebase as well.

Update docker to support a '--log-format' option, which accepts either 'text' (default) or 'json'. Propagate the log format to containerd as well, to ensure that everything will be logged consistently. Signed-off-by: Philip K. Warren <[email protected]>

pkwarren · 2023-06-28T17:50:13Z

Looks like this needs a rebase as well.

Rebased and removed the containerdlog import alias.

thaJeztah

LGTM

pkwarren mentioned this pull request Jun 13, 2023

Configure JSON-formatted logging for dockerd daemon logs #44940

Closed

corhere added area/cli Client status/2-code-review kind/feature Functionality or other elements that the project doesn't currently have. Features are new and shiny area/daemon Core Engine labels Jun 13, 2023

corhere added this to the 25.0.0 milestone Jun 13, 2023

corhere reviewed Jun 13, 2023

View reviewed changes

corhere requested review from cpuguy83, neersighted and thaJeztah June 13, 2023 22:31

thaJeztah reviewed Jun 13, 2023

View reviewed changes

cmd/dockerd/options.go Outdated Show resolved Hide resolved

thaJeztah reviewed Jun 13, 2023

View reviewed changes

pkwarren requested review from corhere and thaJeztah June 14, 2023 14:04

corhere reviewed Jun 14, 2023

View reviewed changes

cmd/dockerd/options.go Outdated Show resolved Hide resolved

pkwarren requested a review from corhere June 14, 2023 21:57

corhere approved these changes Jun 14, 2023

View reviewed changes

neersighted requested changes Jun 14, 2023

View reviewed changes

cmd/dockerd/daemon.go Outdated Show resolved Hide resolved

cmd/dockerd/daemon.go Show resolved Hide resolved

neersighted reviewed Jun 15, 2023

View reviewed changes

cmd/dockerd/daemon.go Outdated Show resolved Hide resolved

pkwarren requested a review from neersighted June 15, 2023 19:44

neersighted approved these changes Jun 20, 2023

View reviewed changes

corhere added the status/needs-attention Calls for a collective discussion during a review session label Jun 23, 2023

neersighted reviewed Jun 26, 2023

View reviewed changes

cmd/dockerd/daemon_test.go Outdated Show resolved Hide resolved

thaJeztah approved these changes Jul 13, 2023

View reviewed changes

corhere merged commit 0c2699d into moby:master Jul 13, 2023

corhere added status/4-merge impact/changelog and removed status/2-code-review status/needs-attention Calls for a collective discussion during a review session labels Jul 13, 2023

neersighted mentioned this pull request Jul 13, 2023

25.0 Release Tracking #45926

Closed

26 tasks

pkwarren deleted the pkw/issue-44940-dockerd-json-logs branch July 15, 2023 16:22

tianon mentioned this pull request Dec 13, 2023

docker:dind Standard Error/Output Format docker-library/docker#449

Closed

This was referenced Sep 16, 2024

man: dockerd: add description for --log-format option #48505

Merged

docs, man: dockerd: add documentation for "--log-format" option docker/cli#5438

Merged

Conversation

pkwarren commented Jun 13, 2023

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah commented Jun 13, 2023

Uh oh!

thaJeztah Jun 13, 2023

Choose a reason for hiding this comment

Uh oh!

pkwarren Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

corhere commented Jun 13, 2023

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Jun 15, 2023

Uh oh!

pkwarren commented Jun 15, 2023

Uh oh!

corhere commented Jun 19, 2023

Uh oh!

pkwarren commented Jun 22, 2023

Uh oh!

Uh oh!

neersighted commented Jun 28, 2023

Uh oh!

pkwarren commented Jun 28, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants