-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Sharding metrics per node via fieldSelector #1864
Sharding metrics per node via fieldSelector #1864
Conversation
41219ce
to
c10b6dd
Compare
/assign |
Ping~ |
78ef86e
to
71b4d04
Compare
cc @rexagod, could you help review as well? Thanks! |
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.
Thanks, overall this looks good. I would suggest using NodeName
instead of Nodename
since it's more readable and it's also not a single word.
I'd be curious about the scaling model here, could you provide some details how this can be used as a reference for users? |
Reply @mrueg
When deployed as a daemonset, set nodename to current NODE_NAME.
|
13da89e
to
e1c9864
Compare
@CatherineF-dev Correct me if I'm wrong, but shouldn't the above example target |
Reply @rexagod It's a daemonset. Each kube-state-metrics agent will only collect pod metrics from the same node.
|
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.
Thank you for adding this change! 🎉
Commenting my review, instead of blocking this PR, since these are all optional suggestions.
036bab9
to
886ecc9
Compare
886ecc9
to
a9a972c
Compare
92f30ae
to
8f9dbea
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.
Overall looks good, I think tests need a bit more work.
8f9dbea
to
1cf0d42
Compare
Cleaned up. @fpetkovski I added some WIP codes after mrueg's review. |
This PR is ready to have a review cc @dgrisonnet |
1cf0d42
to
87a37c3
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.
A few comments, but the implementation looks good to me, great job @CatherineF-dev!
Co-authored-by: Manuel Rüger <manuel@rueg.eu>
417a876
to
9a2f5e0
Compare
9a2f5e0
to
43c6073
Compare
Validate options Sharding per node Clean Move merging fieldselectors into app/server.go and replace namespaceFitler with fieldSelectorFilter Refactoring Provide scaling example Update README.md Co-authored-by: Manuel Rüger <manuel@rueg.eu> Refactoring
Awesome work! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CatherineF-dev, dgrisonnet, mrueg 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 |
What this PR does / why we need it:
Increase scalability for big k8s cluster by sharding metrics per node. It only supports for resources which support nodeName fieldSelector. For example, pod metrics.
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality) No
Which issue(s) this PR fixes *:
Fixes #1863
Tested in a k8s cluster