Skip to content

Pass log-level to containerd#37419

Merged
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:pass_loglevel_to_containerd
Jul 9, 2018
Merged

Pass log-level to containerd#37419
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:pass_loglevel_to_containerd

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 9, 2018 •
edited
Loading

fixes #37343

dockerd allows the --log-level to be specified, but this log-level was not forwarded to the containerd process.

This patch sets containerd's log-level to the same as dockerd if a custom level is provided.

Now that --log-level is also passed to containerd, the default "info" is removed, so that containerd's default (or the level configured in containerd.toml) is still used if no log-level is set.

Before this change:

containerd would always be started without a log-level set (only the level that's configured in containerd.toml);

root      1014  2.5  2.1 496484 43468 pts/0    Sl+  12:23   0:00 dockerd
root      1023  1.2  1.1 681768 23832 ?        Ssl  12:23   0:00  \_ docker-containerd --config /var/run/docker/containerd/containerd.toml

After this change:

when running dockerd without options (same as current);

root      1014  2.5  2.1 496484 43468 pts/0    Sl+  12:23   0:00 dockerd
root      1023  1.2  1.1 681768 23832 ?        Ssl  12:23   0:00  \_ docker-containerd --config /var/run/docker/containerd/containerd.toml

when running dockerd --debug:

root       600  0.8  2.1 512876 43180 pts/0    Sl+  12:20   0:00 dockerd --debug
root       608  0.6  1.1 624428 23672 ?        Ssl  12:20   0:00  \_ docker-containerd --config /var/run/docker/containerd/containerd.toml --log-level debug

when running dockerd --log-level=panic

root       747  0.6  2.1 496548 43996 pts/0    Sl+  12:21   0:00 dockerd --log-level=panic
root       755  0.7  1.1 550696 24100 ?        Ssl  12:21   0:00  \_ docker-containerd --config /var/run/docker/containerd/containerd.toml --log-level panic

combining --debug and --log-level (--debug takes precedence):

root       880  2.7  2.1 634692 43336 pts/0    Sl+  12:23   0:00 dockerd --debug --log-level=panic
root       888  1.0  1.1 616232 23652 ?        Ssl  12:23   0:00  \_ docker-containerd --config /var/run/docker/containerd/containerd.toml --log-level debug

Signed-off-by: Sebastiaan van Stijn [email protected]

- Description for the changelog

+ Configure containerd log-level to be the same as dockerd

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

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 9, 2018

ping @mlaventure @vdemeester PTAL

I'm wondering if we should remove the default "info" from the "defaults" now, and just use whatever containerd uses as default (and/or reads from the containerd.toml) instead?

moby/libcontainerd/remote_daemon_linux.go

Lines 31 to 33 in fd2f2a9

if r.Debug.Level == "" {
r.Debug.Level = "info"
}

@thaJeztah thaJeztah mentioned this pull request Jul 9, 2018
Closed
@thaJeztah thaJeztah requested a review from mlaventure July 9, 2018 13:06
vdemeester
vdemeester approved these changes Jul 9, 2018
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 🐯

and yes, I think we should remove the default "info" 😛

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 9, 2018

Let me update the PR and remove the default

@thaJeztah
Pass log-level to containerd
aaa1392 
dockerd allows the `--log-level` to be specified, but this log-level
was not forwarded to the containerd process.

This patch sets containerd's log-level to the same as dockerd if a
custom level is provided.

Now that `--log-level` is also passed to containerd, the default "info"
is removed, so that containerd's default (or the level configured in containerd.toml)
is still used if no log-level is set.

Before this change:

containerd would always be started without a log-level set (only the level that's configured in `containerd.toml`);

```
root      1014  2.5  2.1 496484 43468 pts/0    Sl+  12:23   0:00 dockerd
root      1023  1.2  1.1 681768 23832 ?        Ssl  12:23   0:00  \_ docker-containerd --config /var/run/docker/containerd/containerd.toml
```

After this change:

when running `dockerd` without options (same as current);

```
root      1014  2.5  2.1 496484 43468 pts/0    Sl+  12:23   0:00 dockerd
root      1023  1.2  1.1 681768 23832 ?        Ssl  12:23   0:00  \_ docker-containerd --config /var/run/docker/containerd/containerd.toml
```

when running `dockerd --debug`:

```
root       600  0.8  2.1 512876 43180 pts/0    Sl+  12:20   0:00 dockerd --debug
root       608  0.6  1.1 624428 23672 ?        Ssl  12:20   0:00  \_ docker-containerd --config /var/run/docker/containerd/containerd.toml --log-level debug
```

when running `dockerd --log-level=panic`

```
root       747  0.6  2.1 496548 43996 pts/0    Sl+  12:21   0:00 dockerd --log-level=panic
root       755  0.7  1.1 550696 24100 ?        Ssl  12:21   0:00  \_ docker-containerd --config /var/run/docker/containerd/containerd.toml --log-level panic
```

combining `--debug` and `--log-level` (`--debug` takes precedence):

```
root       880  2.7  2.1 634692 43336 pts/0    Sl+  12:23   0:00 dockerd --debug --log-level=panic
root       888  1.0  1.1 616232 23652 ?        Ssl  12:23   0:00  \_ docker-containerd --config /var/run/docker/containerd/containerd.toml --log-level debug
```

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the pass_loglevel_to_containerd branch from 921b36b to aaa1392 Compare July 9, 2018 13:40
@codecov
Copy link

codecov bot commented Jul 9, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@c8bda42). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #37419   +/-   ##
=========================================
  Coverage          ?   34.91%           
=========================================
  Files             ?      610           
  Lines             ?    44886           
  Branches          ?        0           
=========================================
  Hits              ?    15673           
  Misses            ?    27096           
  Partials          ?     2117

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 9, 2018

updated 👍

@mlaventure
Copy link
Contributor

mlaventure commented Jul 9, 2018 

FAIL: docker_api_swarm_service_test.go:311: DockerSwarmSuite.TestAPISwarmServicesFailedUpdate

FAIL: docker_cli_swarm_test.go:1372: DockerSwarmSuite.TestSwarmClusterRotateUnlockKey

Flaky swarm tests ?

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 9, 2018

Yup those are flaky 😒

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 9, 2018

restarted to make them go green

@thaJeztah thaJeztah merged commit 42bd8e1 into moby:master Jul 9, 2018
@thaJeztah thaJeztah deleted the pass_loglevel_to_containerd branch July 9, 2018 19:59
@thaJeztah thaJeztah mentioned this pull request Jul 10, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@vdemeester vdemeester vdemeester approved these changes

@mlaventure mlaventure mlaventure approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/runtime Runtime impact/changelog status/4-merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

dockerd seems to ignore loglevel setting

6 participants

@thaJeztah @mlaventure @vdemeester @cpuguy83 @MichaIng @GordonTheTurtle