[RayJob] implement deletion policy API

[andrewsykim]

Why are these changes needed?

Implement RayJob DeletionPolicy API

Related issue number

#2615

Checks

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

[MortalHappiness]

Could you resolve the conflicts? Also, should we add some tests for this feature?

[andrewsykim]

Fixed conflicts, will add tests tomorrow

[andrewsykim]

Added unit tests, going to skip e2e tests for now since it's currently not trivial to enable feature gates in the e2e tests

[MortalHappiness]

LGTM

[kevin85421]

Move

			logger.Info(
				"RayJob deployment status",
				"jobDeploymentStatus", rayJobInstance.Status.JobDeploymentStatus,
				"deletionPolicy", rayJobInstance.Spec.DeletionPolicy,
				"ttlSecondsAfterFinished", ttlSeconds,
				"Status.endTime", rayJobInstance.Status.EndTime,
				"Now", nowTime,
				"ShutdownTime", shutdownTime)
			if shutdownTime.After(nowTime) {
				delta := int32(time.Until(shutdownTime.Add(2 * time.Second)).Seconds())
				logger.Info("shutdownTime not reached, requeue this RayJob for n seconds", "seconds", delta)
				return ctrl.Result{RequeueAfter: time.Duration(delta) * time.Second}, nil
			}

to the above of if features.Enabled(features.RayJobDeletionPolicy) && and remove the similar logics from L391 to L403.

[kevin85421]

do we need to return bool?

[andrewsykim]

no, this is an oversight

[kevin85421]

This function may not work when autoscaling is enabled. If autoscaling is enabled, Pod deletion is always determined by the Ray Autoscaler. KubeRay will not delete any Pods, even if the number of Pods exceeds the goal state.

[andrewsykim]

I see your point, but autoscaling with RayJob is pretty uncommon though right?

[andrewsykim]

One way to fix this is to also set max replicas to 0

[kevin85421]

but autoscaling with RayJob is pretty uncommon though right?

I checked with my colleagues, and this may be incorrect. Autoscaling is not very common for Ray Train. However, it is commonly used for Ray Data, Ray Tune, and RLlib.

[kevin85421]

Most Ray Data users use autoscaling.

[andrewsykim]

I don't mean the Ray API, I mean autoscaling is not common when using the RayJob custom resource. I am sure Ray Data with RayCluster + autoscaling is very common

[kevin85421]

Typically, a K8s controller should only write to the CR status and treat the CR spec as read-only, but implementing this feature without writing to the CR spec is challenging for us.

Perhaps a compromise solution is to add a new field to the RayCluster CRD (e.g., suspendWorkers: bool), where the RayJob controller only sets this field to true, and the RayCluster is responsible for deleting all Ray worker Pods.

This way, the RayJob controller doesn't need to modify replicas and minReplicas, which can also be modified by the Ray Autoscaler or users. Allowing multiple stakeholders to modify a field is typically the root cause of KubeRay's instability issues.

[andrewsykim]

Controllers writing to spec is not necessarily bad, but I see what you mean. I think it would be wrong to write to RayJob spec from RayJob controller, but in this case we're writing to RayCluster spec from RayJob controller. I feel that updating replcias, minReplicas and maxReplicas for ephemeral RayCluster specifcally is actually fine because we don't care about the RayCluster spec once the cluster is deleted.

Will think about this more and get back to you.

[kevin85421]

The main concern is that multiple personas can modify these fields, such as users, the Autoscaler, and the RayJob controller.

[andrewsykim]

Perhaps a compromise solution is to add a new field to the RayCluster CRD (e.g., suspendWorkers: bool), where the RayJob controller only sets this field to true, and the RayCluster is responsible for deleting all Ray worker Pods.

@kevin85421 how about a suspend field per worker group in WorkerGroupSpec? This allows for granularity of suspension per worker, and from RayJob we can just set suspend: true for all worker groups

[andrewsykim]

Here's the draft PR #2663

Let me know what you think, I will clean up the PR and add tests if the API looks good to you

[kevin85421]

The API looks good to me.

[kevin85421]

Update validateRayJobSpec to ensure that the combination of ShutdownAfterJobFinishes: true and rayJobInstance.Spec.DeletionPolicy != rayv1.DeleteNoneDeletionPolicy is invalid.

[kevin85421]

#2643 (comment)

Maybe we can split this issue into 3 PRs if the comment makes sense to you?

1. Add a new field and feature flag in RayJob.
2. Add a new field in RayCluster CRD to terminate all worker Pods.
3. Implement the deletion policy API based on (2)