[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 <[email protected]>

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

@kevin85421 @MortalHappiness
PTAL

MortalHappiness · 2024-11-13T11:50:49Z

ray-operator/test/support/ray.go

+func GetRayService(t Test, namespace, name string) (*rayv1.RayService, error) {
+	return t.Client().Ray().RayV1().RayServices(namespace).Get(t.Ctx(), name, metav1.GetOptions{})
+}


Could you rebase with the master branch? This function already exists on the master branch.

MortalHappiness · 2024-11-13T11:53:26Z

ray-operator/test/e2e/rayservice_in_place_update_test.go

+		serveConfig = strings.Replace(serveConfig, "factor: 5", "factor: 3", -1)
+
+		rayService.Spec.ServeConfigV2 = serveConfig
+		rs, err := UpdateRayServiceConfig(test, namespace.Name, rayServiceFromYaml.Name, serveConfig)


I think we don't need this UpdateRayServiceConfig function. Simply call t.Client().Ray().RayV1().RayServices(namespace).Update directly here is enough.

MortalHappiness · 2024-11-13T11:58:17Z

ray-operator/test/support/core.go

 }

-func SetupPortForward(t Test, podName, namespace string, localPort, remotePort int) (chan struct{}, error) {
+func SetupPortForward(t Test, req *rest.Request, localPort, remotePort int) (chan struct{}, error) {


Just curious about why the function signature changed?

Well, when I was working on this task, I was considering whether to use the method of creating a curl pod or setting up a port-forward. I then encountered an issue where SetupPortForward couldn’t port-forward the service, so I initially switched to a more general approach. Ultimately, I decided to go with creating a curl pod, but I forgot to delete it afterward.

Should I revert this or just keep it?

I think you can revert it because it is not related to this PR.

MortalHappiness · 2024-11-13T11:59:51Z

ray-operator/test/support/core.go

+				{
+					Name:    containerName,
+					Image:   "rancher/curl",
+					Command: []string{"/bin/sh", "-c", "while true; do sleep 10; done"},


I think tail -f /dev/null is better here.

MortalHappiness · 2024-11-13T12:10:07Z

ray-operator/test/e2e/rayservice_in_place_update_test.go

+		rs, err := GetRayService(test, namespace.Name, rayServiceFromYaml.Name)
+		g.Expect(err).NotTo(HaveOccurred())
+		if rs.Status.PendingServiceStatus.RayClusterName != "" {
+			rayClusterName = rs.Status.PendingServiceStatus.RayClusterName
+		} else if rs.Status.ActiveServiceStatus.RayClusterName != "" {
+			rayClusterName = rs.Status.ActiveServiceStatus.RayClusterName
+		}
+		g.Expect(rayClusterName).NotTo(BeEmpty())


I found the logic here also appears in ray-operator/test/sampleyaml/rayservice_test.go. We can refactor this logic to a helper function called UnderlyingRayCluster that takes RayService as input and returns RayCluster, and then use WithTransform in both places.

MortalHappiness · 2024-11-13T12:11:22Z

ray-operator/test/e2e/rayservice_in_place_update_test.go

+	curlPodName := "curl-pod"
+	curlContainerName := "curl-container"
+
+	curlPod, err := CreateCurlPod(test, curlPodName, curlContainerName, namespace.Name)


We should wait until the curl pod becomes ready using Eventually

I check the curl pod status below the create curl pod

Oh I found it. I think you can put it directly under this line to improve readability.

MortalHappiness · 2024-11-13T12:14:07Z

ray-operator/test/e2e/rayservice_in_place_update_test.go

+		rsStatus := RayServiceStatus(rs)
+		g.Expect(rsStatus).To(Equal(rayv1.Running))


Only these 2 lines need to be wrapped with Eventually. We don't want to updte the rayservice config repeatedly.

MortalHappiness · 2024-11-13T12:14:53Z

ray-operator/test/e2e/rayservice_in_place_update_test.go

+	g.Eventually(func(g Gomega) {
+		newRs, err := GetRayService(test, namespace.Name, rayServiceFromYaml.Name)
+		g.Expect(err).NotTo(HaveOccurred())
+		// curl /fruit
+		stdout, _ := curlRayServicePod(test, newRs, curlPod, curlContainerName, "/fruit", `["MANGO", 2]`)
+		g.Expect(stdout.String()).To(Equal("8"))
+		// curl /calc
+		stdout, _ = curlRayServicePod(test, newRs, curlPod, curlContainerName, "/calc", `["MUL", 3]`)
+		g.Expect(stdout.String()).To(Equal("9 pizzas please!"))
+	}, TestTimeoutShort).Should(Succeed())


I don't think we need to wrap these lines in Eventually.

The reason I'm wrapping this with Eventually is that the curl request is made too quickly, and the results are not correct. However, If I retry the steps it will succeed eventually. So, I wrap with Eventually

Why if the result will be incorrect if the curl request is sent too quickly? Once we make sure both the RayService and the curl pod are running and ready, the request must succeed. Do you know the root cause of why the result may be incorrect?

I'm thinking it might be that the service hasn't cleared the original resources yet. Waiting for 5 seconds works. I'll take a look and see how I can handle this.

Great! Once you find the root cause, you can use Eventually to wait for that state change to steady state before sending requests.

I think I forget to check g.Expect(newRs.Status.ObservedGeneration).To(Equal(newRs.Generation))
Eventually is not need anymore

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!

CheyuWu added 6 commits November 12, 2024 01:08

refactor: setup Port forward request

6ac63c2

Signed-off-by: Cheyu Wu <[email protected]>

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 <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

CheyuWu commented Nov 12, 2024

CheyuWu commented Nov 12, 2024

MortalHappiness Nov 13, 2024

MortalHappiness Nov 13, 2024 •

edited

Loading

MortalHappiness Nov 13, 2024

CheyuWu Nov 13, 2024 •

edited

Loading

CheyuWu Nov 13, 2024

MortalHappiness Nov 13, 2024 •

edited

Loading

MortalHappiness Nov 13, 2024

MortalHappiness Nov 13, 2024

MortalHappiness Nov 13, 2024

CheyuWu Nov 13, 2024

MortalHappiness Nov 13, 2024

MortalHappiness Nov 13, 2024

MortalHappiness Nov 13, 2024

CheyuWu Nov 13, 2024 •

edited

Loading

MortalHappiness Nov 13, 2024 •

edited

Loading

CheyuWu Nov 13, 2024 •

edited

Loading

MortalHappiness Nov 13, 2024

CheyuWu Nov 13, 2024 •

edited

Loading

CheyuWu commented Nov 14, 2024

		rsStatus := RayServiceStatus(rs)
		g.Expect(rsStatus).To(Equal(rayv1.Running))

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

Are you sure you want to change the base?

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

Conversation

CheyuWu commented Nov 12, 2024

Why are these changes needed?

Related issue number

Checks

CheyuWu commented Nov 12, 2024

Choose a reason for hiding this comment

MortalHappiness Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CheyuWu Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MortalHappiness Nov 13, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CheyuWu Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

MortalHappiness Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

CheyuWu Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CheyuWu Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

CheyuWu commented Nov 14, 2024

MortalHappiness Nov 13, 2024 •

edited

Loading

CheyuWu Nov 13, 2024 •

edited

Loading

MortalHappiness Nov 13, 2024 •

edited

Loading

CheyuWu Nov 13, 2024 •

edited

Loading

MortalHappiness Nov 13, 2024 •

edited

Loading

CheyuWu Nov 13, 2024 •

edited

Loading

CheyuWu Nov 13, 2024 •

edited

Loading