enhance leader election doc #77585

andyxning · 2019-05-08T08:42:47Z

What type of PR is this?
/kind client-go

What this PR does / why we need it:
When we use leader election pkg under two operator app instances with lease id being Host-A and Host-B, i.e., hostname for lease id. If Host-A is the leader and Host-B is always trying to acquire the lease. After some time of running, we both stop operator app instances running on Host-A and Host-B. After ten minutes later which is longer than lease duration, we then first start operator app instances running on Host-B. But, it can not acquire the lease in the first lease duration time since it is first acquired by an instance running on Host-A.

This will trigger a problem as there is some time that no operator instances can run.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

andyxning · 2019-05-08T09:09:51Z

/test pull-kubernetes-bazel-test

neolit123

@andyxning
this change seems to break unit tests, see here:
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/77585/pull-kubernetes-bazel-test/1126051762252288000

/kind bug

jennybuckley · 2019-05-09T20:08:41Z

/assign @cheftako
/cc @jpbetz

andyxning · 2019-05-10T03:40:55Z

/test pull-kubernetes-integration

andyxning · 2019-05-10T04:17:50Z

@neolit123 @cheftako @jpbetz CI is green now. PTAL.

neolit123 · 2019-05-10T10:53:29Z

the change makes sense, but i will defer to the client-go maintainers.

andyxning · 2019-05-13T05:39:33Z

/cc @caesarxuchao @timothysc @mikedanese

mikedanese · 2019-05-13T06:26:48Z

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

 	// 2. Record obtained, check the Identity & Time
 	if !reflect.DeepEqual(le.observedRecord, *oldLeaderElectionRecord) {
 		le.observedRecord = *oldLeaderElectionRecord
 		le.observedTime = le.clock.Now()
 	}
 	if len(oldLeaderElectionRecord.HolderIdentity) > 0 &&
 		le.observedTime.Add(le.config.LeaseDuration).After(now.Time) &&
+		!(firstStart && oldLeaderElectionRecord.RenewTime.Add(le.config.LeaseDuration).Before(now.Time)) &&


This time stamp is not meaningful if it was collected on another machine. The implementation of this client only acts on locally collected time stamps and cannot rely on the accuracy of time stamp in the record for correctness. The client needs to wait the full lease duration without observing a change to the record before it can attempt to take over. Start isn’t different. This is documented here:

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

Lines 22 to 24 in f5a1ceb

// leader (a.k.a. fencing). A client observes timestamps captured locally to

// infer the state of the leader election. Thus the implementation is tolerant

// to arbitrary clock skew, but is not tolerant to arbitrary clock skew rate.

10 minute lease duration sounds too long. It’s defaulted to 15 seconds in core components:

kubernetes/cmd/cloud-controller-manager/app/options/options_test.go

Line 54 in c48ed1c

LeaseDuration: metav1.Duration{Duration: 15 * time.Second},

@mikedanese So it is designed by purpose not a bug, actually?

This time stamp is not meaningful if it was collected on another machine. The implementation of this client only acts on locally collected time stamps and cannot rely on the accuracy of time stamp in the record for correctness. The client needs to wait the full lease duration without observing a change to the record before it can attempt to take over. Start isn’t different.

This absolutely needs to be added to the leader election package documentation, IMO. It is more cleaer in describing a actual scenario.

Yes the intended behavior:

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

Lines 105 to 108 in 3d12466

// LeaseDuration is the duration that non-leader candidates will

// wait to force acquire leadership. This is measured against time of

// last observed ack.

LeaseDuration time.Duration

And "A client observes timestamps captured locally to infer the state of the leader election" are both documented but an example would definitely make this clearer. We should also document reasonable values for LeaseDruation, RenewDeadline and RetryPeriod.

An ack is any change of the record observed by the client. The RenewTime could change to the previous day, and it would still reset the LeaseDuration countdown for non-leader clients waiting to become leader.

For a starting process, the countdown should start at the time that the initial record is read.

@mikedanese In a distributed system, time coherence in different nodes are hard.

@mikedanese I have updated the PR title and content to add more doc about this behavior. PTAL.

andyxning · 2019-05-14T03:18:50Z

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

mikedanese · 2019-05-14T17:11:23Z

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

@@ -22,6 +22,19 @@ limitations under the License.
 // leader (a.k.a. fencing). A client observes timestamps captured locally to


Maybe break the first sentence of this paragraph into the previous paragraph so that discussion on the finer points of timing is cleanly separated.

mikedanese · 2019-05-14T17:11:26Z

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

+// timestamps and cannot rely on the accuracy of timestamp in the record for
+// correctness.
+//
+// A client needs to wait a full LeaseDuration without observing a change


This is probably better placed in the LeaseDuration field documentation

andyxning · 2019-05-15T05:24:36Z

@mikedanese Done. PTAL.

andyxning · 2019-05-15T07:49:23Z

/test pull-kubernetes-kubemark-e2e-gce-big

andyxning · 2019-05-15T08:06:21Z

/test pull-kubernetes-kubemark-e2e-gce-big

andyxning · 2019-05-15T09:18:46Z

/retest

mikedanese · 2019-05-15T20:24:18Z

/lgtm
/approve

k8s-ci-robot · 2019-05-15T20:24:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyxning, mikedanese

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:

~~staging/src/k8s.io/client-go/tools/leaderelection/OWNERS~~ [mikedanese]

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

…n_start enhance leader election doc Kubernetes-commit: c2847e8

k8s-ci-robot requested review from resouer and wojtek-t May 8, 2019 08:43

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 8, 2019

neolit123 reviewed May 8, 2019

View reviewed changes

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels May 8, 2019

k8s-ci-robot assigned cheftako May 9, 2019

k8s-ci-robot requested a review from jpbetz May 9, 2019 20:08

andyxning force-pushed the fix_leader_election_start branch from 90fc423 to 6ee0ce4 Compare May 10, 2019 03:22

andyxning force-pushed the fix_leader_election_start branch from 6ee0ce4 to 3ec68da Compare May 10, 2019 04:33

k8s-ci-robot requested review from caesarxuchao, mikedanese and timothysc May 13, 2019 05:39

mikedanese reviewed May 13, 2019

View reviewed changes

mikedanese assigned mikedanese and unassigned cheftako May 13, 2019

andyxning force-pushed the fix_leader_election_start branch from 3ec68da to 49f60c8 Compare May 14, 2019 02:36

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 14, 2019

andyxning force-pushed the fix_leader_election_start branch 2 times, most recently from 87b3d05 to 7f1ddb0 Compare May 14, 2019 02:47

andyxning changed the title ~~fix leader election start~~ enhance leader election doc May 14, 2019

andyxning force-pushed the fix_leader_election_start branch from 7f1ddb0 to bebb548 Compare May 14, 2019 02:50

timothysc removed their request for review May 14, 2019 14:16

mikedanese reviewed May 14, 2019

View reviewed changes

andyxning force-pushed the fix_leader_election_start branch from bebb548 to c680306 Compare May 15, 2019 05:23

enhance leader election doc

95f33ce

andyxning force-pushed the fix_leader_election_start branch from c680306 to 95f33ce Compare May 15, 2019 10:51

mikedanese added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label May 15, 2019

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label May 15, 2019

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

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

k8s-ci-robot merged commit c2847e8 into kubernetes:master May 16, 2019

andyxning deleted the fix_leader_election_start branch May 19, 2019 01:47

nikhita pushed a commit to nikhita/kubernetes that referenced this pull request Jun 13, 2019

Merge pull request kubernetes#77585 from andyxning/fix_leader_electio…

0364099

…n_start enhance leader election doc Kubernetes-commit: c2847e8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhance leader election doc #77585

enhance leader election doc #77585

andyxning commented May 8, 2019 •

edited

andyxning commented May 8, 2019

neolit123 left a comment

jennybuckley commented May 9, 2019

andyxning commented May 10, 2019

andyxning commented May 10, 2019

neolit123 commented May 10, 2019

andyxning commented May 13, 2019

mikedanese May 13, 2019

andyxning May 13, 2019

andyxning May 13, 2019

mikedanese May 13, 2019

andyxning May 14, 2019

andyxning May 14, 2019

andyxning commented May 14, 2019

mikedanese May 14, 2019

andyxning May 15, 2019

mikedanese May 14, 2019

andyxning May 15, 2019

andyxning commented May 15, 2019

andyxning commented May 15, 2019

andyxning commented May 15, 2019

andyxning commented May 15, 2019

mikedanese commented May 15, 2019

k8s-ci-robot commented May 15, 2019

	// leader (a.k.a. fencing). A client observes timestamps captured locally to
	// infer the state of the leader election. Thus the implementation is tolerant
	// to arbitrary clock skew, but is not tolerant to arbitrary clock skew rate.

	// LeaseDuration is the duration that non-leader candidates will
	// wait to force acquire leadership. This is measured against time of
	// last observed ack.
	LeaseDuration time.Duration

		@@ -22,6 +22,19 @@ limitations under the License.
		// leader (a.k.a. fencing). A client observes timestamps captured locally to

enhance leader election doc #77585

enhance leader election doc #77585

Conversation

andyxning commented May 8, 2019 • edited

andyxning commented May 8, 2019

neolit123 left a comment

Choose a reason for hiding this comment

jennybuckley commented May 9, 2019

andyxning commented May 10, 2019

andyxning commented May 10, 2019

neolit123 commented May 10, 2019

andyxning commented May 13, 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

Choose a reason for hiding this comment

andyxning commented May 14, 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

andyxning commented May 15, 2019

andyxning commented May 15, 2019

andyxning commented May 15, 2019

andyxning commented May 15, 2019

mikedanese commented May 15, 2019

k8s-ci-robot commented May 15, 2019

andyxning commented May 8, 2019 •

edited