Generate RayCluster Hash on KubeRay Version Change #2320

ryanaoleary · 2024-08-21T01:09:20Z

Why are these changes needed?

This PR adds a new ray.io/kuberay-version annotation to RayClusters created with KubeRay. This annotation is used to check for a KubeRay version change, which can cause a RayService to restart and create a new RayCluster erroneously due to a hash mismatch. If the KubeRay version annotation differs when checking shouldPrepareNewRayCluster and the RayCluster hashes do not match, the controller updates the utils.HashWithoutReplicasAndWorkersToDeleteKey annotation, KubeRay version annotation, and returns DoNothing to avoid restarting the RayService on version change. This unblocks the version upgrade to KubeRay v1.2.0.

This PR was tested by following the reproduction script in the linked issue and verifying that Active RayCluster config doesn't match goal config. RayService operator should prepare a new Ray cluster does not occur.

Related issue number

Closes #2315

Checks

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

Signed-off-by: Ryan O'Leary <[email protected]>

ryanaoleary · 2024-08-21T01:10:19Z

cc: @kevin85421, @andrewsykim

ryanaoleary · 2024-08-21T01:22:24Z

@kevin85421 is the desired behavior here to immediately start a zero-downtime upgrade (r.markRestartAndAddPendingClusterName(ctx, rayServiceInstance)) if the KUBERAY_VERSION changes? Or should we just add the updated annotations on a version change but return DoNothing, and leave the RayCluster as-is that iteration rather than restarting it?

Signed-off-by: Ryan O'Leary <[email protected]>

kevin85421 · 2024-08-21T17:14:47Z

is the desired behavior here to immediately start a zero-downtime upgrade (r.markRestartAndAddPendingClusterName(ctx, rayServiceInstance)) if the KUBERAY_VERSION changes?

No, if the RayService CR's KUBERAY_VERSION annotation (e.g., 1.2.0) is different from (or doesn't exist) the KubeRay operator's KUBERAY_VERSION (e.g., 1.3.0), then we should not perform a zero downtime upgrade until:

The ray.io/hash-without-replicas-and-workers-to-delete annotation is updated.
The KUBERAY_VERSION annotation is updated.

After these updates, we can use the new ray.io/hash-without-replicas-and-workers-to-delete annotation to determine whether to trigger a zero downtime upgrade.

ray-operator/controllers/ray/rayservice_controller.go

kevin85421 · 2024-08-21T17:37:42Z

ray-operator/controllers/ray/rayservice_controller.go

+		// and new KubeRay version, but do not restart the RayCluster.
+		activeKubeRayVersion := activeRayCluster.ObjectMeta.Annotations[utils.KubeRayVersion]
+		if activeKubeRayVersion != utils.KUBERAY_VERSION {
+			activeRayCluster.ObjectMeta.Annotations[utils.HashWithoutReplicasAndWorkersToDeleteKey] = goalClusterHash


I prefer not to update the input argument (activeRayCluster) inside this function. How about removing the logic for updating annotations here and directly return Update instead?

Done in 5b5fe07, we now just log and call Update and the cluster hash and KubeRay version are updated in constructRayClusterForRayService.

Signed-off-by: Ryan O'Leary <[email protected]>

kevin85421 · 2024-08-21T18:59:48Z

Thanks! I will manually test it.

ray-operator/controllers/ray/rayservice_controller.go

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

kevin85421

Would you mind adding a unit test in rayservice_controller_unit_test.go? Maybe in TestReconcileRayCluster, or you can add a new test if needed.

Signed-off-by: Ryan O'Leary <[email protected]>

ryanaoleary · 2024-08-21T21:48:21Z

Would you mind adding a unit test in rayservice_controller_unit_test.go? Maybe in TestReconcileRayCluster, or you can add a new test if needed.

Added a unit test case in f9c61e7

Signed-off-by: Ryan O'Leary <[email protected]>

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

kevin85421

Others look good to me! I will manually test it after the comments are addressed.

ray-operator/controllers/ray/rayservice_controller_unit_test.go

kevin85421 · 2024-08-22T03:31:02Z

ray-operator/controllers/ray/rayservice_controller_unit_test.go

 				runtimeObjects = append(runtimeObjects, tc.activeCluster.DeepCopy())
 			}
 			fakeClient := clientFake.NewClientBuilder().WithScheme(newScheme).WithRuntimeObjects(runtimeObjects...).Build()
 			r := RayServiceReconciler{
 				Client: fakeClient,
+				Scheme: newScheme,


why do we need this?

Without setting the Scheme, the test returns a nil pointer error when creating a new RayCluster in constructRayClusterForRayService which gets called when updating the RayCluster.

ray-operator/controllers/ray/rayservice_controller_unit_test.go

Signed-off-by: Ryan O'Leary <[email protected]>

MortalHappiness · 2024-08-23T08:52:12Z

LGTM

rueian · 2024-08-23T09:30:28Z

LGTM

kevin85421

I manually test this PR by following this issue: #2315 (comment).

Zero downtime upgrade will not be triggered accidentally after upgrading.
The annotation ray.io/kuberay-version is added to the RayCluster CR correctly.
Then, I updated rayVersion from 2.34.0 to 2.100.0 to trigger zero downtime upgrade. It is triggered as expected.

* Re-generate hash when KubeRay version changes Signed-off-by: Ryan O'Leary <[email protected]> * Change logic to DoNothing on KubeRay version mismatch Signed-off-by: Ryan O'Leary <[email protected]> * Add KubeRay version annotation to test Signed-off-by: Ryan O'Leary <[email protected]> * Move update logic Signed-off-by: Ryan O'Leary <[email protected]> * Update rayservice_controller.go Signed-off-by: ryanaoleary <[email protected]> * Add unit test Signed-off-by: Ryan O'Leary <[email protected]> * Add period Signed-off-by: Ryan O'Leary <[email protected]> * Go vet changes Signed-off-by: Ryan O'Leary <[email protected]> * Update rayservice_controller_unit_test.go Signed-off-by: ryanaoleary <[email protected]> * Address test comments Signed-off-by: Ryan O'Leary <[email protected]> --------- Signed-off-by: Ryan O'Leary <[email protected]> Signed-off-by: ryanaoleary <[email protected]>

…) (#2339) * Re-generate hash when KubeRay version changes * Change logic to DoNothing on KubeRay version mismatch * Add KubeRay version annotation to test * Move update logic * Update rayservice_controller.go * Add unit test * Add period * Go vet changes * Update rayservice_controller_unit_test.go * Address test comments --------- Signed-off-by: Ryan O'Leary <[email protected]> Signed-off-by: ryanaoleary <[email protected]> Co-authored-by: ryanaoleary <[email protected]>

Re-generate hash when KubeRay version changes

f291949

Signed-off-by: Ryan O'Leary <[email protected]>

ryanaoleary changed the title ~~Re-generate hash when KubeRay version changes~~ Generate RayCluster Hash on KubeRay Version Change Aug 21, 2024

ryanaoleary added 2 commits August 21, 2024 01:26

Change logic to DoNothing on KubeRay version mismatch

f364148

Signed-off-by: Ryan O'Leary <[email protected]>

Add KubeRay version annotation to test

1f830e8

Signed-off-by: Ryan O'Leary <[email protected]>

kevin85421 self-assigned this Aug 21, 2024

kevin85421 added 1.2.0 release-blocker labels Aug 21, 2024

kevin85421 reviewed Aug 21, 2024

View reviewed changes

Move update logic

5b5fe07

Signed-off-by: Ryan O'Leary <[email protected]>

ryanaoleary requested a review from kevin85421 August 21, 2024 18:01

andrewsykim reviewed Aug 21, 2024

View reviewed changes

ray-operator/controllers/ray/rayservice_controller.go Outdated Show resolved Hide resolved

Update rayservice_controller.go

0824996

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

kevin85421 reviewed Aug 21, 2024

View reviewed changes

Add unit test

f9c61e7

Signed-off-by: Ryan O'Leary <[email protected]>

Add period

27268e8

Signed-off-by: Ryan O'Leary <[email protected]>

ryanaoleary requested a review from kevin85421 August 21, 2024 21:50

ryanaoleary and others added 2 commits August 21, 2024 22:07

Go vet changes

1293683

Signed-off-by: Ryan O'Leary <[email protected]>

Update rayservice_controller_unit_test.go

acf7024

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

kevin85421 reviewed Aug 22, 2024

View reviewed changes

Address test comments

3142ece

Signed-off-by: Ryan O'Leary <[email protected]>

ryanaoleary requested a review from kevin85421 August 23, 2024 00:03

kevin85421 approved these changes Aug 23, 2024

View reviewed changes

kevin85421 merged commit e7f0c2c into ray-project:master Aug 23, 2024
27 checks passed

kevin85421 mentioned this pull request Aug 29, 2024

[Bug] Misleading error message in RayService when upgrading to KubeRay v1.1.0 #2088

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate RayCluster Hash on KubeRay Version Change #2320

Generate RayCluster Hash on KubeRay Version Change #2320

ryanaoleary commented Aug 21, 2024 •

edited

Loading

ryanaoleary commented Aug 21, 2024 •

edited

Loading

ryanaoleary commented Aug 21, 2024 •

edited

Loading

kevin85421 commented Aug 21, 2024

kevin85421 Aug 21, 2024

ryanaoleary Aug 21, 2024

kevin85421 commented Aug 21, 2024

kevin85421 left a comment

ryanaoleary commented Aug 21, 2024

kevin85421 left a comment

kevin85421 Aug 22, 2024

ryanaoleary Aug 22, 2024 •

edited

Loading

MortalHappiness commented Aug 23, 2024

rueian commented Aug 23, 2024

kevin85421 left a comment

Generate RayCluster Hash on KubeRay Version Change #2320

Generate RayCluster Hash on KubeRay Version Change #2320

Conversation

ryanaoleary commented Aug 21, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

ryanaoleary commented Aug 21, 2024 • edited Loading

ryanaoleary commented Aug 21, 2024 • edited Loading

kevin85421 commented Aug 21, 2024

kevin85421 Aug 21, 2024

Choose a reason for hiding this comment

ryanaoleary Aug 21, 2024

Choose a reason for hiding this comment

kevin85421 commented Aug 21, 2024

kevin85421 left a comment

Choose a reason for hiding this comment

ryanaoleary commented Aug 21, 2024

kevin85421 left a comment

Choose a reason for hiding this comment

kevin85421 Aug 22, 2024

Choose a reason for hiding this comment

ryanaoleary Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

MortalHappiness commented Aug 23, 2024

rueian commented Aug 23, 2024

kevin85421 left a comment

Choose a reason for hiding this comment

ryanaoleary commented Aug 21, 2024 •

edited

Loading

ryanaoleary commented Aug 21, 2024 •

edited

Loading

ryanaoleary commented Aug 21, 2024 •

edited

Loading

ryanaoleary Aug 22, 2024 •

edited

Loading