Skip to content

Conversation

ChenYi015
Copy link
Member

Purpose of this PR

Close #2384

Proposed changes:

  • Add option controller.leaderElection.enable and webhook.leaderElection.enable for disabling controller/webhook leader election
  • When leader election was disabled, the related RBAC rules against leases will be removed.

Change Category

  • Bugfix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that could affect existing functionality)
  • Documentation update

Rationale

Checklist

  • I have conducted a self-review of my own code.
  • I have updated documentation accordingly.
  • I have added tests that prove my changes are effective or that my feature works.
  • Existing unit tests pass locally with my changes.

Additional Notes

@ChenYi015
Copy link
Member Author

/assign @jacobsalway

Copy link
Member

@jacobsalway jacobsalway left a comment

Choose a reason for hiding this comment

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

Does it make sense to have leader election on the webhook at all? It's a mostly stateless service that can have multiple replicas. The only part that might be relevant to have leader election on is the TLS certificate generation/setting the secret, but this only needs to be done once and exponentially retries on conflict or error anyway so I think it's fine if multiple replicas attempt to do this.

The core of this PR looks good to me though. Happy to tick and add a separate PR for removing leader election.

@ChenYi015
Copy link
Member Author

For the TLS certificate generation process, I think it can work fine even without leader election.

Does it make sense to have leader election on the webhook at all? It's a mostly stateless service that can have multiple replicas.

Multiple webhook server replicas can work at the same time with/without leader election. But for the controller part,
two reconcilers are set respectively in the manager for patching caBundles of mutating/validating webhooks. I have not tested whether it can work as expected without leader election if there are multiple replicas. I think we would better keep it as it is.

@jacobsalway
Copy link
Member

I have not tested whether it can work as expected without leader election if there are multiple replicas.

I'll have a look at this myself. In the mean time this PR doesn't introduce any breaking changes so happy to approve

@jacobsalway
Copy link
Member

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Feb 10, 2025
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jacobsalway

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

@google-oss-prow google-oss-prow bot merged commit 1f2cfbc into kubeflow:master Feb 10, 2025
11 checks passed
@ChenYi015 ChenYi015 deleted the feature/disable-leader-election branch February 11, 2025 02:30
ChenYi015 added a commit that referenced this pull request Mar 21, 2025
* Add option for disabling leader election

Signed-off-by: Yi Chen <[email protected]>

* Remove related RBAC rules when disabling leader election

Signed-off-by: Yi Chen <[email protected]>

---------

Signed-off-by: Yi Chen <[email protected]>
(cherry picked from commit 1f2cfbc)
@ChenYi015 ChenYi015 mentioned this pull request Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to disable leader election in the controller
2 participants