[Feature][RayCluster]: Implement the HeadReady condition #2261

cchen777 · 2024-07-21T01:08:36Z

Why are these changes needed?

In this PR, we do the following

Add the implementation of HeadReady condition.
Update the logic in rayservice controller, where we will use HeadReady condition to determine whether a ray head is running instead o f checking the pod status directly.
Ensure this change is behind feature gate.
Support --feature-gate in helm chart

Related issue number

#2237, which is part of the ray-project/enhancements#54, the step 5 in the design doc

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests

I've tested the feature with feature gate enabled and verified that ray service controller can work properly using new condition

$ kind delete cluster --name kind && kind create cluster --image=kindest/node:v1.24.0 
$ IMG=kuberay/operator:nightly make docker-build && kind load docker-image kuberay/operator:nightly
$ helm install kuberay-operator --set image.repository=kuberay/operator --set image.tag=nightly --set "featureGates[0].name=RayClusterStatusConditions,featureGates[0].enabled=true" ../helm-chart/kuberay-operator
$ kubectl apply -f config/samples/ray-service.sample.yaml
# verify feature gate is enabled
$ kubectl get deployment kuberay-operator -o json | jq '.spec.template.spec.containers[0].args'  
[
  "--feature-gates=RayClusterStatusConditions=true"
]
$ kubectl logs -f kuberay-operator-599d9fbfd-2k9dc | grep "Loaded feature gates"
{"level":"info","ts":"2024-07-21T07:31:06.024Z","logger":"setup","msg":"Loaded feature gates","featureGates":{"RayClusterStatusConditions":true}}
# verify ray service is up and running
$ kubectl get rayservice
NAME                SERVICE STATUS   NUM SERVE ENDPOINTS
rayservice-sample   Running          2
# verify conditions are set correctly in raycluster
$ kubectl get raycluster rayservice-sample-raycluster-hjlxx -o json | jq .status.conditions
[
  {
    "lastTransitionTime": "2024-07-21T07:33:16Z",
    "message": "Head pod is running and ready",
    "reason": "HeadPodRunningAndReady",
    "status": "True",
    "type": "HeadReady"
  }
]

helm-chart/kuberay-operator/templates/_helpers.tpl

ray-operator/controllers/ray/rayservice_controller.go

ray-operator/controllers/ray/utils/util_test.go

rueian · 2024-07-21T12:00:22Z

ray-operator/controllers/ray/utils/util.go

+				return false, "HeadPodNotRunning", "Head pod is not running"
+			}
+			for _, cond := range pod.Status.Conditions {
+				if cond.Type == corev1.PodReady && cond.Status != corev1.ConditionTrue {


Could we use the utils.IsRunningAndReady here?

my thought was that utils.IsRunningAndReady only give us the information about Pod phase is Running + Pod is ready, it didn't tell us more fine-grain like whether it's due to Pod not running yet or Pod not ready yet (raylet may not pass readiness check yet) so i wrote the own checking

ray-operator/controllers/ray/utils/util.go

kevin85421 · 2024-07-22T06:57:59Z

ray-operator/controllers/ray/raycluster_controller.go


+	// Check if the head node is running and ready with proper reason and message
+	if features.Enabled(features.RayClusterStatusConditions) {
+		isRayHeadRunning, reason, message := utils.CheckRayHeadRunningAndReady(ctx, runtimePods)


I think we can use PodReady directly. We don't need to check whether the Pod is running, and we don't need to generate the reason and the message ourselves.

thanks for the suggestion, I tried using PodReady as much as possible, and notice that pod status condition's reason and message are empty all the time, and since metav1.Condition need to have an non-empty reason

// reason contains a programmatic identifier indicating the reason for the condition's last transition. // Producers of specific condition types may define expected values and meanings for this field, // and whether the values are considered a guaranteed API. // The value should be a CamelCase string. // This field may not be empty. // +required // +kubebuilder:validation:Required // +kubebuilder:validation:MaxLength=1024 // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:Pattern=`^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$` Reason [string](https://pkg.go.dev/builtin#string) `json:"reason" protobuf:"bytes,5,opt,name=reason"`

otherwise will see this error during reconcile

{"level":"info","ts":"2024-07-23T00:24:52.098Z","logger":"controllers.RayCluster","msg":"Error updating status","RayCluster":{"name":"rayservice-sample-raycluster-45jnx","namespace":"default"},"reconcileID":"2fb18e10-3cd9-41a0-b9e8-4660293fb634","name":"rayservice-sample-raycluster-45jnx","error":"RayCluster.ray.io \"rayservice-sample-raycluster-45jnx\" is invalid: status.conditions[0].reason: Invalid value: \"\": status.conditions[0].reason in body should be at least 1 chars long","RayCluster":{"apiVersion":"ray.io/v1","kind":"RayCluster","namespace":"default","name":"rayservice-sample-raycluster-45jnx"}}

thus i add reason HeadPodRunningAndReady and HeadPodNotReady respectively

kevin85421 · 2024-07-22T22:15:38Z

could you also rebase with the master branch? Thanks!

andrewsykim · 2024-07-23T04:26:17Z

ray-operator/controllers/ray/utils/util.go

+	replicaHeadReadyCondition := metav1.Condition{
+		Type:   string(rayv1.HeadReady),
+		Status: metav1.ConditionFalse,
+		Reason: "HeadPodNotReady",


This reason is not particular useful because it's already implied with the HeadReady condition being False. Can the reason be more specific, like CrashLoopBack, ContainerCreating, Pending, etc?

I hope to make the condition consistent with Pod's PodReady condition which both Reason and Message are empty. However, @cchen777 found that metav1.Condition has a limitation where Reason should not be empty, which is different from PodCondition. See #2261 (comment) for more details.

We can't keep Reason and Message empty due to metav1.Condition's limitation, but I still want to make our HeadReady condition closer to PodReady. Hence, I may prefer to keep the Reason and Condition simple ("HeadPodNotReady" is OK for me). We can add more details, such as CrashLoopBack, ContainerCreating, and Pending, when we observe enough use cases in the future.

I am curious whether there are obvious use cases for details about whether the Pod is ready or not. I assume that if there are obvious use cases, the upstream PodCondition should not leave Reason and Message empty.

pod status condition's reason and message are empty all the time

apology that this statement i mentioned was actually not true, Reason is empty when pod is running and ready, but when pod is not ready yet, there's actually non empty reasons, i think we can propagate reason from Pod itself but keep a customized reason HeadPodRunningAndReady when it's up and running to fit with metav1.Condition's limitation

The reason being empty or something like HeadPodReady when the pod is ready is fine I think. But when it's not ready, the reason should never be empty because we have information from the Pod for why it's not ready (failing readiness probe, crashing, OOMkill, etc)

update the logic to use

kuberay/ray-operator/controllers/ray/utils/util.go

Lines 82 to 96 in 41bd563

if cond.Type == corev1.PodReady {

if cond.Status == corev1.ConditionTrue {

replicaPodReadyCondition = metav1.Condition{

Type: string(condType),

Status: metav1.ConditionTrue,

Reason: v1.PodRunningAndReady, // metav1.Condition.Reason requires a non-empty value

Message: cond.Message,

}

} else {

replicaPodReadyCondition = metav1.Condition{

Type: string(condType),

Status: metav1.ConditionFalse,

Reason: cond.Reason, // PodReady condition comes with a reason when it's not ready, e.g. ContainersNotReady

Message: cond.Message,

}

so that when pod is not ready yet we can get the more accurate reason

kubectl get raycluster rayservice-sample-raycluster-tnm5m -o json | jq .status { "conditions": [ { "lastTransitionTime": "2024-07-25T04:25:32Z", "message": "containers with unready status: [ray-head]", "reason": "ContainersNotReady", "status": "False", "type": "Ready" } ], "desiredCPU": "2500m", "desiredGPU": "0", "desiredMemory": "4Gi", "desiredTPU": "0", "desiredWorkerReplicas": 1, "endpoints": { "client": "10001", "dashboard": "8265", "gcs-server": "6379", "metrics": "8080", "serve": "8000" }, "head": { "podIP": "10.244.0.9", "podName": "rayservice-sample-raycluster-tnm5m-head-m8h95", "serviceIP": "10.244.0.9", "serviceName": "rayservice-sample-raycluster-tnm5m-head-svc" }, "lastUpdateTime": "2024-07-25T04:25:54Z", "maxWorkerReplicas": 5, "minWorkerReplicas": 1, "observedGeneration": 1 }

ray-operator/controllers/ray/utils/util.go

kevin85421 · 2024-07-24T04:12:54Z

ray-operator/controllers/ray/utils/util.go

+	}
+
+	replicaHeadReadyCondition := metav1.Condition{
+		Type:   string(rayv1.HeadReady),


Maybe we can rename it from HeadReady to HeadPodReady because the upstream PodConidtion uses PodReady instead of Ready for the condition type.

update to HeadPodReady with Ready as value

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

Line 176 in 41bd563

HeadPodReady RayClusterConditionType = "Ready"

…on instead

kevin85421 · 2024-07-25T03:39:51Z

#2266 has been merged

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

kevin85421 · 2024-07-25T18:41:35Z

ray-operator/controllers/ray/raycluster_controller.go


+	// Check if the head node is running and ready by checking the head pod's status.
+	if features.Enabled(features.RayClusterStatusConditions) {
+		headPod, err := common.GetRayClusterHeadPod(ctx, r, newInstance)


It's possible for GetRayClusterHeadPod to return nil, nil when no head Pod exists. We need to handle the case.

cc @rueian maybe we can return a non-nil error when there is no head Pod exists?

I found that we always return a non-nil error.

headPod, err := common.GetRayClusterHeadPod(ctx, r, newInstance) if headPod == nil { // return an error }

ray-operator/controllers/ray/rayservice_controller.go

ray-operator/controllers/ray/utils/util.go

kevin85421 · 2024-07-27T19:18:37Z

ray-operator/controllers/ray/utils/util.go

+	}
+
+	for _, cond := range pod.Status.Conditions {
+		if cond.Type == corev1.PodReady {


if cond.Type == corev1.PodReady { var reason := cond.Reason if reason is empty { // metav1.Condition.Reason requires a non-empty value reason = cond.Reason } replicaPodReadyCondition = metav1.Condition{ Type: string(condType), Status: cond.Status, Reason: reason, Message: cond.Message, } break }

kevin85421 · 2024-07-27T19:21:41Z

ray-operator/controllers/ray/utils/util.go

 }

+func FindPodReadyCondition(pod *corev1.Pod, condType rayv1.RayClusterConditionType) metav1.Condition {
+	replicaPodReadyCondition := metav1.Condition{


Is it possible for pod.Status.Conditions not to have corev1.PodReady? If so, we will create a condition with an empty Reason.

yes you are right, i update the logic here to have an initial Condition

kuberay/ray-operator/controllers/ray/utils/util.go

Lines 75 to 79 in 01e90f6

replicaPodReadyCondition := metav1.Condition{

Type: string(condType),

Status: metav1.ConditionFalse,

Reason: rayv1.UnknownReason,

}

plus this logic

kuberay/ray-operator/controllers/ray/utils/util.go

Lines 95 to 98 in 01e90f6

// Update the reason if it's not empty

if reason != "" {

replicaPodReadyCondition.Reason = reason

}

to prevent empty reason sneak into later SetStatusCondition

ray-operator/controllers/ray/raycluster_controller_unit_test.go

ray-operator/controllers/ray/utils/util_test.go

ray-operator/controllers/ray/utils/util.go

kevin85421 · 2024-07-27T23:55:31Z

ray-operator/controllers/ray/utils/util.go

+		}
+
+		// Update the reason if it's not empty
+		if reason != "" {


this seems to be unnecessary?

i think it could happen when cond.Status != corev1.ConditionTrue and cond.Reason for some reason give us empty string, which i think is pretty rare when we are checking cond.Type == PodReady, but just to be safe i add a check here so it will fall back to default rayv1.UnknownReason non empty reason

cchen777 added 7 commits July 17, 2024 22:50

wip

6b12c3f

fix

b1c1a7e

refactor rayservice reconciler

0482ae2

Merge branch 'master' into KUBERAY-2238

0ee738c

rm defer code & move check head status into calculateStatus

8cbb04a

clean up

95ebeda

use feature gate

c90a5ef

cchen777 changed the title ~~[WIP][Feature][RayCluster]: Implement the HeadReady condition~~ [Feature][RayCluster]: Implement the HeadReady condition Jul 21, 2024

cchen777 force-pushed the KUBERAY-2237 branch from 67cdb6a to 62af5d9 Compare July 21, 2024 06:10

add testing & support feature-gate in helm chart

44c545f

cchen777 force-pushed the KUBERAY-2237 branch from 62af5d9 to 44c545f Compare July 21, 2024 07:39

cchen777 marked this pull request as ready for review July 21, 2024 07:42

cchen777 commented Jul 21, 2024

View reviewed changes

helm-chart/kuberay-operator/templates/_helpers.tpl Show resolved Hide resolved

rueian reviewed Jul 21, 2024

View reviewed changes

ray-operator/controllers/ray/rayservice_controller.go Outdated Show resolved Hide resolved

rueian reviewed Jul 21, 2024

View reviewed changes

ray-operator/controllers/ray/utils/util_test.go Outdated Show resolved Hide resolved

rueian reviewed Jul 21, 2024

View reviewed changes

address comments

fe6c653

kevin85421 self-assigned this Jul 22, 2024

kevin85421 reviewed Jul 22, 2024

View reviewed changes

cchen777 added 4 commits July 22, 2024 15:59

merge master

24df0bd

use condition, msg from pod status directly

c02b4ff

clean up

ffa4f43

fix test

a497a41

andrewsykim reviewed Jul 23, 2024

View reviewed changes

kevin85421 added the 1.2.0 label Jul 23, 2024

kevin85421 reviewed Jul 24, 2024

View reviewed changes

address comments 1. rename to HeadPodReady 2. use FindPodReadyConditi…

5221359

…on instead

Merge branch 'master' into KUBERAY-2237

3f059e7

clean up

583740c

cchen777 requested review from andrewsykim, kevin85421 and rueian July 25, 2024 16:46

kevin85421 reviewed Jul 25, 2024

View reviewed changes

cchen777 added 2 commits July 26, 2024 17:07

address comments

e6e4a1b

merge master

94cf0f2

cchen777 force-pushed the KUBERAY-2237 branch from 41bd563 to 94cf0f2 Compare July 27, 2024 00:09

kevin85421 reviewed Jul 27, 2024

View reviewed changes

cchen777 added 3 commits July 27, 2024 15:16

fix nit

71e2340

add unknown reason and better handle in FindPodReadyCondition

8fe0751

handle edge case that reason is empty but condition is false

01e90f6

kevin85421 approved these changes Jul 27, 2024

View reviewed changes

kevin85421 merged commit 5062a8c into ray-project:master Jul 28, 2024

kevin85421 mentioned this pull request Jul 28, 2024

[Feature] REP 54: Implement the HeadReady condition #2237

Closed

2 tasks

Yicheng-Lu-llll mentioned this pull request Sep 12, 2024

Add RayClusterReplicaSet initial implementation vllm-project/aibrix#165

Merged

	if cond.Type == corev1.PodReady {
	if cond.Status == corev1.ConditionTrue {
	replicaPodReadyCondition = metav1.Condition{
	Type: string(condType),
	Status: metav1.ConditionTrue,
	Reason: v1.PodRunningAndReady, // metav1.Condition.Reason requires a non-empty value
	Message: cond.Message,
	}
	} else {
	replicaPodReadyCondition = metav1.Condition{
	Type: string(condType),
	Status: metav1.ConditionFalse,
	Reason: cond.Reason, // PodReady condition comes with a reason when it's not ready, e.g. ContainersNotReady
	Message: cond.Message,
	}

	replicaPodReadyCondition := metav1.Condition{
	Type: string(condType),
	Status: metav1.ConditionFalse,
	Reason: rayv1.UnknownReason,
	}

	// Update the reason if it's not empty
	if reason != "" {
	replicaPodReadyCondition.Reason = reason
	}

[Feature][RayCluster]: Implement the HeadReady condition #2261

[Feature][RayCluster]: Implement the HeadReady condition #2261

Uh oh!

Conversation

cchen777 commented Jul 21, 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevin85421 commented Jul 22, 2024

Uh oh!

andrewsykim Jul 23, 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!

cchen777 Jul 24, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevin85421 commented Jul 25, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cchen777 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cchen777 commented Jul 21, 2024 •

edited

Loading

andrewsykim Jul 23, 2024 •

edited

Loading

cchen777 Jul 24, 2024 •

edited

Loading

cchen777 Jul 28, 2024 •

edited

Loading