feat(admission): enable hpa for KAI Admission Service#911
feat(admission): enable hpa for KAI Admission Service#911faizan-exe wants to merge 6 commits intoNVIDIA:mainfrom
Conversation
f312be0 to
0043f41
Compare
Merging this branch changes the coverage (1 decrease, 1 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
1 similar comment
Merging this branch changes the coverage (1 decrease, 1 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
a2448b8 to
0e468f1
Compare
Signed-off-by: Ahmed Faizan <faizanofficial120@gmail.com>
Signed-off-by: Ahmed Faizan <faizanofficial120@gmail.com>
Signed-off-by: Ahmed Faizan <faizanofficial120@gmail.com>
Signed-off-by: Ahmed Faizan <faizanofficial120@gmail.com>
Signed-off-by: Ahmed Faizan <faizanofficial120@gmail.com>
0e468f1 to
f5fef74
Compare
Signed-off-by: Ahmed Faizan <faizanofficial120@gmail.com>
|
|
||
| Add the Prometheus Adapter Helm repository: | ||
| ```bash | ||
| helm repo add prometheus-community https://prometheus-community.github.io/helm-charts |
There was a problem hiding this comment.
I think we can drop these two lines as they're duplicates from lines 4-5 in this file
| name: admission | ||
| namespace: kai-scheduler | ||
| labels: | ||
| accounting: kai-scheduler |
There was a problem hiding this comment.
I think we should have an "accounting" prometheus that's separate from a general prometheus for metrics (and autoscaling).
The reasons are:
- When using time aware fairness, the accounting prometheus is in the critical path of the scheduler. While the scheduler will work even if it's down, it could make very different scheduling decisions, causing unexpected preemptions. So we should keep it very minimal and keep load off of it.
- The accounting prometheus, in some expected use cases, needs to be persistent, even for a year or more of data. Since you can't choose which metrics are persisted and which aren't, this means that every metric that it's collecting will take that space. But the more operational metrics (like the ones used for HPA, or other metrics like scheduler metrics) might not need this persistency, causing wasted storage space
- The accounting prometheus can potentially have different sampling intervals than the general prometheus
Maybe we can find some design where the separation is configurable, or maybe it will just be easier to have them always separate.
What do you think?
Merging this branch changes the coverage (1 decrease, 1 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
| }, | ||
| Target: autoscalingv2.MetricTarget{ | ||
| Type: autoscalingv2.AverageValueMetricType, | ||
| AverageValue: resource.NewMilliQuantity(int64(*config.Autoscaling.CPUUtilizationPercent)*10, resource.DecimalSI), |
There was a problem hiding this comment.
Why is this multiplied by 10? What is the meaning of this Average Value here and why would it get down when scaling?
| deployment.Spec.Strategy.RollingUpdate = nil | ||
| deployment.Spec.Replicas = config.Replicas | ||
| if config.Autoscaling == nil || !*config.Autoscaling.Enabled { | ||
| deployment.Spec.Replicas = config.Replicas |
There was a problem hiding this comment.
Maybe it should be set to nil otherwise? notice that it reads the current deployment in and only updates those fields.
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| prometheus: | ||
| url: http://prometheus-operated.kai-scheduler.svc.cluster.local |
There was a problem hiding this comment.
We should use another instance
Description
This PR enables Horizontal Pod Autoscaling for Admission Pods. Previously, user can only specifiy fixed number of replicas for admission pods using
kai-config.yaml- now they have the option to enable autoscaling and avoid manual configuration.The metrics for autoscaling are
controller_runtime_webhook_requests_totalprocess_cpu_seconds_totalPrometheus Adapter is also introduced for interacting with Custom Metrics API.
Related Issues
Issue #901
Checklist
Breaking Changes
No breaking changes. Backward Compatibility is being ensured.