[RayCluster][Fix] leave .Status.State untouched when there is a reconcile error

[rueian]

Why are these changes needed?

We accidentally set the .State of RayCluster CR to rayv1.Ready even if there was a reconcile error in PR #2258 where we removed rayv1.Failed. This made users unable to deal with false ready RayClusters with no pod actually created, see #2357.

But since #2258 has been released and the rayv1.Failed has gone, I think we can't bring back rayv1.Failed either now. This PR fixes the issue by leaving the .State field untouched when there is a reconcile error but setting the error message in the .Reason field. That means the initial .State for case #2357 will be blank and the pod creation error will be in the .Reason field. The new unit test case in this PR also tests this behavior.

Example result:

▶ kubectl describe raycluster raycluster-mini
Name:         raycluster-mini
Namespace:    default
Labels:       <none>
Annotations:  <none>
API Version:  ray.io/v1
Kind:         RayCluster
Metadata:
  Creation Timestamp:  2024-12-06T22:53:44Z
  Generation:          1
  Resource Version:    1393
  UID:                 da4d6b0d-c3a3-4f00-9b9b-e4c411e2d6be
Spec:
  Head Group Spec:
    ...
  Ray Version:       2.9.0
  Suspend:           false
  Worker Group Specs:
    ...
Status:
  Desired CPU:              2
  Desired GPU:              0
  Desired Memory:           2G
  Desired TPU:              0
  Desired Worker Replicas:  1
  Endpoints:
    Client:     10001
    Dashboard:  8265
    Metrics:    8080
    Redis:      6379
  Head:
    Service Name:       raycluster-mini-head-svc
  Last Update Time:     2024-12-06T22:53:46Z
  Max Worker Replicas:  1
  Min Worker Replicas:  1
  Observed Generation:  1
  Reason:               FailedCreateHeadPod
pods "raycluster-mini-head-v6zqb" is forbidden: exceeded quota: demoquota, requested: cpu=1,memory=1G, used: cpu=100m,memory=512Mi, limited: cpu=1,memory=1G
Events:
  Type     Reason                 Age   From                   Message
  ----     ------                 ----  ----                   -------
  Normal   CreatedService         3s    raycluster-controller  Created service default/raycluster-mini-head-svc
  Warning  FailedToCreateHeadPod  3s    raycluster-controller  Failed to create head Pod default/, pods "raycluster-mini-head-tqljd" is forbidden: exceeded quota: demoquota, requested: cpu=1,memory=1G, used: cpu=100m,memory=512Mi, limited: cpu=1,memory=1G
  Warning  FailedToCreateHeadPod  3s    raycluster-controller  Failed to create head Pod default/, pods "raycluster-mini-head-gt52b" is forbidden: exceeded quota: demoquota, requested: cpu=1,memory=1G, used: cpu=100m,memory=512Mi, limited: cpu=1,memory=1G

Related issue number

Closes #2357

Checks

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

[kevin85421]

do we still need this in case #2357 (comment)?

[rueian]

I think we still need this. Currently, we mark the RayCluster CR as Ready when no Pods are created due to the k8s quota limit. That doesn't look correct in any way.

This PR will make .State left blank in this case since we don't introduce the Failed state back.

[kevin85421]

maybe we need to check the number of Pods before passing to the function CheckAllPodsRunning?

[rueian]

maybe we need to check the number of Pods before passing to the function CheckAllPodsRunning?

Yeap, checking len(runtimePods) > 0 before invoking CheckAllPodsRunning() works as well. Is there any reason we ignore the reconcileErr here?

[kevin85421]

Is there any reason we ignore the reconcileErr here?

does it include in conditions?

[rueian]

Yes, it is included in the ReplicaFailure condition, but if the feature gate is disable, then users can only find the error from logs.

And I think it is not very good to call CheckAllPodsRunning while the reconcileErr is not nil. That can cause we turn a CR to Ready state early. Shouldn't we turn the CR to Ready only when the reconcileErr is nil?

[kevin85421]

And I think it is not very good to call CheckAllPodsRunning while the reconcileErr is not nil. That can cause we turn a CR to Ready state early. Shouldn't we turn the CR to Ready only when the reconcileErr is nil?

This is a bug. We should not call CheckAllPodsRunning when the number of running Pods is not equal to the expected number of Pods.

[kevin85421]

Discussed offline:

1. we will not set Reason
2. only call CheckAllPodsRunning when reconcile error is nil and the number of Pods is as expected.

[rueian]

Discussed offline:

1. we will not set Reason
2. only call CheckAllPodsRunning when reconcile error is nil and the number of Pods is as expected.

Updated. Test cases related to calculateStatus are also modified to make sure we don't change mark the CR as Ready when there are not enough Pods.

[kevin85421]

1. Add comments to explain this test.
2. Add assertions to ensure that testRayCluster fulfills the assumptions made by this test (i.e., the worker group's replicas should not be 0).

[rueian]

Done.

[kevin85421]

same

[rueian]

Done.