[Autoscaler] Add validation to require RayCluster v2 when using idleTimeoutSeconds #4162

alimaazamat · 2025-10-30T21:48:32Z

Why are these changes needed?

V2 Autoscaling allows for configuring idleTimeoutSeconds per worker group, which needs added unit + e2e testing
EDIT: e2e testing already done in #2725 issue should've been closed

Related issue number

#2561

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests
- This PR is not tested :(

marosset · 2025-10-31T16:01:02Z

LGTM

ray-operator/controllers/ray/utils/validation.go

andrewsykim · 2025-11-13T18:18:32Z

@ryanaoleary can you review since you added the feature?

ryanaoleary

LGTM, I tested some of these cases manually and it looks correct.

andrewsykim · 2025-11-13T22:08:42Z

@alimaazamat the e2e buildkite failure seems related? Can you take a look https://buildkite.com/ray-project/ray-ecosystem-ci-kuberay-ci/builds/11598#019a7efb-eeee-48fd-8614-6bd5d5311f4e

Triggered a retry incase it's a flake

alimaazamat · 2025-11-13T22:18:37Z

@alimaazamat the e2e buildkite failure seems related? Can you take a look https://buildkite.com/ray-project/ray-ecosystem-ci-kuberay-ci/builds/11598#019a7efb-eeee-48fd-8614-6bd5d5311f4e

@andrewsykim Yeah I reran it already previously and it did fail again. Because this are just unit tests I thought it wouldn't impact e2e?

alimaazamat · 2025-11-13T22:23:10Z

Actually I think I found the issue:
Seems like part2 autoscaler tests are using env var only and not the spec version.

alimaazamat · 2025-11-14T00:26:57Z

@andrewsykim
In buildkite the only errors I'm seeing are reconcillation errors where worker pod status is failed during teardown. https://github.com/alimaazamat/kuberay/blob/idleTimeoutSecondsTests/ray-operator/controllers/ray/raycluster_controller.go#L173 is the only time we call our ValidateRayClusterSpec changes and if it is invalid, the controller would return immediately and wouldn't even hit the errors? Let me know if my thinking is on the wrong track.

rueian · 2025-12-01T20:40:03Z

ray-operator/test/e2eautoscaler/raycluster_autoscaler_part2_test.go

+			WithAutoscalerOptions(rayv1ac.AutoscalerOptions().
+				WithVersion(rayv1.AutoscalerVersionV2)).


Suggested change

WithAutoscalerOptions(rayv1ac.AutoscalerOptions().

WithVersion(rayv1.AutoscalerVersionV2)).

Are these required? The tests[1] used here already has autoscaler v2 enabled.

Even thought tests[1] has v2 enabled as an env variable, but doesn't actually change the spec without

WithAutoscalerOptions(rayv1ac.AutoscalerOptions(). WithVersion(rayv1.AutoscalerVersionV2)).

I wanted it updated in the spec as well for the validation tests I wrote, since in validation.go the other tests reference the spec not env vars e.g. workerGroup.Template.Spec.RestartPolicy

https://github.com/alimaazamat/kuberay/blob/idleTimeoutSecondsTests/ray-operator/controllers/ray/utils/validation.go#L180 "Please only use the former" spec version not env var

I also get
The RayCluster spec is invalid test-ns-k5rcx/ray-cluster: worker group short-idle-timeout-group has idleTimeoutSeconds set, but autoscaler version is not v2. Please set .spec.autoscalerOptions.version to v2 |

jackfrancis

lgtm

marosset

LGTM

Signed-off-by: alimaazamat <alima.azamat2003@gmail.com>

alimaazamat · 2025-12-04T23:02:39Z

@andrewsykim @ryanaoleary @rueian Changed validation to use env var for autoscaler v2 but according to this: #3578 and this comment #3578 (comment) I think we prefer using spec version not the env var. I am opening up a separate PR to change tests[1] to use the spec version instead of the env var.
EDIT: changed behavior to prefer spec and fall back to env var depending on Kuberay version based on documentation https://docs.ray.io/en/master/cluster/kubernetes/user-guides/configuring-autoscaling.html

win5923

LGTM, validating env var looks better for me. This also supports users of KubeRay versions < 1.4.0, which do not allow using the spec version. Thank you!

ray-operator/controllers/ray/utils/validation.go

Co-authored-by: Jun-Hao Wan <ken89@kimo.com> Signed-off-by: Alima Azamat <92766804+alimaazamat@users.noreply.github.com> Signed-off-by: alimaazamat <alima.azamat2003@gmail.com>

alimaazamat · 2025-12-05T19:53:44Z

First check if spec version is set with IsAutoscalingV2Enabled, if not then fall back to using env var. This works with these instructions in docs: https://docs.ray.io/en/master/cluster/kubernetes/user-guides/configuring-autoscaling.html
If you’re using KubeRay >= 1.4.0, enable V2 by setting RayCluster.spec.autoscalerOptions.version: v2.
If you’re using KubeRay < 1.4.0, enable V2 by setting the RAY_enable_autoscaler_v2 environment variable in the head and using restartPolicy: Never on head and all worker groups.
Updated tests to reflect this validation behavior as well.

win5923

LGTM! cc @rueian for final review.

Future-Outlier · 2025-12-06T07:14:36Z

ray-operator/controllers/ray/utils/validation.go

+		if err := validateWorkerGroupIdleTimeout(workerGroup, spec); err != nil {
+			return err
+		}


Can we put the logic to here?

kuberay/ray-operator/controllers/ray/utils/validation.go

Lines 184 to 195 in 5282a64

if IsAutoscalingV2Enabled(spec) {

if spec.HeadGroupSpec.Template.Spec.RestartPolicy != "" && spec.HeadGroupSpec.Template.Spec.RestartPolicy != corev1.RestartPolicyNever {

return fmt.Errorf("restartPolicy for head Pod should be Never or unset when using autoscaler V2")

}

for _, workerGroup := range spec.WorkerGroupSpecs {

if workerGroup.Template.Spec.RestartPolicy != "" && workerGroup.Template.Spec.RestartPolicy != corev1.RestartPolicyNever {

return fmt.Errorf("restartPolicy for worker group %s should be Never or unset when using autoscaler V2", workerGroup.GroupName)

}

}

}

}

Can we also add validation logic for AutoscalerOptions.IdleTimeoutSeconds?

kuberay/ray-operator/apis/ray/v1/raycluster_types.go

Lines 189 to 192 in 58c2aad

// IdleTimeoutSeconds is the number of seconds to wait before scaling down a worker pod which is not using Ray resources.

// Defaults to 60 (one minute). It is not read by the KubeRay operator but by the Ray autoscaler.

// +optional

IdleTimeoutSeconds *int32 `json:"idleTimeoutSeconds,omitempty"`

@Future-Outlier Why would we put the validation logic in validateWorkerGroupIdleTimeout? Above it we have other validation functions like validateRayGroupLabels and validateRayGroupResources that are structured the same way: where we define those functions seperate.

For validation logic for AutoscalerOptions.IdleTimeoutSeconds I agree, let's do that in a seperate PR. This one is scoped out for worker groups.

alimaazamat requested review from MortalHappiness, andrewsykim, kevin85421 and rueian as code owners October 30, 2025 21:48

marosset self-requested a review October 30, 2025 21:52

alimaazamat changed the title ~~test: add unit and e2e tests for idleTimeoutSeconds config per worker…~~ test: add unit tests for idleTimeoutSeconds config per worker… Oct 30, 2025

alimaazamat changed the title ~~test: add unit tests for idleTimeoutSeconds config per worker…~~ test: unit tests for idleTimeoutSeconds config per worker groups Oct 30, 2025

alimaazamat mentioned this pull request Oct 31, 2025

[Feature] Add e2e Ray v2 Autoscaler Tests with KubeRay idleTimeoutSeconds per worker group #2561

Closed

2 tasks

win5923 reviewed Nov 13, 2025

View reviewed changes

ray-operator/controllers/ray/utils/validation.go Show resolved Hide resolved

alimaazamat force-pushed the idleTimeoutSecondsTests branch 2 times, most recently from a4bc057 to ae88086 Compare November 13, 2025 19:45

ryanaoleary approved these changes Nov 13, 2025

View reviewed changes

rueian reviewed Dec 1, 2025

View reviewed changes

alimaazamat changed the title ~~test: unit tests for idleTimeoutSeconds config per worker groups~~ test: validation tests for idleTimeoutSeconds config per worker groups Dec 3, 2025

alimaazamat changed the title ~~test: validation tests for idleTimeoutSeconds config per worker groups~~ test: idleTimeoutSeconds config per worker groups validation for RayClusters Dec 3, 2025

alimaazamat changed the title ~~test: idleTimeoutSeconds config per worker groups validation for RayClusters~~ Add validation to require RayCluster v2 when using idleTimeoutSeconds Dec 3, 2025

alimaazamat force-pushed the idleTimeoutSecondsTests branch 2 times, most recently from 4475ce5 to 3fada64 Compare December 4, 2025 00:24

jackfrancis approved these changes Dec 4, 2025

View reviewed changes

marosset approved these changes Dec 4, 2025

View reviewed changes

alimaazamat force-pushed the idleTimeoutSecondsTests branch from c9677d9 to 7be6c82 Compare December 4, 2025 22:15

add validation for idleTimeoutSeconds config per worker groups

a608d94

Signed-off-by: alimaazamat <alima.azamat2003@gmail.com>

alimaazamat force-pushed the idleTimeoutSecondsTests branch from 7be6c82 to a608d94 Compare December 4, 2025 22:17

alimaazamat changed the title ~~Add validation to require RayCluster v2 when using idleTimeoutSeconds~~ [Autoscaler] Add validation to require RayCluster v2 when using idleTimeoutSeconds Dec 5, 2025

win5923 approved these changes Dec 5, 2025

View reviewed changes

win5923 self-requested a review December 5, 2025 06:33

win5923 reviewed Dec 5, 2025

View reviewed changes

ray-operator/controllers/ray/utils/validation.go Show resolved Hide resolved

alimaazamat force-pushed the idleTimeoutSecondsTests branch from ded1e68 to db7a2c6 Compare December 5, 2025 18:45

Check spec version then fall back to env var

d76a861

Co-authored-by: Jun-Hao Wan <ken89@kimo.com> Signed-off-by: Alima Azamat <92766804+alimaazamat@users.noreply.github.com> Signed-off-by: alimaazamat <alima.azamat2003@gmail.com>

alimaazamat force-pushed the idleTimeoutSecondsTests branch from db7a2c6 to d76a861 Compare December 5, 2025 19:53

alimaazamat requested review from rueian and win5923 December 5, 2025 22:09

win5923 approved these changes Dec 6, 2025

View reviewed changes

alimaazamat mentioned this pull request Dec 6, 2025

[WIP][Autoscaler] Use spec not env for v2 Autoscaler #4257

Closed

4 tasks

Future-Outlier reviewed Dec 6, 2025

View reviewed changes

medmes approved these changes Dec 9, 2025

View reviewed changes

		WithAutoscalerOptions(rayv1ac.AutoscalerOptions().
		WithVersion(rayv1.AutoscalerVersionV2)).

	if IsAutoscalingV2Enabled(spec) {
	if spec.HeadGroupSpec.Template.Spec.RestartPolicy != "" && spec.HeadGroupSpec.Template.Spec.RestartPolicy != corev1.RestartPolicyNever {
	return fmt.Errorf("restartPolicy for head Pod should be Never or unset when using autoscaler V2")
	}

	for _, workerGroup := range spec.WorkerGroupSpecs {
	if workerGroup.Template.Spec.RestartPolicy != "" && workerGroup.Template.Spec.RestartPolicy != corev1.RestartPolicyNever {
	return fmt.Errorf("restartPolicy for worker group %s should be Never or unset when using autoscaler V2", workerGroup.GroupName)
	}
	}
	}
	}

	// IdleTimeoutSeconds is the number of seconds to wait before scaling down a worker pod which is not using Ray resources.
	// Defaults to 60 (one minute). It is not read by the KubeRay operator but by the Ray autoscaler.
	// +optional
	IdleTimeoutSeconds *int32 `json:"idleTimeoutSeconds,omitempty"`

[Autoscaler] Add validation to require RayCluster v2 when using idleTimeoutSeconds #4162

Are you sure you want to change the base?

[Autoscaler] Add validation to require RayCluster v2 when using idleTimeoutSeconds #4162

Conversation

alimaazamat commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

marosset commented Oct 31, 2025

Uh oh!

Uh oh!

andrewsykim commented Nov 13, 2025

Uh oh!

ryanaoleary left a comment

Choose a reason for hiding this comment

Uh oh!

andrewsykim commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alimaazamat commented Nov 13, 2025

Uh oh!

alimaazamat commented Nov 13, 2025

Uh oh!

alimaazamat commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rueian Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

alimaazamat Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alimaazamat Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

alimaazamat Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

jackfrancis left a comment

Choose a reason for hiding this comment

Uh oh!

marosset left a comment

Choose a reason for hiding this comment

Uh oh!

alimaazamat commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

win5923 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alimaazamat commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

win5923 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Future-Outlier Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alimaazamat Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

alimaazamat Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

alimaazamat commented Oct 30, 2025 •

edited

Loading

andrewsykim commented Nov 13, 2025 •

edited

Loading

alimaazamat commented Nov 14, 2025 •

edited

Loading

alimaazamat Dec 2, 2025 •

edited

Loading

alimaazamat commented Dec 4, 2025 •

edited

Loading

alimaazamat commented Dec 5, 2025 •

edited

Loading

win5923 left a comment •

edited

Loading

Future-Outlier Dec 6, 2025 •

edited

Loading