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

Multi namespaces cache #62

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Multi namespaces cache #62

wants to merge 3 commits into from

Conversation

nishantapatil3
Copy link
Contributor

@nishantapatil3 nishantapatil3 commented Jul 5, 2023

Q A
Bug fix? no
New feature? no
API breaks? no
Deprecations? no
Related tickets fixes memory issue in multicluster scenario
License Apache 2.0

What's in this PR?

Contains the changes required to limit namespace cache based on ResourceSyncRule rules.match.namespaces

  • Adds the functionality to init cache for all namespaces if no namespaces list is provided in RSR or init cache with namespaces from RSR
  • syncReconciler and managedController are updated to init cache from namespaces provided from RSR rules.match.namespaces

Why?

In larger cluster with namespaces more than 30-40, it was observed that CR-controller watches/caches all the namespaces into the mem, which caused frequent OOMKilled issue while attaching peer clusters.
This PR allows users to select namespaces to cache for their RSR, there-by reducing memory usage.

Checklist

  • Implementation tested
  • User guide and development docs updated (if needed)

@nishantapatil3 nishantapatil3 requested a review from a team as a code owner July 5, 2023 19:59
for _, rule := range s.Spec.Rules {
for _, match := range rule.Matches {
if len(match.Namespaces) == 0 {
return namespaces
Copy link

@panyuenlau panyuenlau Jul 6, 2023

Choose a reason for hiding this comment

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

Question: Why are we returning namespaces here (I believe it is always empty if we reach this line)? Don't we need to check the next rule in the outermost for loop?

Copy link
Contributor

Choose a reason for hiding this comment

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

it returns there, because if there is a match without namespace filtering that means actually it cares about resources from every namespace

// GetRelatedNamespaces gathers namespaces from rule matchers
// it returns an empty list if any of the matchers is without namespace filter
// since it means every namespace is related
func (s *ResourceSyncRule) GetRelatedNamespaces() []string {
Copy link

@panyuenlau panyuenlau Jul 6, 2023

Choose a reason for hiding this comment

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

It'd be great if we can add an unit test for this function

for _, rule := range s.Spec.Rules {
for _, match := range rule.Matches {
if len(match.Namespaces) == 0 {
return namespaces
Copy link
Contributor

Choose a reason for hiding this comment

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

it returns there, because if there is a match without namespace filtering that means actually it cares about resources from every namespace

@@ -66,9 +67,10 @@ func WithRequiredClusterFeatures(features ...ClusterFeatureRequirement) ManagedC
}
}

func NewManagedController(name string, r ManagedReconciler, l logr.Logger, options ...ManagedControllerOption) ManagedController {
func NewManagedController(name string, relatedNamespaces []string, r ManagedReconciler, l logr.Logger, options ...ManagedControllerOption) ManagedController {
Copy link
Contributor

Choose a reason for hiding this comment

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

create and use a new option here to set the related namespaces

@nishantapatil3 nishantapatil3 marked this pull request as draft July 6, 2023 16:18
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.

3 participants