fix: only filter RayCluster events for reconciliation #882

davidxia · 2023-01-23T15:43:11Z

#639 accidentally applied event filters for child resources Pods and Services. This change does not filter Pod or Service related events. This means Pod updates will trigger RayCluster reconciliation.

closes #872

Checks

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

ray-operator/controllers/ray/raycluster_controller.go

architkulkarni · 2023-01-23T20:53:14Z

Nice catch, it sounds like this was a pretty subtle issue. I didn't quite understand how the code change works, any chance you could add more detail here? Somehow WithPredicates doesn't apply the filters to child resources, but EventFilter does?

architkulkarni · 2023-01-23T20:53:26Z

Also, I heard that one reason the filter was added originally was to prevent reconciling in a tight loop, because instance.Status.LastUpdateTime is updated at each reconcile (@brucez-anyscale may have more context here):

kuberay/ray-operator/controllers/ray/raycluster_controller.go

Line 885 in 9d50cbd

instance.Status.LastUpdateTime = &timeNow

Would that issue be reintroduced with this PR?

davidxia · 2023-01-23T21:40:41Z

This change only applies the predicates to the RayCluster resources and not all watched resources like Pods and Services. The original bug won't be introduced. @architkulkarni would you be able to test out this change?

architkulkarni · 2023-01-24T00:54:55Z

Sure, I'll run a manual test.

architkulkarni · 2023-01-24T14:49:41Z

The manual test passed for this PR (I confirmed that the job was successfully submitted and didn't hang.) Also I checked the logs and confirmed that reconciling wasn't happening in a tight loop (no more reconciling happened after the last reconcile)

DmitriGekhtman

Looks good -- could you push an empty commit to re-trigger the CI?

ray-project#639 accidentally applied event filters for child resources Pods and Services. This change does not filter Pod or Service related events. This means Pod updates will trigger RayCluster reconciliation. closes ray-project#872

kevin85421

Good catch! LGTM.

kevin85421 · 2023-01-26T13:15:39Z

@davidxia would you mind checking the error message of CI? The new unit test fails.

davidxia · 2023-01-26T17:24:00Z

Thanks

• Failure [0.000 seconds]
Inside the default namespace
/home/runner/work/kuberay/kuberay/ray-operator/controllers/ray/raycluster_controller_test.go:46
  When creating a raycluster
  /home/runner/work/kuberay/kuberay/ray-operator/controllers/ray/raycluster_controller_test.go:[131](https://github.com/ray-project/kuberay/actions/runs/4006247114/jobs/6881074875#step:9:132)
    cluster's .status.state should be updated to 'ready' [It]
    /home/runner/work/kuberay/kuberay/ray-operator/controllers/ray/raycluster_controller_test.go:160

    Expected
        <v1alpha1.ClusterState>: unhealthy
    to equal
        <v1alpha1.ClusterState>: ready

    /home/runner/work/kuberay/kuberay/ray-operator/controllers/ray/raycluster_controller_test.go:164

I added debug logs to see why the start params are invalid.

davidxia · 2023-01-26T18:12:31Z

@kevin85421 The debug logs I added in last commit don't show up in the failing test logs. Do you know how I can debug further? I think it's because the ray start params are invalid.

DmitriGekhtman · 2023-01-27T08:01:11Z

@kevin85421 The debug logs I added in last commit don't show up in the failing test logs. Do you know how I can debug further? I think it's because the ray start params are invalid.

Seems we need to access the logs of the operator in the context of the test.
Or try copying rayStartParams from an example cr.

kevin85421 · 2023-01-27T14:55:51Z

@kevin85421 The debug logs I added in last commit don't show up in the failing test logs. Do you know how I can debug further? I think it's because the ray start params are invalid.

I can reproduce this bug on my laptop by:

# path: kuberay/ray-operator
make test

There are three reasons to make this test fail.

Reason 1

kuberay/ray-operator/controllers/ray/raycluster_controller_test.go

Line 65 in db0ee96

"object-store-memory": "100000000",

object-store-memory is defined, but the RAY_OBJECT_STORE_ALLOW_SLOW_STORAGE (i.e. AllowSlowStorageEnvVar) environment variable does not exist. Hence, the function ValidateHeadRayStartParams will return false. We can either removing object-store-memory or adding RAY_OBJECT_STORE_ALLOW_SLOW_STORAGE to solve this bug.

Reason 2

kuberay/ray-operator/controllers/ray/raycluster_controller_test.go

Lines 151 to 158 in db0ee96

    
           It("should create 3 workers", func() { 
        
           	Eventually( 
        
           		listResourceFunc(ctx, &workerPods, filterLabels, &client.ListOptions{Namespace: "default"}), 
        
           		time.Second*15, time.Millisecond*500).Should(Equal(3), fmt.Sprintf("workerGroup %v", workerPods.Items)) 
        
           	if len(workerPods.Items) > 0 { 
        
           		Expect(workerPods.Items[0].Status.Phase).Should(Or(Equal(corev1.PodRunning), Equal(corev1.PodPending))) 
        
           	} 
        
           })

The new test is just after the test "should create 3 workers". However, the worker Pods are highly possible to be in pending rather than running. Hence, the function CheckAllPodsRunnning will return false. You can print the Pods' states by the following code:

It("cluster's .status.state should be updated to 'ready'", func() {
	listResourceFunc(ctx, &workerPods, filterLabels, &client.ListOptions{Namespace: "default"})
	for _, aPod := range workerPods.Items {
		fmt.Printf("Pod Phase: %v\n", aPod.Status.Phase)
	}
	Eventually(
		getResourceFunc(ctx, client.ObjectKey{Name: myRayCluster.Name, Namespace: "default"}, myRayCluster),
		time.Second*60, time.Millisecond*500).Should(BeNil(), "My myRayCluster  = %v", myRayCluster.Name)
	Expect(myRayCluster.Status.State).Should(Equal(rayiov1alpha1.Ready))
})

Maybe the test case logic should be:

It("cluster's .status.state should be updated to 'ready'", func() {
  // Step 1: Wait for all Pods becoming running.
  // Step 2: Check myRayCluster.Status.State
})

Reason 3

The following change may be unnecessary.
Eventually(..., time.Millisecond*500) -> Eventually(...,time.Second*15, time.Millisecond*500)

davidxia · 2023-01-28T17:37:06Z

ray-operator/controllers/ray/raycluster_controller_test.go

@@ -62,7 +63,6 @@ var _ = Context("Inside the default namespace", func() {
 					"port":                "6379",
 					"object-manager-port": "12345",
 					"node-manager-port":   "12346",
-					"object-store-memory": "100000000",


need to remove for controller to think ray start params are valid

davidxia · 2023-01-28T17:37:25Z

ray-operator/controllers/ray/raycluster_controller_test.go

+		It("cluster's .status.state should be updated to 'ready' shortly after all Pods are Running", func() {
+			Eventually(
+				getClusterState(ctx, "default", myRayCluster.Name),
+				time.Second*(common.RAYCLUSTER_DEFAULT_REQUEUE_SECONDS+5), time.Millisecond*500).Should(Equal(rayiov1alpha1.Ready))
+		})


the important part

davidxia · 2023-01-28T17:38:09Z

ray-operator/controllers/ray/suite_test.go

@@ -79,6 +81,7 @@ var _ = BeforeSuite(func(done Done) {
 	Expect(k8sClient).ToNot(BeNil())

 	// Suggested way to run tests
+	os.Setenv(common.RAYCLUSTER_DEFAULT_REQUEUE_SECONDS_ENV, "10")


decrease requeue delay to make test faster

davidxia · 2023-01-28T17:39:29Z

ray-operator/controllers/ray/raycluster_controller_test.go

+		It("should be able to update all Pods to Running", func() {
+			for _, workerPod := range workerPods.Items {
+				workerPod.Status.Phase = corev1.PodRunning
+				Expect(k8sClient.Status().Update(ctx, &workerPod)).Should(BeNil())
+			}
+			Consistently(
+				listResourceFunc(ctx, &workerPods, workerFilterLabels, &client.ListOptions{Namespace: "default"}),
+				time.Second*5, time.Millisecond*500).Should(Equal(3), fmt.Sprintf("workerGroup %v", workerPods.Items))
+			for _, workerPod := range workerPods.Items {
+				Expect(workerPod.Status.Phase).Should(Equal(corev1.PodRunning))
+			}
+
+			for _, headPod := range headPods.Items {
+				headPod.Status.Phase = corev1.PodRunning
+				Expect(k8sClient.Status().Update(ctx, &headPod)).Should(BeNil())
+			}
+			Consistently(
+				listResourceFunc(ctx, &headPods, headFilterLabels, &client.ListOptions{Namespace: "default"}),
+				time.Second*5, time.Millisecond*500).Should(Equal(1), fmt.Sprintf("headGroup %v", headPods.Items))
+			for _, headPod := range headPods.Items {
+				Expect(headPod.Status.Phase).Should(Equal(corev1.PodRunning))
+			}
+		})


Need to manually update Pod statuses otherwise they'll always be Pending. envtest doesn't create a full K8s cluster. It's only the control plane. There's no container runtime or any other K8s controllers. So Pods are created, but no controller updates them from Pending to Running.

setting up and starting an instance of etcd and the Kubernetes API server, without kubelet, controller-manager or other components

https://book.kubebuilder.io/reference/envtest.html

kevin85421

LGTM! Thank you for this contribution!

ray-operator/controllers/ray/raycluster_controller_test.go

kevin85421 · 2023-01-29T07:04:18Z

ray-operator/controllers/ray/raycluster_controller_test.go

+		It("should be able to update all Pods to Running", func() {
+			for _, workerPod := range workerPods.Items {
+				workerPod.Status.Phase = corev1.PodRunning
+				Expect(k8sClient.Status().Update(ctx, &workerPod)).Should(BeNil())
+			}
+			Consistently(
+				listResourceFunc(ctx, &workerPods, workerFilterLabels, &client.ListOptions{Namespace: "default"}),
+				time.Second*5, time.Millisecond*500).Should(Equal(3), fmt.Sprintf("workerGroup %v", workerPods.Items))
+			for _, workerPod := range workerPods.Items {
+				Expect(workerPod.Status.Phase).Should(Equal(corev1.PodRunning))
+			}
+
+			for _, headPod := range headPods.Items {
+				headPod.Status.Phase = corev1.PodRunning
+				Expect(k8sClient.Status().Update(ctx, &headPod)).Should(BeNil())
+			}
+			Consistently(
+				listResourceFunc(ctx, &headPods, headFilterLabels, &client.ListOptions{Namespace: "default"}),
+				time.Second*5, time.Millisecond*500).Should(Equal(1), fmt.Sprintf("headGroup %v", headPods.Items))
+			for _, headPod := range headPods.Items {
+				Expect(headPod.Status.Phase).Should(Equal(corev1.PodRunning))
+			}
+		})


kevin85421 · 2023-01-29T15:43:34Z

Merge this PR. The CI failure is due to #852.

ray-project#639 accidentally applied event filters for child resources Pods and Services. This change does not filter Pod or Service related events. This means Pod updates will trigger RayCluster reconciliation.

davidxia commented Jan 23, 2023

View reviewed changes

ray-operator/controllers/ray/raycluster_controller.go Show resolved Hide resolved

davidxia force-pushed the fix1 branch from 4939671 to 0f7ba1b Compare January 23, 2023 15:55

davidxia marked this pull request as ready for review January 23, 2023 15:55

davidxia mentioned this pull request Jan 23, 2023

[Bug] Pod updates should trigger RayCluster reconciliation #872

Closed

2 tasks

architkulkarni requested review from DmitriGekhtman and kevin85421 January 24, 2023 00:55

davidxia force-pushed the fix1 branch 2 times, most recently from 2ae9940 to ba5719f Compare January 25, 2023 03:42

DmitriGekhtman approved these changes Jan 25, 2023

View reviewed changes

davidxia force-pushed the fix1 branch from ba5719f to 9ded30a Compare January 25, 2023 13:11

kevin85421 approved these changes Jan 26, 2023

View reviewed changes

kevin85421 mentioned this pull request Jan 26, 2023

feat: enrich kubectl get output #878

Merged

4 tasks

davidxia force-pushed the fix1 branch 4 times, most recently from fcadb8e to acdf703 Compare January 28, 2023 17:36

davidxia commented Jan 28, 2023

View reviewed changes

davidxia force-pushed the fix1 branch from acdf703 to 0dfb134 Compare January 28, 2023 17:42

kevin85421 approved these changes Jan 29, 2023

View reviewed changes

add unit test; squash this commit

813ac99

davidxia force-pushed the fix1 branch from 0dfb134 to 813ac99 Compare January 29, 2023 14:18

kevin85421 merged commit 5953b48 into ray-project:master Jan 29, 2023

kevin85421 mentioned this pull request Feb 2, 2023

[Bug] Fix flaky test: should be able to update all Pods to Running #893

Merged

4 tasks

davidxia deleted the fix1 branch October 28, 2023 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: only filter RayCluster events for reconciliation #882

fix: only filter RayCluster events for reconciliation #882

davidxia commented Jan 23, 2023 •

edited

architkulkarni commented Jan 23, 2023

architkulkarni commented Jan 23, 2023

davidxia commented Jan 23, 2023

architkulkarni commented Jan 24, 2023

architkulkarni commented Jan 24, 2023 •

edited

DmitriGekhtman left a comment

kevin85421 left a comment

kevin85421 commented Jan 26, 2023

davidxia commented Jan 26, 2023

davidxia commented Jan 26, 2023

DmitriGekhtman commented Jan 27, 2023

kevin85421 commented Jan 27, 2023

davidxia Jan 28, 2023

davidxia Jan 28, 2023

davidxia Jan 28, 2023

davidxia Jan 28, 2023

kevin85421 Jan 29, 2023

kevin85421 left a comment

kevin85421 Jan 29, 2023

kevin85421 commented Jan 29, 2023

fix: only filter RayCluster events for reconciliation #882

fix: only filter RayCluster events for reconciliation #882

Conversation

davidxia commented Jan 23, 2023 • edited

Checks

architkulkarni commented Jan 23, 2023

architkulkarni commented Jan 23, 2023

davidxia commented Jan 23, 2023

architkulkarni commented Jan 24, 2023

architkulkarni commented Jan 24, 2023 • edited

DmitriGekhtman left a comment

Choose a reason for hiding this comment

kevin85421 left a comment

Choose a reason for hiding this comment

kevin85421 commented Jan 26, 2023

davidxia commented Jan 26, 2023

davidxia commented Jan 26, 2023

DmitriGekhtman commented Jan 27, 2023

kevin85421 commented Jan 27, 2023

Reason 1

Reason 2

Reason 3

davidxia Jan 28, 2023

Choose a reason for hiding this comment

davidxia Jan 28, 2023

Choose a reason for hiding this comment

davidxia Jan 28, 2023

Choose a reason for hiding this comment

davidxia Jan 28, 2023

Choose a reason for hiding this comment

kevin85421 Jan 29, 2023

Choose a reason for hiding this comment

kevin85421 left a comment

Choose a reason for hiding this comment

kevin85421 Jan 29, 2023

Choose a reason for hiding this comment

kevin85421 commented Jan 29, 2023

davidxia commented Jan 23, 2023 •

edited

architkulkarni commented Jan 24, 2023 •

edited