refactor(model-serving): remove controller label propagation and add autoscaling example#877
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the propagation of the 'kthena.io/model-name' annotation from the ModelServing CR to the corresponding pod labels. The implementation includes validation to ensure the annotation value is a valid Kubernetes label and adds comprehensive unit tests to verify the propagation logic and precedence rules. I have reviewed the code and suggest removing the redundant '!exists' check in the label assignment logic, as the pod labels are initialized with a fixed set of keys that do not include this annotation.
| } else if _, exists := pod.Labels[workloadv1alpha1.ModelNameAnnotationKey]; !exists { | ||
| pod.Labels[workloadv1alpha1.ModelNameAnnotationKey] = modelName | ||
| } |
There was a problem hiding this comment.
The !exists check is redundant here because pod.Labels was just initialized a few lines above (lines 135-142) with a fixed set of keys that does not include ModelNameAnnotationKey. Removing this check simplifies the logic without changing behavior, as any subsequent overrides from the role template are handled later in addPodLabelAndAnnotation.
else {
pod.Labels[workloadv1alpha1.ModelNameAnnotationKey] = modelName
}|
/retest |
|
@FAUST-BENCHOU: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
17e10b4 to
a0851ac
Compare
|
dont see any remove work, only a yml example |
Adds a reference YAML showing how users can autoscale ModelServing with KEDA + Prometheus using a router-level metric. Labels are user-defined in the pod templates; no controller-side label propagation is introduced. Signed-off-by: WHOIM1205 <[email protected]>
a0851ac to
b0641a7
Compare
Hey @FAUST-BENCHOU thanks for the feedback ive cleaned up the pr a bit removed the earlier controller side label propagation approach and kept it aligned with using user defined labels only also squashed the commits so the diff reflects the final state more clearly right now it just adds the autoscaling example with prometheus and keda without introducing any controller changes happy to adjust anything further if needed |
If you’ve done these things, you should update the PR title and description. |
Summary
This PR updates the autoscaling example for ModelServing based on the feedback received.
Instead of adding any controller-level logic or fixed labels, the example now relies on user-defined labels in the ModelServing templates.
What was removed
kthena.io/model-name)The idea here is to keep the controller generic and not enforce any specific labeling pattern. Users can define labels as needed.
What was added
examples/model-serving/autoscaling-with-keda.yamlThe example demonstrates:
Fixes & Improvements
Fixed Prometheus query label:
model_name→model(to match the actual metric)Replaced hardcoded values with placeholders:
<model-name><modelserving-name><prometheus-url>Clarified a few things in comments:
Added:
pollingInterval: 15cooldownPeriod: 120(can be tuned depending on workload)How it works
modellabelspec.replicason ModelServingNotes
Context
In an earlier iteration of this PR, I tried adding controller-side label propagation, but based on feedback, that approach was dropped.
The current version keeps things simple and just adds an example using user-defined labels.
Goal
Provide a simple and flexible example of autoscaling with Prometheus + KEDA, without introducing any opinionated controller changes.