Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove support for EndpointsLeases and ConfigMapsLeases lock from leader election #117558

Merged
merged 1 commit into from Apr 25, 2023
Merged

Remove support for EndpointsLeases and ConfigMapsLeases lock from leader election #117558

merged 1 commit into from Apr 25, 2023

Conversation

wojtek-t
Copy link
Member

@wojtek-t wojtek-t commented Apr 24, 2023 •
edited by liggitt

Ref #80289
Continuation of: #106852

client-go: the leader election library no longer supports leader-election locks for `endpointsleases` or `configmapsleases` resources. Users of those lock resources should move to `leases` which is safe to do in a single release. Users of previously removed `endpoints` or `configmaps` lock should first transition to `endpointsleases` or `configmapsleases`, which are supported in k8s.io/client-go v0.17 - v0.27, before upgrading to version v0.28+ and switching to `leases`

/kind feature
/sig api-machinery
/priority important-soon

/assign @liggitt @deads2k

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 24, 2023
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 24, 2023
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 24, 2023
@liggitt
Copy link
Member

liggitt commented Apr 24, 2023

removing this means that anyone still on configmap / endpoints election would have to jump to a 1.27.x level of client-go to use the transitional resource, then jump to 1.28+

is it important enough to drop support for the transitional resource to make consumers do both of those hops?

@wojtek-t
Copy link
Member Author

removing this means that anyone still on configmap / endpoints election would have to jump to a 1.27.x level of client-go to use the transitional resource, then jump to 1.28+

is it important enough to drop support for the transitional resource to make consumers do both of those hops?

This is a good question and there isn't a clear answer to it:

  • on one hand what you pointed out is a risk here and potentially additional work to some people
  • on the other hand, we really want people to migrate to leases, (staying on multilock which is now possible also isn't great, as they are obviously less reliable (we need to ensure two locks at the same time) and worse for performance)

When discussing it with @deads2k back then, we wanted to remove this support after 3 releases. In 1.28 there will be 4. But maybe we want to leave it for a bit more? I just don't think we should support it forever...

@deads2k
Copy link
Contributor

deads2k commented Apr 24, 2023

is it important enough to drop support for the transitional resource to make consumers do both of those hops?

Allowing consumers to continue using endpoints isn't zero cost. Because we cannot determine the intent of using a given API in p&f, there is additional undesired contention that cannot be reliably resolved. Without removing the capability in the library, I'm unsure how to eliminate the contention.

@wojtek-t
Copy link
Member Author

+1 to David
"as they are obviously less reliable (we need to ensure two locks at the same time)" from my comment was supposed to catch also aspects of P&F too.

@liggitt
Copy link
Member

liggitt commented Apr 25, 2023

that's fair... I guess the joint resource lock functionality has long been supported in client-go, so this would mean a client-go consumer needing to transition would need to roll out a release on a client-go version 0.17.0 - 0.27.0 and use endpointsleases / configmapsleases before updating to 0.28.0+

Update the release note to describe what people using endpoints or configmaps as their lock resource should do (the specific versions of client-go that support the joint lock functionality).

@wojtek-t
Copy link
Member Author

Update the release note to describe what people using endpoints or configmaps as their lock resource should do (the specific versions of client-go that support the joint lock functionality).

@liggitt - thanks; updated - PTAL if the current wording looks ok to you

liggitt
liggitt reviewed Apr 25, 2023
View reviewed changes
staging/src/k8s.io/client-go/tools/leaderelection/resourcelock/interface.go Outdated
Comment on lines 176 to 179
case endpointsResourceLock:
return nil, fmt.Errorf("endpoints lock is removed, migrate to %s", EndpointsLeasesResourceLock)
return nil, fmt.Errorf("endpoints lock is removed, migrate to %s", endpointsLeasesResourceLock)
case configMapsResourceLock:
return nil, fmt.Errorf("configmaps lock is removed, migrate to %s", ConfigMapsLeasesResourceLock)
return nil, fmt.Errorf("configmaps lock is removed, migrate to %s", configMapsLeasesResourceLock)
Copy link
Member

@liggitt liggitt Apr 25, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these messages now point people at something unsupported by the current client-go version... maybe include the recommended client-go version for this in this message (v0.27.x)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done - PTAL

@liggitt
Copy link
Member

liggitt commented Apr 25, 2023

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 4a22d3f43244c6b24fb38079ec3986cc39dfb149

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, wojtek-t

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:

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 k8s-ci-robot merged commit ff1e3ee into kubernetes:master Apr 25, 2023
12 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone Apr 25, 2023
@cici37
Copy link
Contributor

cici37 commented Apr 25, 2023

/triage accepted

@k8s-ci-robot 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 Apr 25, 2023
@sunnylovestiramisu sunnylovestiramisu mentioned this pull request Aug 8, 2023
Merged
@galal-hussein galal-hussein mentioned this pull request Aug 15, 2023
Merged
@jinja2 jinja2 mentioned this pull request Aug 21, 2023
Merged
@neogopher neogopher mentioned this pull request Aug 28, 2023
Merged
@crobert-1 crobert-1 mentioned this pull request Aug 28, 2023
Merged
This was referenced Sep 1, 2023
Merged
Merged
panslava added a commit to panslava/ingress-gce that referenced this pull request Dec 8, 2023
@panslava
Change ConfigMapsLeasesResourceLock to LeasesResourceLock
bc4ba5d 
- ConfigMapsLeasesResourceLock was removed.
  kubernetes/kubernetes#117558 says "Users of
  those lock resources should move to `leases` which is safe to do in a
  single release"
panslava added a commit to panslava/ingress-gce that referenced this pull request Dec 8, 2023
@panslava
Change ConfigMapsLeasesResourceLock to LeasesResourceLock
f6386f4 
- ConfigMapsLeasesResourceLock was removed.
  kubernetes/kubernetes#117558 says "Users of
  those lock resources should move to `leases` which is safe to do in a
  single release"
panslava added a commit to panslava/ingress-gce that referenced this pull request Dec 8, 2023
@panslava
Change ConfigMapsLeasesResourceLock to LeasesResourceLock
62bc802 
- ConfigMapsLeasesResourceLock was removed.
  kubernetes/kubernetes#117558 says "Users of
  those lock resources should move to `leases` which is safe to do in a
  single release"
panslava added a commit to panslava/ingress-gce that referenced this pull request Dec 9, 2023
@panslava
Remove flag.DefaultResourceLock. This value was not used anywhere
6d47f6f 
- ConfigMapsLeasesResourceLock was removed.
  kubernetes/kubernetes#117558.
- Instead of changing value, I found that this is just remnants of old
  code, and this value is not used anywhere, so it is safe to remove.
- Verified with local run

WIP
panslava added a commit to panslava/ingress-gce that referenced this pull request Dec 9, 2023
@panslava
Remove flag.DefaultResourceLock. This value was not used anywhere
b407ef1 
- ConfigMapsLeasesResourceLock was removed.
  kubernetes/kubernetes#117558.
- Instead of changing value, I found that this is just remnants of old
  code, and this value is not used anywhere, so it is safe to remove.
- Verified with local run
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@deads2k deads2k Awaiting requested review from deads2k

@ingvagabund ingvagabund Awaiting requested review from ingvagabund

Assignees

@liggitt liggitt

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@wojtek-t @liggitt @deads2k @k8s-ci-robot @cici37