[Feature] Support configurable RayCluster deletion delay in RayService #3864

machichima · 2025-07-12T06:51:27Z

Add the RayClusterDeletionDelaySeconds field to the RayService CRD to allow customization of the deletion delay (in seconds) before removing old RayClusters.
Enforce a minimum value constraint (>= 0) for RayClusterDeletionDelaySeconds in the CRD schema.
Update the controller logic to use the default value (60 seconds) if RayClusterDeletionDelaySeconds is omitted or set to a negative value.
Add tests to verify the correct handling of RayClusterDeletionDelaySeconds.

Why are these changes needed?

Previously, RayService would delete old RayCluster resources 60 seconds after they became unused. This could result in longer-running tasks on the old cluster being terminated before completion.

By making the deletion delay here configurable, users can adjust the delay to better suit their workloads and avoid premature termination of tasks.

Related issue number

Closes #3602

Checks

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

Signed-off-by: machichima <[email protected]>

machichima · 2025-07-13T04:52:03Z

@kevin85421 PTAL, Thanks!

fscnick · 2025-07-13T16:18:26Z

ray-operator/controllers/ray/rayservice_controller_unit_test.go

+			ts, exists := r.RayClusterDeletionTimestamps.Get(rayClusterName)
+			assert.True(t, exists, "Deletion timestamp should be set for the cluster")
+			expectedTs := now.Add(tc.expectedDuration)
+			assert.Equal(t, expectedTs.Unix(), ts.Unix(), "Deletion timestamp should match expected timestamp")


Could it be a potential flaky test to compare these two timestamps directly if the execution isn't quick enough between time.Now() and metav1.now() in cleanUpRayClusterInstance(...)?

If it is, how about using WithinDuration or WithinRange ?

Make sense! I changed to InDelta instead

Thanks!

ray-operator/controllers/ray/rayservice_controller.go

Signed-off-by: machichima <[email protected]>

rueian · 2025-07-14T22:55:00Z

ray-operator/controllers/ray/rayservice_controller.go

+			)
+		} else {
+			deletionDelay = time.Duration(*rayServiceInstance.Spec.RayClusterDeletionDelaySeconds) * time.Second
+			logger.Info("RayClusterDeletionDelaySeconds is set correctly and will be used.",


I wonder if the logs are too verbose. Will they be printed for every reconciliation before the shutdown is complete?

Yes I think so.

I removed the logging here when rayServiceInstance.Spec.RayClusterDeletionDelaySeconds is a valid value to prevent the loggings become too noisy.

I log the deletionDelay alongside with the deletionTimestamp in the existing logger here instead

Signed-off-by: machichima <[email protected]>

kevin85421

Overall LGTM. Because this is a public API and user-facing behavior change, would you mind opening an issue to track the progress of e2e tests?

kevin85421 · 2025-07-15T04:37:11Z

ray-operator/apis/ray/v1/rayservice_types.go

@@ -65,6 +65,11 @@ type RayServiceUpgradeStrategy struct {

 // RayServiceSpec defines the desired state of RayService
 type RayServiceSpec struct {
+	// RayClusterDeletionDelaySeconds specifies the delay in seconds before deleting old RayClusters.


Suggested change

// RayClusterDeletionDelaySeconds specifies the delay in seconds before deleting old RayClusters.

// RayClusterDeletionDelaySeconds specifies the delay, in seconds, before deleting old RayClusters.

// The default value is 60 seconds.

kevin85421 · 2025-07-15T04:37:22Z

ray-operator/apis/ray/v1/rayservice_types.go

@@ -65,6 +65,11 @@ type RayServiceUpgradeStrategy struct {

 // RayServiceSpec defines the desired state of RayService
 type RayServiceSpec struct {
+	// RayClusterDeletionDelaySeconds specifies the delay in seconds before deleting old RayClusters.
+	// Only values >= 0 are allowed. If omitted or negative, defaults to 60 seconds.


we should not reconcile the CR instead of falling back to the default value.

Suggested change

// Only values >= 0 are allowed. If omitted or negative, defaults to 60 seconds.

kevin85421 · 2025-07-15T04:40:15Z

ray-operator/controllers/ray/rayservice_controller.go

+	// Determine the ray cluster deletion delay seconds
+	deletionDelay := RayClusterDeletionDelayDuration
+	if rayServiceInstance.Spec.RayClusterDeletionDelaySeconds != nil {
+		if *rayServiceInstance.Spec.RayClusterDeletionDelaySeconds < 0 {


move the check to https://github.com/ray-project/kuberay/blob/master/ray-operator/controllers/ray/utils/validation.go

In addition, we have already checked the value in OpenAPI. Do we still need the check?

If we create the rayService in Golang from the RayService struct, the check in OpenAPI will not be performed. I add this condition here to ensure in any case, we can prevent the use of negative delay seconds.

Moved the check!

Signed-off-by: machichima <[email protected]>

machichima · 2025-07-15T12:40:23Z

Opened an issue for adding e2e test here: #3870

machichima · 2025-07-15T12:45:23Z

ray-operator/controllers/ray/rayservice_controller_unit_test.go

+			name:             "Use custom delay when set to positive",
+			delaySeconds:     ptr.To[int32](5),
+			expectedDuration: 5 * time.Second,
+		},


Does not test for delay value < 0 because the check is moved to ray-operator/controllers/ray/utils/validation.go

Will probably add tests for checking the behavior of setting negative deletion delay value in e2e test

kevin85421

LGTM. Please add a test to validation_test.go.

kevin85421 · 2025-07-15T16:23:37Z

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

@@ -256,5 +256,11 @@ func ValidateRayServiceSpec(rayService *rayv1.RayService) error {
 		*rayService.Spec.UpgradeStrategy.Type != rayv1.NewCluster {
 		return fmt.Errorf("Spec.UpgradeStrategy.Type value %s is invalid, valid options are %s or %s", *rayService.Spec.UpgradeStrategy.Type, rayv1.NewCluster, rayv1.None)
 	}
+


please also update validation_test.go.

Added, thanks!

Signed-off-by: machichima <[email protected]>

machichima added 14 commits July 12, 2025 14:47

feat: add CRD field for setting delay of deleting ray cluster

f2fc2a1

Signed-off-by: machichima <[email protected]>

feat: handle and doc non positive value

d8ce6ca

Signed-off-by: machichima <[email protected]>

test: for setting delete delay seconds

0244e78

Signed-off-by: machichima <[email protected]>

refactor: change order of new field

ada58ef

Signed-off-by: machichima <[email protected]>

refactor: log when delay second is set in CRD

a438a72

Signed-off-by: machichima <[email protected]>

fix: delay second int64 to int32

240319e

Signed-off-by: machichima <[email protected]>

fix: accept delay value 0

8d68e58

Signed-off-by: machichima <[email protected]>

fix: set min to 0 in spec

65b1214

Signed-off-by: machichima <[email protected]>

fix: update CRD, api, code-gen

69da6a7

Signed-off-by: machichima <[email protected]>

docs: simplify docs for delete delay second field

a0e7d9e

Signed-off-by: machichima <[email protected]>

fix: log time string instead of Second

2c30330

Signed-off-by: machichima <[email protected]>

fix: run make generate

0af9ce6

Signed-off-by: machichima <[email protected]>

test: update assert message

9af6c56

Signed-off-by: machichima <[email protected]>

refactor: move scheme and context init into each test case

7938362

Signed-off-by: machichima <[email protected]>

machichima marked this pull request as ready for review July 13, 2025 04:34

machichima changed the title ~~[Feature] Zero downtime upgrade for long-running requests~~ [Feature] Support configurable RayCluster deletion delay in RayService Jul 13, 2025

fscnick reviewed Jul 13, 2025

View reviewed changes

test: assert timestamp with InDelta to prevent flaky

880196c

Signed-off-by: machichima <[email protected]>

fscnick approved these changes Jul 14, 2025

View reviewed changes

kevin85421 assigned rueian, MortalHappiness and andrewsykim Jul 14, 2025

rueian reviewed Jul 14, 2025

View reviewed changes

refactor: prevent logging if delay second is correctly set

6f15507

Signed-off-by: machichima <[email protected]>

rueian approved these changes Jul 15, 2025

View reviewed changes

kevin85421 reviewed Jul 15, 2025

View reviewed changes

machichima added 3 commits July 15, 2025 19:47

docs: update crd field description

1665b01

Signed-off-by: machichima <[email protected]>

fix: move deletion delete validate to validation.go

d8db52e

Signed-off-by: machichima <[email protected]>

fix: run make api-docs

d28f193

Signed-off-by: machichima <[email protected]>

test: remove neg delay case

326aaec

Signed-off-by: machichima <[email protected]>

machichima mentioned this pull request Jul 15, 2025

[Feature] Add e2e test for setting RayCluster deletion delay in RayService #3870

Open

2 tasks

machichima commented Jul 15, 2025

View reviewed changes

kevin85421 approved these changes Jul 15, 2025

View reviewed changes

machichima added 2 commits July 16, 2025 20:20

test: validate when delay time is negative

f754828

Signed-off-by: machichima <[email protected]>

fix: validation test

f27461a

Signed-off-by: machichima <[email protected]>

kevin85421 approved these changes Jul 16, 2025

View reviewed changes

kevin85421 merged commit 017e58f into ray-project:master Jul 16, 2025
25 checks passed

	// RayClusterDeletionDelaySeconds specifies the delay in seconds before deleting old RayClusters.
	// RayClusterDeletionDelaySeconds specifies the delay, in seconds, before deleting old RayClusters.
	// The default value is 60 seconds.

[Feature] Support configurable RayCluster deletion delay in RayService #3864

[Feature] Support configurable RayCluster deletion delay in RayService #3864

Uh oh!

Conversation

machichima commented Jul 12, 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!

machichima commented Jul 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

machichima Jul 13, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevin85421 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

machichima commented Jul 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevin85421 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

machichima commented Jul 12, 2025 •

edited

Loading

machichima Jul 13, 2025 •

edited

Loading