[RayService] don't update serveConfigV2 in current ray cluster if ray… #3559

fscnick · 2025-05-07T04:34:35Z

Why are these changes needed?

When the serveConfigV2 and RayCluster has changed at the same time, it would also update the current active ray cluster with the new serveConfigV2. It might be possible to cause a failure if something in the runtime environment is different.

Related issue number

Closes #3423

Checks

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

fscnick · 2025-05-07T04:44:06Z

This is the screenshot of reproduction.

Before applying the fix:

After applying the fix: (the script periodically check if the new serveConfigV2 has been updated or not until the ray cluster has been terminated.)

Attach the reproduction script for your reference.
issue-3423-reproduce.sh.txt

fscnick · 2025-05-07T11:22:09Z

@kevin85421 PTAL

… cluster spec has changed at the same time. Signed-off-by: fscnick <[email protected]>

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

fscnick · 2025-05-07T17:01:22Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+		if err != nil &&
+			(strings.Contains(err.Error(), "exit code 52") ||
+				strings.Contains(err.Error(), "exit code 6")) {
+			// which is "curl: (52) Empty reply from server" or "curl: (6) Couldn't resolve host"
+			// it implies the ray cluster is terminated.
+			break
+		}
+
+		// Except the errors above, it should not occur.
+		g.Expect(err).NotTo(HaveOccurred())
+
+		g.Expect(stdout.String()).NotTo(ContainSubstring("\"price\": 456"), "new price should not be updated on the old ray cluster")


This test condition is not that straight forward. It would keep looping because it should check serveConfigV2 is not modified until it terminated.
There are a few of explanations about these conditions.

check the request to dashboard is unreachable, which implies the ray cluster terminated. it will exit the loop. (code 52 or 6 might happen). This is test pass if the following condition is not happened.

check the serveConfigV2 is not contain updated price until the ray cluster terminated. If it contains the updated price, test fails immediately.

fscnick · 2025-05-09T13:09:31Z

Hi @kevin85421 , the test is ready. PTAL

kevin85421 · 2025-05-09T18:40:14Z

cc @rueian would you mind reviewing this PR? Thanks.

rueian · 2025-05-10T18:24:03Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+		// Except the errors above, it should not occur.
+		g.Expect(err).NotTo(HaveOccurred())
+
+		g.Expect(stdout.String()).NotTo(ContainSubstring("\"price\": 456"), "new price should not be updated on the old ray cluster")


The implementation looks good to me, but I am afraid that this g.Expect(stdout.String()).NotTo(ContainSubstring("\"price\": 456"), "new price should not be updated on the old ray cluster") could never hit due to timing races. Could we simply wait for the old cluster to be destroyed and verify we don't reconcileServe on it by checking there are no new UpdatedServeApplications or FailedToUpdateServeApplications events on the cluster?

rueian · 2025-05-10T18:26:02Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

@@ -82,3 +86,97 @@ func TestRayServiceInPlaceUpdate(t *testing.T) {
 		g.Expect(stdout.String()).To(Equal("9 pizzas please!"))
 	}, TestTimeoutShort).Should(Succeed())
 }
+
+func TestRayServiceInPlaceUpdateWithRayClusterSpec(t *testing.T) {


Can we have another test for zero time upgrade enabled?

It looks like enabled already.

There is no upgradeStrategy in RayServiceSampleYamlApplyConfiguration()

screenshot:

Okay, then can we have a test for zero downtime upgrade being disabled?

I'm a bit of confusing about what it should do if zero downtime upgrade is disabled.

If it is disabled, it seems the new RayCluster wouldn't be created. The serveConfigV2 will be update on the existed cluster. Which might be an potential issue that this pr is trying to solve. Because ShouldPrepareNewCluster(...) wouldn't be true under these conditions.

However, if serveConfigV2 is not allow to update on the existed cluster under zero downtime upgrade is disabled with RayClusterSpec and serveConfigV2 modified at the same time. It might violate the design https://docs.ray.io/en/master/cluster/kubernetes/user-guides/rayservice.html#step-7-in-place-update-for-ray-serve-applications.

If it is disabled, it seems the new RayCluster wouldn't be created. The serveConfigV2 will be update on the existed cluster.

I think that is the correct behavior. Is there any existing test guarantee that already?

It seems no such test so add a test without zero downtime upgrade.

…lications happen or not Signed-off-by: fscnick <[email protected]>

rueian · 2025-05-11T21:10:01Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+	g.Eventually(func() string {
+		_, err := GetRayCluster(test, activeRayClusterBeforeUpdate.Namespace, activeRayClusterBeforeUpdate.Name)
+		if err == nil {
+			return ""
+		}
+		return err.Error()
+	}, TestTimeoutMedium).Should(ContainSubstring(fmt.Sprintf("rayclusters.ray.io \"%s\" not found", activeRayClusterBeforeUpdate.Name)))


Suggested change

g.Eventually(func() string {

_, err := GetRayCluster(test, activeRayClusterBeforeUpdate.Namespace, activeRayClusterBeforeUpdate.Name)

if err == nil {

return ""

}

return err.Error()

}, TestTimeoutMedium).Should(ContainSubstring(fmt.Sprintf("rayclusters.ray.io \"%s\" not found", activeRayClusterBeforeUpdate.Name)))

g.Eventually(func() bool {

_, err := GetRayCluster(test, activeRayClusterBeforeUpdate.Namespace, activeRayClusterBeforeUpdate.Name)

return k8sApiErrors.IsNotFound(err)

}, TestTimeoutMedium).Should(BeTrue())

rueian · 2025-05-11T21:16:20Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+
+	// event UpdatedServeApplications or FailedToUpdateServeApplications should not occur on activeRayClusterBeforeUpdate after RayService update.
+	for _, event := range events {
+		if event.CreationTimestamp.Before(updateTimestamp) {


shouldn't we test the LastTimestamp?

LastTimestamp might be later than CreationTimestamp which might need to consider more situation. It would be simple to choose CreationTimestamp. Kindly let me know if there are any other concern.

CreationTimestamp definition:
https://github.com/kubernetes/apimachinery/blob/202cba0f14e50d9b4106b34623f8c8056db3f86e/pkg/apis/meta/v1/types.go#L179-L188

FirstTimestamp and LastTimestamp definition:
https://github.com/kubernetes/api/blob/f7e72be095ee5a3e60d9df2f79b6591ee329734a/core/v1/types.go#L7205-L7211

Yes, LastTimestamp could be later than CreationTimestamp because k8s will aggregate the same event into the old one, therefore if you skip events by the CreationTimestamp, you might skip all the events.

How about you first check if the reason is what we want to test, and then test the LastTimestamp?

Oh, the LastTimestamp should record the event we update to the new cluster. How about test the Count = 2?

I'm not sure if it is okay to test the Count == 2. It seems message are excluded from aggregation key so that it is not knowing that belong to which RayCluster. However, if it happens more than 2 or less than 2, it definitely goes wrong. But it seems no other information in the event to tell which is which.

rueian · 2025-05-11T21:17:30Z

ray-operator/test/e2erayservice/support.go

@@ -90,6 +90,22 @@ func CurlRayServicePod(
 	return ExecPodCmd(t, curlPod, curlPodContainerName, cmd)
 }

+func CurlRayClusterDashboard(


I guess this and the ExecPodCmdReturnError can be removed.

rueian · 2025-05-11T21:19:08Z

ray-operator/test/support/events.go

@@ -128,3 +131,15 @@ func getWhitespaceStr(size int) string {
 	}
 	return whiteSpaceStr
 }
+
+func RayServiceEvents(t Test, rayService *rayv1.RayService) ([]corev1.Event, error) {


This by itself seems too small to be a helper.

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

rueian · 2025-05-12T15:12:07Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+
+	// event UpdatedServeApplications or FailedToUpdateServeApplications should not occur on activeRayClusterBeforeUpdate after RayService update.
+	for _, event := range events {
+		if event.CreationTimestamp.Before(updateTimestamp) {


Yes, LastTimestamp could be later than CreationTimestamp because k8s will aggregate the same event into the old one, therefore if you skip events by the CreationTimestamp, you might skip all the events.

How about you first check if the reason is what we want to test, and then test the LastTimestamp?

rueian · 2025-05-12T15:13:56Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+	g.Expect(err).NotTo(HaveOccurred())
+
+	updateTimestamp := rayService.Status.LastUpdateTime
+


Add a curl test to make sure the new serve config is updated on the new cluster.

Add it after checking the old RayCluster has terminated.

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

rueian · 2025-05-15T18:16:17Z

LGTM. cc @kevin85421 for review.

kevin85421

Looks good!

kevin85421 · 2025-05-15T20:38:49Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+		}
+		count++
+	}
+	g.Expect(count).To(Equal(2))


Can you also check these two events? One is for the old RayCluster, and the other is for the new RayCluster.

The RayCluster name appears on message which might be exclude from aggregation. It would be difficult to distinguish which is which. There are some discussion about this condition #3559 (comment). Or do I misunderstand something?

In this case, why can we check the number of events?

Discuss offline:

I am OK to merge the PR if:

Survey when will aggregation be triggered.

It can pass 20 times consecutively in your local env.

Check these two events are for different RayCluster CRs

Add a comment / TODO for the possibility of aggregation

https://gist.github.com/rueian/fca6c2597ff2ba1dfc2ab5eee148bee7

Aggregation is performed if a similar event is recorded 10 times in a 10 minute rolling interval.
https://github.com/kubernetes/client-go/blob/46965213e4561ad1b9c585d1c3551a0cc8d3fcd6/tools/record/events_cache.go#L430-L432

Here is the screenshot

Compare two RayClusters name from the event message at the end.

Aggregation is performed if a similar event is recorded 10 times in a 10 minute rolling interval.
https://github.com/kubernetes/client-go/blob/46965213e4561ad1b9c585d1c3551a0cc8d3fcd6/tools/record/events_cache.go#L430-L432

Oh, sorry. In that case, aggregation is quite likely to happen since we share the Kubernetes cluster across multiple end-to-end tests. We might need an alternative way to verify that the Ray Serve applications in the old RayCluster are not being updated.

A possible way is:

Add a finalizer to old RayCluster

Trigger zero downtime upgrade

Wait until RayService switches to the new RayCluster

Verify the old RayCluster's Serve applications are not updated.

Remove the finalizer

Fixed. PTAL

kevin85421 · 2025-05-15T20:48:23Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+	)
+	g.Expect(err).NotTo(HaveOccurred())
+
+	// Wait active RayCluster has been terminated


Can you use logic similar to

kuberay/ray-operator/test/e2erayservice/rayservice_ha_test.go

Line 200 in 8779e92

LogWithTimestamp(test.T(), "Waiting for RayService %s/%s UpgradeInProgress condition to be true", newRayService.Namespace, newRayService.Name)

?

Maybe we can add a utility function to wait until the RayService switches to the new RayCluster.

kevin85421 · 2025-05-15T20:51:13Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+	g.Expect(rayService).NotTo(BeNil())
+
+	// Create curl pod
+	curlPodName := "curl-pod"


Follow up: Update CreateCurlPod to include the logic to wait until it is running and ready. Can you open an issue to track the progress?

Created. #3613

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

…dated Signed-off-by: fscnick <[email protected]>

kevin85421

@rueian would you mind reviewing another iteration? I will do an iteration later.

rueian · 2025-05-19T06:20:55Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+	finalizers := activeRayClusterBeforeUpdate.Finalizers
+	activeRayClusterBeforeUpdate.Finalizers = activeRayClusterBeforeUpdate.Finalizers[:0]
+	for _, finalizer := range finalizers {
+		if finalizer == "no-existed-finalizer" {
+			continue
+		}
+		activeRayClusterBeforeUpdate.Finalizers = append(activeRayClusterBeforeUpdate.Finalizers, finalizer)
+	}


Suggested change

finalizers := activeRayClusterBeforeUpdate.Finalizers

activeRayClusterBeforeUpdate.Finalizers = activeRayClusterBeforeUpdate.Finalizers[:0]

for _, finalizer := range finalizers {

if finalizer == "no-existed-finalizer" {

continue

}

activeRayClusterBeforeUpdate.Finalizers = append(activeRayClusterBeforeUpdate.Finalizers, finalizer)

}

activeRayClusterBeforeUpdate.Finalizers = nil

Can't we just clear it?

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

kevin85421 · 2025-05-19T15:53:13Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+	stdout, _ = CurlRayServicePod(test, rayService, curlPod, curlContainerName, "/calc", `["MUL", 3]`)
+	g.Expect(stdout.String()).To(Equal("15 pizzas please!"))
+
+	// In-place update


This is not in-place update.

Suggested change

// In-place update

// Update both `serveConfigV2` and the Ray head's environment variables.

// Then, verify the following two conditions:

// (1) The new `serveConfigV2` is not applied to the old RayCluster.

// (2) A zero-downtime upgrade is triggered.

kevin85421 · 2025-05-19T16:07:36Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+	activeRayClusterBeforeUpdate, err = GetRayCluster(test, namespace.Name, rayService.Status.ActiveServiceStatus.RayClusterName)
+	g.Expect(err).NotTo(HaveOccurred())
+
+	// apply a non-existed-finalizer to the active ray cluster to avoiding be removed before checking the serveConfigV2


Suggested change

// apply a non-existed-finalizer to the active ray cluster to avoiding be removed before checking the serveConfigV2

// Apply a finalizer to the active RayCluster to prevent it from being deleted after the zero-downtime upgrade process is complete.

kevin85421 · 2025-05-19T16:10:43Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+	g.Expect(stdout.String()).To(Equal("15 pizzas please!"))
+
+	// In-place update
+	// Parse ServeConfigV2 and replace the string in the simplest way to update it.


Suggested change

// Parse ServeConfigV2 and replace the string in the simplest way to update it.

kevin85421 · 2025-05-19T16:11:04Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+	serveConfig := rayService.Spec.ServeConfigV2
+	serveConfig = strings.Replace(serveConfig, "price: 3", "price: 456", -1)
+	rayService.Spec.ServeConfigV2 = serveConfig
+


Suggested change

kevin85421 · 2025-05-19T16:20:33Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+	// Make sure the old ray cluster is not updated.
+	stdout, _, err = CurlRayClusterDashboard(test, activeRayClusterBeforeUpdate, curlPod, curlContainerName, utils.DeployPathV2)
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(stdout.String()).NotTo(ContainSubstring("\"price\": 456"), "new price should not be updated on the old ray cluster")


There are a lot of possible cases that the stdout doesn't contain NotTo. It's better to query /fruit and check the results.

kevin85421 · 2025-05-19T16:35:09Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

@@ -6,8 +6,11 @@ import (

 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
+	k8sApiErrors "k8s.io/apimachinery/pkg/api/errors"


this is k8s import convention

Suggested change

k8sApiErrors "k8s.io/apimachinery/pkg/api/errors"

k8serrors "k8s.io/apimachinery/pkg/api/errors"

kevin85421 · 2025-05-19T16:39:39Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+	)
+	g.Expect(err).NotTo(HaveOccurred())
+
+	// make sure the old ray cluster is removed


I guess this might take a long time. We only need to remove the finalizer. Can you measure how long L190 - L194 take? If it is long, we can remove this Eventually logic.

it take elapsed time: 1m0.446727917s out of 114.902s. It might be a bit of long. Remove in advance. Kindly let me know if you'd like to keep it.

kevin85421 · 2025-05-19T16:45:58Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+	stdout, _ = CurlRayServicePod(test, rayService, curlPod, curlContainerName, "/calc", `["MUL", 3]`)
+	g.Expect(stdout.String()).To(Equal("15 pizzas please!"))
+
+	// In-place update


Suggested change

// In-place update

// Update both `serveConfigV2` and the Ray head's environment variables.

// Since `upgradeStrategy` is set to `None`, a zero-downtime upgrade will not be triggered,

// and the new `serveConfigV2` will be applied to the existing RayCluster.

kevin85421 · 2025-05-19T16:46:15Z

ray-operator/test/e2erayservice/rayservice_in_place_update_test.go

+	g.Expect(stdout.String()).To(Equal("15 pizzas please!"))
+
+	// In-place update
+	// Parse ServeConfigV2 and replace the string in the simplest way to update it.


Suggested change

// Parse ServeConfigV2 and replace the string in the simplest way to update it.

kevin85421 · 2025-05-19T16:51:32Z

ray-operator/test/e2erayservice/support.go

@@ -90,6 +91,21 @@ func CurlRayServicePod(
 	return ExecPodCmd(t, curlPod, curlPodContainerName, cmd)
 }

+func CurlRayClusterDashboard(t Test,


do we need to expose this function (use the uppercase)?

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

fscnick added 2 commits May 8, 2025 00:46

[RayService] don't update serveConfigV2 in current ray cluster if ray…

d211030

… cluster spec has changed at the same time. Signed-off-by: fscnick <[email protected]>

[RayService] add test case for issue 3423

c02c81f

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

fscnick force-pushed the update-serve-config-v2-not-affecting-running-cluster-2 branch from e54f5ee to c02c81f Compare May 7, 2025 16:49

fscnick commented May 7, 2025

View reviewed changes

fscnick marked this pull request as ready for review May 9, 2025 13:08

kevin85421 assigned rueian May 9, 2025

rueian reviewed May 10, 2025

View reviewed changes

[RayService] check UpdatedServeApplications or FailedToUpdateServeApp…

1ca4517

…lications happen or not Signed-off-by: fscnick <[email protected]>

fscnick requested a review from rueian May 11, 2025 11:48

rueian reviewed May 11, 2025

View reviewed changes

fscnick added 2 commits May 12, 2025 20:54

[RayService] add test for no zero downtime upgrade

aac91ac

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

[RayService] remove unused code

fd9e897

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

rueian reviewed May 12, 2025

View reviewed changes

[RayService] verify updated serveConfigV2 and check event count

78c45cc

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

fscnick force-pushed the update-serve-config-v2-not-affecting-running-cluster-2 branch from 3c44f1d to 78c45cc Compare May 12, 2025 18:39

[RayService] remove counting FailedToUpdateServeApplications

25c41c0

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

fscnick requested a review from rueian May 13, 2025 12:30

[RayService] check raycluster is not changed if zero downtime disable

6fd7b2d

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

rueian approved these changes May 15, 2025

View reviewed changes

kevin85421 reviewed May 15, 2025

View reviewed changes

fscnick mentioned this pull request May 16, 2025

Update CreateCurlPod to include the logic to wait until it is running and ready #3613

Open

[RayService] add utility function to wait for RayCluster switch

111efce

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

fscnick requested a review from kevin85421 May 16, 2025 13:52

fscnick added 2 commits May 17, 2025 21:45

[RayService] compare two events are from different RayCluster

f1f9989

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

[RayService] add finalizer to wait for verifying serveConfigV2 not up…

37937e4

…dated Signed-off-by: fscnick <[email protected]>

kevin85421 reviewed May 19, 2025

View reviewed changes

rueian reviewed May 19, 2025

View reviewed changes

[RayService] remove test finalizer using nil

3047114

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

fscnick requested a review from rueian May 19, 2025 12:24

kevin85421 reviewed May 19, 2025

View reviewed changes

fscnick added 2 commits May 20, 2025 22:19

[RayService] remove comment and minor fix

b69fce0

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

[RayService] an e2e test without waiting for RayCluster being removed

c4cf1cc

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

		g.Expect(err).NotTo(HaveOccurred())

		updateTimestamp := rayService.Status.LastUpdateTime

-	// In-place update
+        // Update both `serveConfigV2` and the Ray head's environment variables.
+        // Then, verify the following two conditions:
+        // (1) The new `serveConfigV2` is not applied to the old RayCluster.
+        // (2) A zero-downtime upgrade is triggered.

	// apply a non-existed-finalizer to the active ray cluster to avoiding be removed before checking the serveConfigV2
	// Apply a finalizer to the active RayCluster to prevent it from being deleted after the zero-downtime upgrade process is complete.

	k8sApiErrors "k8s.io/apimachinery/pkg/api/errors"
	k8serrors "k8s.io/apimachinery/pkg/api/errors"

[RayService] don't update serveConfigV2 in current ray cluster if ray… #3559

Are you sure you want to change the base?

[RayService] don't update serveConfigV2 in current ray cluster if ray… #3559

Conversation

fscnick commented May 7, 2025 • edited Loading

Why are these changes needed?

Related issue number

Checks

fscnick commented May 7, 2025

fscnick commented May 7, 2025

fscnick May 7, 2025 • edited Loading

Choose a reason for hiding this comment

fscnick commented May 9, 2025

kevin85421 commented May 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fscnick May 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fscnick May 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rueian commented May 15, 2025

kevin85421 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin85421 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fscnick commented May 7, 2025 •

edited

Loading

fscnick May 7, 2025 •

edited

Loading

fscnick May 12, 2025 •

edited

Loading

fscnick May 12, 2025 •

edited

Loading