Succeeded is always 0 or 1 for now, but this could be useful in the future if we want a success policy siimlar to Job: https://kubernetes.io/docs/concepts/workloads/controllers/job/#success-policy

Maybe we can transition the status from JobDeploymentStatusRunning to JobDeploymentStatusRestarting directly. If we update the status here, it may enter the if rayJobInstance.Spec.ShutdownAfterJobFinishes && len(rayJobInstance.Spec.ClusterSelector) == 0 statement.

Maybe we can transition the status from JobDeploymentStatusRunning to JobDeploymentStatusRestarting directly.

I think it'll be confusing to skip the JobDeploymentStatusFailed state though. Job going directly from Running -> Restarting could imply the job was restarted for another reason

If we update the status here, it may enter the if rayJobInstance.Spec.ShutdownAfterJobFinishes && len(rayJobInstance.Spec.ClusterSelector) == 0 statement.

This is a good catch. Should we instead just add a check for JobDeploymentStatus == JobDeploymentStatusComplete here?

I think rayv1.JobDeploymentStatusComplete and rayv1.JobDeploymentStatusFailed should be terminal states, meaning that no state transitions from these two states to any other states are allowed.

Maybe consider adding a Failing status, so the state transition would be Running -> Failing -> Restarting -> New ... -> Failed or Complete. This is how the Spark operator does it.

https://github.com/kubeflow/spark-operator/blob/5ce3dbacff76bba364055b9b786110f1a4ab3174/pkg/controller/sparkapplication/controller.go#L485-L520

I renamed Restarting -> Retrying and updating state flow to Running -> Retrying -> New

Do we need to include the status of JobStatusFailed here? For example, there is a user who collects the driver logs by reading the submitter K8s Job's STDOUT / STDERR. If the submitter K8s job fails but the Ray job succeeds, should we restart? The user doesn't collect any driver logs.

Ah, I thought JobStatusFailed only occurs if the job failed from Ray API, but it wouldn't work if it's also Failed if the submitter job fails. Will update to only check JobDeploymentStatus

Should we enforce shutdownAfterJobFinishes=true when setting backoffLimit > 0? Or should we still respect it but only for the last retry?

Maybe the later one for now?

sounds good, I think this is already the case then

Out of curiosity, what's the reason to use *int32 instead of int32? This PR checks whether it is nil or not multiple times.

I was being safe in case a user updates the kuberay-operator version before they update the CRD manifest. In that case all the code for backoffLimit is guarded by rayJobInstance.Spec.BackoffLimit != nil and never runs. Also, it effects the omitempty behavior, if it's not a pointer we will omit the field if set to 0.

Thinking about it more, this maybe doesn't need to be a pointer. Unlike with numOfHosts, if we default backOffLimit to 0 there should be no changes in behavior if there's version skew between kuberay-operator and the CRD. So I guess this mainly boils down to the omitempty behavior. This would need to be a pointer if we defaulted backoffLimit to a value > 1 though.

@kevin85421 I'm keeping this as a pointer per the recommendation in the Kubernetes API conventions for optoinal / omitempty fields: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

Maybe the later one for now?

Maybe move the logic to

			if jobInfo.JobStatus == rayv1.JobStatusFailed {
				jobDeploymentStatus = rayv1.JobDeploymentStatusFailed
				reason = rayv1.AppFailed
				failedCount++
			}

?

done

We can also check whether BackoffLimit is smaller than 0 or not in validateRayJobSpec, and then we don't need to check *rayJob.Spec.BackoffLimit > 0 here.

done

add some comments

done

There's 3 different places where we check for retry now:

1. checkK8sJobAndUpdateStatusIfNeeded
2. checkActiveDeadlineAndUpdateStatusIfNeeded
3. Reconcile -- inside rayv1.JobDeploymentStatusRunning case

I'd like to refactor this code a bit so that we only need to check for retry in one place in a future PR

How about we just update Failed / Succeeded and transition to Retrying if needed in updateRayJobStatus? Then, we don't need to update Failed / Succeeded and check backoffLimit in the switch rayJobInstance.Status.JobDeploymentStatus statement.

func (r *RayJobReconciler) updateRayJobStatus(ctx context.Context, oldRayJob *rayv1.RayJob, newRayJob *rayv1.RayJob) error {
	logger := ctrl.LoggerFrom(ctx)
	oldRayJobStatus := oldRayJob.Status
	newRayJobStatus := newRayJob.Status
        if newRayJobStatus.JobDeploymentStatus == rayv1.JobDeploymentStatusFailed {
            // (1) Update `Failed` 
            // (2) Check backoffLimit: update the status to `Retrying` if needed.
        } else if newRayJobStatus.JobDeploymentStatus == rayv1.JobDeploymentStatusComplete {
            // (1) Update `Succeeded`
        }


        ...
	return nil
}

I like that updateRayJobStatus doesn't have any logic for the RayJob state machine and only does equality check + the API call. I think adding the backoffLimit logic here would be more confusing in the end because we're pushing the statemachine logic outside the switch in Reconcile.

I removed case 2) for checkActiveDeadlineAndUpdateStatusIfNeeded so there's only 2 places where backoffLimit is checked. I think this is fine for now. What do you think?

I may prefer to add logic in updateRayJobStatus. It's fine to update the status in this function because it is the only place to update the CR status. Maintaining retry in a single place also enables us to easily understand when we should and should not retry. Sorry for the last-minute change.

Agree it's better to keep it in a single place. How about a function like checkBackoffLimt() before updateRayJobStatus() that checks for retries and updates the deployment status? I had this in the initial version but moved it into the switch case

How about a function like checkBackoffLimt() before updateRayJobStatus() that checks for retries and updates the deployment status?

SGTM

I had this in the initial version but moved it into the switch case

My bad. Try to add a GIF from my GIPHY Chrome extensions to this comment, but I failed lol

finally figure it out

haha nice :)

Added new method checkBackoffLimitAndUpdateStatusIfNeeded to consolidate retry checks into a single place

nit: Please do NOT add any code between checkBackoffLimitAndUpdateStatusIfNeeded and the following code.

done, but is the comment really relevant anymore?

We should also check the reason for failure. If the reason is DeadlineExceeded, we should not retry.

Good catch, I'll also have a follow-up PR to add tests for active deadline seconds + backoff limit

Thinking about it more, it's kind of weird having to check DeadlineExceeded inside checkBackoffLimitAndUpdateStatusIfNeeded, it should be agnostic to the reason.

What do you think about calling checkBackoffLimitAndUpdateStatusIfNeeded twice inside the Running switch case:

1. Right after calling checkK8sJobAndUpdateStatusIfNeeded
2. At the end of the Running switch case

And we can exclude it after checkActiveDeadlineAndUpdateStatusIfNeeded. I think this will be less error prone since we're not checking retry for all future cases as well

See the latest commit: a7bc5cd

I may prefer to call checkBackoffLimitAndUpdateStatusIfNeeded once, but I am completely fine with merging this PR for now. I will open a follow up PR to update it a bit.