Cancellable leader election #57932

ash2k · 2018-01-06T09:26:59Z

What this PR does / why we need it:
Adds ability to cancel leader election. Useful in integration tests where the whole app is started and stopped in each test.

Special notes for your reviewer:
I used the context package - it is impossible/hard to achieve the same behaviour with just channels without spawning additional goroutines but it is trivial with context. See acquire() and renew() methods.

Release note:

NONE

/kind enhancement
/sig api-machinery

caesarxuchao · 2018-01-08T21:05:42Z

/assign @cheftako

ash2k · 2018-01-17T10:38:23Z

/retest

ash2k · 2018-01-19T20:44:26Z

/retest

ash2k · 2018-02-02T10:07:27Z

@kubernetes/sig-api-machinery-pr-reviews PTAL

timothysc · 2018-02-05T14:45:13Z

@ash2k where is your integration test that makes your case? It's pretty trivial to plumb signal-handlers through the stop channel and others have written code that depends on this external to k8s.

You're basically forcing everyone who uses this to change, so let's be certain to dot all the i's and cross all the t's.

ash2k · 2018-02-06T05:36:55Z

@timothysc There seem to be some misunderstanding here. I'm saying once you start the leader election, there is no way to ask it to stop. I.e. there is no way to make the method LeaderElector.Run() return. It only ever returns if it fails to renew the lease. I want to start and stop integration tests that start the whole application (in a Go test) with leader election enabled (because it is code that needs to be tested). But once the application does what it needs to do in the test, the test should be able to tell the application to cleanly shutdown. No way to do it right now.
You seem to be talking about plumbing a channel though to stop the application itself - that is not the problem I'm trying to solve here.

Examples of my integration tests can be found here https://github.com/atlassian/smith/tree/master/it
You probably want to take a look at the SetupApp() function that starts the application for each test and then does clean tear down.

timothysc · 2018-02-07T23:49:44Z

I'm ok with it, but you're changing common code that will affect multiple parties that use the client. I'm not certain how apimachinery folks communicate these changes, and what policies are in place nowadays.

/cc @sttts

sttts · 2018-02-12T08:09:04Z

Would be much easier to review if the conversion from stopCh to context would be separate.

ash2k · 2018-02-12T08:56:05Z

It would be harder to implemented the same logic with a stop channel - channels panic if closed more than once unlike the cancel() function that is used to cancel a context. So I would need to have some sort of locking to ensure the channel has been closed only once.
For example the renew() function would need to start an additional goroutine to receive from the stopCh and then close a temporary channel which would signal wait.Until() to stop. And that temporary channel would be used to cancel the wait from inside of the wait.Until(). So need a lock around that - could use sync.Once to do this. Temporary channel is needed because it cannot and must not close the stop channel that was passed to it.
All of that is not needed with a context and it would probably be harder to review.

sttts · 2018-02-12T09:00:40Z

All of that is not needed with a context and it would probably be harder to review.

I don't argue against a context :) just that we have two changes mixed into one commit here.

sttts · 2018-02-12T09:01:24Z

I don't argue against a context :)

Would like us to move to contexts much more throughout the code base.

ash2k · 2018-02-12T09:14:03Z

I understand and do not argue that it's two changes in 1 commit. My point is that it would not be easier to review. But if you insist I'll do it in another branch as an experiment. I'll post a link.
Yes, moving from stop channels to context would be nice.

sttts · 2018-02-12T09:17:44Z

But if you insist I'll do it in another branch as an experiment

Two commits in this PR would be fine.

In general, change sgtm.

ash2k · 2018-06-08T12:26:29Z

/retest

mikedanese · 2018-06-08T16:31:18Z

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

@@ -144,7 +145,7 @@ func Run(c *config.CompletedConfig) error {
 		}
 	}

-	run := func(stop <-chan struct{}) {
+	run := func(runCtx context.Context) {


same nit as above. s/runCtx/ctx/

It clashes with ctx, err := CreateControllerContext(c, rootClientBuilder, clientBuilder, runCtx.Done()) below.

I think that in general, variables of type context.Context prefer to be named ctx. Naming a "ControllerContext" struct variable ctx is probably worth changing so that readers don't get confused.

In fact, looking at the definition of ControllerContext, I don't think it is named correctly. It looks more like a config struct. (-:

mikedanese · 2018-06-08T16:32:42Z

One more comment.

/lgtm
/hold

misterikkit

It would have been nicer to add new functionality to leaderelect without requiring clients to change in the same PR. Each controller could be updated in a follow-up PR so that you don't get caught up in all the approvals needed.

Since the work is already done here, I won't push for this to be split up. See my comment about how I think it could have been done, though.

misterikkit · 2018-06-08T23:44:13Z

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

@@ -144,7 +145,7 @@ func Run(c *config.CompletedConfig) error {
 		}
 	}

-	run := func(stop <-chan struct{}) {
+	run := func(runCtx context.Context) {


I think that in general, variables of type context.Context prefer to be named ctx. Naming a "ControllerContext" struct variable ctx is probably worth changing so that readers don't get confused.

In fact, looking at the definition of ControllerContext, I don't think it is named correctly. It looks more like a config struct. (-:

misterikkit · 2018-06-08T23:48:49Z

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

 		le.maybeReportTransition()
 		if !succeeded {
 			glog.V(4).Infof("failed to acquire lease %v", desc)
 			return
 		}
 		le.config.Lock.RecordEvent("became leader")
 		glog.Infof("successfully acquired lease %v", desc)
-		close(stop)
-	}, le.config.RetryPeriod, JitterFactor, true, stop)
+		once.Do(func() { close(tmpStop) })


What is the meaning of tmpStop in this function? cancellation propagation?

JitterUntil may call this anonymous func multiple times... aha

Okay, tmpStop signals to JitterUntil that it has succeeded. I suggest renaming it to done. I also think that the entire wait library tends to result in unreadable code like this, when we could have just written a for loop.

misterikkit · 2018-06-08T23:57:21Z

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

 			glog.Fatalf("error running controllers: %v", err)
 		}
 	}

+	runCtx, cancel := context.WithCancel(context.Background())
+	defer cancel()


defer cancel() is a pretty common pattern with context.WithCancel. I think the expectation is that after creating the context, you are going to call some blocking Run func. If that func starts goroutines, it should pass the context (or child contexts) to them. So if Run ever exits, we will signal to any stray goroutines to clean up and exit.

misterikkit · 2018-06-09T00:01:32Z

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

@@ -146,28 +146,28 @@ type LeaderElector struct {
 }

 // Run starts the leader election loop
-func (le *LeaderElector) Run(stop <-chan struct{}) {
+func (le *LeaderElector) Run(ctx context.Context) {


You could also create an overload like this:
func (le *LeaderElector) Run() { le.Run(context.TODO()) }

Then updating clients to use the new thing would be much more graceful.

Go doesn't support overloading. We discussed adding WithContext style functions to maintain compatibility but the package is documented as having an alpha API. #57932 (review)

If people feel strongly we can do this, but I wouldn't fight for it.

Oops - I am still learning quite a bit about golang.

I am not concerned with backward compatibility, but I thought the author's life would have been easier by temporarily preserving backward compatibility.

mikedanese · 2018-06-11T21:16:42Z

/hold cancel
/lgtm

k8s-ci-robot · 2018-06-11T21:16:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, 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/cloud-controller-manager/OWNERS~~ [mikedanese]
~~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

smarterclayton · 2018-06-13T05:54:56Z

This is too late for 1.11 given the potential for regression. I'm marking at as 1.12 to be explicit with that.

fejta-bot · 2018-06-13T09:01:24Z

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

fejta-bot · 2018-06-17T10:39:55Z

/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-06-20T00:25:51Z

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@ash2k @cheftako @deads2k @fgrzadkowski @mikedanese @pmorie

Action required: This pull request requires label changes. If the required changes are not made within 3 days, the pull request will be moved out of the v1.12 milestone.

priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.

Help

k8s-github-robot · 2018-06-21T00:22:19Z

Automatic merge from submit-queue (batch tested with PRs 65256, 64236, 64919, 64879, 57932). If you want to cherry-pick this change to another branch, please follow the instructions here.

@sttts

Automatic merge from submit-queue (batch tested with PRs 60012, 63692, 63977, 63960, 64008). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Use Dial with context **What this PR does / why we need it**: `net/http/Transport.Dial` field is deprecated: ```go // DialContext specifies the dial function for creating unencrypted TCP connections. // If DialContext is nil (and the deprecated Dial below is also nil), // then the transport dials using package net. DialContext func(ctx context.Context, network, addr string) (net.Conn, error) // Dial specifies the dial function for creating unencrypted TCP connections. // // Deprecated: Use DialContext instead, which allows the transport // to cancel dials as soon as they are no longer needed. // If both are set, DialContext takes priority. Dial func(network, addr string) (net.Conn, error) ``` This PR switches all `Dial` usages to `DialContext`. Fixes #63455. **Special notes for your reviewer**: Also related: kubernetes/kubernetes#59287 kubernetes/kubernetes#58532 kubernetes/kubernetes#815 kubernetes/community#1166 kubernetes/kubernetes#58677 kubernetes/kubernetes#57932 **Release note**: ```release-note HTTP transport now uses `context.Context` to cancel dial operations. k8s.io/client-go/transport/Config struct has been updated to accept a function with a `context.Context` parameter. This is a breaking change if you use this field in your code. ``` /sig api-machinery /kind enhancement /cc @sttts Kubernetes-commit: ddf551c24b7d88454f8332ce6855e53281440958

k8s-github-robot assigned pmorie and fgrzadkowski Jan 6, 2018

k8s-ci-robot assigned cheftako Jan 8, 2018

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

ash2k force-pushed the cancellable-leader-election branch from a67cfa9 to b013fef Compare January 17, 2018 09:41

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

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

ash2k force-pushed the cancellable-leader-election branch from b013fef to ddf6fdb Compare January 19, 2018 11:48

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

ash2k force-pushed the cancellable-leader-election branch from ddf6fdb to fdd468d Compare February 1, 2018 10:15

timothysc self-assigned this Feb 5, 2018

k8s-ci-robot requested a review from sttts February 7, 2018 23:49

mikedanese reviewed Jun 8, 2018

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 8, 2018

misterikkit reviewed Jun 9, 2018

View reviewed changes

Rename context

e458cfe

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

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 11, 2018

smarterclayton added this to the v1.12 milestone Jun 13, 2018

k8s-github-robot added the milestone/incomplete-labels label Jun 20, 2018

k8s-github-robot merged commit 571b9be into kubernetes:master Jun 21, 2018

ash2k deleted the cancellable-leader-election branch June 21, 2018 00:56

ash2k mentioned this pull request Jul 2, 2018

leader election: fix dead loop bug #65667

Merged

karlhungus mentioned this pull request Jul 5, 2018

Leader Election: ceding leadership, leading on renew callback #62120

Closed

wgliang mentioned this pull request Jul 6, 2018

[bug] garbagecollector not start successfully #65894

Closed

ash2k mentioned this pull request Oct 11, 2018

Kubernetes 1.12 libraries atlassian-archive/ctrl#35

Merged

fisherxu mentioned this pull request Dec 19, 2018

Enable LeaderElect for federation controller kubernetes-retired/kubefed#394

Closed

ravisantoshgudimetla mentioned this pull request Aug 13, 2019

Scheduler should terminate on loosing leader lock #81306

Merged

negz mentioned this pull request Mar 14, 2020

UMBRELLA: design and refactor graceful termination kubernetes-sigs/controller-runtime#764

Closed

manusa mentioned this pull request Aug 9, 2022

Missing cancellation support in LeaderElector fabric8io/kubernetes-client#2207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cancellable leader election #57932

Cancellable leader election #57932

ash2k commented Jan 6, 2018

caesarxuchao commented Jan 8, 2018

ash2k commented Jan 17, 2018

ash2k commented Jan 19, 2018

ash2k commented Feb 2, 2018

timothysc commented Feb 5, 2018 •

edited

Loading

ash2k commented Feb 6, 2018

timothysc commented Feb 7, 2018

sttts commented Feb 12, 2018

ash2k commented Feb 12, 2018

sttts commented Feb 12, 2018

sttts commented Feb 12, 2018

ash2k commented Feb 12, 2018

sttts commented Feb 12, 2018

ash2k commented Jun 8, 2018

mikedanese Jun 8, 2018

ash2k Jun 8, 2018

misterikkit Jun 8, 2018

ash2k Jun 9, 2018

mikedanese commented Jun 8, 2018

misterikkit left a comment

misterikkit Jun 8, 2018

misterikkit Jun 8, 2018

misterikkit Jun 8, 2018

misterikkit Jun 9, 2018

mikedanese Jun 11, 2018

misterikkit Jun 11, 2018

mikedanese commented Jun 11, 2018

k8s-ci-robot commented Jun 11, 2018

smarterclayton commented Jun 13, 2018

fejta-bot commented Jun 13, 2018

fejta-bot commented Jun 17, 2018

k8s-github-robot commented Jun 20, 2018

k8s-github-robot commented Jun 21, 2018

Cancellable leader election #57932

Cancellable leader election #57932

Conversation

ash2k commented Jan 6, 2018

caesarxuchao commented Jan 8, 2018

ash2k commented Jan 17, 2018

ash2k commented Jan 19, 2018

ash2k commented Feb 2, 2018

timothysc commented Feb 5, 2018 • edited Loading

ash2k commented Feb 6, 2018

timothysc commented Feb 7, 2018

sttts commented Feb 12, 2018

ash2k commented Feb 12, 2018

sttts commented Feb 12, 2018

sttts commented Feb 12, 2018

ash2k commented Feb 12, 2018

sttts commented Feb 12, 2018

ash2k commented Jun 8, 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

mikedanese commented Jun 8, 2018

misterikkit 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese commented Jun 11, 2018

k8s-ci-robot commented Jun 11, 2018

smarterclayton commented Jun 13, 2018

fejta-bot commented Jun 13, 2018

fejta-bot commented Jun 17, 2018

k8s-github-robot commented Jun 20, 2018

k8s-github-robot commented Jun 21, 2018

timothysc commented Feb 5, 2018 •

edited

Loading