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

Add support for Multi Namespace watch mode #483

Merged
merged 2 commits into from
Jan 20, 2024
Merged

Add support for Multi Namespace watch mode #483

merged 2 commits into from
Jan 20, 2024

Conversation

a-hilaly
Copy link
Member

@a-hilaly a-hilaly commented Dec 31, 2023 •
edited
Loading

This commit leverages new functionalities recently introduces in the k8s
upstream controller-runtime, enabling users to configure the controller
for multi-namespace watch mode.

Now users can specify multiple namespaces using a comma-separated list,
such as default,namespace1,namespace2 (either using helm values or
binary flags (for the bravest, running ACK controllers using k8s
Deployments)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ack-prow ack-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 31, 2023
@ack-prow ack-prow bot requested review from ivelichkovich and jljaco December 31, 2023 21:35
@ack-prow ack-prow bot added the approved label Dec 31, 2023
@a-hilaly
Copy link
Member Author

/hold

@ack-prow ack-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 31, 2023
acornett21
acornett21 reviewed Jan 4, 2024
View reviewed changes
templates/cmd/controller/main.go.tpl Outdated
Cache: ctrlrtcache.Options{
Scheme: scheme,
DefaultNamespaces: defaultNamesSpaces,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is the ideal approach. I'm not certain but I think we'd want to use cache.MultiNamespacedCacheBuilder in a more dynamic way. See example here

Copy link
Member Author

Choose a reason for hiding this comment

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

That was possible in 0.15.3 which is the version sdk-operator still using, however it was deprecated in 0.16.0 (kubernetes-sigs/controller-runtime#2423) and a newer granular configurations were possible with:

Copy link
Contributor

Choose a reason for hiding this comment

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

AHH okay cool. I'm still not up to speed on 0.16.x

@acornett21
Copy link
Contributor

If we start supporting multi namespace we should also add in OwnNamespace SingleNamespace and MultiNamespace modes in the CSV. Code that would need to be updated is here

@ack-prow ACK Prow
Copy link

ack-prow bot commented Jan 4, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, acornett21

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

@a-hilaly a-hilaly force-pushed the multi-namespace-watch-mode branch 2 times, most recently from 162eb6b to 2146fc4 Compare January 16, 2024 05:10
@a-hilaly a-hilaly changed the title [WIP]: Multi namespace watch mode Add support for Multi Namespace watch mode Jan 16, 2024
@ack-prow ack-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 16, 2024
@a-hilaly a-hilaly force-pushed the multi-namespace-watch-mode branch from 43561da to 142b6b5 Compare January 17, 2024 01:40
@a-hilaly
Copy link
Member Author

/unhold

@ack-prow ack-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 17, 2024
@a-hilaly a-hilaly force-pushed the multi-namespace-watch-mode branch from 142b6b5 to 79a2a99 Compare January 19, 2024 23:41
@a-hilaly
Copy link
Member Author

/retest

@a-hilaly a-hilaly merged commit d3b23cd into aws-controllers-k8s:main Jan 20, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acornett21 acornett21 acornett21 approved these changes

@ivelichkovich ivelichkovich Awaiting requested review from ivelichkovich

@jljaco jljaco Awaiting requested review from jljaco

Assignees
No one assigned
Labels
approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@a-hilaly @acornett21