🐛 Support running alongside other Cluster API pods in the same namespace with leader election enabled #273

noamran · 2019-10-04T22:24:37Z

What this PR does / why we need it:
Currently LeaderElectionID is set to the default controller-leader-election-helper. This causes an issue when several Cluster API pods are deployed in the same namespace. Adding a unique ID per service should resolve this.
Which issue(s) this PR fixes :
Fixes #271

k8s-ci-robot · 2019-10-04T22:24:52Z

Hi @noamran. Thanks for your PR.

I'm waiting for a kubernetes-sigs or 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.

Lokicity · 2019-10-05T00:11:31Z

main.go

@@ -103,6 +103,7 @@ func main() {
 		Scheme:             scheme,
 		MetricsBindAddress: metricsAddr,
 		LeaderElection:     enableLeaderElection,
+		LeaderElectionID:   "controller-leader-election-helper-cabpk",


Non-blocking but it would be nice to make this into a constant

ncdc · 2019-10-05T13:28:35Z

/ok-to-test

vincepri

/approve

Do we want to make this configurable in the future?

k8s-ci-robot · 2019-10-05T21:56:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: noamran, vincepri

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:

~~OWNERS~~ [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

main.go

detiber · 2019-10-07T13:25:47Z

Do we want to make this configurable in the future?

I don't believe so, since it is scoped to a namespace (We can set ctrl.Options.LeaderElectionNamespace ~~in the case that watchNamespace != ""~~). Otherwise we are setting users up to deploy competing controllers.

It looks like we can just set ctrl.Options.LeaderElectionNamespace to watchNamespace, since controller-runtime handles defaulting if it is "": https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/leaderelection/leader_election.go#L61-L67

ncdc · 2019-10-07T13:41:11Z

Do you think it makes more sense to match the configmap's namespace to the pod's namespace or the watchNamespace? I guess I would expect it to match the pod.

detiber · 2019-10-07T13:45:07Z

Do you think it makes more sense to match the configmap's namespace to the pod's namespace or the watchNamespace? I guess I would expect it to match the pod.

Maybe... This would be the default semantics today based on using the in cluster client for getting the Namespace.

detiber · 2019-10-07T13:47:23Z

Using watchNamespace is nice, since we know the user is specifically limiting the watchNamespace to a single Namespace, but it also then possibly exposes the leader election configmap to users, which might not be desirable...

That said, I wouldn't expect users to override the namespace for the Deployment unless they knew they wanted to run multiple controllers scoped down to a single namespace...

ncdc · 2019-10-07T13:48:44Z

I would recommend (ordered by preference, high to low):

Match pod namespace, don't expose --leader-election-namespace
Match pod namespace, expose --leader-election-namespace to allow the user to customize
Match watchNamespace, don't expose --leader-election-namespace
Match watchNamespace, expose --leader-election-namespace to allow the user to customize

And 💯 that users 99.999% of the time don't need to know about the leader election namespace, or need to customize it.

detiber · 2019-10-07T14:04:29Z

Match pod namespace, don't expose --leader-election-namespace
Match pod namespace, expose --leader-election-namespace to allow the user to customize

This exposes a similar footgun to overriding leaderElectionID

Match watchNamespace, don't expose --leader-election-namespace
Match watchNamespace, expose --leader-election-namespace to allow the user to customize

This exposes a similar footgun to overriding leaderElectionID

Another question I just thought of: Do we ever think it should be possible to deploy multiple CABPK( or any Cluster API controller) in the same namespace with different watchNamespaces? If so, maybe it would make sense to alter leaderElectionID to include the watchNamespace if set?

vincepri · 2019-10-07T14:08:27Z

Should we move this conversation to an issue? I'd propose to merge this change as-is given that we're planning to release CABPK today, and explore different options (as outlined above) in a new issue, possibly making it consistent for all of CAP* projects.

How does that sound?

ncdc · 2019-10-07T14:11:03Z

Yeah, I don't think we should expose any of this as flags.

I think including the watchNamespace in the name of the configmap could be a slippery slope - what if the watchNamespace's length is 250 characters (with the max being 253)? Then we can't have both controller-leader-election-cabpk and the watchNamespace in the name...

+1 to merging as-is (pending @detiber's suggestion to remove helper). Note, we'd have to think very carefully before we change the naming convention in this minor version (upgrade impact).

detiber · 2019-10-07T14:18:06Z

So, the tricky part is that any change to the leaderElectionID and/or leaderElectionNamespace is a dangerous change... The release note should include:

Before upgrading the controller deployment, first scale to 0, then apply the updated manifest, and then scale back to 1.

Otherwise there will be competing controllers using different resource locks during the course of the deployment.

I think we should be careful to limit these types of disruptive changes. If we are going to release today, then we should likely not include this change, or we should be good with the naming/namespacing convention we choose for at least a few releases.

ncdc · 2019-10-07T14:28:36Z

@noamran FYI, the title of the PR is used in the release notes. I would recommend retitling to something like "Support running alongside other Cluster API pods in the same namespace with leader election enabled"

ncdc · 2019-10-07T15:21:56Z

LGTM. Could you please squash down to 1 commit?

Co-Authored-By: Jason DeTiberus <[email protected]>

ncdc · 2019-10-07T15:30:34Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 4, 2019

k8s-ci-robot requested review from chuckha and ncdc October 4, 2019 22:24

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 4, 2019

Lokicity reviewed Oct 5, 2019

View reviewed changes

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 Oct 5, 2019

vincepri approved these changes Oct 5, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2019

detiber reviewed Oct 7, 2019

View reviewed changes

main.go Outdated Show resolved Hide resolved

ncdc added this to the v0.1.x milestone Oct 7, 2019

noamran changed the title ~~🐛 Hard coding the LeaderElectionID~~ 🐛 Support running alongside other Cluster API pods in the same namespace with leader election enabled Oct 7, 2019

hard coding LeaderElectionID

10c99fd

Co-Authored-By: Jason DeTiberus <[email protected]>

noamran force-pushed the 271_uniqueID branch from 9c1dd59 to 10c99fd Compare October 7, 2019 15:27

k8s-ci-robot assigned ncdc Oct 7, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2019

k8s-ci-robot merged commit 175bf91 into kubernetes-retired:master Oct 7, 2019

noamran deleted the 271_uniqueID branch October 9, 2019 18:43

noamran mentioned this pull request Jan 22, 2020

REQUEST: New membership for noamran kubernetes/org#1587

Closed

6 tasks

🐛 Support running alongside other Cluster API pods in the same namespace with leader election enabled #273

🐛 Support running alongside other Cluster API pods in the same namespace with leader election enabled #273

Uh oh!

Conversation

noamran commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 4, 2019

Uh oh!

Lokicity Oct 5, 2019

Choose a reason for hiding this comment

Uh oh!

ncdc commented Oct 5, 2019

Uh oh!

vincepri left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 5, 2019

Uh oh!

Uh oh!

detiber commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncdc commented Oct 7, 2019

Uh oh!

detiber commented Oct 7, 2019

Uh oh!

detiber commented Oct 7, 2019

Uh oh!

ncdc commented Oct 7, 2019

Uh oh!

detiber commented Oct 7, 2019

Uh oh!

vincepri commented Oct 7, 2019

Uh oh!

ncdc commented Oct 7, 2019

Uh oh!

detiber commented Oct 7, 2019

Uh oh!

ncdc commented Oct 7, 2019

Uh oh!

ncdc commented Oct 7, 2019

Uh oh!

ncdc commented Oct 7, 2019

Uh oh!

Uh oh!

noamran commented Oct 4, 2019 •

edited

Loading

detiber commented Oct 7, 2019 •

edited

Loading