[Feat][RayCluster] new RayClusterReplicaFailure condition

[rueian]

This PR uses a new RayClusterReplicaFailure condition to reflect the result of creating/deleting Pods when the features.RayClusterStatusConditions gate is enabled.

The idea is borrowed from the ReplicaSetReplicaFailure:

// These are valid conditions of a replica set.
const (
	// ReplicaSetReplicaFailure is added in a replica set when one of its pods fails to be created
	// due to insufficient quota, limit ranges, pod security policy, node selectors, etc. or deleted
	// due to kubelet being down or finalizers are failing.
	ReplicaSetReplicaFailure ReplicaSetConditionType = "ReplicaFailure"
)

which will be set by the Kubernetes ReplicaSet controller when there is a manageReplicasErr. The error will only occur when its API call to create or delete pods has failed. (ref)

We mirror the above behavior to make the new RayClusterReplicaFailure condition. More specifically, we turn the condition on when the controller fails to create or delete pods in the reconcilePods function. Additionally, We have covered all five kinds of failure reasons in the function:

• FailedDeleteAllPods
• FailedDeleteHeadPod
• FailedCreateHeadPod
• FailedDeleteWorkerPod
• FailedCreateWorkerPod

These reasons are more fine-grained than the ReplicaSetReplicaFailure which has only two kinds of reasons: FailedCreate, and FailedDelete.

Related issue number

Closes #2232

ray-project/enhancements#54

Checks

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

[kevin85421]

I think we should try to consolidate the code path to set up the condition. It is hard to maintain if we set the condition in multiple places.

[rueian]

I think we should try to consolidate the code path to set up the condition. It is hard to maintain if we set the condition in multiple places.

Indeed, we should keep them in one place. Actually, I think the place should be the defer block in this draft and we will consolidate them to the defer block in the #2235

[rueian]

The defaultRayClusterConditions returns a simple map. I made it a function because it can be reused in tests.

[rueian]

Conditions will only be set if the gate is enabled.

[kevin85421]

I prefer not to set conditions in the reconcile functions. Instead, we should set conditions in calculateStatus based on reconcileErr.

func calculateStatus(...) {
    if reconcileErr is PodError {
       // set condition
    }

}

[kevin85421]

• It is not very important for me to support fine-grained condition reasons. If we can easily support it, that is good. Otherwise, it is not necessary to support it, and use something like PodFailure instead.

• I prefer not to set conditions in the reconcile functions. Instead, we should set conditions in calculateStatus based on reconcileErr.

  func calculateStatus(...) {
      if reconcileErr is PodError {
         // set condition
      }
  }

• I am not sure whether there is an easy way to determine whether an error is thrown by the Pod creation or deletion. If not, we can consider another way.

[rueian]

Hi @kevin85421,

I prefer not to set conditions in the reconcile functions. Instead, we should set conditions in calculateStatus based on reconcileErr.

I am afraid that is not always feasible. We have multiple reconcileFuncs but have only one reconcileErr. Consider a case that the reconcileErr is not nil and is not an error from the reconcilePods function either, we will have difficulty with whether to turn the RayClusterReplicaFailure condition off or not.

If the reconcileErr comes from other preceding reconcileFuncs, we should leave the condition untouched.

If the reconcileErr comes from other succeeding reconcileFuncs, we should turn the condition off.

In other words, we will need to keep track of each execution of reconcileFuncs additionally if we set conditions based on the reconcileErr.

[kevin85421]

@rueian can we close this one?

[rueian]

Sure. No need to keep this.