set leader election client and renew timeout #65094

hzxuzhonghu · 2018-06-14T11:10:56Z

What this PR does / why we need it:

set leader-election client timeout

set timeout for tryAcquireOrRenew

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

Special notes for your reviewer:

Release note:

NONE

hzxuzhonghu · 2018-06-14T11:15:41Z

/assign @mikedanese @timothysc

/cc @kubernetes/api-reviewers

hzxuzhonghu · 2018-06-20T08:15:26Z

/cc @deads2k @liggitt @lavalamp @caesarxuchao

mikedanese · 2018-06-20T16:20:50Z

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

@@ -201,7 +201,19 @@ func (le *LeaderElector) renew() {
 	stop := make(chan struct{})
 	wait.Until(func() {
 		err := wait.Poll(le.config.RetryPeriod, le.config.RenewDeadline, func() (bool, error) {


Poll already retries and timesout. What does this do differently?

Indeed wait.Poll check timeout before running ConditionFunc. If ConditionFunc takes too long, it can not quit quickly.

Ahhh, gross. Ok, let me think about this.

mikedanese · 2018-06-22T18:28:13Z

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

-	leaderElectionClient := clientset.NewForConfigOrDie(restclient.AddUserAgent(kubeconfig, "leader-election"))
+	// shallow copy, do not modify the kubeconfig.Timeout.
+	config := *kubeconfig
+	config.Timeout = s.GenericComponent.LeaderElection.RenewDeadline.Duration


Would it be sufficient to set this to RetryPeriod? I think then we can get away without the other timeout.

No, this timeout is to set for http client, used to prevent blocking forever.

I understand what it is used for. If we retry every RetryPeriod anyway, why would we want a client to block for longer than RetryPeriod?

In general, the timeout less than RetryPeriod can work well. But if the network is unstable, and it takes longer to finish the renew. During this period, this client loses leadership.

This is my own thought. Would not mind different voice.

mikedanese · 2018-06-22T18:29:08Z

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

-			return le.tryAcquireOrRenew(), nil
+			var succeed bool
+			done := make(chan struct{})
+			go func() {


These goroutines are going to leak. This concerns me.

After setting timeout, the goroutine can definitely exit with at most less than twice RenewDeadline time.

awly · 2018-06-27T01:20:56Z

cmd/kube-scheduler/app/options/options.go

@@ -289,7 +290,9 @@ func createClients(config componentconfig.ClientConnectionConfiguration, masterO
 		return nil, nil, nil, err
 	}

-	leaderElectionClient, err := clientset.NewForConfig(restclient.AddUserAgent(kubeConfig, "leader-election"))
+	restConfig := *restclient.AddUserAgent(kubeConfig, "leader-election")
+	restConfig.Timeout = timeout


You set Timeout on return of restclient.AddUserAgent here and on kubeconfig in cmd/kube-controller-manager/app/options/options.go
Make them consistent

awly · 2018-06-27T01:22:43Z

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

+			done := make(chan struct{})
+			go func() {
+				defer close(done)
+				succeed = le.tryAcquireOrRenew()


How about making done = make(chan bool, 1) and sending result onto it and removing succeed?

This can also achieve the same effect, but I think it is not direct.

I would argue this is more direct.
And avoids the potential of racing on the succeed var. Access is synchronized on done channel right now but my first reaction was "it's read and written to concurrently".

you persuaded me

awly · 2018-06-27T01:23:06Z

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

+
+			select {
+			case <-time.After(le.config.LeaseDuration - le.config.RetryPeriod):
+				return false, fmt.Errorf("timeout trying acquire or renew")


Add timeout duration to the message

awly · 2018-06-27T01:25:40Z

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

+			}()
+
+			select {
+			case <-time.After(le.config.LeaseDuration - le.config.RetryPeriod):


Why not re-use timeoutCtx.Done()?

hzxuzhonghu · 2018-06-27T03:45:33Z

/retest

awly · 2018-06-27T17:03:23Z

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

+
+			select {
+			case <-timeoutCtx.Done():
+				return false, fmt.Errorf("tryAcquireOrRenew timed out after %s", le.config.RenewDeadline)


Hmm, now that this is based on the context, add timeoutCtx.Err() to the message.
The parent ctx passed to renew could've been cancelled.

awly · 2018-06-27T17:04:37Z

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

+			done := make(chan struct{})
+			go func() {
+				defer close(done)
+				succeed = le.tryAcquireOrRenew()


I would argue this is more direct.
And avoids the potential of racing on the succeed var. Access is synchronized on done channel right now but my first reaction was "it's read and written to concurrently".

awly · 2018-06-29T16:05:46Z

/lgtm

mikedanese · 2018-06-29T16:08:29Z

/approve

mikedanese · 2018-06-29T16:13:38Z

/hold
needs a squash

hzxuzhonghu · 2018-06-30T02:40:08Z

@awly @mikedanese squashed the last two commits

awly · 2018-07-02T15:40:27Z

/lgtm
@mikedanese for re-approval

k8s-ci-robot · 2018-07-02T15:40:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awly, hzxuzhonghu, 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:

~~cmd/kube-controller-manager/OWNERS~~ [mikedanese]
~~cmd/kube-scheduler/OWNERS~~ [mikedanese]
~~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

mikedanese · 2018-07-02T17:41:54Z

/hold cancel

fejta-bot · 2018-07-02T22:20:42Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-07-03T01:19:05Z

Automatic merge from submit-queue (batch tested with PRs 65094, 65533, 63522, 65694, 65702). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 14, 2018

k8s-ci-robot requested review from davidopp and markturansky June 14, 2018 11:11

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Jun 14, 2018

k8s-ci-robot assigned mikedanese and timothysc Jun 14, 2018

hzxuzhonghu mentioned this pull request Jun 20, 2018

Antiaffinity pods scheduled to the same node during scheduler leader-election #65257

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2018

hzxuzhonghu force-pushed the le-client-timeout branch from 6751dbd to 2dc249b Compare June 20, 2018 08:08

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2018

k8s-ci-robot requested review from caesarxuchao, deads2k, lavalamp and liggitt June 20, 2018 08:15

hzxuzhonghu changed the title ~~set leader election client timeout~~ set leader election client and renew timeout Jun 20, 2018

mikedanese reviewed Jun 20, 2018

View reviewed changes

hzxuzhonghu force-pushed the le-client-timeout branch from 2dc249b to 256f6bf Compare June 21, 2018 03:05

mikedanese reviewed Jun 22, 2018

View reviewed changes

timothysc removed their assignment Jun 25, 2018

mikedanese assigned awly and mikedanese and unassigned mikedanese Jun 26, 2018

awly reviewed Jun 27, 2018

View reviewed changes

hzxuzhonghu force-pushed the le-client-timeout branch from 256f6bf to 1ea4560 Compare June 27, 2018 02:51

k8s-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 27, 2018

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 27, 2018

awly reviewed Jun 27, 2018

View reviewed changes

set leader election client timeout

7c6213e

hzxuzhonghu force-pushed the le-client-timeout branch from 1ea4560 to d344648 Compare June 29, 2018 02:33

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 29, 2018

hzxuzhonghu force-pushed the le-client-timeout branch from 7b6080c to ff847bd Compare June 29, 2018 05:58

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

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

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 29, 2018

leaderelection: set timeout for tryAcquireOrRenew

90b287c

hzxuzhonghu force-pushed the le-client-timeout branch from ff847bd to 90b287c Compare June 30, 2018 02:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 30, 2018

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2018

k8s-github-robot merged commit 47020f3 into kubernetes:master Jul 3, 2018

hzxuzhonghu deleted the le-client-timeout branch July 3, 2018 01:19

set leader election client and renew timeout #65094

set leader election client and renew timeout #65094

Conversation

hzxuzhonghu commented Jun 14, 2018 • edited

hzxuzhonghu commented Jun 14, 2018

hzxuzhonghu commented Jun 20, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese Jun 22, 2018 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awly commented Jun 29, 2018

mikedanese commented Jun 29, 2018

mikedanese commented Jun 29, 2018

hzxuzhonghu commented Jun 30, 2018

awly commented Jul 2, 2018

k8s-ci-robot commented Jul 2, 2018

mikedanese commented Jul 2, 2018

fejta-bot commented Jul 2, 2018

k8s-github-robot commented Jul 3, 2018

hzxuzhonghu commented Jun 14, 2018 •

edited

mikedanese Jun 22, 2018 •

edited