Fixes exited messages when leaderelection lost #107724

kkkkun · 2022-01-24T14:43:21Z

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.:

k8s-ci-robot · 2022-01-24T14:43:30Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

binacs · 2022-01-25T02:56:20Z

/ok-to-test

kkkkun · 2022-01-25T03:02:37Z

/release-note-none

MadhavJivrajani · 2022-01-25T06:13:36Z

/remove-kind flake
/kind cleanup
/priority backlog
@pohly FYI, since you had commented on the issue.

pohly · 2022-01-25T07:51:19Z

cmd/kube-controller-manager/app/controllermanager.go

@@ -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)


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.

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.

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.

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

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.

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.

@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 ?

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.

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!

See #108725

kkkkun · 2022-02-07T03:01:31Z

/assign deads2k
/assign mikedanese
/assign sig-scheduling-maintainers

Could you please approve this ?

k8s-ci-robot · 2022-02-07T03:01:33Z

@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.
For more information please see the contributor guide

In response to this:

/assign deads2k
/assign mikedanese
/assign sig-scheduling-maintainers

Could you please giva a approved ?

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.

kkkkun · 2022-03-24T10:07:31Z

/test pull-kubernetes-unit

kkkkun · 2022-03-24T10:29:43Z

/retest

pohly

/lgtm

kkkkun · 2022-03-24T11:08:13Z

/retest

kkkkun · 2022-03-24T12:00:50Z

The failed case is not change of this.
add an issue to track #108975

kkkkun · 2022-03-24T12:09:38Z

/test pull-kubernetes-unit

kkkkun · 2022-03-28T09:13:23Z

/assign @liggitt @dims @jiahuif

Please cc

dims · 2022-03-28T10:54:22Z

/approve
/lgtm

k8s-ci-robot · 2022-03-28T10:55:04Z

[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:

~~cmd/kube-controller-manager/OWNERS~~ [dims]
~~cmd/kube-scheduler/OWNERS~~ [dims]
~~staging/src/k8s.io/cloud-provider/OWNERS~~ [dims]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jan 24, 2022

k8s-ci-robot requested review from ahg-g and deads2k January 24, 2022 14:43

kkkkun force-pushed the fix-leaderlost branch from 3241213 to 671118c Compare January 24, 2022 14:45

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 25, 2022

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 25, 2022

pohly reviewed Jan 25, 2022

View reviewed changes

kkkkun force-pushed the fix-leaderlost branch from 671118c to f684b6e Compare January 25, 2022 13:35

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 25, 2022

k8s-ci-robot assigned deads2k and mikedanese Feb 7, 2022

kkkkun force-pushed the fix-leaderlost branch from f684b6e to e619f83 Compare February 8, 2022 03:01

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2022

kkkkun requested a review from jiahuif February 8, 2022 03:05

kkkkun changed the title ~~Fix exit message when leaderelection lost~~ Fixes exited messages when leaderelection lost Feb 8, 2022

pohly mentioned this pull request Feb 17, 2022

utility function for "flush and exit" kubernetes/klog#301

Closed

k8s-ci-robot requested a review from sanposhiho February 17, 2022 19:42

Fixes exited messages when leaderelection lost

6f8e9aa

kkkkun force-pushed the fix-leaderlost branch from e619f83 to 6f8e9aa Compare March 24, 2022 09:35

pohly approved these changes Mar 24, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 24, 2022

k8s-ci-robot assigned dims and liggitt Mar 28, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2022

k8s-ci-robot merged commit b5f8d9e into kubernetes:master Mar 28, 2022

k8s-ci-robot added this to the v1.24 milestone Mar 28, 2022

kkkkun mentioned this pull request Jul 16, 2022

REQUEST: New membership for @kkkkun kubernetes/org#3561

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes exited messages when leaderelection lost #107724

Fixes exited messages when leaderelection lost #107724

kkkkun commented Jan 24, 2022 •

edited

k8s-ci-robot commented Jan 24, 2022

binacs commented Jan 25, 2022

kkkkun commented Jan 25, 2022

MadhavJivrajani commented Jan 25, 2022

pohly Jan 25, 2022

kkkkun Jan 25, 2022 •

edited

jiahuif Feb 7, 2022

kkkkun Feb 8, 2022

pohly Feb 8, 2022

pohly Feb 18, 2022

kkkkun Mar 15, 2022 •

edited

pohly Mar 15, 2022

kkkkun Mar 15, 2022

pohly Mar 16, 2022

kkkkun commented Feb 7, 2022 •

edited

k8s-ci-robot commented Feb 7, 2022

kkkkun commented Mar 24, 2022

kkkkun commented Mar 24, 2022

pohly left a comment

kkkkun commented Mar 24, 2022

kkkkun commented Mar 24, 2022 •

edited

kkkkun commented Mar 24, 2022

kkkkun commented Mar 28, 2022

dims commented Mar 28, 2022

k8s-ci-robot commented Mar 28, 2022

Fixes exited messages when leaderelection lost #107724

Fixes exited messages when leaderelection lost #107724

Conversation

kkkkun commented Jan 24, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Jan 24, 2022

binacs commented Jan 25, 2022

kkkkun commented Jan 25, 2022

MadhavJivrajani commented Jan 25, 2022

Choose a reason for hiding this comment

kkkkun Jan 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkkkun Mar 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkkkun commented Feb 7, 2022 • edited

k8s-ci-robot commented Feb 7, 2022

kkkkun commented Mar 24, 2022

kkkkun commented Mar 24, 2022

pohly left a comment

Choose a reason for hiding this comment

kkkkun commented Mar 24, 2022

kkkkun commented Mar 24, 2022 • edited

kkkkun commented Mar 24, 2022

kkkkun commented Mar 28, 2022

dims commented Mar 28, 2022

k8s-ci-robot commented Mar 28, 2022

kkkkun commented Jan 24, 2022 •

edited

kkkkun Jan 25, 2022 •

edited

kkkkun Mar 15, 2022 •

edited

kkkkun commented Feb 7, 2022 •

edited

kkkkun commented Mar 24, 2022 •

edited