[Feat][RayCluster] Use a new RayClusterReplicaFailure condition to reflect the result of reconcilePods #2259

rueian · 2024-07-19T14:01:01Z

This is a revised draft of #2245 based on the review.

In this version, a RayClusterReplicaFailure condition will be set whenever the reconcilePods() returns a reconcileErr. This is done by the calculateStatus() based on the reconcileErr solely. It assumes the reconcilePods() is the last reconciliation function.

We also consolidate the fine-grained condition reasons in the previous draft into one FailedReconcilePods for simplicity.

Tests are conducted in three parts:

Assert the reconcilePods() will return a reconcileErr.
Assert the calculateStatus() will set the condition only when the feature gate is enabled.
Assert the inconsistentRayClusterStatus() will report true when conditions are changed.

Checks

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

…flect the result of reconcilePods Signed-off-by: Rueian <rueiancsie@gmail.com>

andrewsykim · 2024-07-19T18:03:19Z

ray-operator/controllers/ray/raycluster_controller.go

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


Failing to reconcile pods is different from replica failure, do we need to make this distinction clearer?

For example, replica failure could mean one of the pods are unhealthy and need to be recreated, but a reconcile error could happen for other reasons before the pod is running

Hi @andrewsykim,

Possible distinctions are those in the previous draft:

FailedDeleteAllPods

FailedDeleteHeadPod

FailedCreateHeadPod

FailedDeleteWorkerPod

FailedCreateWorkerPod

Do they make sense to you?

Yes, but my point is that maybe reconcile error and replica failures are better off as separate conditions instead of different reasons within the same condition.

We borrow the idea of the replica failures condition from the Kubernetes ReplicaSet controller. It has only two condition reasons:

FailedCreate

FailedDelete

And it doesn't reflect the health of pods either. I guess that is because the health of pods is handled by the pod controller already.

The FailedReconcilePods reason in this PR may be oversimplified and exceeds the semantic of ReplicaFailure indeed. I think I can bring the previous draft back by introducing them as individual error instances and wrapping them at the appropriate points. However, those points are still in the reconcilePods function.

var ( failedDeleteAllPods = errstd.New("...") failedDeleteHeadPod = errstd.New("...") failedCreateHeadPod = errstd.New("...") failedDeleteWorkerPod = errstd.New("...") failedCreateWorkerPod = errstd.New("...") ) func (r *RayClusterReconciler) reconcilePods(ctx context.Context, instance *rayv1.RayCluster) error { ... if err := r.createHeadPod(ctx, *instance); err != nil { common.FailedClustersCounterInc(instance.Namespace) return errstd.Join(failedCreateHeadPod, err) } ... }

var ( failedDeleteAllPods = errstd.New("...") failedDeleteHeadPod = errstd.New("...") failedCreateHeadPod = errstd.New("...") failedDeleteWorkerPod = errstd.New("...") failedCreateWorkerPod = errstd.New("...") ) ...

This makes sense to me.

… errors Signed-off-by: Rueian <rueiancsie@gmail.com>

kevin85421 · 2024-07-20T00:49:48Z

ray-operator/controllers/ray/raycluster_controller.go

 func (r *RayClusterReconciler) reconcilePods(ctx context.Context, instance *rayv1.RayCluster) error {
+	if err := r.doReconcilePods(ctx, instance); err != nil {
+		return fmt.Errorf("%w: %w", reconcilePodsErr, err)


Maybe use https://pkg.go.dev/errors#Join and Unwrap instead?

We can use Join. But Unwrap doesn't work well with the Join.

To unwrap a joined error, we need a special trick:

errs := err.(interface{ Unwrap() []error }).Unwrap()

kevin85421 · 2024-07-22T07:02:03Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -591,7 +600,17 @@ func (r *RayClusterReconciler) reconcileHeadlessService(ctx context.Context, ins
 	return nil
 }

+// reconcilePodsErr is a marker used by the calculateStatus() for setting the RayClusterReplicaFailure condition.
+var reconcilePodsErr = errstd.New("reconcile pods error")


move to constant.go?

kevin85421 · 2024-07-22T07:05:47Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -391,6 +394,12 @@ func (r *RayClusterReconciler) inconsistentRayClusterStatus(ctx context.Context,
 			oldStatus.Endpoints, newStatus.Endpoints, oldStatus.Head, newStatus.Head))
 		return true
 	}
+	if !reflect.DeepEqual(oldStatus.Conditions, newStatus.Conditions) {
+		logger.Info("inconsistentRayClusterStatus", "detect inconsistency", fmt.Sprintf(


Avoid using fmt.Sprintf. Please use

logger.Info(..., ..., "old conditions", oldStatus.Conditions, "new conditions", newStatus.Conditions)

instead. See this doc for more details.

kevin85421 · 2024-07-22T07:08:15Z

ray-operator/controllers/ray/raycluster_controller.go

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


var ( failedDeleteAllPods = errstd.New("...") failedDeleteHeadPod = errstd.New("...") failedCreateHeadPod = errstd.New("...") failedDeleteWorkerPod = errstd.New("...") failedCreateWorkerPod = errstd.New("...") ) ...

This makes sense to me.

Signed-off-by: Rueian <rueiancsie@gmail.com>

…flect the result of reconcilePods Signed-off-by: Rueian <rueiancsie@gmail.com>

rueian · 2024-07-22T16:13:57Z

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

+)
+
+func RayClusterReplicaFailureReason(err error) string {
+	if e, ok := err.(interface{ Unwrap() []error }); ok {


err.(interface{ Unwrap() []error }).Unwrap() is the only special trick to unwrap a joined error.

https://cs.opensource.google/go/go/+/refs/tags/go1.22.5:src/errors/join.go;l=60-62

kevin85421

LGTM. Just left a nit comment.

kevin85421 · 2024-07-22T16:20:02Z

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

@@ -523,3 +523,11 @@ env_vars:
 		})
 	}
 }
+
+func TestErrRayClusterReplicaFailureReason(t *testing.T) {


can we test the case that the error is not a RayClusterReplicaFailure?

Sure. I will add a test that the RayClusterReplicaFailureReason() should return an empty string for a random error.

Signed-off-by: Rueian <rueiancsie@gmail.com>

rueian force-pushed the replica-failure-condition-by-err branch from 1478100 to 42b99fd Compare July 19, 2024 15:13

rueian added 2 commits July 19, 2024 23:55

[Feat][RayCluster] Use a new RayClusterReplicaFailure condition to re…

8482a25

…flect the result of reconcilePods Signed-off-by: Rueian <rueiancsie@gmail.com>

[Test][RayCluster] Use a new RayClusterReplicaFailure condition to re…

41462a9

…flect the result of reconcilePods Signed-off-by: Rueian <rueiancsie@gmail.com>

rueian force-pushed the replica-failure-condition-by-err branch from 4d8a1e6 to 41462a9 Compare July 19, 2024 15:55

rueian marked this pull request as ready for review July 19, 2024 16:23

andrewsykim reviewed Jul 19, 2024

View reviewed changes

kevin85421 self-assigned this Jul 20, 2024

rueian added 2 commits July 20, 2024 10:43

Merge branch 'master' into replica-failure-condition-by-err

0b78fb2

[Feat][RayCluster] Use errors.Join instead of fmt.Errorf for wrapping…

07811b3

… errors Signed-off-by: Rueian <rueiancsie@gmail.com>

kevin85421 reviewed Jul 22, 2024

View reviewed changes

rueian added 3 commits July 22, 2024 23:18

[Feat][RayCluster] avoid using fmt.Sprintf in logging

f08fa29

Signed-off-by: Rueian <rueiancsie@gmail.com>

[Feat][RayCluster] Use a new RayClusterReplicaFailure condition to re…

5000630

…flect the result of reconcilePods Signed-off-by: Rueian <rueiancsie@gmail.com>

[Feat][RayCluster] Use a new RayClusterReplicaFailure condition to re…

416d0bf

…flect the result of reconcilePods Signed-off-by: Rueian <rueiancsie@gmail.com>

rueian commented Jul 22, 2024

View reviewed changes

kevin85421 approved these changes Jul 22, 2024

View reviewed changes

[Feat][RayCluster] make the errRayClusterReplicaFailure marker private

000cda4

Signed-off-by: Rueian <rueiancsie@gmail.com>

kevin85421 merged commit ca39dc9 into ray-project:master Jul 22, 2024
25 checks passed

This was referenced Jul 22, 2024

[Feature] REP 54: Implement the ReplicaFailures condition #2232

Closed

[Bug] no feedback about failure to create submitter pod due to invalid spec #2210

Closed

[Feat][RayCluster] Use a new RayClusterReplicaFailure condition to reflect the result of reconcilePods #2259

[Feat][RayCluster] Use a new RayClusterReplicaFailure condition to reflect the result of reconcilePods #2259

Uh oh!

Conversation

rueian commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checks

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!

rueian Jul 20, 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!

kevin85421 left a comment

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!

rueian commented Jul 19, 2024 •

edited

Loading

rueian Jul 20, 2024 •

edited

Loading