Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added support to specify default docker log driver in plugin options. #10156

Merged
merged 3 commits into from
Mar 12, 2021
Merged

Added support to specify default docker log driver in plugin options. #10156

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
302071d
Added support to specify default docker driver in plugin options. Fix…
apollo13 Mar 10, 2021
9c6dde7
Apply suggestions from code review
apollo13 Mar 10, 2021
8cf1785
changelog entry
tgross Mar 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions drivers/docker/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,18 @@ var (
// extra docker labels, globs supported
"extra_labels": hclspec.NewAttr("extra_labels", "list(string)", false),

// logging options
"logging": hclspec.NewDefault(hclspec.NewBlock("logging", false, hclspec.NewObject(map[string]*hclspec.Spec{
"type": hclspec.NewAttr("type", "string", false),
"config": hclspec.NewBlockAttrs("config", "string", false),
})), hclspec.NewLiteral(`{
type = "json-file"
config = {
max-file = "2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially after reading the docs updates, I'm realizing there's a bit of a conceptual gap here we need to solve. The log driver configuration with max-file and max-size is not the same as the Nomad logs block; one is Docker's own logs and the other is the Nomad allocation logs.

I feel like the original issue #6563 that @powellchristoph opened was ambiguous as to whether it was understood that these are different values. If so, we should probably clarify that further in the docs. I'm also wondering whether there's value in being able to set the Nomad defaults in the client config (either as well or instead of).

Copy link
Contributor Author

@apollo13 apollo13 Mar 10, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha, I never knew that nomad has a logs block. For me the main goal is to send the task logs somewhere else. For docker the easiest way to do this is via a logdriver. I think in an ideal world nomad would be able to send those logs away (with metadata attached) -- for all task drivers without relying on some hacks like docker logging drivers.

As for the logs block, no idea -- I'll leave it up to you to decide what to do with it. Personally I think it would belong in another PR to keep to scope small.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in an ideal world nomad would be able to send those logs away (with metadata attached) -- for all task drivers without relying on some hacks like docker logging drivers.

Agreed. We have an issue open for some kind of log driver plugin concept (#9211) but that hasn't quite made its way onto the near-term roadmap yet.

As for the logs block, no idea -- I'll leave it up to you to decide what to do with it. Personally I think it would belong in another PR to keep to scope small.

I'm inclined to agree, but lemme run that by a couple of other folks on the team just to get a little consensus.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, had a quick discussion about that internally and I think we're just going to keep the code here as is. I'm going to push a commit onto this PR just to add a few choice warnings about the difference between the two configuration values and then we'll get this merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm happy with what you've got here. Let's merge this.

max-size = "2m"
}
}`)),

// garbage collection options
// default needed for both if the gc {...} block is not set and
// if the default fields are missing
Expand Down Expand Up @@ -616,6 +628,7 @@ type DriverConfig struct {
PullActivityTimeout string `codec:"pull_activity_timeout"`
pullActivityTimeoutDuration time.Duration `codec:"-"`
ExtraLabels []string `codec:"extra_labels"`
Logging LoggingConfig `codec:"logging"`

AllowRuntimesList []string `codec:"allow_runtimes"`
allowRuntimes map[string]struct{} `codec:"-"`
Expand Down Expand Up @@ -646,6 +659,11 @@ type VolumeConfig struct {
SelinuxLabel string `codec:"selinuxlabel"`
}

type LoggingConfig struct {
Type string `codec:"type"`
Config map[string]string `codec:"config"`
}

func (d *Driver) PluginInfo() (*base.PluginInfoResponse, error) {
return pluginInfo, nil
}
Expand Down
9 changes: 3 additions & 6 deletions drivers/docker/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,12 +876,9 @@ func (d *Driver) createContainerConfig(task *drivers.TaskConfig, driverConfig *T
}

if hostConfig.LogConfig.Type == "" && hostConfig.LogConfig.Config == nil {
logger.Trace("no docker log driver provided, defaulting to json-file")
hostConfig.LogConfig.Type = "json-file"
hostConfig.LogConfig.Config = map[string]string{
"max-file": "2",
"max-size": "2m",
}
logger.Trace("no docker log driver provided, defaulting to plugin config")
hostConfig.LogConfig.Type = d.config.Logging.Type
hostConfig.LogConfig.Config = d.config.Logging.Config
}

logger.Debug("configured resources",
Expand Down
31 changes: 31 additions & 0 deletions drivers/docker/driver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,37 @@ func TestDockerDriver_ExtraLabels(t *testing.T) {
}
}

func TestDockerDriver_LoggingConfiguration(t *testing.T) {
if !tu.IsCI() {
t.Parallel()
}
testutil.DockerCompatible(t)

task, cfg, ports := dockerTask(t)
defer freeport.Return(ports)

require.NoError(t, task.EncodeConcreteDriverConfig(cfg))

dockerClientConfig := make(map[string]interface{})
loggerConfig := map[string]string{"gelf-address": "udp://1.2.3.4:12201", "tag": "gelf"}

dockerClientConfig["logging"] = LoggingConfig{
Type: "gelf",
Config: loggerConfig,
}
client, d, handle, cleanup := dockerSetup(t, task, dockerClientConfig)
defer cleanup()
require.NoError(t, d.WaitUntilStarted(task.ID, 5*time.Second))

container, err := client.InspectContainer(handle.containerID)
if err != nil {
t.Fatalf("err: %v", err)
}
apollo13 marked this conversation as resolved.
Show resolved Hide resolved

require.Equal(t, "gelf", container.HostConfig.LogConfig.Type)
require.Equal(t, loggerConfig, container.HostConfig.LogConfig.Config)
}

func TestDockerDriver_ForcePull(t *testing.T) {
if !tu.IsCI() {
t.Parallel()
Expand Down
10 changes: 9 additions & 1 deletion website/content/docs/drivers/docker.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,10 +868,18 @@ plugin "docker" {
capabilities and exclusively use host based log aggregation, you may consider
this option to disable nomad log collection overhead.

- `extra_labels` - Extra labels to add to Docker containers.
- `extra_labels` - Extra labels to add to Docker containers.
Available options are `job_name`, `job_id`, `task_group_name`, `task_name`,
`namespace`, `node_name`, `node_id`. Globs are supported (e.g. `task*`)

- `logging` stanza:

- `type` - Defaults to `"json-file"`. Specifies the logging driver docker should
use for all containers Nomad starts.

- `config` - Defaults to `{ max-file = "2", max-size = "2m" }`. This option can be
used to pass further configuration to the logging driver.
apollo13 marked this conversation as resolved.
Show resolved Hide resolved

- `gc` stanza:

- `image` - Defaults to `true`. Changing this to `false` will prevent Nomad
Expand Down