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

Support specifying a specific logrus log level for shim log output #1058

Merged
merged 1 commit into from
Jul 8, 2021

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented Jul 6, 2021

Sometimes debug is a bit too noisy and can cause log rotation at a higher than
ideal rate.

This will be accompanied by an audit of our use of log levels throughout to make sure
they actually fit what level they're under.

This will be a new runtime config option for the shim, and the supported values are the seven logrus log levels.

Example:

[plugins.cri.containerd.default_runtime.options]
          LogLevel = "warning"
          DebugType = 2
          SandboxImage = "mcr.microsoft.com/windows/nanoserver:2009"          
          SandboxPlatform = "windows/amd64"

Signed-off-by: Daniel Canter dcanter@microsoft.com

@dcantah dcantah requested a review from a team as a code owner July 6, 2021 05:11
@dcantah
Copy link
Contributor Author

dcantah commented Jul 6, 2021

So this is annoying as we already have the debug option. What's our policy on deprecating/breaking existing options? Because as of now there's the redundant debug field that I'm just overriding if they specified log_level.

Edit: After consulting, we'll keep debug and just log a warning (hopefully the level the user chose is below this :P) if they were both specified. LogLevel will override debug

Copy link

@mainred mainred left a comment

Choose a reason for hiding this comment

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

Left three nits, thanks

cmd/containerd-shim-runhcs-v1/options/runhcs.proto Outdated Show resolved Hide resolved
cmd/containerd-shim-runhcs-v1/serve.go Outdated Show resolved Hide resolved

// log_level specifies the logrus log level for the shim. Supported values are a string representation of the
// logrus log levels: "trace", "debug", "info", "warn", "error", "fatal", "panic".
string log_level = 16;
Copy link

Choose a reason for hiding this comment

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

I know we have code explaining the relationship between log_level and debug, but could we add a comment for log_level also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Sometimes debug is a bit too noisy and can cause log rotation at a higher than
ideal rate.

This will be accompanied by an audit of our use of log levels throughout to make sure
they actually fit what level they're under.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
string NCProxyAddr = 15;

// log_level specifies the logrus log level for the shim. Supported values are a string representation of the
// logrus log levels: "trace", "debug", "info", "warn", "error", "fatal", "panic". This setting will override
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to make an enum for this?

Copy link
Contributor Author

@dcantah dcantah Jul 8, 2021

Choose a reason for hiding this comment

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

I didn't want to have to supply 0-n for the setting in the containerd config, it seems more intuitive to supply "debug", "warning" etc. as a user then have to remember/figure out what number corresponds to what level. Also makes it so you can feed the string directly to logrus

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah that's fine

Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants