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.

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

Migrate leader election to leases #711

Merged
merged 1 commit into from
Jul 22, 2022
Merged

Conversation

acumino
Copy link
Member

@acumino acumino commented May 3, 2022

/kind enhancement

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

Special notes for your reviewer:
/cc @ialidzhikov

Release note:

The default leader election resource lock of `machine-controller-manager` has been changed from `endpointsleases` to `leases`.
Please make sure, that you had at least `machine-controller-manager@v0.43.0` running before upgrading to `v0.46.0`, so that it has successfully acquired leadership with the hybrid resource lock (`endpointsleases`) at least once.

@gardener-robot gardener-robot added the kind/enhancement Enhancement, improvement, extension label May 3, 2022
@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 May 3, 2022
@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 May 3, 2022
@gardener-robot-ci-1 gardener-robot-ci-1 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 May 3, 2022
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.

Thanks for the PR!
IMO we can mark it ready for review - all MCM providers were released with the endpointsleases resource lock.
We could also check whether the RBAC rules for endpoints can be now removed.

@acumino acumino marked this pull request as ready for review May 20, 2022 04:54
@acumino acumino requested a review from a team as a code owner May 20, 2022 04:54
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

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels May 20, 2022
@ialidzhikov
Copy link
Member

We could also check whether the RBAC rules for endpoints can be now removed.

@acumino , any comments on this one?

@acumino
Copy link
Member Author

acumino commented May 20, 2022

We could also check whether the RBAC rules for endpoints can be now removed.

@acumino , any comments on this one?

There was not any specific RBAC for endpoints for leader election. I am not sure if mcm still need endpoints apart from the use for leader election if not we can drop it else not.

@ialidzhikov
Copy link
Member

There was not any specific RBAC for endpoints for leader election. I am not sure if mcm still need endpoints apart from the use for leader election if not we can drop it else not.

That was the whole point of my comment - to check it :)

I recall some cases when client-go or controller-runtime can try to list endpoints for the kubernetes service in the default namespace. At least we should be able to remove create/patch/update verbs for endpoints from the example RBAC.

@himanshu-kun
Copy link
Contributor

I have tested by removing the endpoints rules from the clusterrole, and tested the following MCM scenarios:

  • scaling-up machinedeployment
  • scaling-down
  • rolling update

There were no logs found stating cannot list endpoint or similar, and there were no restarts of MCM.
Hope these are enough @ialidzhikov

@ialidzhikov
Copy link
Member

Okay, my comment was to adapt the example RBAC and remove the endpoints rule. In #662 we adapted the example RBAC to add rules for leases, I guess now we can remove the rules for endpoints.

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has 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 Jul 22, 2022
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.

/lgtm

@himanshu-kun himanshu-kun added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 22, 2022
@gardener-robot-ci-1 gardener-robot-ci-1 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 Jul 22, 2022
@himanshu-kun himanshu-kun merged commit d3fbf0c into gardener:master Jul 22, 2022
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Jul 22, 2022
@acumino acumino deleted the mig/leases branch October 10, 2024 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants