[Test] Implement RayService In-place update test in Golang #2536

CheyuWu · 2024-11-12T15:41:24Z

Why are these changes needed?

Implement test_in_place_update in Golang, and add a file rayservice_in_place_update_test.go under https://github.com/ray-project/kuberay/tree/master/ray-operator/test/e2e.

Related issue number

Close #2516

Checks

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

Signed-off-by: Cheyu Wu <cheyu1220@gmail.com>

CheyuWu · 2024-11-12T16:47:16Z

@kevin85421 @MortalHappiness
PTAL

ray-operator/test/support/ray.go

ray-operator/test/e2e/rayservice_in_place_update_test.go

ray-operator/test/support/core.go

ray-operator/test/e2e/rayservice_in_place_update_test.go

This reverts commit 6ac63c2.

…ually

CheyuWu · 2024-11-14T15:43:49Z

I've made all the necessary changes. If there’s anything else that needs to be modified, please let me know. Thanks!

ray-operator/test/support/ray.go

ray-operator/test/e2e/rayservice_in_place_update_test.go

…Cluster

CheyuWu · 2024-11-16T03:11:46Z

@MortalHappiness The pipeline is stuck. Could you help me to rerun it?

ray-operator/test/e2e/support.go

MortalHappiness · 2024-11-16T03:50:32Z

ray-operator/test/support/ray.go

 	return cluster.Status.DesiredWorkerReplicas
 }

+func UnderlyingRayCluster(rayService *rayv1.RayService) func() (*rayv1.RayCluster, error) {


Suggested change

func UnderlyingRayCluster(rayService *rayv1.RayService) func() (*rayv1.RayCluster, error) {

func UnderlyingRayClusterName(rayService *rayv1.RayService) string {

Change the function signature. And then we can use

Eventually(RayService(...)).WithTransform(UnderlyingRayClusterName, ...).Should(...) rayClusterName := UnderlyingRayClusterName(...)

MortalHappiness · 2024-11-16T03:57:22Z

ray-operator/test/e2e/rayservice_in_place_update_test.go

+	namespace := test.NewTestNamespace()
+	test.StreamKubeRayOperatorLogs()
+
+	fileName := "ray-service.sample.yaml"


Because we now put this file under e2e test folder, we should not depend on the yaml file. Please apply the RayService with ApplyConfiguration

kuberay/ray-operator/pkg/client/applyconfiguration/ray/v1/rayservice.go

Line 13 in 966d9b3

type RayServiceApplyConfiguration struct {

You can also reference other test files under e2e folder that import rayv1ac "github.com/ray-project/kuberay/ray-operator/pkg/client/applyconfiguration/ray/v1" to see how to use it.

…ge the comment in curlRayServicePod

CheyuWu · 2024-11-17T15:15:58Z

Well, This is my revised version. Free to let me know if there are any issues because I'm not entirely sure if this is the best approach. I turned each config into a separate function instead of modifying the original template function to make it suitable as a shared function.

kevin85421 · 2024-11-17T18:47:39Z

ray-operator/test/e2e/rayservice_in_place_update_test.go

+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(rayService).NotTo(BeNil())
+
+	// Wait for RayCluster name to be populated


// Wait for RayCluster name to be populated g.Eventually(RayService(test, namespace.Name, rayServiceName), TestTimeoutShort).Should( WithTransform(UnderlyingRayClusterName, Not(BeEmpty())), ) rayClusterName := UnderlyingRayClusterName(rayService) test.T().Logf("Waiting for RayCluster %s/%s to be ready", namespace.Name, rayClusterName) g.Eventually(RayCluster(test, namespace.Name, rayClusterName), TestTimeoutMedium). Should(WithTransform(RayClusterState, Equal(rayv1.Ready))) rayCluster, err := GetRayCluster(test, namespace.Name, rayClusterName) g.Expect(err).NotTo(HaveOccurred()) // Check if the head pod is ready g.Eventually(HeadPod(test, rayCluster), TestTimeoutShort).Should(WithTransform(sampleyaml.IsPodRunningAndReady, BeTrue())) // Check if all worker pods are ready g.Eventually(WorkerPods(test, rayCluster), TestTimeoutShort).Should(WithTransform(sampleyaml.AllPodsRunningAndReady, BeTrue()))

We don't need to check the RayCluster or the readiness of Pods for RayService. We only need to make sure RayService.Status.ServiceStatus is Running. The readiness of worker Pods also depends on the Ray Serve configurations. For example, if Ray Serve doesn't schedule any Ray Serve replicas to a worker Pod, its readiness probe will fail, preventing it from receiving any requests. Hence, it is possible for a healthy RayService to have non-ready worker Pods. I think we can remove the code I quoted above.

kevin85421 · 2024-11-17T18:48:54Z

ray-operator/test/e2e/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


nit

Suggested change

// In place update

// In-place update

kevin85421 · 2024-11-17T19:14:22Z

ray-operator/test/e2e/rayservice_in_place_update_test.go

+	)
+	g.Expect(err).NotTo(HaveOccurred())
+
+	g.Eventually(func(g Gomega) {


ObservedGeneration is not well-maintained in the KubeRay codebase. It's better not to use this field.

The observability of the progress of in-place updates highly depends on Ray Serve's implementation, which may vary across different Ray releases.

Maybe we can use Eventually to send requests via the curl Pod to ensure that the return values are eventually updated and remove this line?

I think this can be removed

We can remove this g.Eventually(func(g Gomega) {...}).

Oops, sorry, I misunderstood your meaning.

kevin85421 · 2024-11-17T19:14:47Z

ray-operator/test/e2e/support.go

+						WithContainers(corev1ac.Container().
+							WithName("ray-worker").
+							WithImage(GetRayImage()).
+							WithLifecycle(corev1ac.Lifecycle().


this is unnecessary

kevin85421 · 2024-11-17T19:16:27Z

ray-operator/test/e2e/support.go

+
+func rayServiceSampleYamlApplyConfiguration() *rayv1ac.RayServiceSpecApplyConfiguration {
+	return rayv1ac.RayServiceSpec().WithServeConfigV2(`applications:
+      - name: fruit_app


can we remove the unnecessary parts of ServeConfigV2? For example, OrangeStand.

I’ve tried several times, and it turns out that at least two fruit stands need to be retained; otherwise, it will keep getting stuck at "wait for deployment ready."

There are three types of fruits here. Do we want to remove one, or keep them all?

at least two fruit stands need to be retained; otherwise, it will keep getting stuck at "wait for deployment ready."

Interesting. Could you open an issue about the reproduction? We don't need to handle the issue in this PR, but we should track the progress.

There are three types of fruits here. Do we want to remove one, or keep them all?

Remove one. Just try to make this AC as short as possible.

OK, I have created this issue #2557

…rvice

kevin85421 · 2024-11-20T05:24:36Z

ray-operator/test/e2e/support.go

+
+func rayServiceSampleYamlApplyConfiguration() *rayv1ac.RayServiceSpecApplyConfiguration {
+	return rayv1ac.RayServiceSpec().WithServeConfigV2(`applications:
+      - name: fruit_app


at least two fruit stands need to be retained; otherwise, it will keep getting stuck at "wait for deployment ready."

Interesting. Could you open an issue about the reproduction? We don't need to handle the issue in this PR, but we should track the progress.

There are three types of fruits here. Do we want to remove one, or keep them all?

Remove one. Just try to make this AC as short as possible.

kevin85421 · 2024-11-20T05:31:31Z

ray-operator/test/e2e/support.go

+              num_cpus: 0.1
+          - name: Router
+            num_replicas: 1`).
+		WithRayClusterSpec(rayv1ac.RayClusterSpec().


Does reusing rayClusterSpec() work?

kuberay/ray-operator/test/e2e/support.go

Line 102 in 8d60d61

func rayClusterSpec() *rayv1ac.RayClusterSpecApplyConfiguration {

kevin85421 · 2024-11-20T05:37:37Z

ray-operator/test/sampleyaml/rayservice_test.go

+			)
+			rayClusterName := UnderlyingRayClusterName(rayService)

 			test.T().Logf("Waiting for RayCluster %s/%s to be ready", namespace.Name, rayClusterName)


It is possible for a healthy RayService to have its underlying RayCluster status not be rayv1.Ready, and for some worker Pods to be not ready. We can remove the checks of Equal(rayv1.Ready) and WorkerPods(test, rayCluster).

Yeah, you are correct. We only need to make sure the Head Pod is ready.

kevin85421 · 2024-11-20T05:43:18Z

ray-operator/test/sampleyaml/rayservice_test.go

-				g.Expect(rayClusterName).NotTo(BeEmpty())
-			}, TestTimeoutShort).Should(Succeed())
+			g.Eventually(RayService(test, namespace.Name, rayServiceFromYaml.Name), TestTimeoutShort).Should(
+				WithTransform(UnderlyingRayClusterName, Not(BeEmpty())),


We can get ActiveServiceStatus.RayClusterName directly because the RayService has become Running, and zero-downtime has not been triggered.

Ok, I will also remove function UnderlyingRayClusterName, because this is not used anymore

… ServeConfigV2

kevin85421

Left a comment. Others look good to me.

kevin85421 · 2024-11-20T19:21:01Z

ray-operator/test/e2e/rayservice_in_place_update_test.go

+	)
+	g.Expect(err).NotTo(HaveOccurred())
+
+	g.Eventually(func(g Gomega) {


We can remove this g.Eventually(func(g Gomega) {...}).

MortalHappiness · 2024-11-21T07:40:41Z

Please fix the CI error. Thanks!

CheyuWu · 2024-11-21T13:30:05Z

Does reusing rayClusterSpec() work?

kuberay/ray-operator/test/e2e/support.go

Line 102 in 8d60d61

func rayClusterSpec() *rayv1ac.RayClusterSpecApplyConfiguration {

@MortalHappiness @kevin85421

I’d like to discuss a situation where the CI pipeline is stuck at Ray Serve applications are not ready to serve requests.
The issue seems to be that using the default rayClusterSpec() leads to insufficient CPU and memory resources.

I’d like to ask which approach would be better:

Revert this commit. 63fb8db
Modify rayClusterSpec() to accept parameters (but this would require changes to other scripts and functions as well).
Any other suggestions I haven’t considered?

MortalHappiness · 2024-11-21T15:08:17Z

@kevin85421 Do you think it's better to lower the resource requests in our codebase, or to move the test to buildkite, which has more resources?

kevin85421 · 2024-11-21T16:32:13Z

Do you think it's better to lower the resource requests in our codebase, or to move the test to buildkite, which has more resources?

@MortalHappiness @CheyuWu I think the issue is not related to physical resources, so moving to Buildkite will likely result in the same issue. Instead, the issue seems to be related to Ray's logical resources.

@CheyuWu, Router will use 1 CPU. Can you set ray_actor_options to use num_cpus: 0.1?

          - name: Router
            num_replicas: 1

CheyuWu · 2024-11-21T18:10:57Z

Do you think it's better to lower the resource requests in our codebase, or to move the test to buildkite, which has more resources?

@MortalHappiness @CheyuWu I think the issue is not related to physical resources, so moving to Buildkite will likely result in the same issue. Instead, the issue seems to be related to Ray's logical resources.

@CheyuWu, Router will use 1 CPU. Can you set ray_actor_options to use num_cpus: 0.1?
          - name: Router
            num_replicas: 1

2. rayClusterSpec()

I tested it locally by increasing the CPU and memory for the head pod to match the values specified in ray-service.sample.yaml, and it worked. However, if your approach works, that's even better since we can directly use rayClusterSpec().

Let me check if the pipeline passes.

kevin85421 · 2024-11-21T19:22:50Z

@CheyuWu, you can check the Ray dashboard to see which Ray Serve replica is not scheduled. A possible reason might be MangoStand. There are two replicas for MangoStand, with at most one replica per Ray node. However, it is possible for the Ray head Pod to be occupied by other Ray Serve replicas, so MangoStand cannot be scheduled on the Ray head. You can try removing max_replicas_per_node: 1 and changing num_replicas from 2 to 1.

          - name: MangoStand
            num_replicas: 2
            max_replicas_per_node: 1
            user_config:
              price: 3
            ray_actor_options:
              num_cpus: 0.1

CheyuWu · 2024-11-22T18:30:27Z

@CheyuWu, you can check the Ray dashboard to see which Ray Serve replica is not scheduled. A possible reason might be MangoStand. There are two replicas for MangoStand, with at most one replica per Ray node. However, it is possible for the Ray head Pod to be occupied by other Ray Serve replicas, so MangoStand cannot be scheduled on the Ray head. You can try removing max_replicas_per_node: 1 and changing num_replicas from 2 to 1.
          - name: MangoStand
            num_replicas: 2
            max_replicas_per_node: 1
            user_config:
              price: 3
            ray_actor_options:
              num_cpus: 0.1

I conducted a few experiments and checked Ray Serve's logs on my local machine. Here are my findings:

I found that if fruit_stand is not included in WithServeConfigV2, it will automatically apply the default configuration to deployments that are not explicitly set. This causes unused fruit_stand instances not to be properly removed.

I couldn't get rid of those fruit stand

If the num_replicas for some fruit_stand deployments is set to 0, Ray Serve fails to initialize properly.
The Ray Serve logs show issues related to CPU and OOM.
serve_controller_416.log

kevin85421 · 2024-11-22T18:56:43Z

@CheyuWu got it. maybe use buildkite instead and increase the resources in rayClusterSpec().

CheyuWu · 2024-11-23T05:51:21Z

@CheyuWu got it. maybe use buildkite instead and increase the resources in rayClusterSpec().

I would like to ask if it’s okay to directly modify the resource in this way using headPodTemplateApplyConfiguration() within rayClusterSpec() at this location?

kuberay/ray-operator/test/e2e/rayjob_lightweight_test.go

Lines 45 to 54 in 925effe

    
           WithRayClusterSpec(rayv1ac.RayClusterSpec(). 
        
           	WithRayVersion(GetRayVersion()). 
        
           	WithHeadGroupSpec(rayv1ac.HeadGroupSpec(). 
        
           		WithRayStartParams(map[string]string{ 
        
           			"dashboard-host": "0.0.0.0", 
        
           			"num-gpus":       "4", 
        
           			"num-cpus":       "4", 
        
           			"resources":      `'{"R1": 4}'`, 
        
           		}). 
        
           		WithTemplate(podTemplateSpecApplyConfiguration(headPodTemplateApplyConfiguration(),

I’m not familiar with it and because the testcase is named "lightweight," I’m worried it might affect the original setup.

outdated

…ct#2536)

CheyuWu added 6 commits November 12, 2024 01:08

refactor: setup Port forward request

6ac63c2

Signed-off-by: Cheyu Wu <cheyu1220@gmail.com>

feat: Update Ray Service Config func

5607d39

feat: create curl pod func

dd31b9d

chore: add return option to execute pod cmd

3df7be7

feat: rayservice in place update and curl ray service pod func

a68703c

Signed-off-by: Cheyu Wu <cheyu1220@gmail.com>

Merge remote-tracking branch 'upstream/master' into feat/inplaceUpdate

f84d480

chore: set default curl url to g.Eventually

e5f022e

kevin85421 assigned MortalHappiness Nov 13, 2024

MortalHappiness requested changes Nov 13, 2024

View reviewed changes

CheyuWu added 5 commits November 13, 2024 21:51

fix: GetRayService is already in master branch

7801b2f

fix: use tail -f /dev/null instead of while loop

c36580c

Revert "refactor: setup Port forward request"

aac0c68

This reverts commit 6ac63c2.

fix: curl pod wait until finish mv to front, remove unnecessary Event…

ea089d0

…ually

refactor: rayservice get cluster name logic

ce9a0fc

MortalHappiness requested changes Nov 15, 2024

View reviewed changes

ray-operator/test/support/ray.go Outdated Show resolved Hide resolved

ray-operator/test/e2e/rayservice_in_place_update_test.go Outdated Show resolved Hide resolved

ray-operator/test/e2e/rayservice_in_place_update_test.go Outdated Show resolved Hide resolved

CheyuWu added 5 commits November 15, 2024 21:56

fix: rm when curl api

c8bd4e0

chore: mv check headpod and workerpods running below the

70204d4

fix: UnderlyingRayCluster rm and not get ray service in UnderlyingRay…

1c25f57

…Cluster

style: fix the linter issue

fb29465

stlye: fix linter issue

16bc013

MortalHappiness previously requested changes Nov 16, 2024

View reviewed changes

CheyuWu added 2 commits November 16, 2024 14:51

fix: change UnderlyingRayCluster to UnderlyingRayClusterName and chan…

4df9d2e

…ge the comment in curlRayServicePod

fix: use SpecApplyConfiguration instead of apply yaml

7657ae7

Merge remote-tracking branch 'upstream/master' into feat/inplaceUpdate

9bd2130

kevin85421 reviewed Nov 17, 2024

View reviewed changes

fix(pass): rm useless code, fix typo, and use g.Eventually to curl se…

df1986e

…rvice

kevin85421 reviewed Nov 20, 2024

View reviewed changes

CheyuWu added 3 commits November 21, 2024 01:50

chore: rm one fruit stand in rayServiceSampleYamlApplyConfiguration's…

c1abd66

… ServeConfigV2

fix: rm unnecessary checking, we only need to check head pod ready

ef8c79f

test: use default cluster spec to run

63fb8db

kevin85421 approved these changes Nov 20, 2024

View reviewed changes

fix: remove unnecessary code checking

fecc462

test: add ray_actor_options and num_cpus to Router

d150fed

CheyuWu closed this Nov 21, 2024

CheyuWu reopened this Nov 21, 2024

kevin85421 self-assigned this Nov 22, 2024

test: reduce MangoStand resource usage

9f420ad

CheyuWu added 3 commits November 23, 2024 14:13

chore: increase resourceCPU

5e2b8b1

Revert "chore: increase resourceCPU" This reverts commit 5e2b8b1.

5aaf78d

chore: use customize rayClusterSpec instead

8db3dad

kevin85421 merged commit 8dda8b3 into ray-project:master Nov 24, 2024
25 checks passed

Ygnas pushed a commit to Ygnas/kuberay that referenced this pull request Mar 20, 2025

[Test] Implement RayService In-place update test in Golang (ray-proje…

9400bb9

…ct#2536)

	func UnderlyingRayCluster(rayService rayv1.RayService) func() (rayv1.RayCluster, error) {
	func UnderlyingRayClusterName(rayService *rayv1.RayService) string {

[Test] Implement RayService In-place update test in Golang #2536

[Test] Implement RayService In-place update test in Golang #2536

Uh oh!

Conversation

CheyuWu commented Nov 12, 2024

Why are these changes needed?

Related issue number

Checks

Uh oh!

CheyuWu commented Nov 12, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CheyuWu commented Nov 14, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CheyuWu commented Nov 16, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CheyuWu commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

CheyuWu Nov 21, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CheyuWu Nov 18, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

CheyuWu Nov 20, 2024 • 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!

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!

CheyuWu Nov 20, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

CheyuWu Nov 20, 2024 • 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!

kevin85421 left a comment

Choose a reason for hiding this comment

CheyuWu commented Nov 17, 2024 •

edited

Loading

CheyuWu Nov 21, 2024 •

edited

Loading

CheyuWu Nov 18, 2024 •

edited

Loading

CheyuWu Nov 20, 2024 •

edited

Loading

CheyuWu Nov 20, 2024 •

edited

Loading

CheyuWu Nov 20, 2024 •

edited

Loading

CheyuWu commented Nov 21, 2024 •

edited

Loading

MortalHappiness commented Nov 21, 2024 •

edited

Loading

CheyuWu commented Nov 21, 2024 •

edited

Loading

CheyuWu commented Nov 22, 2024 •

edited

Loading

CheyuWu commented Nov 23, 2024 •

edited

Loading