add log rotation to docker driver log defaults #5846

jazzyfresh · 2019-06-17T18:23:44Z

Overview

Nomad uses Docker log defaults if no user log configuration provided
Docker log driver defaults to json-file with no log rotation, which will make logs grow infinitely
This adds log rotation to the default Docker logging

Behavior

rotate docker logs with max-file=2 and max-size=2m by default
"by default" = only add log rotation if no user config provided (neither log Type nor log Config is set)

Implementation

https://docs.docker.com/config/containers/logging/configure/

endocrimes

This is a really good start, a couple of quick comments on logging then LGTM.

We also need to update https://www.nomadproject.io/docs/drivers/docker.html#logging to say that if you specify json-file then the default max-file/max-size are x/y.

endocrimes · 2019-06-17T18:26:12Z

drivers/docker/driver.go

@@ -740,6 +740,20 @@ func (d *Driver) createContainerConfig(task *drivers.TaskConfig, driverConfig *T
 		Config: driverConfig.Logging.Config,
 	}

+	if hostConfig.LogConfig.Type == "json-file" || hostConfig.LogConfig.Type == "" {
+		logger.Debug("configuring docker json-file logs")


This should probably be logger.Trace, because it's likely to be primarily useful during development

endocrimes · 2019-06-17T18:27:26Z

drivers/docker/driver.go

@@ -740,6 +740,20 @@ func (d *Driver) createContainerConfig(task *drivers.TaskConfig, driverConfig *T
 		Config: driverConfig.Logging.Config,
 	}

+	if hostConfig.LogConfig.Type == "json-file" || hostConfig.LogConfig.Type == "" {


hmm - I think we only need this if we're using json-file (the default syslog doesn't need this afaik)?

i thought json-file is the default for docker https://docs.docker.com/config/containers/logging/json-file/

oh or if type is unset/unconfigured by the user, do we set it to syslog somewhere?

Indeed, starting with Nomad 0.9.0, we use the docker daemon setting by default, which defaults to json-file. However, users can configure that using daemon.json.

I'd suggest that we check daemon setting as well as the task logging setting. You can inspect the daemon logging default from dockerInfo in fingerprint function[1] and store it in a field to inspect here.

Additionally, I'd suggest setting the values (e.g. max-file) only if not set by user. This allows users to override our defaults if they so choose.

[1]

nomad/drivers/docker/fingerprint.go

Lines 149 to 162 in 51d4fa0

runtimeNames := make([]string, 0, len(dockerInfo.Runtimes))

for name := range dockerInfo.Runtimes {

if d.config.GPURuntimeName == name {

// Nvidia runtime is detected by Docker.

// It makes possible to run GPU workloads using Docker driver on this host.

d.gpuRuntime = true

}

runtimeNames = append(runtimeNames, name)

}

sort.Strings(runtimeNames)

fp.Attributes["driver.docker.runtimes"] = pstructs.NewStringAttribute(

strings.Join(runtimeNames, ","))

fp.Attributes["driver.docker.os_type"] = pstructs.NewStringAttribute(dockerInfo.OSType)

Oh cool - we should update the docs then, because we currently say syslog is the default.

schmichael · 2019-06-19T18:52:27Z

drivers/docker/driver.go

+			hostConfig.LogConfig.Config = make(map[string]string)
+		}
+		hostConfig.LogConfig.Config["max-file"] = "3"
+		hostConfig.LogConfig.Config["max-size"] = "10m"


Minimize these resources by default as they're in addition to the logs Nomad and only used as a buffer during agent restarts. (If the operator wants to use them directly they can manually configure them.)

Maybe:

hostConfig.LogConfig.Config["max-file"] = "2" hostConfig.LogConfig.Config["max-size"] = "2m"

notnoop · 2019-06-21T22:57:27Z

drivers/docker/driver.go

+	}
+	logger.Debug("configured logs", "type", hostConfig.LogConfig.Type,
+		"max-file", hostConfig.LogConfig.Config["max-file"],
+		"max-size", hostConfig.LogConfig.Config["max-size"])


I'm not sure if there much of a value in emitting the logs here, so I'd probably skip it

notnoop · 2019-06-21T22:58:38Z

drivers/docker/driver.go

@@ -740,6 +740,20 @@ func (d *Driver) createContainerConfig(task *drivers.TaskConfig, driverConfig *T
 		Config: driverConfig.Logging.Config,
 	}

+	if hostConfig.LogConfig.Type == "" {


Should we also check if hostConfig.LogConfig.Config is nil and only override if user didn't specify any logging at all? Or ensure that users can override max-file|size as well.

drivers/docker/driver_test.go

notnoop

lgtm - thanks for following through this!

notnoop · 2019-07-02T03:25:13Z

drivers/docker/driver.go

+		hostConfig.LogConfig.Type = "json-file"
+		hostConfig.LogConfig.Config = make(map[string]string)
+		hostConfig.LogConfig.Config["max-file"] = "2"
+		hostConfig.LogConfig.Config["max-size"] = "2m"


You can also do the following that I find a bit more readable

hostConfig.LogConfig.Config = map[string]string{ "max-file": "2", "max-size": "2m", }

oof, good catch

github-actions · 2023-02-07T02:15:29Z

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

jazzyfresh requested review from notnoop, endocrimes and preetapan June 17, 2019 18:24

jazzyfresh assigned endocrimes Jun 17, 2019

endocrimes suggested changes Jun 17, 2019

View reviewed changes

schmichael reviewed Jun 19, 2019

View reviewed changes

jazzyfresh marked this pull request as ready for review June 20, 2019 19:52

notnoop reviewed Jun 21, 2019

View reviewed changes

jazzyfresh force-pushed the f-docker-log-constraints branch 2 times, most recently from 37f3e63 to b3a2e2c Compare July 1, 2019 17:40

jazzyfresh requested a review from endocrimes July 1, 2019 17:45

jazzyfresh changed the title ~~add constraints to docker driver for json-file logs~~ add log rotation to docker driver log defaults Jul 1, 2019

notnoop approved these changes Jul 2, 2019

View reviewed changes

endocrimes approved these changes Jul 2, 2019

View reviewed changes

default to json-file log rotation for docker driver

8e7a2d0

jazzyfresh force-pushed the f-docker-log-constraints branch from cc05a28 to 8e7a2d0 Compare July 3, 2019 16:08

jazzyfresh merged commit 3fdb3cb into master Jul 3, 2019

endocrimes deleted the f-docker-log-constraints branch July 3, 2019 17:52

camerondavison mentioned this pull request Jul 23, 2019

Docker logs growing infinitely #5782

Closed

lens0021 mentioned this pull request Mar 24, 2021

Docker Swarm을 Nomad로 대체 femiwiki/femiwiki#116

Closed

15 tasks

github-actions bot locked as resolved and limited conversation to collaborators Feb 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add log rotation to docker driver log defaults #5846

add log rotation to docker driver log defaults #5846

jazzyfresh commented Jun 17, 2019 •

edited

Loading

endocrimes left a comment

endocrimes Jun 17, 2019

endocrimes Jun 17, 2019 •

edited

Loading

jazzyfresh Jun 17, 2019

jazzyfresh Jun 17, 2019

notnoop Jun 17, 2019

endocrimes Jun 17, 2019

schmichael Jun 19, 2019

jazzyfresh Jun 20, 2019

notnoop Jun 21, 2019

notnoop Jun 21, 2019

notnoop left a comment

notnoop Jul 2, 2019 •

edited

Loading

jazzyfresh Jul 2, 2019

github-actions bot commented Feb 7, 2023

	runtimeNames := make([]string, 0, len(dockerInfo.Runtimes))
	for name := range dockerInfo.Runtimes {
	if d.config.GPURuntimeName == name {
	// Nvidia runtime is detected by Docker.
	// It makes possible to run GPU workloads using Docker driver on this host.
	d.gpuRuntime = true
	}
	runtimeNames = append(runtimeNames, name)
	}
	sort.Strings(runtimeNames)

	fp.Attributes["driver.docker.runtimes"] = pstructs.NewStringAttribute(
	strings.Join(runtimeNames, ","))
	fp.Attributes["driver.docker.os_type"] = pstructs.NewStringAttribute(dockerInfo.OSType)

add log rotation to docker driver log defaults #5846

add log rotation to docker driver log defaults #5846

Conversation

jazzyfresh commented Jun 17, 2019 • edited Loading

Overview

Behavior

Implementation

endocrimes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endocrimes Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notnoop left a comment

Choose a reason for hiding this comment

notnoop Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 7, 2023

jazzyfresh commented Jun 17, 2019 •

edited

Loading

endocrimes Jun 17, 2019 •

edited

Loading

notnoop Jul 2, 2019 •

edited

Loading