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
Fixes exited messages when leaderelection lost #107724
Conversation
Hi @kkkkun. 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. |
3241213
to
671118c
Compare
/ok-to-test |
/release-note-none |
/remove-kind flake |
@@ -275,7 +275,8 @@ func Run(c *config.CompletedConfig, stopCh <-chan struct{}) error { | |||
run(ctx, startSATokenController, initializersFunc) | |||
}, | |||
OnStoppedLeading: func() { | |||
klog.Fatalf("leaderelection lost") | |||
klog.ErrorS(nil, "leaderelection lost") | |||
os.Exit(255) |
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.
How quickly do you want to exit here?
Originally I had implemented JSON output handling so that ErrorS
flushes buffered info messages automatically. @serathius later removed that because flushing also did an fsync, which caused a performance issue.
If you want to ensure that info messages are written, you have to add klog.Flush
before os.Exit
, but only if a (probably small) delay is acceptable.
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 met this case several time.
When it crash, i think the currect message is enough. It doesn't need to print all groutine stacks.
If you want to ensure that info messages are written, you have to add klog.Flush before os.Exit, but only if a (probably small) delay is acceptable.
Added.
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.
Could you take a look at the official migration guide here?
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#replacing-fatal-calls
If we really need a Flush, then we are to add flush everywhere considering how many klog.Fatalf
we had previously.
@serathius your input is greatly appreciated.
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.
Could you take a look at the official migration guide here? https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#replacing-fatal-calls
If we really need a Flush, then we are to add flush everywhere considering how many
klog.Fatalf
we had previously.@serathius your input is greatly appreciated.
Thanks. I fixed according to #replacing-fatal-calls
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 we really need a Flush, then we are to add flush everywhere considering how many klog.Fatalf we had previously.
The question is whether you want a Flush. Fatalf flushes for 10 seconds before exiting:
https://github.com/kubernetes/klog/blob/d78dad3276f1f4139a6614b08b02e4678d2c3393/klog.go#L1108-L1111
The migration guide doesn't cover that aspect; I don't know whether that was intentional or an oversight.
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.
the Exit APIs fits the bill
You mean klog.Exitf
and friends? That is part of the old-style structured logging API. We need a replacement for that when migrating to structured logging, just as we need a replacement for klog.Fatalf
.
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.
@pohly Hi. I notice kubernetes/klog#303 merged.
So in here, we should exit by klog.FlushAndExit(klog.ExitFlushTimeout, 1)
?
May be we need a new tag in klog and replace by kubernetes
?
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.
So in here, we should exit by
klog.FlushAndExit(klog.ExitFlushTimeout, 1)
?
Yes, that would be good.
May be we need a new tag in klog and replace by kubernetes ?
I'll get to that as soon as possible. There's still kubernetes/klog#297 pending, then klog should be ready for a new release.
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'll get to that as soon as possible. There's still kubernetes/klog#297 pending, then klog should be ready for a new release.
Look forward to it!
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.
See #108725
671118c
to
f684b6e
Compare
/assign deads2k Could you please approve this ? |
@kkkkun: GitHub didn't allow me to assign the following users: sig-scheduling-maintainers. Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. 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. |
f684b6e
to
e619f83
Compare
/test pull-kubernetes-unit |
/retest |
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.
/lgtm
/retest |
The failed case is not change of this. |
/test pull-kubernetes-unit |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, kkkkun, pohly 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #107665
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: