-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(source): use transformers in pod informers to reduce memory footprint #5596
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
feat(source): use transformers in pod informers to reduce memory footprint #5596
Conversation
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. |
/ok-to-test |
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.
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
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look at what is needed here.
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.
Looking at the code I think the informer can be fully split:
- when
fqdnTemplate
is unset this change will be used - when
fqdnTemplate
is 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 |
8638370
to
c6772e5
Compare
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.
Probably missing evidences that there is a memory reduction. Rest looks quite nice, going to apply similar transfomers elsewhere soon
source/pod.go
Outdated
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.
Do we need annotation kubectl.kubernetes.io/last-applied-configuration
? Just a question, not a big deal
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.
Good remark, we could at least drop this one. We don't use it internally so had no impact but definitely a potential memory horder for people using apply.
Will add this
source/pod.go
Outdated
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.
Worth to rebase with master branch. This most likely not going to behave correctly in cases when
- annotationFilter is present
- labelSelector is present
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.
Let me rebase and assess what we need to store
Sorry for the delay, I actually discovered a missed detail when testing the upstream client-go that is needed for this, which led me to prioritize this fix first.
From internal forks, those are examples of gains with and without the gains from client-go:
with (different cluster hence initial state is worse)
|
source/service.go
Outdated
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.
How this will behave in a scenario
- someone decided to execute
kubectl patch pod ....
and modify/change annotaions, labels or some other value of the pod?
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.
The informer will receive a new object that will be transformed from scratch in this case. Client-go does not alter objects after them being passed to informers
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.
gotcha
@valerian-roche It looks really great. Do you think you can rebase this PR ? |
I think this PR will require
I think this is a fantastic solution. I'm already looking at how we can apply these improvements to our other sources as well. |
Add a transformer to the pods informer of the pod and service sources. Refs: kubernetes-sigs#5595 Signed-off-by: Valerian Roche <[email protected]>
c6772e5
to
2ed7bb3
Compare
I rebased the PR. This should be mergeable without the other PR from client-go being available, even if it will bring limited benefits for now |
/lgtm |
Co-authored-by: Michel Loiseleur <[email protected]>
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ivankatliarchuk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…print (kubernetes-sigs#5596) * feat: use transformers in pod informers to reduce memory footprint Add a transformer to the pods informer of the pod and service sources. Refs: kubernetes-sigs#5595 Signed-off-by: Valerian Roche <[email protected]> * Do not use transformer when fqdnTemplate is set * Update source/pod_test.go Co-authored-by: Michel Loiseleur <[email protected]> --------- Signed-off-by: Valerian Roche <[email protected]> Co-authored-by: Michel Loiseleur <[email protected]>
What does it do ?
This PR adds transformers to the pod informers in the
pod
andservice
sources 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