feat: Add json-file log driver - #392
Merged
Merged
Conversation
Wraps moby's jsonfilelog (github.com/docker/docker/daemon/logger/jsonfilelog)
behind the shim-logger driver pattern used by awslogs / fluentd / splunk:
--log-driver json-file --log-path <abs-path> [--max-size <n>]
[--max-file <n>]
[--compress true|false]
[--json-file-tag <tmpl>]
[--json-file-labels <csv>]
[--json-file-labels-regex <re>]
[--json-file-env <csv>]
[--json-file-env-regex <re>]
Notes:
* --log-path is required and is forwarded to moby via the new
logger.WithLogPath InfoOpt; the directory is the caller's responsibility
(containerd / agent), the shim does not mkdir.
* --json-file-{tag,labels,labels-regex,env,env-regex} are renamed at the
pflag boundary to avoid colliding with splunk/fluentd flags in the flat
pflag namespace; they are passed to moby under their bare names
(tag, labels, labels-regex, env, env-regex).
* Optional fields are only added to the moby config map when non-empty so
moby's ValidateLogOpts does not reject stray empty values.
* compress defaults to off (not set in config) — moby rejects
compress=true unless max-file>=2 and max-size is set, so making it
opt-in keeps a partial config from becoming a hard failure.
* Buffered (non-blocking) mode is honored via the same NonBlockingMode
wrapping pattern used by the existing drivers.
Unit tests:
* logger/jsonfile/logger_test.go — config translation, optional-field
omission, tag suppression when not specified, validate-keys-only
contract of moby's ValidateLogOpts.
* args_test.go — log-path required, log-path-only defaults, full
option pass-through via viper.
Six Ginkgo specs run against a local containerd, mirroring the
fluentd e2e style:
* envelope format — every line is {"log":...,"stream":...,"time":...}
* --max-size rotation — forces 2-3 rotated files, asserts cap
* --max-file cap — over-rotation is bounded by the cap
* no-rotation lower bound — single small printf produces 1 file
* file mode 0640 — moby's hardcoded mode is preserved end-to-end
* --log-path required — missing flag fails the container task fast
Two infra additions:
* e2e/common.go grows SendCommandByContainerd, a sibling of
SendTestLogByContainerd that runs an arbitrary shell command in the
test container. Needed when a spec has to control payload size or
shape (e.g., to force log rotation).
* --log-path is resolved to an absolute path at suite-build time via
filepath.Abs. The shim-logger inherits its CWD from containerd, not
from the test runner, so a relative log-path would fail to resolve
in the shim's context.
Wires the new spec into e2e/main_test.go and adds a test-e2e-for-json-file
target to the Makefile mirroring test-e2e-for-fluentd / -splunk.
aaithal
force-pushed
the
add-json-file-driver
branch
from
May 27, 2026 22:35
1765b78 to
9f4204b
Compare
TestMemoryScenario_LargeLines_NonBlocking_DefaultBuffer's threshold of 210 MiB is too close to the actual peak under -race instrumentation. Observed values: * Linux + Go 1.24 + race → 188.8 MiB * Windows + Go 1.24 + race → 224.1 MiB (CI failure on PR aws#392) The pre-existing comment said "with -race instrumentation, add ~30% headroom," pinning the threshold at 1.30 × the 160 MiB baseline = 208, rounded to 210. In practice the OS allocator and Go version each add their own jitter on top of -race, which puts Windows + Go 1.24 ~6.7% over the original budget. Bump the threshold to 256 MiB (1.6 × baseline). The test still serves as a regression detector — a real leak would double the working set, which would still trip the check.
JoseVillalta
previously approved these changes
May 28, 2026
JoseVillalta
left a comment
Contributor
There was a problem hiding this comment.
non-blocking minor comments.
xxx0624
previously approved these changes
May 28, 2026
* args.go: trim the inline GetString-everywhere rationale on getJSONFileArgs (~30 lines) to a 3-line summary; move the full rationale to a package-level doc comment, since it applies equally to splunk/fluentd's existing GetString patterns and isn't json-file-specific. (per JoseVillalta@) * e2e/jsonfile_test.go: drop the leading './' on jsonFileLogDir; '../jsonfile-logs' reads cleaner and the './' was a no-op. (per JoseVillalta@) * e2e/jsonfile_test.go: drop the redundant '<base>.gz' clause in listLogFiles — '<base>.<anything>' already matches '<base>.2.gz' via HasPrefix(name, baseName+'.'). (per JoseVillalta@) * e2e/jsonfile_test.go: add a non-blocking-mode buffer-pressure spec (aws#7). Configures --mode=non-blocking --max-buffer-size=64k, produces ~200 KiB of output, asserts the binary exits cleanly and every line that lands in the file is a valid Docker envelope. The spec exercises the NonBlockingMode wrapping in jsonfile.RunLogDriver introduced by this PR. Doesn't assert specific drop counts (timing- dependent). (per xxx0624@)
xxx0624
approved these changes
May 28, 2026
aaithal
added a commit
that referenced
this pull request
May 28, 2026
TestMemoryScenario_LargeLines_NonBlocking_DefaultBuffer's threshold of 210 MiB is too close to the actual peak under -race instrumentation. Observed values: * Linux + Go 1.24 + race → 188.8 MiB * Windows + Go 1.24 + race → 224.1 MiB (CI failure on PR #392) The pre-existing comment said "with -race instrumentation, add ~30% headroom," pinning the threshold at 1.30 × the 160 MiB baseline = 208, rounded to 210. In practice the OS allocator and Go version each add their own jitter on top of -race, which puts Windows + Go 1.24 ~6.7% over the original budget. Bump the threshold to 256 MiB (1.6 × baseline). The test still serves as a regression detector — a real leak would double the working set, which would still trip the check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat: Add
json-filelog driverOverview
This PR adds a
json-filelog driver to the shim-logger, wrapping moby'supstream
jsonfilelogbehind the same driver pattern that
awslogs,fluentd, andsplunkuse today.The driver writes container
stdout/stderrto a host file in theDocker JSON-file format — one JSON object per line, e.g.:
{"log":"hello\n","stream":"stdout","time":"2026-05-27T21:07:48.123456789Z"}It supports moby's
max-size,max-file,compress,tag,labels,labels-regex,env, andenv-regexoptions unchanged, plus a required--log-pathfor the per-container output file.Usage
Changes Made
New driver package
logger/jsonfile— wraps moby'sjsonfilelog.New,honoring the existing
NonBlockingModewrapping pattern when buffered modeis requested. Optional fields are only added to the moby config map when
non-empty so moby's
ValidateLogOptsdoes not reject stray empty values.New common option
logger.WithLogPath— setsinfo.LogPathon thelogger.Infostruct moby'sjsonfilelog.Newreads to derive the outputfilename. The shim does not create the directory; the caller
(containerd / agent) is responsible for precreating it with the right
ownership and mode.
Wiring through
init.go/args.go/main.go— registers all 9flags, adds
getJSONFileArgs()(returns an error if--log-pathis unset),and dispatches
case jsonfile.DriverNametorunJSONFileDriver. Addsjson-fileto the--log-driverhelp text.Prefixed input flag names for the 5 options that collide with other
drivers:
--json-file-tag,--json-file-labels,--json-file-labels-regex,--json-file-env,--json-file-env-regex. Same convention splunk andfluentd already use — shim-logger's pflag namespace is flat across drivers.
The flags are forwarded to moby under their bare names (
tag,labels,labels-regex,env,env-regex).compressdefaults to "do not set" — moby rejectscompress=trueunless
max-file >= 2andmax-sizeis set, so making it opt-in keeps apartial config from becoming a hard failure.
Drive-by: relax heap threshold for
TestMemoryScenario_LargeLines_NonBlocking_DefaultBufferCommit 3 of this PR (
26c2185) is unrelated to the json-file work. Thetest added in #391 uses a 210 MiB threshold derived from a 160 MiB baseline
plus a 30% race-instrumentation buffer; in practice the OS allocator and Go
version each add their own jitter on top of
-race, so Windows + Go 1.24Go 1.24 + race observes 188.8 MiB). Bumped to 256 MiB (1.6× the 160 MiB
baseline) — a real regression doubling the working set would still trip.
Happy to drop this commit and split into a separate PR if maintainers
prefer; calling it out so it's visible.
Testing
Unit tests —
make test-unitpasses across all packages:logger/jsonfile/logger_test.gocovers config translation, empty-args →empty config map,
TagSpecified=falsesuppression, and thevalidate-keys-not-values contract of moby's
ValidateLogOpts.args_test.goaddsTestGetJSONFileArgscovering the--log-path-requirederror path, log-path-only defaults, and the full options pass-through.
E2E tests —
make test-e2e-for-json-fileruns 6 specs against alocal containerd, all pass:
{"log":...,"stream":"stdout","time":"<RFC3339Nano>"}--max-sizerotation--max-filecap--max-sizeprintfproduces 1 file0640(moby's hardcoded value)--log-pathrequiredTwo e2e infra additions:
e2e/common.gogrows aSendCommandByContainerdhelper (the existing
SendTestLogByContainerddelegates to it) so specscan run arbitrary shell commands when they need to control payload shape;
--log-pathis resolved to an absolute path at suite-build time becausethe shim-logger inherits its CWD from containerd, not from the test
runner. Comment in the test file explains the latter.
By submitting this pull request, I confirm that you can use, modify, copy,
and redistribute this contribution, under the terms of your choice.