✨ added per controller leader election #444

GrigoriyMikhalkin · 2019-05-25T16:42:05Z

Attempt to solve #364

GrigoriyMikhalkin · 2019-05-25T17:01:22Z

mengqiy

We should add some validation to check if there are 2 different controller or manager using the same LE namespace and ID when starting the manager.
We may need to add a new method in the controller interface to get the LE config.

mengqiy · 2019-05-30T00:26:52Z

pkg/controller/controller.go

 	"sigs.k8s.io/controller-runtime/pkg/source"
 )

+const (
+	// Values taken from: https://github.com/kubernetes/apiserver/blob/master/pkg/apis/config/v1alpha1/defaults.go
+	defaultLeaseDuration = 15 * time.Second


These are shared bits between controller and manager packages and they can potentially moved to sigs.k8s.io/controller-runtime/pkg/leaderelection/internal.

GrigoriyMikhalkin · 2019-05-30T19:34:25Z

We should add some validation to check if there are 2 different controller or manager using the same LE namespace and ID when starting the manager.

@mengqiy You mean, check duplicate LE lock names used by manager and controllers that it manages? Or you mean totally unrelated managers and controllers? If later, i'm not sure how we can check that LE lock is created by different controller or manager.

mengqiy · 2019-05-30T19:50:32Z

You mean, check duplicate LE lock names used by manager and controllers that it manages? Or you mean totally unrelated managers and controllers? If later, i'm not sure how we can check that LE lock is created by different controller or manager.

I mean the former.

DirectXMan12

this all functions somewhat awkwardly with the "needs leader election" logic we've just implemented. I think what we really want to do is let things that request leader election (via the interface) return an "id suffix", then have the manager take care of launching the leader election.

DirectXMan12 · 2019-05-31T21:25:31Z

that would also allow the manager to check for duplicate ids within a given manager

GrigoriyMikhalkin · 2019-06-01T15:36:51Z

@mengqiy @DirectXMan12 Completely rewrote code. Added new runnable interface(called it SingletonRunnable, probably not the best naming). Now manager takes care of leader election for controllers, also checks that there's no duplicate LE IDs.

DirectXMan12

we already have the LeaderElectionRunnable interface, we should be able to use that.

GrigoriyMikhalkin · 2019-06-04T04:36:37Z

we already have the LeaderElectionRunnable interface, we should be able to use that.

@DirectXMan12 LeaderElectionRunnable interface used for checking if runnable have to be run before or after manager was elected. Are you implying that we can add GetLeaderElectionID method to it and use NeedLeaderElection to determine if need to perform leader election on runnable? It seems like it can be a breaking change for some users(who implemented they own runnables), would it be ok?

DirectXMan12 · 2019-06-05T23:04:36Z

interface used for checking if runnable have to be run before or after manager was elected

Correct -- "run before" is equivalent to "don't need leader election".

We could actually just have NeedLeaderElection return an ID as well as a bool (or we could have the separate "GetID" method).

It seems like it can be a breaking change for some users(who implemented they own runnables), would it be ok

We're doing breaking changes this release anyway, but we could still have the default be to run under some standard leader election ID from the manager.

DirectXMan12

I think this is going in the right direction, but I think you need to pull out the global leader lock, which isn't serving any purpose anymore.

DirectXMan12 · 2019-06-13T20:07:26Z

pkg/manager/internal.go

+	if resourceLock := cm.resourceLocksMap[cm.leaderElectionID]; resourceLock != nil {
+		err := cm.startLeaderElection(cm.leaderElectionID, leaderelection.LeaderCallbacks{
+			OnStartedLeading: func(_ context.Context) {
+				cm.startLeaderElectionRunnables()


I don't think we need this double-leader election here -- in fact, it kinda defeats the purpose of per-controller leader election.

Really, you want to try to start all of the locks independently.

DirectXMan12 · 2019-06-13T20:09:04Z

pkg/manager/internal.go

+
+				// Check that leader election ID is unique
+				if _, exists := cm.resourceLocksMap[leID]; exists {
+					cm.errChan <- errors.New("LeaderElectionID must be unique")


that's not entirely true -- a group of controllers could choose to run under the same leader election

(which might make sense if they needed the same resources, etc.

GrigoriyMikhalkin · 2019-06-17T05:59:31Z

Updated. Now there's no global leader lock. But i use sameLeaderElectionNamespace(and different duration parameters) for all per controller locks, is that fine? IMHO, i think it is, seems like specifying different namespaces for locks is redundant.

mengqiy · 2019-06-17T17:24:44Z

But i use same LeaderElectionNamespace (and different duration parameters) for all per controller locks, is that fine? IMHO, i think it is, seems like specifying different namespaces for locks is redundant.

How to handle LE namespace is debatable. Using the same LeaderElectionNamespace probably is not flexible enough. What if some controllers want to do LE in the same namespace as the manager and some controllers want to do LE in a dedicated namespace e.g. leader-election-system or kube-system?
IMHO we can make GetID return namespace and object ID or have another function for getting namespace e.g. GetNamespace(). If getting an empty namespace, we still default it to the same namespace as the manager.

@DirectXMan12 WDYT?

DirectXMan12 · 2020-02-05T23:51:43Z

Outcome of the f2f at kubecon was to support this, but default to manager's LE ID. We need to rebase this and make sure that's the state it's in. I'll take a poke at it.

vincepri · 2020-02-20T18:47:29Z

@GrigoriyMikhalkin are you still interested in working on this change?

GrigoriyMikhalkin · 2020-02-20T19:14:07Z

@GrigoriyMikhalkin are you still interested in working on this change?

Yes. I'm gonna update this PR this weekend.

vincepri · 2020-02-20T19:53:17Z

Thank you!

GrigoriyMikhalkin · 2020-02-23T07:24:16Z

Outcome of the f2f at kubecon was to support this, but default to manager's LE ID.

@DirectXMan12 So, by default, we want to make leader election for controllers?

k8s-ci-robot · 2020-02-23T08:26:48Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: GrigoriyMikhalkin
To complete the pull request process, please assign directxman12
You can assign the PR to them by writing /assign @directxman12 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

DirectXMan12 · 2020-02-26T20:20:09Z

Basically, by default, keep the current behavior unless explicitly overridden by the controller.

GrigoriyMikhalkin · 2020-02-27T18:35:16Z

Updated PR. Now LeaderElectionRunnable should implement GetLeaderElectionMode method, which returns one of (Non, per-manager, per-controller) leader election modes. Controllers, by default, use per-manager mode(current behavior).

@DirectXMan12, @vincepri please review

fejta-bot · 2020-06-09T22:04:59Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

k8s-ci-robot · 2020-06-09T22:05:06Z

@GrigoriyMikhalkin: PR needs rebase.

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.

GrigoriyMikhalkin · 2020-06-10T06:33:14Z

Planning to fix conflicts this weekend. Is there still plans to merge this?

alvaroaleman · 2020-06-10T13:14:30Z

pkg/manager/manager.go

-	// e.g. controllers need to be run in leader election mode, while webhook server doesn't.
-	NeedLeaderElection() bool
+	// GetLeaderElectionMode returns leader election mode in which Runnable needs to be run.
+	GetLeaderElectionMode() leaderelection.Mode


We can not do this. This will break all existing LeaderElection runnables in a way that they won't work anymore but without throwing any kind of compile error.

The current interface must stay as it is and remain having the same result as it has today. We can probably add a new interface and check for both.

fejta-bot · 2020-07-10T14:02:00Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2020-08-09T14:43:14Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2020-08-09T14:43:21Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

k8s-ci-robot requested review from DirectXMan12 and pwittrock May 25, 2019 16:42

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 25, 2019

GrigoriyMikhalkin changed the title ~~✨added leader election per controller~~ ✨added per controller leader election May 25, 2019

GrigoriyMikhalkin force-pushed the CR-364 branch from 64c22b1 to 84122d0 Compare May 25, 2019 16:51

k8s-ci-robot assigned droot May 25, 2019

coderanger mentioned this pull request May 25, 2019

Remove defaulting for leader election ID #445

Closed

mengqiy reviewed May 30, 2019

View reviewed changes

DirectXMan12 suggested changes May 31, 2019

View reviewed changes

GrigoriyMikhalkin force-pushed the CR-364 branch from 84122d0 to c2243f7 Compare June 1, 2019 15:32

DirectXMan12 suggested changes Jun 4, 2019

View reviewed changes

GrigoriyMikhalkin force-pushed the CR-364 branch 2 times, most recently from 2a0249f to 62aa08d Compare June 9, 2019 08:52

GrigoriyMikhalkin changed the title ~~✨added per controller leader election~~ ✨ added per controller leader election Jun 9, 2019

DirectXMan12 suggested changes Jun 13, 2019

View reviewed changes

DirectXMan12 mentioned this pull request Jun 13, 2019

🐛 fix runnable run twice issue #487

Merged

GrigoriyMikhalkin force-pushed the CR-364 branch 2 times, most recently from b490d36 to 8dfd21a Compare June 17, 2019 05:57

GrigoriyMikhalkin force-pushed the CR-364 branch from 8dfd21a to a23fe42 Compare June 17, 2019 06:08

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

DirectXMan12 assigned vincepri Feb 5, 2020

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

GrigoriyMikhalkin force-pushed the CR-364 branch from b2a1572 to 3dfb42d Compare February 23, 2020 08:26

k8s-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 23, 2020

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 23, 2020

GrigoriyMikhalkin added 2 commits March 11, 2020 22:34

added leader election for runnables

ff25f2d

added leader election modes

0a1076a

GrigoriyMikhalkin force-pushed the CR-364 branch from f3c711c to 0a1076a Compare March 11, 2020 21:35

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 9, 2020

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

alvaroaleman reviewed Jun 10, 2020

View reviewed changes

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 10, 2020

k8s-ci-robot closed this Aug 9, 2020

GrigoriyMikhalkin mentioned this pull request Jan 31, 2022

Per controller leader election? timebertt/kubernetes-controller-sharding#1

Closed

✨ added per controller leader election #444

✨ added per controller leader election #444

Uh oh!

Conversation

GrigoriyMikhalkin commented May 25, 2019

Uh oh!

GrigoriyMikhalkin commented May 25, 2019

Uh oh!

mengqiy left a comment

Choose a reason for hiding this comment

Uh oh!

mengqiy May 30, 2019

Choose a reason for hiding this comment

Uh oh!

GrigoriyMikhalkin commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mengqiy commented May 30, 2019

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 commented May 31, 2019

Uh oh!

GrigoriyMikhalkin commented Jun 1, 2019

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

GrigoriyMikhalkin commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DirectXMan12 commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Jun 13, 2019

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Jun 13, 2019

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Jun 13, 2019

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Jun 13, 2019

Choose a reason for hiding this comment

Uh oh!

GrigoriyMikhalkin commented Jun 17, 2019

Uh oh!

mengqiy commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DirectXMan12 commented Feb 5, 2020

Uh oh!

vincepri commented Feb 20, 2020

Uh oh!

GrigoriyMikhalkin commented Feb 20, 2020

Uh oh!

vincepri commented Feb 20, 2020

Uh oh!

GrigoriyMikhalkin commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 23, 2020

Uh oh!

DirectXMan12 commented Feb 26, 2020

Uh oh!

GrigoriyMikhalkin commented Feb 27, 2020

Uh oh!

fejta-bot commented Jun 9, 2020

Uh oh!

k8s-ci-robot commented Jun 9, 2020

Uh oh!

GrigoriyMikhalkin commented Jun 10, 2020

Uh oh!

alvaroaleman Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GrigoriyMikhalkin commented May 30, 2019 •

edited

Loading

GrigoriyMikhalkin commented Jun 4, 2019 •

edited

Loading

DirectXMan12 commented Jun 5, 2019 •

edited

Loading

mengqiy commented Jun 17, 2019 •

edited

Loading

GrigoriyMikhalkin commented Feb 23, 2020 •

edited

Loading

alvaroaleman Jun 10, 2020 •

edited

Loading