-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Added AEP for multi-dimensional pod autoscaler #5342
Merged
Merged
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
e0786ef
added KEP for multidimensional pod autoscaler
James-QiuHaoran b146789
Merge branch 'kubernetes:master' into master
James-QiuHaoran 1d4077a
added imgs to KEP
James-QiuHaoran 51fc51c
add future design diagram for mpa actuation & clarify questions in KE…
James-QiuHaoran ac07862
addressed comments from KEP review
James-QiuHaoran 1fc7345
added comments to container name in the spec
James-QiuHaoran 3bac442
Update multidimensional-pod-autoscaler/KEP.md
James-QiuHaoran 2cdd9f9
renamed from KEP to AEP - Autoscaler Enhancement Proposal
James-QiuHaoran ccba16a
Merge branch 'master' of https://github.com/James-QiuHaoran/autoscaler
James-QiuHaoran 08933f0
addressed review questions
James-QiuHaoran 89d736d
replaced SLO with SLA
James-QiuHaoran ab8ceaa
added SLO definition
James-QiuHaoran b65e03d
added percentageSLO to specify the percentage that an SLO value is met
James-QiuHaoran 9fd214d
moved SLO metrics to the recommender field and added metric name
James-QiuHaoran e2c0e18
removed slo specification
James-QiuHaoran 6c22e90
aligned naming with Google API
James-QiuHaoran File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 seems to me that just the target number is not enough.
Let's use the example from the doc: we're targeting 99% of requests handled in 100 ms or faster.
It seems that we need to provide at least 2 pieces of information:
The proposed API has only 1 field of type double, so I can provide 2) (target SLO value) but I don't see a way to specify 1) (metric we're targeting).
For beta I think we ill need ability to specify something more complicated than just a metric value (for example percentile of histogram metric, support for incremental metrics). But for alpha I think we need at least ability to specify what metric we're targetting.
Please amend the proposal to allow that or explain how it's possible with the current proposdal
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.
Hi @jbartosik , Thank you for the suggestion! We added an additional field to specify the percentage. It's a floating point number between 0 and 1. For example, if the latency SLO is in use, then it could be 99% of the requests finish within 100ms (i.e.,
applicationSLO = 100
andpercentageSLO = 0.99
). Accordingly, the replica set can be horizontally scaled when the measured latency is greater than 100ms, i.e., violating the SLO value. Similarly, throughput SLOs can be defined as throughput greater than 100/s 90% of the time, i.e.,applicationSLO = 100
andpercentageSLO = 0.9
.For alpha, the percentage is always the 99th percentile and only the SLO value will be used for horizontal autoscaling.
For beta, we will use both fields and to add the ability to specify more complicated types (e.g., incremental metrics).
Our previous design encodes both the percentage and value into a single field, i.e., the SLO field in the API object is simply used to represent the 99th percentile latency. Then the recommender compared the measured 99th percentile latency with the SLO value. This old design is deprecated because of the non-flexibility of adjusting the percentage and SLO value separately.
Please review the amended proposal and let us know if it clarifies your question. Thank you!
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.
@wangchen615 @James-QiuHaoran
I still think the proposal doesn't work. Now there are 2 fields:
applicationSLO
andpercentageSLO
This allows us to specify that we want value of 100 or less 99% of the time.
It still doesn't allow us to specify what metric we're targeting. (Which metric should have value 100 or less 99% of the time?)
Maybe it's better to do like @pbetkier suggested and put those parameters in recommender-specific place instead of putting them in the general API?
In addition to what Piotr pointed out (OS recommender is not going to use those fields so it's a bit weird to have them) it looks like this part of the API is not fully ready yet.