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
Improve logging on kube-proxy exit #120375
Conversation
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
You have to adapt the unit test , check if this can serve as guide #120050 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, pegasas 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 |
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.
Looks good to me, but before I /lgtm let me mention another thought - perhaps you'll want to fix this in this PR.
Would it make sense to support writing a termination message in kube-proxy?
that is a container field, isn't it? |
@@ -57,7 +57,7 @@ func (n *NodePodCIDRHandler) OnNodeAdd(node *v1.Node) { | |||
if !reflect.DeepEqual(n.podCIDRs, podCIDRs) { | |||
klog.ErrorS(nil, "Using NodeCIDR LocalDetector mode, current PodCIDRs are different than previous PodCIDRs, restarting", | |||
"node", klog.KObj(node), "newPodCIDRs", podCIDRs, "oldPodCIDRs", n.podCIDRs) | |||
panic("Current Node PodCIDRs are different than previous PodCIDRs, restarting") | |||
klog.FlushAndExit(klog.ExitFlushTimeout, 1) |
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.
This will lost the exit message
klog.FlushAndExit(klog.ExitFlushTimeout, 1) | |
klog.InfoS("Current Node PodCIDRs are different than previous PodCIDRs, restarting") | |
klog.FlushAndExit(klog.ExitFlushTimeout, 1) |
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.
sure.
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.
It seems klog.ErrorS
log this in the error level,
need we log it another time?
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.
Ops, sorry, miss that.
Hi, @pohly , Yes, for sure. It seems this is another proposal. When we host kube-proxy in a pod, we should write an error reason related log into /dev/termination-log, I would like to open another thread for tracking. |
Doing this in a follow-up makes sense. |
9582a4d
to
d23bff6
Compare
/lgtm |
LGTM label has been added. Git tree hash: 52afa76a4ab9648251a16b4e525d39f6ab155e0b
|
Does this need a release note? |
/release-note-edit
|
@aojea: /release-note-edit must be used with a single release note block. 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. |
@pegasas please update the release note block in the description |
you need to put a block of text like this
|
Thanks! |
FYI you don't need to rebase, the CI tooling does it for you ;) /lgtm |
LGTM label has been added. Git tree hash: e1e8be1db8296425a2e52a4a8609160de0f2cf90
|
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #120355
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: