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

Add option to restrict opensearch operator to watch a desired namespace #158

Conversation

aarontams
Copy link
Contributor

Task Optionally restrict the opensearch operator to watch a specific namespace.

Scenarios 1 When a k8s cluster has 100 namespaces and each namespace holds one opensearch cluster. If there is only one opensearch operator to manager all 100 opensearch clusters, it will take a lot time to sync all of them.

Scenarios 2 With a single operator to control all the opensearch clusters within the same k8s cluster, you won't have the flexibility to upgrade, downgrade, or pin down certain opensearch clusters.
In production, sometimes users need to run opensearch clusters with different favors - stable, canary, and long term support images within a k8s cluster.

Solution To solve the issues from both scenarios, we can deploy a opensearch operator to each namespace and each operator will be restricted to only manager the opensearch cluster within the same namespace.
When each namespace contains it's own opensearch operator and opensearch cluster, we can selectively upgrade/downgrade some of the namespaces with different opensearch opertor and cluster images. Or even pin down certain namespaces with current older version and upgrade the rest to latest.

watchNamespace
The watchNamespace can be set during helm deployment in values.yaml manager.watchNamespace, and past to the operator as env variable --watch-namespace={{ .Values.manager.watchNamespace }}, then has the operator to start the controller manager with Namespace option.

### sigs.k8s.io manager/manager.go
	// Namespace if specified restricts the manager's cache to watch objects in
	// the desired namespace Defaults to all namespaces
	Namespace string

Note In our production system, our operator used to watch all the CRs from all namespaces. After we grew our CRs/namespaces to few tens, we got hit really hard by the performance. We decided to change our design and used local (namespaced) operator few years ago. With that solution, we have been syncing of our applications every 30s and running different image favors on all our k8s fleets.

@@ -29,6 +29,7 @@ spec:
- --health-probe-bind-address=:8081
- --metrics-bind-address=127.0.0.1:8080
- --leader-elect
- --watch-namespace={{ .Values.manager.watchNamespace }}
Copy link
Contributor Author

@aarontams aarontams May 31, 2022

Choose a reason for hiding this comment

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

If we don't want to show this env setting when watchNamespace is not set in values.yaml, we can add an if statement around this line.

{{- if . Values.manager.watchNamespace }}
        - --watch-namespace={{ .Values.manager.watchNamespace }}
{{- end }}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. Please wrap it in the if block so the option is only added when a namespace is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.
Will add and test it out when I get a chance.
Thanks for reviewing this MR.

Copy link
Contributor Author

@aarontams aarontams Jun 1, 2022

Choose a reason for hiding this comment

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

Addressed the comment in latest push.

@aarontams aarontams force-pushed the add-restricted-watch-namespace-support branch from 2728e63 to 1267884 Compare May 31, 2022 16:55
Copy link
Collaborator

@swoehrl-mw swoehrl-mw left a comment

Choose a reason for hiding this comment

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

Hi @aarontams. Thank you for this PR. Please have a look at my review comments.

@@ -29,6 +29,7 @@ spec:
- --health-probe-bind-address=:8081
- --metrics-bind-address=127.0.0.1:8080
- --leader-elect
- --watch-namespace={{ .Values.manager.watchNamespace }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. Please wrap it in the if block so the option is only added when a namespace is set.

@@ -24,3 +24,4 @@ spec:
- "--health-probe-bind-address=:8081"
- "--metrics-bind-address=127.0.0.1:8080"
- "--leader-elect"
- "--watch-namespace"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove it in next commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@@ -11,3 +11,4 @@ manager:
image:
repository: public.ecr.aws/opsterio/opensearch-operator
tag: latest
watchNamespace:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a comment here to explain that an empty watchNamespace means all namespaces are watched. Just so we have it documented somewhere.
For the future I think we will need a section in the user guide to explain the available helm values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Contributor Author

@aarontams aarontams Jun 1, 2022

Choose a reason for hiding this comment

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

Addressed the comment in latest push.

Signed-off-by: aaron.tam <aaron.tam@oracle.com>
@aarontams aarontams force-pushed the add-restricted-watch-namespace-support branch from 1267884 to b926aca Compare June 1, 2022 17:42
@swoehrl-mw swoehrl-mw merged commit 2b71fef into opensearch-project:main Jun 3, 2022
@aarontams
Copy link
Contributor Author

Thanks @swoehrl-mw
Appreciated your review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants