Fix memory usage in catalog operator #362
Conversation
ecordell
force-pushed
the
catalog-fixes
branch
2 times, most recently
from
123a8bb to
1fcfd1c
Compare
namespace, not all namespaces
|
(Pipeline is passing now, status is just slow to update) |
| @@ -69,6 +67,13 @@ func NewOperator(kubeconfigPath string, wakeupInterval time.Duration, operatorNa | |||
| subSharedIndexInformers = append(subSharedIndexInformers, nsInformerFactory.Subscription().V1alpha1().Subscriptions().Informer()) | |||
| } | |||
|
|
|||
| // Create an informer for each catalog namespace | |||
| catsrcSharedIndexInformers := []cache.SharedIndexInformer{} | |||
| for _, namespace := range []string{operatorNamespace} { | |||
There was a problem hiding this comment.
Why range over a slice with a single item?
There was a problem hiding this comment.
Just because I know we'll implement "local" catalogs in the near future, where we'll want to iterate over operatorNamespace ∪ watchedNamespaces instead of just operatorNamespace
| @@ -139,7 +142,7 @@ func (o *Operator) syncCatalogSources(obj interface{}) (syncError error) { | |||
| defer o.sourcesLock.Unlock() | |||
| o.sources[catsrc.GetName()] = src | |||
| o.sourcesLastUpdate = timeNow() | |||
| return err | |||
| return nil | |||
There was a problem hiding this comment.
We mix this convention in the codebase (we know err == nil, but return err anyway). Which is preferred?
There was a problem hiding this comment.
I feel this is clearer, but I don't know if there's a golang convention for this. Which do you think is more readable?
| @@ -21,23 +21,22 @@ const ( | |||
|
|
|||
| // ConfigMapCatalogResourceLoader loads a ConfigMap of resources into the in-memory catalog | |||
| type ConfigMapCatalogResourceLoader struct { | |||
| Catalog *InMem | |||
There was a problem hiding this comment.
Why the change in this interface?
There was a problem hiding this comment.
This is fixing the memory leak, since the catalog returned was a field of the loader, and the loader holds a reference to part of the operator (operator client), every time we sync the catalog we were loading into memory and retaining it indefinitely.
We might still be leaking the loader itself now that you mention it...but at least that's a tiny amount instead of an entire in memory catalog
Fix memory usage in catalog operator
Fix memory usage in catalog operator
Also scopes the catalog operator to only watch the "catalogNamespace" for catalog sources (it was watching all before).
Mem usage before:
Mem usage after:
