restrict manager resource cache based on namespaces from environment #186

leelavg · 2024-12-11T05:17:41Z

always cache resources from operator deployed namespace and provider an option to user for caching resources from other namespaces via WATCH_NAMESPACE env var with a comma separated namespace values.

fixes: #184

leelavg · 2024-12-11T05:48:31Z

tested, peak usage was 50 Mi for startup down from ~ 1.5 Gi on a semi-prod cluster.

Madhu-1 · 2024-12-11T06:47:23Z

The namespace(s) name needs to be configurable as this operator is developed to work with multiple namespaces

https://sdk.operatorframework.io/docs/building-operators/golang/operator-scope/#configuring-watch-namespaces-dynamically is the way we need to look to fix this one.

leelavg · 2024-12-11T08:45:35Z

Yes, I want to refactor when we actually need it and so reused operator namespace.

Madhu-1 · 2024-12-11T08:59:21Z

Yes, I want to refactor when we actually need it and so reused operator namespace.

IMO, it should be implemented in the above-requested manner instead of immediately changing it and later refactoring it. @nb-ohad WDYT?

leelavg · 2024-12-11T11:04:13Z

The namespace(s) name needs to be configurable as this operator is developed to work with multiple namespaces

https://sdk.operatorframework.io/docs/building-operators/golang/operator-scope/#configuring-watch-namespaces-dynamically is the way we need to look to fix this one.

multi namespace is not working, need to go through controller-runtime code on what changed.

leelavg · 2024-12-19T04:05:37Z

done.

leelavg · 2024-12-19T04:09:33Z

/assign @nb-ohad @Madhu-1

Madhu-1 · 2025-01-02T07:31:01Z

cmd/main.go

+}
+
+// getOperatorNamespace returns the Namespace the operator is running
+func getOperatorNamespace() (string, error) {


we already have OperatorNamespace in utils. can you please use it?

Could you pls point, didn't find it. There's something similar in controllers but not usable.

sorry i was looking at my local copy where the getOperatorNamespace was exposed, can you please update

ceph-csi-operator/internal/controller/defaults.go

Line 68 in bdd5baa

var operatorNamespace = utils.Call(func() string {

to use it outside it would duplicate functions

as mentioned in controllers but not usable, that function serves different purpose.

I am trying to avoid duplication of the ENV var, so if we change something tomorrow, I don't need to change it in multiple places.

Madhu-1 · 2025-01-02T07:35:21Z

cmd/main.go

+
+// getWatchNamespace returns the Namespace the operator should be watching for changes
+func getWatchNamespace() (string, error) {
+	var watchNamespaceEnvVar = "WATCH_NAMESPACE"


can you please change for WATCH_NAMESPACE in makefile and container ENV as we did for serviceaccount_name_prefix #116?

I'm trying not to touch manifests as during backports it's always creating extra work and that should be fixed first.

please open a tracker if you are planning to fix it as followupl

can you please change for WATCH_NAMESPACE in makefile

done.

Madhu-1 · 2025-01-02T09:38:03Z

cmd/main.go

+		os.Exit(1)
+	}
+	// ensure we always cache items from operator namespace
+	defaultNamespaces[operatorNamespace] = cache.Config{}


how will this change work where the operator should be able to manage the csi drivers in all the namespaces? https://github.com/ceph/ceph-csi-operator/blob/main/docs/design/operator.md#goals-and-guidelines

in my tests probably csi driver was already deployed, do you know how rook does it?

Rook isn't doing anything explicitly for cluster scoped resources and based on my testing in red-hat-storage/ocs-operator#2911 we do get events for them, ocs-op only watches 2 namespaces.

retested, it's working w/o any changes

> date; k get csidriver -w Fri Jan 3 06:26:07 UTC 2025 NAME ATTACHREQUIRED PODINFOONMOUNT STORAGECAPACITY TOKENREQUESTS REQUIRESREPUBLISH MODES AGE rbd.csi.ceph.com true false false <unset> false Persistent 64s rbd.csi.ceph.com true false false <unset> false Persistent 87s rbd.csi.ceph.com true false false <unset> false Persistent 0s > date; k delete csidriver rbd.csi.ceph.com; date Fri Jan 3 06:26:31 UTC 2025 csidriver.storage.k8s.io "rbd.csi.ceph.com" deleted Fri Jan 3 06:26:31 UTC 2025

its not for Rook, its for standalone deployment of csi driver

No, I meant rook also deploys csidriver even if it is watching single namespace.

always cache resources from operator deployed namespace and provider an option to user for caching resources from other namespaces via `WATCH_NAMESPACE` env var with a comma separated namespace values. Signed-off-by: Leela Venkaiah G <[email protected]>

Signed-off-by: Leela Venkaiah G <[email protected]>

Madhu-1 · 2025-01-03T08:08:48Z

cmd/main.go

+
+	watchNamespace, err := getWatchNamespace()
+	if err != nil {
+		setupLog.Error(err, "manager will only watch for resources in the operator deployed namespace")


os.Exit(1) needs to be done as its an actual error and the user might expect that things are working for the watch namespace but the operator is silently ignoring it.

If the namespace exists we wouldn't get an error and watchnamespace is additional env as we always use operator namespace in the cache.

Madhu-1 · 2025-01-03T08:10:11Z

cmd/main.go

+}
+
+// getOperatorNamespace returns the Namespace the operator is running
+func getOperatorNamespace() (string, error) {


I am trying to avoid duplication of the ENV var, so if we change something tomorrow, I don't need to change it in multiple places.

Madhu-1 · 2025-01-03T08:11:28Z

Makefile

@@ -7,6 +7,8 @@ IMAGE_NAME ?= ceph-csi-operator
 # Allow customization of the name prefix and/or namespace
 NAME_PREFIX ?= ceph-csi-operator-
 NAMESPACE ?= $(NAME_PREFIX)system
+# A comma separated list of namespaces for operator to cache objects from
+WATCH_NAMESPACE ?= ""


use NAMESPACE as default instead of an empty string?

I suppose we can rename this to ADDITIONAL_WATCH_NAMESPACES or always have operator ns in this.

I didn't go with latter as the ns is known by downward api which has different semantics and so left this as user configurable option.

leelavg force-pushed the oom-fix branch from 50bc611 to 8e05abf Compare December 11, 2024 10:59

leelavg changed the title ~~restrict manager resource cache to only operator namespace~~ restrict manager resource cache based on namespaces from environment Dec 11, 2024

leelavg force-pushed the oom-fix branch from 8e05abf to 2d808b9 Compare December 11, 2024 11:03

leelavg force-pushed the oom-fix branch 2 times, most recently from 37772c2 to e69eb50 Compare December 19, 2024 04:04

leelavg force-pushed the oom-fix branch from e69eb50 to cd022f9 Compare December 19, 2024 04:16

Madhu-1 reviewed Jan 2, 2025

View reviewed changes

leelavg force-pushed the oom-fix branch from cd022f9 to a2e787d Compare January 3, 2025 06:16

leelavg added 2 commits January 3, 2025 06:17

add generated manifests

a2e787d

Signed-off-by: Leela Venkaiah G <[email protected]>

Madhu-1 reviewed Jan 3, 2025

View reviewed changes

Madhu-1 requested a review from nb-ohad January 3, 2025 08:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

restrict manager resource cache based on namespaces from environment #186

restrict manager resource cache based on namespaces from environment #186

leelavg commented Dec 11, 2024 •

edited

Loading

leelavg commented Dec 11, 2024

Madhu-1 commented Dec 11, 2024

leelavg commented Dec 11, 2024

Madhu-1 commented Dec 11, 2024

leelavg commented Dec 11, 2024 •

edited

Loading

leelavg commented Dec 19, 2024

leelavg commented Dec 19, 2024

Madhu-1 Jan 2, 2025

leelavg Jan 2, 2025

Madhu-1 Jan 2, 2025

leelavg Jan 3, 2025 •

edited

Loading

Madhu-1 Jan 3, 2025

Madhu-1 Jan 2, 2025

leelavg Jan 2, 2025

Madhu-1 Jan 2, 2025

leelavg Jan 3, 2025

Madhu-1 Jan 2, 2025

leelavg Jan 3, 2025

leelavg Jan 3, 2025

leelavg Jan 3, 2025

Madhu-1 Jan 3, 2025

leelavg Jan 3, 2025

Madhu-1 Jan 3, 2025

leelavg Jan 3, 2025

Madhu-1 Jan 3, 2025

Madhu-1 Jan 3, 2025

leelavg Jan 3, 2025

restrict manager resource cache based on namespaces from environment #186

Are you sure you want to change the base?

restrict manager resource cache based on namespaces from environment #186

Conversation

leelavg commented Dec 11, 2024 • edited Loading

leelavg commented Dec 11, 2024

Madhu-1 commented Dec 11, 2024

leelavg commented Dec 11, 2024

Madhu-1 commented Dec 11, 2024

leelavg commented Dec 11, 2024 • edited Loading

leelavg commented Dec 19, 2024

leelavg commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leelavg Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leelavg commented Dec 11, 2024 •

edited

Loading

leelavg commented Dec 11, 2024 •

edited

Loading

leelavg Jan 3, 2025 •

edited

Loading