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.

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

Pass log-level to containerd #37419

Merged
merged 1 commit into from Jul 9, 2018

Conversation

Projects
None yet
4 participants
@thaJeztah
Copy link
Member

thaJeztah commented Jul 9, 2018

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 github@gone.nl

- 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

This comment has been minimized.

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?

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

@vdemeester
Copy link
Member

vdemeester left a comment

LGTM 🐯

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

@thaJeztah

This comment has been minimized.

Copy link
Member Author

thaJeztah commented Jul 9, 2018

Let me update the PR and remove the default

Pass log-level to containerd
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 <github@gone.nl>

@thaJeztah thaJeztah force-pushed the thaJeztah:pass_loglevel_to_containerd branch from 921b36b to aaa1392 Jul 9, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 9, 2018

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

This comment has been minimized.

Copy link
Member Author

thaJeztah commented Jul 9, 2018

updated 👍

@mlaventure

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

thaJeztah commented Jul 9, 2018

Yup those are flaky 😒

@thaJeztah

This comment has been minimized.

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

7 of 8 checks passed

codecov/patch 0% of diff hit (target 50%)
Details
codecov/project No report found to compare against
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 41259 has succeeded
Details
janky Jenkins build Docker-PRs 50025 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 10440 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 21368 has succeeded
Details
z Jenkins build Docker-PRs-s390x 10329 has succeeded
Details

@thaJeztah thaJeztah deleted the thaJeztah:pass_loglevel_to_containerd branch Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.