🐛 Fix to enable Cluster Autoscaler with Replicas field set in a Classy Cluster #7708
Conversation
|
Hi @abhijit-dev82. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. |
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
|
/retitle 🐛 Fix to enable Cluster Autoscaler with Replicas field set in a Classy Cluster |
| // Set the desired replicas only if external autoscaler is not controlling the MD replica count. | ||
| _, ok1 := machineDeploymentTopology.Metadata.Annotations[clusterv1.ClusterAutoscalerMaxAnnotation] | ||
| _, ok2 := machineDeploymentTopology.Metadata.Annotations[clusterv1.ClusterAutoscalerMinAnnotation] | ||
| if !ok1 || !ok2 { | ||
| desiredMachineDeploymentObj.Spec.Replicas = machineDeploymentTopology.Replicas | ||
| } |
There was a problem hiding this comment.
Overall I think that the approach described in #7293 (comment) makes more sense than what is proposed in this PR, but probably we should make another round of discussion when @sbueringer is back
Getting to the proposed change
- If I got right, this is going to modify the semantics about how the API behaves for adding the autoscaler, which is most likely a breaking that requires an API bump; the approach suggested in the link above instead does not require an API bump, making it possible to merge this feature in 1.4.
- The proposed implementation is going tol tie current behavior to one autoscaler implementation, which will prevent eg. usage of other autoscalers (Add Karpenter support #7198); I don't think this is advisable
- I'm not sure that the proposed change addresses the other concerns discussed on the related issue e.g.
- if we create a new cluster with the autoscaler set and min replicas >1, the MS will continue to be created with replicas 1 and the autoscale won't add the missing replicas until the scheduler will require more compute resources
- if we create a new cluster without the autoscaler and then add the autoscaler, the number of replicas will be reset to 1 no matter the current number of replicas or min/max replicas
- other edge cases due to operations order/async controllers working on the same object
|
PR needs rebase. 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. |
|
/close |
|
@fabriziopandini: Closed this PR. 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. |
What this PR does / why we need it:
For classy cluster if replica field set for each NodePool, the cluster topology reconciler overwrites the replicas count as set by the cluster autoscaler. This limits enabling cluster autoscaler as a Day-1 op on a classy cluster. With this fix the topology reconciler will skip updating the replica field if the autoscaler annotations are present under the NodePool.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #7293