feat: use transformers in pod informers to reduce memory footprint #5596
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @valerian-roche! |
|
Hi @valerian-roche. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
There was a problem hiding this comment.
There is a PR open #5583. Not yet merged, as w8 for confirmation.
I think is going to be quite nice to move transfromers under source/informers/transformers.go file. We could identify some cool patterns and parity acros sources.
source/pod.go
Outdated
| // Transformer is used to reduce the memory usage of the informer. | ||
| // The pod informer will otherwise store a full in-memory, go-typed copy of all pod schemas in the cluster. | ||
| // If watchList is not used it will not prevent memory bursts on the initial informer sync. | ||
| podInformer.Informer().SetTransform(func(i interface{}) (interface{}, error) { |
There was a problem hiding this comment.
I wonder how this is going to work with FQDN template, we could do some pre-processing and attach it as a field ?
There was a problem hiding this comment.
I'll take a look at what is needed here.
There was a problem hiding this comment.
Looking at the code I think the informer can be fully split:
- when
fqdnTemplateis unset this change will be used - when
fqdnTemplateis set we actually do not need the pods at all, only informer events and we can drop even more content. I will update this PR to not use the transformer when the template is set for now.
The shared transformers are an interesting question as each source may use different parts of the pod. I can add one that's the superset (it will still drop a lot of the blueprint) but I'm unsure how this will evolve long term |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add a transformer to the pods informer of the pod and service sources. Refs: kubernetes-sigs#5595 Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
valerian-roche
force-pushed
the
vr/transformer
branch
from
8638370 to
c6772e5
Compare
| Name: pod.Name, | ||
| Namespace: pod.Namespace, | ||
| // Used by the controller. This includes non-external-dns prefixed annotations. | ||
| Annotations: pod.Annotations, |
There was a problem hiding this comment.
Do we need annotation kubectl.kubernetes.io/last-applied-configuration? Just a question, not a big deal
| // Name/namespace must always be kept for the informer to work. | ||
| Name: pod.Name, | ||
| Namespace: pod.Namespace, | ||
| // Used by the controller. This includes non-external-dns prefixed annotations. |
There was a problem hiding this comment.
Worth to rebase with master branch. This most likely not going to behave correctly in cases when
- annotationFilter is present
- labelSelector is present
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
needs-rebase
What does it do ?
This PR adds transformers to the pod informers in the
podandservicesources to reduce the resulting memory footprint.Motivation
As described in the issue, the pod informers currently lead to a large memory usage of the external-dns pods. As the controller only requires a few specific fields we can leverage transformers in the informers to greatly reduce the footprint.
In our environment this reduces the average memory usage by ~10 times, and once using watch list (disabled by default) and the next client-go release, we now have a peak memory usage 10 times smaller, greatly reducing our controller footprint.
This PR does not enable watchlist (but I'll raise a PR to optionally do so) as it is currently not really valuable without a fix in the next release of client-go.
More
Yes, I updated end user documentation accordinglyNot applicable, this change is purely technical within the informerRefs: #5595