change AutoscalingPolicyBinding minReplicas minimum from 0 to 1#904
change AutoscalingPolicyBinding minReplicas minimum from 0 to 1#904Abirdcfly wants to merge 1 commit intovolcano-sh:mainfrom
Conversation
Signed-off-by: Abirdcfly <fp544037857@gmail.com>
|
[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 |
|
Welcome @Abirdcfly! It looks like this is your first PR to volcano-sh/kthena 🎉 |
There was a problem hiding this comment.
Pull request overview
This PR raises the minimum allowed minReplicas for AutoscalingPolicyBinding’s homogeneous target from 0 to 1 to prevent scale-to-zero from getting “stuck” (no pods → no metrics → autoscaler skips recommendations), and updates the shipped CRD/doc artifacts accordingly.
Changes:
- Update kubebuilder validation for
HomogeneousTarget.minReplicastoMinimum=1 - Update generated CRD reference documentation to reflect the new minimum
- Update the Helm chart CRD schema to enforce
minimum: 1for homogeneousminReplicas
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/apis/workload/v1alpha1/autoscalingpolicybinding_types.go | Tightens API validation for homogeneous minReplicas from 0 → 1 |
| docs/kthena/docs/reference/crd/workload.serving.volcano.sh.md | Updates CRD reference table to reflect the new minimum |
| charts/kthena/charts/workload/crds/workload.serving.volcano.sh_autoscalingpolicybindings.yaml | Updates Helm-distributed CRD OpenAPI schema minimum for homogeneous minReplicas |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // MinReplicas defines the minimum number of replicas to maintain. | ||
| // +kubebuilder:validation:Minimum=0 | ||
| // +kubebuilder:validation:Minimum=1 | ||
| // +kubebuilder:validation:Maximum=1000000 | ||
| MinReplicas int32 `json:"minReplicas"` |
There was a problem hiding this comment.
This change tightens validation for HomogeneousTarget.minReplicas, but AutoscalingPolicyBinding also has HeterogeneousTargetParam.minReplicas still allowing 0. If all heterogeneous params are configured with minReplicas=0 and the workloads scale to 0, the optimizer path can still get stuck skipping recommendations (no pods => no metrics) and never scale back up. Consider also enforcing a non-zero floor for heterogeneous mode (e.g., an XValidation requiring sum(params[].minReplicas) >= 1 or at least one param.minReplicas >= 1), or adjusting the recommendation logic to recover from 0 replicas without metrics.
| Target Target `json:"target,omitempty"` | ||
| // MinReplicas defines the minimum number of replicas to maintain. | ||
| // +kubebuilder:validation:Minimum=0 | ||
| // +kubebuilder:validation:Minimum=1 |
There was a problem hiding this comment.
Raising the CRD minimum from 0 to 1 is a breaking validation change: existing AutoscalingPolicyBinding objects with spec.homogeneousTarget.minReplicas=0 will become invalid and may block updates/helm upgrades until users edit them. Consider adding an explicit upgrade note (and/or a conversion/mutating webhook behavior) so clusters can migrate smoothly.
| // +kubebuilder:validation:Minimum=1 | |
| // Keep 0 allowed for backward compatibility with existing persisted AutoscalingPolicyBinding objects. | |
| // +kubebuilder:validation:Minimum=0 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the minimum value for minReplicas from 0 to 1 across the CRD definition, documentation, and the HomogeneousTarget struct to prevent autoscaling issues. A review comment identifies that the same change should be applied to the HeterogeneousTargetParam struct to ensure consistency and prevent similar issues when using heterogeneous targets.
| Target Target `json:"target,omitempty"` | ||
| // MinReplicas defines the minimum number of replicas to maintain. | ||
| // +kubebuilder:validation:Minimum=0 | ||
| // +kubebuilder:validation:Minimum=1 |
There was a problem hiding this comment.
The change to minReplicas minimum should also be applied to HeterogeneousTargetParam (line 116) to ensure consistency and prevent the same 'scale to 0' issue when using heterogeneous targets. Currently, HeterogeneousTargetParam.MinReplicas still allows a minimum of 0, which according to the PR description, causes the autoscaler to skip recommendation calculations due to lack of metrics.
What type of PR is this?
/kind bug
What this PR does / why we need it:
User Guide: Kthena Autoscaler
show:
kthena/docs/kthena/docs/user-guide/autoscaler.md
Lines 82 to 83 in 26678da
When
minReplicas=0, workloads scale down to 0 and cannot auto-scale back up - no running pods mean no metrics collection, so autoscaler always skips recommendation calculation, requires manual intervention to restore. controller logs always showpod list is nullandskip recommended instancesWhich issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: