[RayCluster][Feature] skip suspending worker groups if the in-tree autoscaler is enabled #2748
Conversation
rueian
force-pushed
the
autoscaler-workergroup-suspend
branch
from
a68ca2e to
dfd71b8
Compare
rueian
force-pushed
the
autoscaler-workergroup-suspend
branch
from
dfd71b8 to
3e99db1
Compare
|
cc @andrewsykim, would you mind reviewing this PR after #2643 is merged? Thanks |
|
discussed offline: validate RayCluster spec and RayJob spec |
|
@rueian can you let me know once the validation logic is addeed? |
…toscaler is enabled to prevent ray cluster from malfunctioning Signed-off-by: Rueian <[email protected]>
…toscaler is enabled to prevent ray cluster from malfunctioning Signed-off-by: Rueian <[email protected]>
rueian
force-pushed
the
autoscaler-workergroup-suspend
branch
from
3e99db1 to
cb8a866
Compare
…toscaler is enabled to prevent ray cluster from malfunctioning Signed-off-by: Rueian <[email protected]>
…toscaler is enabled to prevent ray cluster from malfunctioning Signed-off-by: Rueian <[email protected]>
|
Hi @kevin85421 and @andrewsykim, This PR is ready and only focuses on validation on the RayCluster side. I will open another PR for validationon the RayJob side as well. |
|
Using the feature gate for this suspend field will also be another PR. |
ray-operator/controllers/ray/raycluster_controller_unit_test.go
Outdated
Show resolved
Hide resolved
| @@ -861,6 +861,47 @@ var _ = Context("Inside the default namespace", func() { | |||
| }) | |||
| }) | |||
|
|
|||
| Describe("Suspend RayCluster worker group with Autoscaler enabled", Ordered, func() { | |||
There was a problem hiding this comment.
A unit test seems sufficient if the validation happens at the very beginning of the reconciliation, so no other logic is involved.
There was a problem hiding this comment.
I think this test case could still be useful when we later add the support to suspend worker groups in an autoscaler-enabled cluster. Do you think we should delete it for now?
…toscaler is enabled to prevent ray cluster from malfunctioning Signed-off-by: Rueian <[email protected]>
rueian
force-pushed
the
autoscaler-workergroup-suspend
branch
from
68eb153 to
3155e13
Compare
|
@andrewsykim I merged this PR for now to move forward. Feel free to open a follow up PR if you have any comments. |
Why are these changes needed?
Skip suspending worker groups if the in-tree autoscaler is enabled to prevent ray cluster from malfunctioning.
The old autoscaler can't know if a worker group has been suspended on the KubeRay side, therefore, it will keep making wrong scaling decisions if some of the worker groups have been suspended. Such as:
To prevent a ray cluster from malfunctioning like that, we must forbid the usage of work group suspension on all old Ray versions that don't recognize the
suspendfield in the CR. However, we are unable to forbid usage by the controller without the validation webhook. In the case without the validation webhook, we can only fire a warning eventInvalidRayClusterSpecto let users know that this is forbidden.The validation webhook implementation will be in another PR.
Related issue number
Checks