-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: improve memory usage by using metadata informer #5424
fix: improve memory usage by using metadata informer #5424
Conversation
71f8be6
to
6c31947
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.
lgtm. Do you have numbers comparing memory usage before/after the PR?
If this is useful, the same change should be done for the alertmanager and thanosruler controllers (but can be follow-up PRs).
pkg/informers/kube.go
Outdated
} | ||
|
||
// NewMetadataInformerFactory creates factories for kube resources without | ||
// loading only the metadata information of the resource for the given allowed, |
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 doesn't read correctly?
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 catch! :) sometimes, find and replace just doesn't cut it. 😂
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! Awsome work, I wasn't aware that such informers existed 🤯
Thanks to @simonpasquier for pointing it out! 🙇 |
@simonpasquier yes, I have been updating #5410 with new numbers. I will update here as well. Thanks a lot for the feedback |
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. Waiting for the latest numbers before merging :)
As I wrote earlier, we probably need the same change for Alertmanager & ThanosRuler but it can be a follow-up PR.
Here is the difference between 2 versions after adding 100 secrets; tldr is ...
create-secrets.sh
New Operator using MetadataInformer
The current operator
|
This patch uses metadatainformer package to watch secrets and configmaps so that all content need not be loaded into memory thus reducing the amount of memory consumed. Fixes: prometheus-operator#5410 Signed-off-by: Sunil Thaha <sthaha@redhat.com>
6c31947
to
4ea54d8
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.
great!
The Prometheus agent controller didn't use the metadata informer for secrets and configmaps as implemented in prometheus-operator#5424 and prometheus-operator#5448 for the other controllers. Signed-off-by: Simon Pasquier <spasquie@redhat.com>
The Prometheus agent controller didn't use the metadata informer for secrets and configmaps as implemented in prometheus-operator#5424 and prometheus-operator#5448 for the other controllers. Signed-off-by: Simon Pasquier <spasquie@redhat.com>
The Prometheus agent controller didn't use the metadata informer for secrets and configmaps as implemented in prometheus-operator#5424 and prometheus-operator#5448 for the other controllers. Signed-off-by: Simon Pasquier <spasquie@redhat.com>
The Prometheus agent controller didn't use the metadata informer for secrets and configmaps as implemented in prometheus-operator#5424 and prometheus-operator#5448 for the other controllers. Signed-off-by: Simon Pasquier <spasquie@redhat.com>
The Prometheus agent controller didn't use the metadata informer for secrets and configmaps as implemented in prometheus-operator#5424 and prometheus-operator#5448 for the other controllers. Signed-off-by: Simon Pasquier <spasquie@redhat.com>
This patch uses metadatainformer package to watch secrets and configmaps so that all content need not be loaded into memory thus reducing the amount of memory consumed.
Description
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request.
If it fixes a bug or resolves a feature request, be sure to link to that issue.
Type of change
What type of changes does your code introduce to the Prometheus operator? Put an
x
in the box that apply.CHANGE
(fix or feature that would cause existing functionality to not work as expected)FEATURE
(non-breaking change which adds functionality)BUGFIX
(non-breaking change which fixes an issue)ENHANCEMENT
(non-breaking change which improves existing functionality)NONE
(if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)Changelog entry
Prometheus Operator now consumes less memory that before when there are lots of secrets and config-maps
present in the cluster.