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
Fix json log format config bug #100409
Fix json log format config bug #100409
Conversation
/triage accepted |
/assign @serathius |
Can we wait till #99693 is merged? It should simplify how Kubelet logging configuration is applied. We are currently in code freeze so this will still need to wait. |
sure |
Result in
|
This should be ready for review soon, dependent PR is about to merge. |
/hold cancel |
@liggitt Hi, jordan, i move kubelet config logic ahead, please have an review. |
I'm not 100% sure of the kubelet config loading ordering/behavior, so I'll defer to kubelet approvers. Is there good test coverage of success and error scenarios both with logging options specified via flags and via config? |
@kubernetes/sig-node-pr-reviews Please give more review.
We don't have these test now, Only run |
ping @ehashman for this pr need kubelet team's help |
/assign @ehashman |
Ping @ehashman for this pr need kubelet team's help |
cc @pohly |
The current PR unconditionally moves logging initialization before config file loading. That cannot work when logging options are specified via that config file and not via command line options, which is the goal for kubelet (command line flags are marked as deprecated). I have a less intrusive PR pending for review which initializes logging as soon as possible, i.e. directly after parsing the config file: #104997 But the underlying problem is that it cannot be guaranteed that there is no log output in the default text format. The only way to achieve that would be to suppress all output by default and only activate output in the selected format once it is known what that format is. That's not ideal either because then |
I did this in former commit, but #100409 (comment) point out that kubelet config file will be not honored, so move loadConfigFile ahead, and if kubelet config file not specified, kubeletConfig is from command line options. |
Or did i miss something? |
You are right, I missed that you also moved loading of the file. What's broken in that case is this combination:
Because logging gets initialized before re-parsing the command line flags, it uses JSON although command line flags are supposed to have higher priority. |
In command line flags , logging-format has default "text" set, so if set format to json, and command line flags not set logging-format, we get text logging format? |
Here's the combination that doesn't work as it should with this PR:
Logging got initialized as JSON instead of text as requested by the command line parameter. The command line parameter should have higher priority, according to comments in the source code. |
That whole problem goes away when the deprecated command line flags actually get removed. But apparently it is not clear if or when that'll happen - I recently asked in a SIG-Node meeting about that. |
/unassign Danielle can you PTAL? This relates to another PR you were looking at with order of flag loading/validation. |
} | ||
|
||
logOption := &logs.Options{Config: kubeletConfig.Logging} | ||
logOption.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.
#106090 changed the API for validating and applying log options - that simply has to be done much sooner than it was done before (validate, log something, etc, then apply log options).
This code here shouldn't compile anymore.
/test pull-verify
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.
/test pull-kubernetes-verify
@pohly: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
@yuzhiquan: 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. |
@yuzhiquan: PR needs rebase. 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. |
Is this PR still needed, please rebase if so (or we can close it?) |
I think it can be closed. |
/close |
@yuzhiquan: Closed this PR. In response to this:
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. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #100152
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: