Feature/kubectl plugin/improve support for autoscaling clusters 3832 #4146
Conversation
AndySung320
force-pushed
the
feature/3832-kubectl-autoscale-support
branch
from
591c59e to
a7fd7dd
Compare
AndySung320
force-pushed
the
feature/3832-kubectl-autoscale-support
branch
from
a7fd7dd to
18a5c9f
Compare
|
@win5923 hi~ please take a look, thanks! |
machichima
left a comment
machichima
left a comment
There was a problem hiding this comment.
Can we also add e2e test to test the actual behavior?
| _, err = k8sClient.RayClient().RayV1().RayClusters(options.namespace).Update(ctx, cluster, metav1.UpdateOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to scale worker group %s in Ray cluster %s in namespace %s: %w", options.workerGroup, options.cluster, options.namespace, err) | ||
| var currentMinReplicas, currentMaxReplicas int32 |
There was a problem hiding this comment.
I think we need to set currentMaxReplicas default to math.MaxInt32
| if options.minReplicas != nil && *options.minReplicas >= 0 && *options.minReplicas != currentMinReplicas { | ||
| cluster.Spec.WorkerGroupSpecs[workerGroupIndex].MinReplicas = options.minReplicas | ||
| changes = append(changes, fmt.Sprintf("Updated minReplicas: %d to %d", currentMinReplicas, *options.minReplicas)) | ||
| hasChanges = true |
There was a problem hiding this comment.
I think we can set hasChanges above in if options.minReplicas != nil && *options.minReplicas >= 0 block here
And for setting value cluster.Spec.WorkerGroupSpecs[workerGroupIndex].MinReplicas, we can make it equal to finalMinReplicas so that we can remove this part to make things cleaner
There was a problem hiding this comment.
Thanks for pointing this out!
I’ll refactor the update logic for minReplicas, maxReplicas, and replicas to use the final* variables directly within their respective blocks, so that the hasChanges and changes handling can be done inline.
Thanks for the suggestion. |
In a PR, thank you! |
…mmand (ray-project#3832) Signed-off-by: AndySung320 <[email protected]>
Signed-off-by: AndySung320 <[email protected]>
- Add getWorkerGroupValues helper function to support.go - Add e2e tests for 'kubectl ray scale cluster' command - Add e2e tests for 'kubectl ray get workergroups' command Signed-off-by: AndySung320 <[email protected]>
Refactor the update logic for minReplicas, maxReplicas, and replicas to use the final* variables directly within their respective blocks. Signed-off-by: AndySung320 <[email protected]>
Signed-off-by: AndySung320 <[email protected]>
AndySung320
force-pushed
the
feature/3832-kubectl-autoscale-support
branch
from
9f2aef6 to
a3901e8
Compare
|
hi @machichima @Future-Outlier , |
4 participants
Why are these changes needed?
This PR introduces two enhancements to the kubectl plugin to improve autoscaling support.
kubectl ray get workergroupsminandmaxreplica counts for autoscaling groups, in addition to the existingcurrentanddesiredfields.kubectl ray scale clusterminandmaxreplicas, along with the existingdesiredreplica count.Related issue number
Closes #3832
Checks