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

Fix: Do not cache native resources created without CommonLabels #1818

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Baarsgaard
Copy link
Contributor

@Baarsgaard Baarsgaard commented Jan 11, 2025

I read a blog post on operator memory pitfalls mentioning Owns() being a footgun, which is used in the grafana_reconciler SetupWithManager.

TLDR: By declaring Owns() or using Get/List you tell the the controller-runtime to watch and cache all instances of the client.Object, which on large clusters could result in a lot of ConfigMaps, Secrets and Deployments in the Grafana-Operators case.

I expected this to be a problem due to the pprof profiles uploaded in #1622 which was verified by following the steps outlined below.

The post linked to an Operator SDK trick for configuring the client.Object cache with labels.

mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
  Cache: cache.Options{
    ByObject: map[client.Object]cache.ByObject{
      &corev1.Secret{}: cache.ByObject{
	Label: labels.SelectorFromSet(labels.Set{"app": "app-name"}),
      },
    },
  },
})

I remembered that #1661 added common labels to resources created by the operator to reduce memory consumption.

Verifying cache issues:

  1. Start a local kind cluster with some default resources (oneliner)
make start-kind && \
kind export kubeconfig --name kind-grafana && \
make ko-build-kind && \
IMG=ko.local/grafana/grafana-operator make deploy && \
kubectl patch deploy -n grafana-operator-system grafana-operator-controller-manager-v5  --type='json' -p='[{"op": "replace", "path": "/spec/template/spec/containers/0/imagePullPolicy", "value":"IfNotPresent"}]'
  1. Get a baseline heap reading
kubectl port-forward -n grafana-operator-system deploy/grafana-operator-controller-manager-v5 8888 &
go tool pprof -top -nodecount 20 http://localhost:8888/debug/pprof/heap
  1. Create empty test file: fallocate -l 393216 large_file
  2. Create a couple hundred ConfigMaps
for i in {0..200}; do kubectl create cm test-cm-$i --from-file=./large_file ; done
  1. Get Updated heap
go tool pprof -top -nodecount 20 http://localhost:8888/debug/pprof/heap
# Output on master branch
ile: v5
Type: inuse_space
Time: Jan 11, 2025 at 8:34pm (CET)
Showing nodes accounting for 54.72MB, 100% of 54.72MB total
Showing top 20 nodes out of 107
      flat  flat%   sum%        cum   cum%
   46.91MB 85.72% 85.72%    46.91MB 85.72%  k8s.io/api/core/v1.(*ConfigMap).Unmarshal # <--- this one
       2MB  3.66% 89.38%        2MB  3.66%  runtime.malg
       1MB  1.83% 91.20%        1MB  1.83%  encoding/json.typeFields
    0.75MB  1.37% 92.58%     0.75MB  1.37%  go.uber.org/zap/zapcore.newCounters
    0.54MB  0.99% 93.56%     0.54MB  0.99%  github.com/gogo/protobuf/proto.RegisterType
    0.52MB  0.94% 94.51%     0.52MB  0.94%  k8s.io/apimachinery/pkg/watch.(*Broadcaster).Watch.func1
    0.50MB  0.92% 95.43%     0.50MB  0.92%  unicode.map.init.1
    0.50MB  0.92% 96.34%     0.50MB  0.92%  k8s.io/apimachinery/pkg/runtime.(*Scheme).AddKnownTypeWithName
    0.50MB  0.91% 97.26%     0.50MB  0.91%  github.com/go-openapi/swag.(*indexOfInitialisms).sorted.func1
    0.50MB  0.91% 98.17%     0.50MB  0.91%  go.mongodb.org/mongo-driver/bson/bsoncodec.(*kindDecoderCache).Clone
....

Current progress

Watching and caching has been limited to resources controlled by the operator of Kind:

  • Deployment
  • Ingress
  • Service
  • ServiceAccount
  • PersistentVolumeClaim
  • Route if IsOpenShift

This is done with the existing CommonLabels selector introduced in #1661:
app.kubernetes.io/managed-by: "grafana-operator"

Memory consumption in an empty kind cluster after ~1 minute1:

Change Heap (kb) % reduction2 Note
None (master) 8579.22 0%
Limited resource listed above 5743.40 -33% New default
Limited ConfigMaps and Secrets 3585.48 -58% An experiment for now

TODO:

  • Allow users to tune the Informers Cache controlling ConfigMap/Secret using label selectors, similar to WATCH_NAMESPACE_SELECTOR
    Potentially a way to tune them individually.
  • Somehow test this? (Input is welcome)

Footnotes

  1. Heap will increase over time as the operator stabilizes.

  2. The reduction is by no means representative of real deployments.
    For clusters mixing the Grafana-Operator and other workloads in cluster scoped mode, the reduction is likely significantly higher.
    Even if the Grafana-Operator was the only Deployment in a cluster, this should still reduce memory as it won't cache itself 😉

@Baarsgaard Baarsgaard changed the title feat(internal): Ignore deployments/Configmaps missing CommonLabels WIP: Ignore deployments/Configmaps missing CommonLabels Jan 11, 2025
@Baarsgaard Baarsgaard force-pushed the reduce_cache_size branch 2 times, most recently from 389e8d6 to e4ed220 Compare January 11, 2025 23:56
@Baarsgaard Baarsgaard changed the title WIP: Ignore deployments/Configmaps missing CommonLabels Fix: Do not cache native resources created without CommonLabels Jan 12, 2025
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.

1 participant