Add RayClusterReady Condition Type #2271

Yicheng-Lu-llll · 2024-07-26T21:15:47Z

Why are these changes needed?

This PR adds RayClusterReady condition type. See here for the reason: https://docs.google.com/document/d/1bRL0cZa87eCX6SI7gqthN68CgmHaB6l3-vJuIse-BrY/edit.

RayClusterReady indicates whether all Ray Pods are ready when the RayCluster is first created.

  Conditions:
    Last Transition Time:   2024-07-30T05:58:56Z
    Message:                
    Reason:                 HeadPodRunningAndReady
    Status:                 True
    Type:                   HeadPodReady
    Last Transition Time:   2024-07-30T05:58:56Z
    Message:                All Ray Pods are ready. Future checks focus on the head
    Reason:                 AllPodRunningAndReady
    Status:                 True
    Type:                   RayClusterReady

After RayClusterReady is set to true for the first time, it only indicates whether the RayCluster's Head Pod is ready for

    Last Transition Time:   2024-07-30T05:58:56Z
    Message:                
    Reason:                 HeadPodRunningAndReady
    Status:                 True
    Type:                   HeadPodReady
    Last Transition Time:   2024-07-30T05:58:56Z
    Message:                Only check head after all Ray Pods are initially ready
    Reason:                 HeadPodRunningAndReady
    Status:                 True
    Type:                   RayClusterReady

Related issue number

Checks

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

Yicheng-Lu-llll · 2024-07-28T05:53:10Z

ray-operator/apis/ray/v1/raycluster_types.go

@@ -168,14 +168,19 @@ type RayClusterConditionType string

 // Custom Reason for RayClusterCondition
 const (
+	AllPodRunningAndReady = "AllPodRunningAndReady"
+	HeadPodNotFound       = "HeadPodNotFound"
 	// PodRunningAndReady says that the pod is running and ready.
 	PodRunningAndReady = "PodRunningAndReady"


I actually prefer changing PodRunningAndReady to HeadPodRunningAndReady to make it more explicit. This also makes it easier for me to copy the Reason field from HeadPodReady to RayClusterReady.

I actually prefer changing PodRunningAndReady to HeadPodRunningAndReady to make it more explicit.

makes sense to me

Yicheng-Lu-llll · 2024-07-28T05:54:50Z

ray-operator/apis/ray/v1/raycluster_types.go

 	// PodRunningAndReady says that the pod is running and ready.
 	PodRunningAndReady = "PodRunningAndReady"
 	// UnknownReason says that the reason for the condition is unknown.
 	UnknownReason = "Unknown"
 )

 const (
-	// HeadPodReady is added in a RayCluster when its Head Pod is ready for requests.


I changed this comment a little bit because HeadPodReady will always be there no matter whether the Head Pod is ready or not; it is just its status that will be true or false.

kevin85421 · 2024-07-29T23:39:19Z

ray-operator/apis/ray/v1/raycluster_types.go

@@ -168,14 +168,19 @@ type RayClusterConditionType string

 // Custom Reason for RayClusterCondition
 const (
+	AllPodRunningAndReady = "AllPodRunningAndReady"
+	HeadPodNotFound       = "HeadPodNotFound"
 	// PodRunningAndReady says that the pod is running and ready.
 	PodRunningAndReady = "PodRunningAndReady"


I actually prefer changing PodRunningAndReady to HeadPodRunningAndReady to make it more explicit.

makes sense to me

kevin85421 · 2024-07-29T23:40:46Z

ray-operator/apis/ray/v1/raycluster_types.go

 	// PodRunningAndReady says that the pod is running and ready.
 	PodRunningAndReady = "PodRunningAndReady"
 	// UnknownReason says that the reason for the condition is unknown.
 	UnknownReason = "Unknown"
 )

 const (
-	// HeadPodReady is added in a RayCluster when its Head Pod is ready for requests.
+	// RayClusterReady indicates whether all Ray Pods are ready when the RayCluster is first created.
+	// After RayClusterReady is set to true for the first time, it only indicates whether the RayCluster's Head Pod is ready for requests.


Suggested change

// After RayClusterReady is set to true for the first time, it only indicates whether the RayCluster's Head Pod is ready for requests.

// After RayClusterReady is set to true for the first time, it only indicates whether the RayCluster's head Pod is ready for requests.

kevin85421 · 2024-07-29T23:41:02Z

ray-operator/apis/ray/v1/raycluster_types.go

+	// RayClusterReady indicates whether all Ray Pods are ready when the RayCluster is first created.
+	// After RayClusterReady is set to true for the first time, it only indicates whether the RayCluster's Head Pod is ready for requests.
+	RayClusterReady RayClusterConditionType = "RayClusterReady"
+	// HeadPodReady indicates weather RayCluster's Head Pod is ready for requests.


Suggested change

// HeadPodReady indicates weather RayCluster's Head Pod is ready for requests.

// HeadPodReady indicates weather RayCluster's head Pod is ready for requests.

kevin85421 · 2024-07-29T23:44:05Z

ray-operator/controllers/ray/raycluster_controller.go

+				Type:    string(rayv1.RayClusterReady),
+				Status:  headPodReadyCondition.Status,
+				Reason:  headPodReadyCondition.Reason,
+				Message: headPodReadyCondition.Message,


update Message

kevin85421 · 2024-07-29T23:44:12Z

ray-operator/controllers/ray/raycluster_controller.go

+				meta.SetStatusCondition(&newInstance.Status.Conditions, metav1.Condition{
+					Type:   string(rayv1.RayClusterReady),
+					Status: metav1.ConditionTrue,
+					Reason: rayv1.AllPodRunningAndReady,


add Message

Yicheng-Lu-llll · 2024-07-30T06:01:48Z

ray-operator/controllers/ray/raycluster_controller_unit_test.go

@@ -1717,8 +1717,6 @@ func TestCalculateStatus(t *testing.T) {
 	// Test CheckRayHeadRunningAndReady with head pod running and ready
 	newInstance, _ = r.calculateStatus(ctx, testRayCluster, nil)
 	assert.True(t, meta.IsStatusConditionPresentAndEqual(newInstance.Status.Conditions, string(rayv1.HeadPodReady), metav1.ConditionTrue))
-	condition := meta.FindStatusCondition(newInstance.Status.Conditions, string(rayv1.HeadPodReady))


This check is the same as assert.True(t, meta.IsStatusConditionPresentAndEqual(newInstance.Status.Conditions, string(rayv1.HeadPodReady), metav1.ConditionTrue)), so I deleted it.

Yicheng-Lu-llll · 2024-07-30T06:04:31Z

ray-operator/controllers/ray/raycluster_controller_unit_test.go


 	// Test reconcilePodsErr with the feature gate enabled
 	newInstance, err = r.calculateStatus(ctx, testRayCluster, utils.ErrFailedCreateHeadPod)
 	assert.Nil(t, err)
 	assert.True(t, meta.IsStatusConditionPresentAndEqual(newInstance.Status.Conditions, string(rayv1.RayClusterReplicaFailure), metav1.ConditionTrue))
 }

+func TestRayClusterReadyCondition(t *testing.T) {


After several tries, I decided to make it a separate function with TestCalculateStatus. Otherwise, it is too difficult to write with so many things coupled together. I think the test is fine being a little long, and these tests will be rewritten to use a real client in the future anyway.

kevin85421 · 2024-07-31T06:18:51Z

ray-operator/apis/ray/v1/raycluster_types.go

+	// RayClusterReady indicates whether all Ray Pods are ready when the RayCluster is first created.
+	// After RayClusterReady is set to true for the first time, it only indicates whether the RayCluster's head Pod is ready for requests.
+	RayClusterReady RayClusterConditionType = "RayClusterReady"
+	// HeadPodReady indicates weather RayCluster's head Pod is ready for requests.


Suggested change

// HeadPodReady indicates weather RayCluster's head Pod is ready for requests.

// HeadPodReady indicates whether RayCluster's head Pod is ready for requests.

kevin85421 · 2024-07-31T06:34:05Z

ray-operator/controllers/ray/raycluster_controller_unit_test.go

+	_ = fakeClient.Status().Update(ctx, headPod)
+	_ = fakeClient.Status().Update(ctx, workerPod)
+	testRayCluster, _ = r.calculateStatus(ctx, testRayCluster, nil)
+	assert.Nil(t, meta.FindStatusCondition(testRayCluster.Status.Conditions, string(rayv1.RayClusterReady)))


can you also check reasons or messages?

Updated. I did not check line 1814 because RayClusterReady should not be added at that time.

Signed-off-by: Yicheng-Lu-llll <luyc58576@gmail.com>

kevin85421 self-assigned this Jul 27, 2024

kevin85421 added the 1.2.0 label Jul 27, 2024

Yicheng-Lu-llll force-pushed the Redefine-rayv1-Ready branch from 6bbe9e7 to dd3c27b Compare July 28, 2024 04:00

Yicheng-Lu-llll changed the title ~~WIP: Redefine Rayv1 Ready~~ WIP: Add RayClusterReady Condition Type Jul 28, 2024

Yicheng-Lu-llll commented Jul 28, 2024

View reviewed changes

Yicheng-Lu-llll changed the title ~~WIP: Add RayClusterReady Condition Type~~ Add RayClusterReady Condition Type Jul 28, 2024

Yicheng-Lu-llll marked this pull request as ready for review July 28, 2024 05:56

kevin85421 reviewed Jul 29, 2024

View reviewed changes

Yicheng-Lu-llll commented Jul 30, 2024

View reviewed changes

kevin85421 reviewed Jul 31, 2024

View reviewed changes

add RayClusterReady condition type

9f90369

Signed-off-by: Yicheng-Lu-llll <luyc58576@gmail.com>

Yicheng-Lu-llll force-pushed the Redefine-rayv1-Ready branch from 7d0c5c9 to 9f90369 Compare July 31, 2024 15:26

kevin85421 approved these changes Jul 31, 2024

View reviewed changes

kevin85421 merged commit cbaf5d7 into ray-project:master Jul 31, 2024
25 checks passed

This was referenced Aug 4, 2024

[Feature] REP 54: Re-define rayv1.Ready #2238

Closed

[Bug] KubeRay cluster resource status is reporting Ready when there are pods still pending #2188

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add RayClusterReady Condition Type #2271

Add RayClusterReady Condition Type #2271

Uh oh!

Yicheng-Lu-llll commented Jul 26, 2024 •

edited

Loading

Uh oh!

Yicheng-Lu-llll Jul 28, 2024 •

edited

Loading

Uh oh!

kevin85421 Jul 29, 2024

Uh oh!

Yicheng-Lu-llll Jul 28, 2024

Uh oh!

kevin85421 Jul 29, 2024

Uh oh!

kevin85421 Jul 29, 2024

Uh oh!

kevin85421 Jul 29, 2024

Uh oh!

kevin85421 Jul 29, 2024

Uh oh!

kevin85421 Jul 29, 2024

Uh oh!

Yicheng-Lu-llll Jul 30, 2024

Uh oh!

Yicheng-Lu-llll Jul 30, 2024

Uh oh!

kevin85421 Jul 31, 2024

Uh oh!

kevin85421 Jul 31, 2024

Uh oh!

Yicheng-Lu-llll Jul 31, 2024

Uh oh!

Uh oh!

Uh oh!

	// After RayClusterReady is set to true for the first time, it only indicates whether the RayCluster's Head Pod is ready for requests.
	// After RayClusterReady is set to true for the first time, it only indicates whether the RayCluster's head Pod is ready for requests.

	// HeadPodReady indicates weather RayCluster's Head Pod is ready for requests.
	// HeadPodReady indicates weather RayCluster's head Pod is ready for requests.

	// HeadPodReady indicates weather RayCluster's head Pod is ready for requests.
	// HeadPodReady indicates whether RayCluster's head Pod is ready for requests.

Add RayClusterReady Condition Type #2271

Add RayClusterReady Condition Type #2271

Uh oh!

Conversation

Yicheng-Lu-llll commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

Yicheng-Lu-llll Jul 28, 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!

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!

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!

Uh oh!

Uh oh!

Yicheng-Lu-llll commented Jul 26, 2024 •

edited

Loading

Yicheng-Lu-llll Jul 28, 2024 •

edited

Loading