-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
KEP for HPA scaling based on container resources #1609
KEP for HPA scaling based on container resources #1609
Conversation
Hi @arjunrn. Thanks for your PR. I'm waiting for a kubernetes 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/test-infra repository. |
/assign josephburnett |
### Risks and Mitigations | ||
|
||
In order to keep backward compatibility with the existing API both `ResourceMetricSource` and | ||
`PodResourceMetricSource` will be supported. Existing HPAs will continue functioning like before. |
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.
Again, maybe we should wait to rename a field until the V2 GA API.
67016b7
to
9b1fadd
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
/approve
Nicely written!
Signed-off-by: Arjun Naik <arjun.rn@gmail.com>
9b1fadd
to
1c6f86b
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.
Nicely written, and well expressed.
One thing other than some minor nitpicks is whether it's worth explicitly calling out in the user stories how it would behave if ContainerResource
and PodResource
/Resource
metrics were used as I could see users adopting this cautiously alongside existing working setups.
of the individual container usage values of the pod. This is unsuitable for workloads where | ||
the usage of the containers are not strongly correlated or change in lockstep. This KEP |
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.
This sentence currently reads unclearly to me - it seems to imply that the current HPA behaviour is unsuitable for workloads where the usage of the containers do change in lockstep though I realise that's not the intent.
467bdfe
to
6b54b56
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
/approve
Looks good, let's merge it, there will be probably minor adjustments after code/api review anyway. |
Please fix Table of Content.
|
Co-Authored-By: Guy Templeton <guyjtempleton@googlemail.com>
6b54b56
to
28b274b
Compare
/test pull-enhancements-verify |
@arjunrn: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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/test-infra repository. |
/ok-to-test |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arjunrn, josephburnett, mwielgus 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 |
this-week: 2024-04-19
This is an initial proposal to have scaling for target based on container resources.