-
Notifications
You must be signed in to change notification settings - Fork 462
Bug 1903290: kubelet: refactor KUBELET_LOG_LEVEL into KubeletConfig field #2262
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
Bug 1903290: kubelet: refactor KUBELET_LOG_LEVEL into KubeletConfig field #2262
Conversation
|
/cc @darkmuggle |
darkmuggle
left a comment
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.
Other than indentation, this looks good.
54d6c83 to
8bd8497
Compare
|
vpc exceeded error, reported in 4-dev |
|
/retest |
|
/approve |
|
/retest |
|
aws/gcp are having issues across the board today would like to see passing runs before approving |
|
@rphillips: This pull request references Bugzilla bug 1903290, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
DetailsIn 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. |
1388e8d to
5a1f653
Compare
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 thought level 2 was not sufficient for debugging?
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.
Problem is that level 3 is logging dead pod logs into the kubelet log. Instead of chasing all level 3 logs... I'm proposing just setting it to level 2. This new approach with a kubelog.conf would allow us to manage the kubelet log level within the kubeletconfig perhaps.
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.
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.
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.
@kikisdeliveryservice I'm on board with this
5a1f653 to
2a47bdc
Compare
|
@rphillips: This pull request references Bugzilla bug 1903290, which is valid. 3 validation(s) were run on this bug
DetailsIn 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. |
36fe533 to
2a47bdc
Compare
|
uh since ci is still underwater could you update: https://github.com/openshift/machine-config-operator/blob/master/docs/KubeletConfigDesign.md |
|
unit and verify failures don't look to be flakes.. |
443daca to
5d6b4d1
Compare
|
Updated to fix the unit tests and allow support for only setting the logLevel |
|
@rphillips: This pull request references Bugzilla bug 1903290, which is valid. 3 validation(s) were run on this bug
DetailsIn 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. |
5d6b4d1 to
a39d9de
Compare
|
Vpc limit /retest |
|
@rphillips brought this up again in 4-dev no aws resolution yet |
|
/skip |
kikisdeliveryservice
left a comment
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.
Tested & seems to work ok - made kubelet config applied, then changed kubelet logging seemed to change appropriately.
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
still hitting VPC limits issues 😞 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Since Seth found an issue, I think we should fix it first then merge /hold |
a39d9de to
c6cec50
Compare
|
/hold cancel fixed the issue. |
|
Trying it out now. Will lgtm when verified. |
c6cec50 to
1030b2c
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: darkmuggle, kikisdeliveryservice, rphillips, sjenning The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/skip |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@rphillips: All pull requests linked via external trackers have merged: Bugzilla bug 1903290 has been moved to the MODIFIED state. DetailsIn 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. |
|
/cherry-pick release-4.6 |
|
@rphillips: #2262 failed to apply on top of branch "release-4.6": DetailsIn 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 I did
Takes Ben's suggestion and refactors the Kubelet log level into a systemd environment file.
Obsoletes: #2261
This also changes the default of the log level to 2, since stale logs are printed into the kubelet log at level 3: BZ1903290.
Allows the logLevel to be configured on the KubeletConfig CRD:
- How to verify it
- Description for the changelog