Handling OptimisticLockError in kubelet node lease controller #79341

krzysied · 2019-06-24T15:21:34Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Updating node lease assumes that node lease hasn't changed between get and update calls. It sometimes happens that kubelet doesn't get ack from apiserver even though that node lease is correctly updated. Kubelet tries to re-update node lease, which fails due to the apiserver having never version of lease. This PR adds additional lease get when optimistic lock error happens.

This bug was described by @mborsz in #79096 (comment)

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

krzysied · 2019-06-24T15:25:17Z

/priority important-soon
/assign @wojtek-t
/cc @mborsz

pkg/kubelet/nodelease/controller.go

krzysied · 2019-06-25T17:34:02Z

/test pull-kubernetes-e2e-gce-100-performance

wojtek-t · 2019-06-26T09:49:14Z

pkg/kubelet/nodelease/controller_test.go

+	patchCalls            chan pair
+}
+
+func newFakeLeaseClient() *fakeLeaseClient {


You don't want to do that.
You want to use this one:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/kubernetes/typed/coordination/v1/fake/fake_coordination_client.go
instead.

Then you register Reactor:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/testing/fake.go#L102

and I think you don't need much more...

I simplified the test. Reactor works perfectly.

wojtek-t

I need to take a deeper look too and will have a couple more cosmetic comments.

wojtek-t · 2019-06-26T14:09:20Z

pkg/kubelet/nodelease/controller.go

 	for i := 0; i < maxUpdateRetries; i++ {
-		_, err := c.leaseClient.Update(c.newLease(base))
+		// OptimisticLockError requires getting the newer version of lease to proceed.


Move it to the bottm of the loop - then you don't need to define err, and obviously in the first iteration it will always be nil.

Done.
I wanted to avoid additional get call. I assume that code is more readable now?

wojtek-t · 2019-06-26T14:10:12Z

pkg/kubelet/nodelease/controller.go

 	for i := 0; i < maxUpdateRetries; i++ {
-		_, err := c.leaseClient.Update(c.newLease(base))
+		// OptimisticLockError requires getting the newer version of lease to proceed.
+		if err != nil && strings.Contains(err.Error(), registry.OptimisticLockErrorMsg) {


You don't want to leak OptimisticLockErrorMsg here. Instead use:
apierrors.IsConflict() function

wojtek-t · 2019-06-26T14:17:03Z

/assign @wangzhen127

pkg/kubelet/nodelease/controller.go

wojtek-t · 2019-06-27T06:20:43Z

pkg/kubelet/nodelease/controller_test.go

+			UID:  types.UID("foo-uid"),
+		},
+	}
+	noConnectionUpdateErr := apierrors.NewServerTimeout(schema.GroupResource{Group: "v1", Resource: "lease"}, "put", 1)


gr := schema.GroupResource{Group: "v1", Resource: "lease"}

and use it here and below (the lines will be shorter then).

wojtek-t · 2019-06-27T06:21:22Z

pkg/kubelet/nodelease/controller_test.go

+	optimistcLockUpdateErr := apierrors.NewConflict(schema.GroupResource{Group: "v1", Resource: "lease"}, "lease", fmt.Errorf("conflict"))
+	cases := []struct {
+		desc                       string
+		updateReactor              func(action clienttesting.Action) (handled bool, ret runtime.Object, err error)


remove names of parameters in the here and below

wojtek-t · 2019-06-27T06:21:56Z

pkg/kubelet/nodelease/controller_test.go

+	}{
+		{
+			desc: "no errors",
+			updateReactor: func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {


remove names of output params - they are not used anywhere

both here and in all following cases.

krzysied · 2019-06-27T09:36:12Z

/test pull-kubernetes-bazel-test

wojtek-t · 2019-06-27T09:38:17Z

pkg/kubelet/nodelease/controller_test.go

+				leaseDurationSeconds:       10,
+				onRepeatedHeartbeatFailure: tc.onRepeatedHeartbeatFailure,
+			}
+			if err := c.retryUpdateLease(nil); fmt.Sprintf("%v", err) != fmt.Sprintf("%v", tc.expectErr) {


Why we need fmt.Sprintf?

Please fix so that we could simply compare errors.

This is a hacky way to compare errors - normally you use Error(), however this will not work with nil. To avoid nil checks etc, using Spritnf will cause to call Error() if err != nil, or just returns .

Assuming that we don't really need to compare errors, but rather check if error was returned or not, I can change ecpectErr to bool and verify of err != nil. WDYT?

Assuming that we don't really need to compare errors, but rather check if error was returned or not, I can change ecpectErr to bool and verify of err != nil. WDYT?

+1

wojtek-t · 2019-06-27T11:53:15Z

/lgtm
/approve

k8s-ci-robot · 2019-06-27T11:53:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krzysied, wojtek-t

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:

~~pkg/kubelet/OWNERS~~ [wojtek-t]

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 requested review from dchen1107 and resouer June 24, 2019 15:22

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 24, 2019

k8s-ci-robot assigned wojtek-t Jun 24, 2019

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 Jun 24, 2019

k8s-ci-robot requested a review from mborsz June 24, 2019 15:25

krzysied force-pushed the kubelet_lease_fix branch from a1b3b13 to b6edc95 Compare June 24, 2019 15:26

wojtek-t reviewed Jun 24, 2019

View reviewed changes

pkg/kubelet/nodelease/controller.go Outdated Show resolved Hide resolved

krzysied force-pushed the kubelet_lease_fix branch from b6edc95 to 307e1f7 Compare June 25, 2019 16:37

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 25, 2019

krzysied force-pushed the kubelet_lease_fix branch from 307e1f7 to cf67243 Compare June 25, 2019 16:42

wojtek-t reviewed Jun 26, 2019

View reviewed changes

krzysied force-pushed the kubelet_lease_fix branch 4 times, most recently from fdebc07 to d490d9e Compare June 26, 2019 13:49

wojtek-t reviewed Jun 26, 2019

View reviewed changes

k8s-ci-robot assigned wangzhen127 Jun 26, 2019

krzysied force-pushed the kubelet_lease_fix branch 2 times, most recently from e25b569 to 4dd8892 Compare June 26, 2019 19:05

wojtek-t reviewed Jun 27, 2019

View reviewed changes

krzysied force-pushed the kubelet_lease_fix branch from 4dd8892 to 8a66376 Compare June 27, 2019 08:30

wojtek-t reviewed Jun 27, 2019

View reviewed changes

handling OptimisticLockError in kubelet

d45197a

krzysied force-pushed the kubelet_lease_fix branch from 8a66376 to d45197a Compare June 27, 2019 11:44

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 27, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 27, 2019

k8s-ci-robot merged commit f208769 into kubernetes:master Jun 27, 2019

krzysied deleted the kubelet_lease_fix branch June 27, 2019 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handling OptimisticLockError in kubelet node lease controller #79341

Handling OptimisticLockError in kubelet node lease controller #79341

krzysied commented Jun 24, 2019

krzysied commented Jun 24, 2019

krzysied commented Jun 25, 2019

wojtek-t Jun 26, 2019

krzysied Jun 26, 2019

wojtek-t left a comment

wojtek-t Jun 26, 2019

krzysied Jun 26, 2019

wojtek-t Jun 26, 2019

krzysied Jun 26, 2019

wojtek-t commented Jun 26, 2019

wojtek-t Jun 27, 2019

wojtek-t Jun 27, 2019

krzysied Jun 27, 2019

wojtek-t Jun 27, 2019

krzysied Jun 27, 2019

krzysied commented Jun 27, 2019

wojtek-t Jun 27, 2019

krzysied Jun 27, 2019

wojtek-t Jun 27, 2019

krzysied Jun 27, 2019

wojtek-t commented Jun 27, 2019

k8s-ci-robot commented Jun 27, 2019

Handling OptimisticLockError in kubelet node lease controller #79341

Handling OptimisticLockError in kubelet node lease controller #79341

Conversation

krzysied commented Jun 24, 2019

krzysied commented Jun 24, 2019

krzysied commented Jun 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t left a 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

Choose a reason for hiding this comment

wojtek-t commented Jun 26, 2019

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

krzysied commented Jun 27, 2019

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

wojtek-t commented Jun 27, 2019

k8s-ci-robot commented Jun 27, 2019