Report KCM as unhealthy if leader election is wedged. #70971

cheftako · 2018-11-13T02:50:59Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Mitigation on #70819.
Too late in the cycle to make a large x/net/http2 vendor change.
Adding code to track if the lease election is wedged and report as unhealthy.
That should allow the kubelet to restart the KCM.
Please note that this only works for clusters without --leader-elect turned off. (HA clusters must run leader election, it is theoretically optional in single master clusters )

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Mitigates #70819

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

None

cheftako · 2018-11-13T02:51:57Z

/assign @lavalamp

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go

lavalamp · 2018-11-13T05:21:31Z

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go

+	// If we are more than 20 seconds after the lease durection that is past the timeout on the lease renew.
+	// Time to start reporting ourselves as unhealthy. We should have died but conditions like deadlock can
+	// prevent this. (See #70819)
+	expiry := l.le.observedTime.Add(l.le.config.LeaseDuration).Add(time.Second * 20)


I'm a little conflicted about a hardcoded 20 second timeout. I think not every user of this library will exit like controller manager does, so maybe it should be configurable?

Good point. I have pushed it as configurable up one level. However I'm not sure I want to push it all the way to the CLI just yet.

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go

lavalamp · 2018-11-13T05:35:58Z

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go

+	// If we are more than 20 seconds after the lease durection that is past the timeout on the lease renew.
+	// Time to start reporting ourselves as unhealthy. We should have died but conditions like deadlock can
+	// prevent this. (See #70819)
+	expiry := l.le.observedTime.Add(l.le.config.LeaseDuration).Add(time.Second * 20)


If you add a function that exposes observedTime (or the observedRecord's acquireTime), then maybe you can put all of this code in another package?

I could but I don't want to expose the inner workings of leaderelection as I feel it should remain obfuscated. The more we expose these details the less room we have to fix it later. I also think that having the optional healthz check which is responsible for/understands the inner workings of leaderelection is a good thing. I agree with not wanting to pull in server code to the client package but I think you other suggestions have essentially accomplished that.

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go

deads2k · 2018-11-13T22:27:29Z

I like the idea of this change.

/milestone 1.13

k8s-ci-robot · 2018-11-13T22:27:29Z

@deads2k: The provided milestone is not valid for this repository. Milestones in this repository: [next-candidate, v1.10, v1.11, v1.12, v1.13, v1.14, v1.4, v1.5, v1.6, v1.7, v1.8, v1.9]

Use /milestone clear to clear the milestone.

In response to this:

I like the idea of this change.

/milestone 1.13

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.

dims · 2018-11-14T01:04:55Z

/priority important-soon

cheftako · 2018-11-14T01:37:05Z

/test pull-kubernetes-integration

lavalamp

No structural comments this time, just a bunch of little nits.

cmd/controller-manager/app/serve.go

staging/src/k8s.io/client-go/tools/leaderelection/healthzadaptor.go

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go

staging/src/k8s.io/client-go/tools/leaderelection/healthzadaptor_test.go

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go

lavalamp

Just nits now.

staging/src/k8s.io/client-go/tools/leaderelection/healthzadaptor.go

staging/src/k8s.io/client-go/tools/leaderelection/healthzadaptor_test.go

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go

lavalamp · 2018-11-15T00:26:14Z

staging/src/k8s.io/client-go/tools/leaderelection/healthzadaptor.go

+	l.le = le
+}
+
+// NewHealthzAdaptor creates a basic healthz adaptor to monitor a leader election.


This comment missed :)

staging/src/k8s.io/client-go/tools/leaderelection/healthzadaptor_test.go

Feedback from lavalamp and deads2k. Changed Check() logic to be central to LeaderElector. Further changes, especially cleaning up the test code.

lavalamp · 2018-11-15T07:07:18Z

/approve
/lgtm

Do we need to provide a way to disable this just in case it is causes a problem? I can't think of a specific thing that can go wrong-- actually, what about daylight savings time getting turned off? I know (hope?) we tell everyone to use UTC for servers, but...

It seems remote so I won't hold this up any more.

I guess folks can use Han's change to stop monitoring this if necessary anyway?

k8s-ci-robot · 2018-11-15T07:07:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, lavalamp

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/cloud-controller-manager/OWNERS~~ [lavalamp]
~~cmd/controller-manager/OWNERS~~ [cheftako,lavalamp]
~~cmd/kube-controller-manager/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/client-go/tools/leaderelection/OWNERS~~ [lavalamp]

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

logicalhan · 2018-11-15T20:58:50Z

I guess folks can use Han's change to stop monitoring this if necessary anyway?

Verified:

$ curl "localhost:10252/healthz?verbose&exclude=leaderElection"

[+]leaderElection excluded: ok
healthz check passed

#70676-#70971-upstream-release-1.12 Automated cherry pick of #70753 #70676 #70971 upstream release 1.12

…25-upstream-release-1.10 Automated cherry pick of #70753, #70676 and #70971 upstream release 1.10

k8s-ci-robot requested review from deads2k and eparis November 13, 2018 02:51

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 13, 2018

k8s-ci-robot assigned lavalamp Nov 13, 2018

lavalamp reviewed Nov 13, 2018

View reviewed changes

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go Outdated Show resolved Hide resolved

lavalamp reviewed Nov 13, 2018

View reviewed changes

deads2k reviewed Nov 13, 2018

View reviewed changes

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go Outdated Show resolved Hide resolved

deads2k reviewed Nov 13, 2018

View reviewed changes

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go Outdated Show resolved Hide resolved

deads2k added this to the v1.13 milestone Nov 13, 2018

dims mentioned this pull request Nov 14, 2018

Update golang.org/x/net/http2 vendored files #71016

Closed

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 14, 2018

cheftako force-pushed the leaseCheck branch from 0a2b3e2 to d0ccf94 Compare November 14, 2018 02:46

lavalamp reviewed Nov 14, 2018

View reviewed changes

cheftako force-pushed the leaseCheck branch from d0ccf94 to bc29853 Compare November 14, 2018 23:43

lavalamp reviewed Nov 15, 2018

View reviewed changes

staging/src/k8s.io/client-go/tools/leaderelection/healthzadaptor_test.go Outdated Show resolved Hide resolved

Report KCM as unhealthy if leader election is wedged.

9c43ee6

Feedback from lavalamp and deads2k. Changed Check() logic to be central to LeaderElector. Further changes, especially cleaning up the test code.

cheftako force-pushed the leaseCheck branch from bc29853 to 9c43ee6 Compare November 15, 2018 00:57

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2018

k8s-ci-robot merged commit f48e18f into kubernetes:master Nov 15, 2018

bsalamat mentioned this pull request Nov 15, 2018

Report kube-scheduler unhealthy if leader election is wedged #71085

Merged

k8s-ci-robot added a commit that referenced this pull request Dec 5, 2018

Merge pull request #71285 from cheftako/automated-cherry-pick-of-#70753-

ee860a5

#70676-#70971-upstream-release-1.12 Automated cherry pick of #70753 #70676 #70971 upstream release 1.12

k8s-ci-robot added a commit that referenced this pull request Dec 13, 2018

Merge pull request #71334 from cheftako/automated-cherry-pick-of-#713…

15008f0

…25-upstream-release-1.10 Automated cherry pick of #70753, #70676 and #70971 upstream release 1.10

cheftako mentioned this pull request Feb 6, 2019

kube-controller-manager becomes deadlocked but still passes healthcheck #70819

Closed

dims mentioned this pull request Mar 13, 2019

Update golang.org/x/net/... dependencies to release-branch.go1.12 #75289

Merged

wallrj mentioned this pull request Apr 18, 2023

cainjector in a zombie state after attempting to shut down cert-manager/cert-manager#5889

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report KCM as unhealthy if leader election is wedged. #70971

Report KCM as unhealthy if leader election is wedged. #70971

cheftako commented Nov 13, 2018 •

edited

Loading

cheftako commented Nov 13, 2018

lavalamp Nov 13, 2018

cheftako Nov 13, 2018

lavalamp Nov 13, 2018

cheftako Nov 13, 2018

deads2k commented Nov 13, 2018

k8s-ci-robot commented Nov 13, 2018

dims commented Nov 14, 2018

cheftako commented Nov 14, 2018

lavalamp left a comment

lavalamp left a comment

lavalamp Nov 15, 2018

lavalamp commented Nov 15, 2018

k8s-ci-robot commented Nov 15, 2018

logicalhan commented Nov 15, 2018

Report KCM as unhealthy if leader election is wedged. #70971

Report KCM as unhealthy if leader election is wedged. #70971

Conversation

cheftako commented Nov 13, 2018 • edited Loading

cheftako commented Nov 13, 2018

lavalamp Nov 13, 2018

Choose a reason for hiding this comment

cheftako Nov 13, 2018

Choose a reason for hiding this comment

lavalamp Nov 13, 2018

Choose a reason for hiding this comment

cheftako Nov 13, 2018

Choose a reason for hiding this comment

deads2k commented Nov 13, 2018

k8s-ci-robot commented Nov 13, 2018

dims commented Nov 14, 2018

cheftako commented Nov 14, 2018

lavalamp left a comment

Choose a reason for hiding this comment

lavalamp left a comment

Choose a reason for hiding this comment

lavalamp Nov 15, 2018

Choose a reason for hiding this comment

lavalamp commented Nov 15, 2018

k8s-ci-robot commented Nov 15, 2018

logicalhan commented Nov 15, 2018

cheftako commented Nov 13, 2018 •

edited

Loading