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

Switch leader election to endpointleases #662

Merged
merged 1 commit into from
Jan 4, 2022
Merged

Switch leader election to endpointleases #662

merged 1 commit into from
Jan 4, 2022

Conversation

acumino
Copy link
Member

@acumino acumino commented Dec 22, 2021 •
edited
Loading

/kind enhancement

Which issue(s) this PR fixes:
Part of gardener/gardener#4742

CC: @ialidzhikov

Release note:

The default leader election resource lock of `machine-controller-manager` has been changed from `endpoints` to `endpointsleases`.

Components that deploy the `machine-controller-manager` will now have to adapt the RBAC rules to allow `machine-controller-manager` to maintain its leader election resource lock in `leases` as well.

@acumino acumino requested a review from a team as a code owner December 22, 2021 14:52
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 22, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 22, 2021
@gardener-robot gardener-robot added the kind/enhancement Enhancement, improvement, extension label Dec 22, 2021
@gardener-robot
Copy link

@acumino Thank you for your contribution.

@gardener-robot gardener-robot added needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Dec 22, 2021
ialidzhikov
ialidzhikov suggested changes Dec 22, 2021
View reviewed changes
Copy link
Member

@ialidzhikov ialidzhikov left a comment

Choose a reason for hiding this comment

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

Generally lgtm, minor nit.

pkg/util/client/leaderelectionconfig/config.go Outdated Show resolved Hide resolved
@ialidzhikov ialidzhikov mentioned this pull request Dec 22, 2021
Closed
13 tasks
@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Dec 22, 2021
@ialidzhikov
Copy link
Member

@acumino found that the RBAC under kubernetes/deployment has to be adapted accordingly. Nice catch!

himanshu-kun
himanshu-kun approved these changes Dec 22, 2021
View reviewed changes
Copy link
Contributor

@himanshu-kun himanshu-kun left a comment

Choose a reason for hiding this comment

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

/lgtm
is there a resource by the name endpointsleases also, I can find leases and endpoint resources

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review labels Dec 22, 2021
@ialidzhikov
Copy link
Member

/hold

@gardener-robot gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Dec 22, 2021
@acumino
Copy link
Member Author

acumino commented Dec 22, 2021 •
edited
Loading

/lgtm
is there a resource by the name endpointsleases also, I can find leases and endpoint resources

No, we want to migrate from endpoints to leases.
We exactly don't want to change it randomly but in two phases:
(1) acquire old object lock, once acquired, acquire new object lock; only when both acquired we have a lock and can proceed
(2) (next release) since now master has to acquire the lock on the new object (lease) to be master, we can now delete the need to acquire the old object (endpoints or configmap).
And now we're using lease as it should be.

@himanshu-kun
Copy link
Contributor

No, we want to migrate from endpoint to leases.
We exactly don't want to change it randomly but in two phases:
(1) acquire old object lock, once acquired, acquire new object lock; only when both acquired we have a lock and can proceed
(2) (next release) since now master has to acquire the lock on the new object (lease) to be master, we can now delete the need to acquire the old object (endpoints or configmap).
And now we're using lease as it should be.

Thanks @acumino , for the clear explanation:thumbsup:

@acumino acumino force-pushed the migrate_mcm_to_endpoints_lease_lock branch from dadac24 to 1269a19 Compare December 23, 2021 13:58
@gardener-robot gardener-robot added size/s Size of pull request is small (see gardener-robot robot/bots/size.py) and removed size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Dec 23, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 23, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 23, 2021
@acumino acumino force-pushed the migrate_mcm_to_endpoints_lease_lock branch from 1269a19 to 7c42765 Compare December 23, 2021 14:19
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 23, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 23, 2021
ialidzhikov
ialidzhikov approved these changes Jan 3, 2022
View reviewed changes
Copy link
Member

@ialidzhikov ialidzhikov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/lgtm
/unhold

@gardener-robot gardener-robot removed the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Jan 3, 2022
@AxiomSamarth AxiomSamarth merged commit c988f63 into gardener:master Jan 4, 2022
@ialidzhikov
Copy link
Member

@acumino can you prepare draft PRs to provider-extensions with the required RBAC changes? Thanks in advance.

@acumino acumino mentioned this pull request Jan 6, 2022
Closed
This was referenced Jan 17, 2022
Merged
Merged
@ary1992 ary1992 mentioned this pull request Jan 18, 2022
Merged
@himanshu-kun himanshu-kun mentioned this pull request Jan 18, 2022
Closed
@ialidzhikov ialidzhikov mentioned this pull request Jul 22, 2022
Merged
@himanshu-kun himanshu-kun mentioned this pull request Feb 21, 2023
Open
@acumino acumino deleted the migrate_mcm_to_endpoints_lease_lock branch October 10, 2024 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ialidzhikov ialidzhikov ialidzhikov approved these changes

@AxiomSamarth AxiomSamarth AxiomSamarth approved these changes

@himanshu-kun himanshu-kun himanshu-kun approved these changes

Assignees
No one assigned
Labels
kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/s Size of pull request is small (see gardener-robot robot/bots/size.py)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@acumino @gardener-robot @ialidzhikov @himanshu-kun @AxiomSamarth @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3