-
Notifications
You must be signed in to change notification settings - Fork 38.9k
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
add --logging-format flag to kubelet #91532
add --logging-format flag to kubelet #91532
Conversation
Welcome @afrouzMashaykhi! |
Hi @afrouzMashaykhi. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cc @serathius |
Hey @afrouzMashaykhi Thanks for contributing! |
cdb5d5a
to
3d32a4e
Compare
cmd/kubelet/app/options/options.go
Outdated
} | ||
|
||
errs = append(errs, f.Logs.Validate()...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the validation passes here, would this append a nil error to the slice, and result in a validation failure further up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed validation of logformat
cmd/kubelet/app/server.go
Outdated
@@ -500,6 +500,8 @@ func run(s *options.KubeletServer, kubeDeps *kubelet.Dependencies, featureGate f | |||
if err != nil { | |||
klog.Errorf("unable to register KubeletConfiguration with configz, error: %v", err) | |||
} | |||
// set klog of kubelet | |||
s.Logs.Apply() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applying the config to global settings should probably happen super-early in the bootstrap.
We can start writing logs almost immediately:
kubernetes/cmd/kubelet/app/server.go
Lines 152 to 164 in 0891f69
Run: func(cmd *cobra.Command, args []string) { | |
// initial flag parse, since we disable cobra's flag parsing | |
if err := cleanFlagSet.Parse(args); err != nil { | |
cmd.Usage() | |
klog.Fatal(err) | |
} | |
// check if there are non-flag arguments in the command line | |
cmds := cleanFlagSet.Args() | |
if len(cmds) > 0 { | |
cmd.Usage() | |
klog.Fatalf("unknown command: %s", cmds[0]) | |
} |
Also, are any additional changes required wrt the InitLogs call in https://github.com/kubernetes/kubernetes/blob/master/cmd/kubelet/kubelet.go#L38?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on KEP the logging format flag just sets the format of logging and I think, changing log messages handled by new klog methods. @serathius please correct me if I am wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Michael are you proposing then that we move this --logging-format
flag init/setup/apply to InitLogs
? This would mean that we would need to make this flag global.
In #89683 we have implemented it as standard flag initialized with component. Like you mentioned that it has problem that we invoke klog before we initialize components.
Pros of global flag:
- Consistent logging configuration (as initialization happens before component bootstrap)
Cons:
- All logging flags will need to be kept as global flag
- LoggerConfig cannot be added to Components (no dynamic config, no control for component owner about this flag)
I think that having consistent log format is important so we should go with global flag. This would mean that all PRs adding LoggingConfig
are no longer needed :(. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily into InitLogs - just that logging config should probably be set before any log messages are written?
And as @serathius mentioned, there are a few additional changes required to support the kubelet config file. You can find details in our contributor guide for migrating flags to config: https://docs.google.com/document/d/10Kz__miCWEOekC6WnsUdxYWSLrf-t1AkHHpZN57ei5c/edit |
/ok-to-test |
/milestone v1.19 |
to fix the fuzzer test, update pkg/kubelet/apis/config/fuzzer/fuzzer.go to match the default logging format: --- a/pkg/kubelet/apis/config/fuzzer/fuzzer.go
+++ b/pkg/kubelet/apis/config/fuzzer/fuzzer.go
@@ -99,6 +99,9 @@ func Funcs(codecs runtimeserializer.CodecFactory) []interface{} {
obj.ConfigMapAndSecretChangeDetectionStrategy = "Watch"
obj.AllowedUnsafeSysctls = []string{}
obj.VolumePluginDir = kubeletconfigv1beta1.DefaultVolumePluginDir
+ if obj.Logging.Format == "" {
+ obj.Logging.Format = "text"
+ }
},
}
} |
/approve lgtm once the fuzzer test is fixed |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrouzMashaykhi, liggitt, mtaufen, wojtek-t The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
18a5ad4
to
734c240
Compare
734c240
to
b92b04e
Compare
/retest |
1 similar comment
/retest |
/lgtm |
/retest |
/retest Review the full test history for this PR. Silence the bot with an |
/retest |
1 similar comment
/retest |
/priority important-soon |
/retest |
@afrouzMashaykhi: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Ref kubernetes/enhancements#1602
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/1602-structured-logging