⚠️ Add watch label to allow multiple manager instances #4119

MarcelMue · 2021-01-25T19:19:42Z

What this PR does / why we need it:

This PR adds a flag which defines which CRs will be processed depending on a given label.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #4004

k8s-ci-robot · 2021-01-25T19:19:50Z

Hi @MarcelMue. Thanks for your PR.

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

api/v1alpha4/common_types.go

controllers/types.go

MarcelMue · 2021-01-26T10:40:31Z

util/predicates/generic_predicates.go

@@ -157,6 +177,12 @@ func ResourceNotPaused(logger logr.Logger) predicate.Funcs {
 	}
 }

+// ResourceNotPausedAndHasLabel returns a predicate that returns true only if the
+// ResourceNotPaused and ResourceHasLabel predicates return true.
+func ResourceNotPausedAndHasLabel(logger logr.Logger, labelValue string) predicate.Funcs {


Something similar was implemented before but then removed. I tried to follow that structure and naming.

If there are not performance concerns, I would prefer to keep paused and filter by label as a separated predicates

JoelSpeed · 2021-01-26T10:44:40Z

I think this might need some further discussion before we review. Personally I'd like to understand what the benefit of this approach is over using namespaces to separate the resources that should be reconciled by particular controllers

fabriziopandini

/ok-to-test

If this behaviour applies to all the objects managed by Cluster API, we should get this documented in the contract, and implemented also in CAPBK, KCP & CAPD too

fabriziopandini · 2021-01-26T11:10:34Z

util/predicates/generic_predicates.go

@@ -157,6 +177,12 @@ func ResourceNotPaused(logger logr.Logger) predicate.Funcs {
 	}
 }

+// ResourceNotPausedAndHasLabel returns a predicate that returns true only if the
+// ResourceNotPaused and ResourceHasLabel predicates return true.
+func ResourceNotPausedAndHasLabel(logger logr.Logger, labelValue string) predicate.Funcs {


If there are not performance concerns, I would prefer to keep paused and filter by label as a separated predicates

api/v1alpha4/common_types.go

controllers/types.go

main.go

MarcelMue · 2021-01-26T13:23:45Z

I think this might need some further discussion before we review. Personally I'd like to understand what the benefit of this approach is over using namespaces to separate the resources that should be reconciled by particular controllers

There has been discussion on this topic in several different places so far - it was also acknowledged here:
#4074

Google doc which was shared with sig-cluster-lifecycle:
https://docs.google.com/document/d/1Icttu56PmbDm70g8GyXupRq3MX4NADuGDoVhjwQwHMo/

Discussions during sync (just two examples):
https://www.youtube.com/watch?t=649&v=FLUV7QB2QL8
https://www.youtube.com/watch?t=269&v=tV1yoeypfWk

To answer your question briefly:
The namespace separation is not well suited for multi tenancy (in particular with multiple core controllers in different versions) as it implies moving CRs between namespaces.

Please let me know if there is a clear concern here - I am open to discuss but there has been IMO a lot of discussion leading up to this PR already.

MarcelMue · 2021-01-26T13:26:58Z

If this behaviour applies to all the objects managed by Cluster API, we should get this documented in the contract, and implemented also in CAPBK, KCP & CAPD too

Should I align the docs in this same PR? Or would this be a follow up?
I am totally fine with bringing this change to the other providers once this is merged.

api/v1alpha4/common_types.go

vincepri

Only concern that came up during review is that now the predicate has to be added to each controller separately (which is also hard to test and could break any time).

Is Namespace based filtering not enough to run multiple controllers?

With Namespace based filters we also get the benefit of having the informers and all internal cache to only ever watch that namespace. With label based predicates instead, the cache is still being populated entirely, and watch notifications are still coming through but discarded

MarcelMue · 2021-01-26T16:44:04Z

Is Namespace based filtering not enough to run multiple controllers?

To me this mostly boils down to usability. I agree that there are some performance downside but this is a completely optional flag and from my experience with using this approach, the performance impact is negligible.

The usability is important to me because this enables switching the reconciling controller in a single command: Changing the label. I feel like that is easy to understand, execute and handle.

With a namespaced approach we would firstly have our namespace structure to fit the controller versions we use - this is not very nice IMO.
Additionally switching the version now becomes a rather complex action:

Pause all CRs of the Cluster.
Move all CRs of the Cluster to a different namespace.
Unpause all CRs of the Cluster.

A basically atomic action in the workflow I propose here becomes a rather error prone, multi step process.

I feel like we discussed these points before in a meeting - I am also more than happy to provide E2E tests for this functionality (I think the e2e workthrough is not out yet) if this increases your confidence.

MarcelMue · 2021-01-28T13:52:53Z

@CecileRobertMichon You also had opinions on this IIRC.

CecileRobertMichon · 2021-01-28T22:55:16Z

Should I align the docs in this same PR? Or would this be a follow up?
I am totally fine with bringing this change to the other providers once this is merged.

Yes, please add to the bootstrap and infra provider contracts in https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/developer/providers/bootstrap.md, https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/developer/providers/cluster-infrastructure.md, and https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/developer/providers/machine-infrastructure.md

let's also add a quick note about it in https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/developer/providers/v1alpha3-to-v1alpha4.md (see https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/developer/providers/v1alpha2-to-v1alpha3.md#support-the-clusterx-k8siopaused-annotation-and-clusterspecpaused-field for an example)

CecileRobertMichon · 2021-01-28T22:59:31Z

Only concern that came up during review is that now the predicate has to be added to each controller separately (which is also hard to test and could break any time).

I'm +1 on this PR. There is precedent for this with the Pause annotation. In addition, the default behavior does not change and this is completely opt-in, so the risk is low. We should definitely consider adding an E2E test for preventing future regressions though.

vincepri · 2021-02-01T21:36:13Z

I'm +1 on this PR. There is precedent for this with the Pause annotation

@CecileRobertMichon Did you meant the feature gates, or did I miss the use case you were referring to?

CecileRobertMichon · 2021-02-01T21:45:34Z

@vincepri no I meant the pause annotation predicate (which had to be added to every single controller, including in infra providers)

vincepri · 2021-02-01T21:47:29Z

@CecileRobertMichon Oh I see, the implementation bits, yes. The flag though also has to be kept in sync, which is what I was thinking w.r.t. the feature gates.

Thanks @MarcelMue for the explanation above, as long as we document these requirements and limitations +1

vincepri

/lgtm

CecileRobertMichon · 2021-02-05T18:07:21Z

I would suggest to title the PR with warning given we are changing the contract and to ask for a quick +1 to provider's maintainer as well.

Azure provider maintainer here - /lgtm

/assign @randomvariable @yastij @cpanato

CecileRobertMichon · 2021-02-05T18:09:30Z

Actually, I just realized we should also update https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/developer/architecture/controllers/support-multiple-instances.md @MarcelMue

MarcelMue · 2021-02-05T18:14:53Z

Actually, I just realized we should also update https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/developer/architecture/controllers/support-multiple-instances.md @MarcelMue

This PR already contains a simple addition of the label in the same section where the namespace flag is mentioned. Would you like to something additional there? I felt like the general statement in that file remained unchanged:
https://github.com/kubernetes-sigs/cluster-api/pull/4119/files#diff-e022c3c4584b8bb87c39431d43bdac3c6d62743735ede072b8a1a1bf9e1d64ce

CecileRobertMichon · 2021-02-05T18:27:15Z

This PR already contains a simple addition of the label in the same section where the namespace flag is mentioned. Would you like to something additional there

Ahh I must have missed that on my re-read. Thanks, all good from my side then.

yastij

/lgtm

pending rebase, fyi @gab-satchi

MarcelMue · 2021-02-09T16:44:05Z

/lgtm

pending rebase, fyi @gab-satchi

Should be all rebased correctly now :)

CecileRobertMichon

/lgtm

vincepri

/approve

k8s-ci-robot · 2021-02-09T17:09:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

vincepri · 2021-02-09T17:09:46Z

/retitle ⚠️ Add watch label to allow multiple instances of controllers

vincepri · 2021-02-09T17:10:04Z

/retitle ⚠️ Add watch label to allow multiple manager instances

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 25, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 25, 2021

k8s-ci-robot requested review from JoelSpeed and justinsb January 25, 2021 19:20

MarcelMue changed the title ~~✨ Add watch label to allow running multiple instances of the operator~~ ✨ Add watch label to allow running multiple instances of the operator Jan 25, 2021

MarcelMue marked this pull request as ready for review January 26, 2021 10:38

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 26, 2021

MarcelMue commented Jan 26, 2021

View reviewed changes

fabriziopandini reviewed Jan 26, 2021

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 Jan 26, 2021

vincepri reviewed Jan 26, 2021

View reviewed changes

api/v1alpha4/common_types.go Show resolved Hide resolved

vincepri reviewed Jan 26, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2021

MarcelMue force-pushed the add-watch-label branch from db1001c to 2a79c2a Compare February 1, 2021 21:42

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2021

MarcelMue force-pushed the add-watch-label branch from 1f1fceb to f0d4357 Compare February 1, 2021 21:51

vincepri approved these changes Feb 5, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2021

k8s-ci-robot assigned cpanato, randomvariable and yastij Feb 5, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2021

yastij reviewed Feb 8, 2021

View reviewed changes

Add watch-filter label

fc41bf6

MarcelMue force-pushed the add-watch-label branch from eff6b20 to fc41bf6 Compare February 9, 2021 10:05

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 9, 2021

CecileRobertMichon reviewed Feb 9, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2021

vincepri approved these changes Feb 9, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2021

k8s-ci-robot changed the title ~~⚠️ Add watch label to allow running multiple instances of the operator~~ ⚠️ Add watch label to allow multiple instances of controllers Feb 9, 2021

k8s-ci-robot changed the title ~~⚠️ Add watch label to allow multiple instances of controllers~~ ⚠️ Add watch label to allow multiple manager instances Feb 9, 2021

k8s-ci-robot merged commit 4a6cccd into kubernetes-sigs:master Feb 9, 2021

mig4 mentioned this pull request Mar 10, 2021

Add CAPI labels that will be used to filter CR's by version giantswarm/apiextensions#697

Closed

3 tasks

MarcelMue deleted the add-watch-label branch March 16, 2021 09:06

fiunchinho mentioned this pull request Jul 13, 2021

🐛 Missing watch label to allow multiple manager instances #4929

Merged

richardcase mentioned this pull request Dec 19, 2022

Extend the ability for CAPI to filter what CRs it reconciles #7775

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Add watch label to allow multiple manager instances #4119

⚠️ Add watch label to allow multiple manager instances #4119

MarcelMue commented Jan 25, 2021

k8s-ci-robot commented Jan 25, 2021

MarcelMue Jan 26, 2021

fabriziopandini Jan 26, 2021

JoelSpeed commented Jan 26, 2021

fabriziopandini left a comment

fabriziopandini Jan 26, 2021

MarcelMue commented Jan 26, 2021

MarcelMue commented Jan 26, 2021

vincepri left a comment

MarcelMue commented Jan 26, 2021

MarcelMue commented Jan 28, 2021

CecileRobertMichon commented Jan 28, 2021

CecileRobertMichon commented Jan 28, 2021

vincepri commented Feb 1, 2021

CecileRobertMichon commented Feb 1, 2021

vincepri commented Feb 1, 2021

vincepri left a comment

CecileRobertMichon commented Feb 5, 2021

CecileRobertMichon commented Feb 5, 2021

MarcelMue commented Feb 5, 2021

CecileRobertMichon commented Feb 5, 2021

yastij left a comment

MarcelMue commented Feb 9, 2021

CecileRobertMichon left a comment

vincepri left a comment

k8s-ci-robot commented Feb 9, 2021

vincepri commented Feb 9, 2021

vincepri commented Feb 9, 2021

⚠️ Add watch label to allow multiple manager instances #4119

⚠️ Add watch label to allow multiple manager instances #4119

Conversation

MarcelMue commented Jan 25, 2021

k8s-ci-robot commented Jan 25, 2021

MarcelMue Jan 26, 2021

Choose a reason for hiding this comment

fabriziopandini Jan 26, 2021

Choose a reason for hiding this comment

JoelSpeed commented Jan 26, 2021

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Jan 26, 2021

Choose a reason for hiding this comment

MarcelMue commented Jan 26, 2021

MarcelMue commented Jan 26, 2021

vincepri left a comment

Choose a reason for hiding this comment

MarcelMue commented Jan 26, 2021

MarcelMue commented Jan 28, 2021

CecileRobertMichon commented Jan 28, 2021

CecileRobertMichon commented Jan 28, 2021

vincepri commented Feb 1, 2021

CecileRobertMichon commented Feb 1, 2021

vincepri commented Feb 1, 2021

vincepri left a comment

Choose a reason for hiding this comment

CecileRobertMichon commented Feb 5, 2021

CecileRobertMichon commented Feb 5, 2021

MarcelMue commented Feb 5, 2021

CecileRobertMichon commented Feb 5, 2021

yastij left a comment

Choose a reason for hiding this comment

MarcelMue commented Feb 9, 2021

CecileRobertMichon left a comment

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 9, 2021

vincepri commented Feb 9, 2021

vincepri commented Feb 9, 2021