Add prioritized leader election #103442

howardjohn · 2021-07-02T15:29:33Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This introduces a new concept to the leaderelection package, allowing
participants to declare a precedence order between certain clients.

Example use cases:

I have multiple versions of an application running concurrently in the
cluster; I want the newest one to become the leader (cc @Monkeyanator)
I have multiple replicas with different resources. I want the largest
replica to be the leader
I am implementing a cross-region controller (think multicluster
service discovery) and the geographically closest region to be the
leader (to avoid something like a US controller acting on a cluster in
Asia, when I have a controller replica already running in Asia) (cc @JeremyOT)

Library users can define an arbitrary key to indicate whatever metadata
they choose. They may also choose to implement a KeyComparison function.
If present, this function will be used to determine if our key is higher
priority than the existing leader; if it is, we will acquire the lock
even though its already held by another.

Version skew compatibility: old clients will not set a key, nor will
they implement a KeyComparison function. If they see a lock with a key
set, they will gracefully ignore it, which is not an issue - this just
means they will never steal a lock. Because the key is empty, a
KeyComparison function must account for "" as a key value, but they
would need to do that regardless as the field is optional. As a result,
I do not expect any version skew issues.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added a new `KeyComparison` feature to the client-go leaderelection package to allow prioritized leader elections.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-07-02T15:29:41Z

Hi @howardjohn. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

nmittler · 2021-07-02T15:34:03Z

@JeremyOT FYI

BenTheElder · 2021-07-02T19:53:19Z

/ok-to-test
FYI I'm pretty sure you've been active enough to apply for https://github.com/kubernetes/community/blob/master/community-membership.md and get automatic ok-to-test

dims · 2021-07-06T16:43:47Z

/assign @deads2k @sttts

caesarxuchao · 2021-07-08T20:08:55Z

/triage accepted
/assgin @jiahuif

jiahuif · 2021-07-09T15:26:16Z

Could you add tests where KeyComparison is set?

This introduces a new concept to the `leaderelection` package, allowing participants to declare a precedence order between certain clients. Example use cases: * I have multiple versions of an application running concurrently in the cluster; I want the newest one to become the leader * I have multiple replicas with different resources. I want the largest replica to be the leader * I am implementing a cross-region controller (think multicluster service discovery) and the geographically closest region to be the leader (to avoid something like a US controller acting on a cluster in Asia, when I have a controller replica already running in Asia) Library users can define an arbitrary key to indicate whatever metadata they choose. They may also choose to implement a KeyComparison function. If present, this function will be used to determine if our key is higher priority than the existing leader; if it is, we will acquire the lock even though its already held by another. Version skew compatibility: old clients will not set a key, nor will they implement a KeyComparison function. If they see a lock with a key set, they will gracefully ignore it, which is not an issue - this just means they will never steal a lock. Because the key is empty, a KeyComparison function must account for `""` as a key value, but they would need to do that regardless as the field is optional. As a result, I do not expect any version skew issues.

howardjohn · 2021-07-12T18:27:24Z

Could you add tests where KeyComparison is set?

Yes, I have added them and updated the PR

k8s-ci-robot · 2021-07-12T18:27:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: howardjohn
To complete the pull request process, please ask for approval from deads2k after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

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

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 · 2021-07-12T19:07:46Z

@howardjohn: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-unit	`67fa728`	link	`/test pull-kubernetes-unit`
pull-kubernetes-integration	`67fa728`	link	`/test pull-kubernetes-integration`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

howardjohn · 2021-07-12T23:26:51Z

After adding tests, realized this doesn't work with Lease lock, since we need the API change to staging/src/k8s.io/api/coordination/v1/types.go. Possible options:

Change the API
Use an annotation to store the field
Do not support Lease

I am open to any of the three; (1) seems right but I don't know how much we are able to modify v1 api

howardjohn · 2021-07-19T20:25:22Z

@jiahuif any feedback on #103442 (comment)? If not I will go with (3)

howardjohn · 2021-07-23T22:42:40Z

Ping on #103442 (comment)? Thanks!

jiahuif · 2021-07-23T23:18:48Z

@howardjohn Sorry for missing this out. option 3 will greatly limits the usefulness of this improvement because Lease is mostly used type of leader election. Because you are about to change a stable API, I believe it is better to write a KEP to purpose the feature in a more formal way. See https://github.com/kubernetes/enhancements/tree/master/keps

jiahuif · 2021-07-23T23:19:51Z

Also, preemptible leader lock is entirely new, which I believe requires a KEP anyways.

howardjohn · 2021-07-26T22:46:23Z

@jiahuif I have opened kubernetes/enhancements#2835

yuzhiquan · 2021-07-27T01:21:55Z

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

@@ -120,3 +120,8 @@ func (cml *ConfigMapLock) Describe() string {
 func (cml *ConfigMapLock) Identity() string {
 	return cml.LockConfig.Identity
 }
+
+// Identity returns the Identity of the lock


Suggested change

// Identity returns the Identity of the lock

// Key returns the Key of the lock

yuzhiquan · 2021-07-27T01:23:38Z

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

+			// We will pre-empt the existing leader.
+			klog.V(4).Infof("lock is held by %v with key %v, but our key (%v) evicts it", oldLeaderElectionRecord.HolderIdentity, oldLeaderElectionRecord.HolderKey, le.config.Lock.Key())
+		} else {
+			klog.V(4).Infof("lock is held by %v and has not yet expired", oldLeaderElectionRecord.HolderIdentity)


Suggested change

klog.V(4).Infof("lock is held by %v and has not yet expired", oldLeaderElectionRecord.HolderIdentity)

klog.V(4).InfoS("lock is held and has not yet expired", "holderIdentity", oldLeaderElectionRecord.HolderIdentity)

k8s-ci-robot · 2021-09-17T16:36:54Z

@howardjohn: 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.

Monkeyanator · 2021-09-24T18:18:45Z

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

@@ -101,6 +101,11 @@ func (ll *LeaseLock) Identity() string {
 	return ll.LockConfig.Identity
 }

+// Key returns the Key of the lock
+func (ll *LeaseLock) Key() string {


Calling Update() or Create() on the LeaseLock leaves out the HolderKey. Lease lock is different from the rest of the resource lock impls since it uses the lease spec instead of cramming all the fields in an annotation so we need to find a new place to put the key (putting HolderKey and priority as a concept in v1.Lease seems wrong but I'm not super clear on what lease gets used for in k8s besides kubelet heartbeats).

k8s-triage-robot · 2021-12-23T18:46:35Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

dims · 2022-01-10T17:05:55Z

Is this PR still needed, please rebase if so (or we can close it?)

k8s-triage-robot · 2022-02-09T17:42:18Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2022-03-11T18:27:56Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2022-03-11T18:28:15Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

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

nmittler · 2022-03-14T16:14:30Z

@howardjohn is this still active? Should it be re-opened?

howardjohn · 2022-03-14T16:19:18Z

@nmittler currently in Istio we have forked the leader election library. kubernetes/enhancements#2835 has been scope-creeped beyond the initial requirements, moving it beyond my bandwidth + expertise. If we can drop those additional requirements or get someone else involved as well I think we could progress this.

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 2, 2021

k8s-ci-robot requested review from ingvagabund and resouer July 2, 2021 15:30

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 2, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 2, 2021

k8s-ci-robot assigned deads2k and sttts Jul 6, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 8, 2021

howardjohn force-pushed the leader/prioritized branch from b9338f4 to 67fa728 Compare July 12, 2021 18:27

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 12, 2021

yuzhiquan reviewed Jul 27, 2021

View reviewed changes

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

Monkeyanator reviewed Sep 24, 2021

View reviewed changes

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

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 Feb 9, 2022

k8s-ci-robot closed this Mar 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prioritized leader election #103442

Add prioritized leader election #103442

howardjohn commented Jul 2, 2021 •

edited

Loading

k8s-ci-robot commented Jul 2, 2021

nmittler commented Jul 2, 2021

BenTheElder commented Jul 2, 2021

dims commented Jul 6, 2021

caesarxuchao commented Jul 8, 2021

jiahuif commented Jul 9, 2021

howardjohn commented Jul 12, 2021

k8s-ci-robot commented Jul 12, 2021

k8s-ci-robot commented Jul 12, 2021 •

edited

Loading

howardjohn commented Jul 12, 2021

howardjohn commented Jul 19, 2021

howardjohn commented Jul 23, 2021

jiahuif commented Jul 23, 2021

jiahuif commented Jul 23, 2021

howardjohn commented Jul 26, 2021

yuzhiquan Jul 27, 2021

yuzhiquan Jul 27, 2021

k8s-ci-robot commented Sep 17, 2021

Monkeyanator Sep 24, 2021

k8s-triage-robot commented Dec 23, 2021

dims commented Jan 10, 2022

k8s-triage-robot commented Feb 9, 2022

k8s-triage-robot commented Mar 11, 2022

k8s-ci-robot commented Mar 11, 2022

nmittler commented Mar 14, 2022

howardjohn commented Mar 14, 2022

	// Identity returns the Identity of the lock
	// Key returns the Key of the lock

	klog.V(4).Infof("lock is held by %v and has not yet expired", oldLeaderElectionRecord.HolderIdentity)
	klog.V(4).InfoS("lock is held and has not yet expired", "holderIdentity", oldLeaderElectionRecord.HolderIdentity)

Add prioritized leader election #103442

Add prioritized leader election #103442

Conversation

howardjohn commented Jul 2, 2021 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Jul 2, 2021

nmittler commented Jul 2, 2021

BenTheElder commented Jul 2, 2021

dims commented Jul 6, 2021

caesarxuchao commented Jul 8, 2021

jiahuif commented Jul 9, 2021

howardjohn commented Jul 12, 2021

k8s-ci-robot commented Jul 12, 2021

k8s-ci-robot commented Jul 12, 2021 • edited Loading

howardjohn commented Jul 12, 2021

howardjohn commented Jul 19, 2021

howardjohn commented Jul 23, 2021

jiahuif commented Jul 23, 2021

jiahuif commented Jul 23, 2021

howardjohn commented Jul 26, 2021

yuzhiquan Jul 27, 2021

Choose a reason for hiding this comment

yuzhiquan Jul 27, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 17, 2021

Monkeyanator Sep 24, 2021

Choose a reason for hiding this comment

k8s-triage-robot commented Dec 23, 2021

dims commented Jan 10, 2022

k8s-triage-robot commented Feb 9, 2022

k8s-triage-robot commented Mar 11, 2022

k8s-ci-robot commented Mar 11, 2022

nmittler commented Mar 14, 2022

howardjohn commented Mar 14, 2022

howardjohn commented Jul 2, 2021 •

edited

Loading

k8s-ci-robot commented Jul 12, 2021 •

edited

Loading