-
Notifications
You must be signed in to change notification settings - Fork 8
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -137,6 +137,32 @@ const ( | |
|
||
type ResourceSyncRuleStatus struct{} | ||
|
||
// 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 { | ||
namespaces := []string{} | ||
|
||
_namespaces := map[string]struct{}{} | ||
|
||
for _, rule := range s.Spec.Rules { | ||
for _, match := range rule.Matches { | ||
if len(match.Namespaces) == 0 { | ||
return namespaces | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Why are we returning There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
for _, ns := range match.Namespaces { | ||
_namespaces[ns] = struct{}{} | ||
} | ||
} | ||
} | ||
|
||
for ns := range _namespaces { | ||
namespaces = append(namespaces, ns) | ||
} | ||
|
||
return namespaces | ||
} | ||
|
||
// +kubebuilder:object:root=true | ||
|
||
// ResourceSyncRule is the Schema for the resource sync rule API | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# Resource Sync Rule | ||
|
||
### Limit namespace cache | ||
In larger cluster with namespaces more than 30-40, it was observed that CR-controller watches/caches all the namespaces into the pod memory, | ||
which caused frequent OOMKilled issue while attaching peer clusters. | ||
|
||
To reduce memory caching by cluster-registry-controller we can use `namespaces` field in `ResourceSyncRule` to allow caching on selected | ||
namespaces. | ||
|
||
custom-resource | ||
`spec.rules.match.namespaces` | ||
``` | ||
namespaces: | ||
items: | ||
type: string | ||
type: array | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,7 @@ type ManagedControllers map[string]ManagedController | |
|
||
type managedController struct { | ||
name string | ||
relatedNamespaces []string | ||
reconciler ManagedReconciler | ||
log logr.Logger | ||
mgr ctrl.Manager | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. create and use a new option here to set the related namespaces |
||
m := &managedController{ | ||
name: name, | ||
relatedNamespaces: relatedNamespaces, | ||
reconciler: r, | ||
log: l.WithName(name), | ||
requiredClusterFeatures: make([]ClusterFeatureRequirement, 0), | ||
|
@@ -188,7 +190,15 @@ func (c *managedController) createAndStartCache() (cache.Cache, error) { | |
return nil, errors.New("context is nil") | ||
} | ||
|
||
cche, err := cache.New(c.mgr.GetConfig(), cache.Options{ | ||
cacheFunc := cache.New | ||
if len(c.relatedNamespaces) > 0 { | ||
c.log.Info("create multi namespace cache", "namespaces", c.relatedNamespaces) | ||
cacheFunc = cache.MultiNamespacedCacheBuilder(c.relatedNamespaces) | ||
} else { | ||
c.log.Info("create cache") | ||
} | ||
|
||
cche, err := cacheFunc(c.mgr.GetConfig(), cache.Options{ | ||
Scheme: c.mgr.GetScheme(), | ||
Mapper: c.mgr.GetRESTMapper(), | ||
}) | ||
|
There was a problem hiding this comment.
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