kuberay icon indicating copy to clipboard operation
kuberay copied to clipboard

[Feature] Add `DeleteWorkersOnFailure` deletion policy for RayJob

Open kevin85421 opened this issue 11 months ago • 8 comments

Search before asking

  • [x] I had searched in the issues and found no similar feature requirement.

Description

DeleteWorkersOnFailure: Deletes workers only when the Ray job fails and deletes the entire RayCluster when the Ray job succeeds. This seems to be a more common pattern for users.

Should we add this policy or rename DeleteWorkers to DeleteWorkersOnFailure? Does it need to be in v1.3.0?

Use case

No response

Related issues

No response

Are you willing to submit a PR?

  • [ ] Yes I am willing to submit a PR!

kevin85421 avatar Jan 17 '25 06:01 kevin85421

cc @andrewsykim any thoughts?

kevin85421 avatar Jan 17 '25 06:01 kevin85421

Having both policies probably makes sense. I'm in favor of a new policy like DeleteWorkersOnFailure. Would it be too verbose to name it something like DeleteClusterOnSuccessOrWorkersOnFailure?

andrewsykim avatar Jan 17 '25 15:01 andrewsykim

DeleteWorkersOnFailure is probably fine as long as the deletion policy on success is well documented in the API or documentation

andrewsykim avatar Jan 17 '25 15:01 andrewsykim

On second thought, I realized that users may have more combinations. For example,

  1. DeleteCluster on success, DeleteNone on failure
  2. DeleteSelf on success, DeleteNone on failure
  3. DeleteCluster on success, DeleteWorkers on failure
  4. DeleteSelf on success, DeleteWorkers on failure

There are two solutions:

  1. Keep the current API, but adds new API like DeleteClusterOnSuccessOrWorkersOnFailure if needed
  2. Separate deletion API into OnFailureDeletionPolicy and OnSuccessDeletionPolicy.

kevin85421 avatar Jan 17 '25 19:01 kevin85421

mark this issue as v1.3.0 because we need to make a decision about the API before the release.

kevin85421 avatar Jan 17 '25 21:01 kevin85421

On second thought, I realized that users may have more combinations. For example,

DeleteCluster on success, DeleteNone on failure DeleteSelf on success, DeleteNone on failure DeleteCluster on success, DeleteWorkers on failure DeleteSelf on success, DeleteWorkers on failure There are two solutions:

Keep the current API, but adds new API like DeleteClusterOnSuccessOrWorkersOnFailure if needed Separate deletion API into OnFailureDeletionPolicy and OnSuccessDeletionPolicy.

These are really good considerations, since we put the feature behind an alpha feature gate I feel fine about breaking the API in v1.4 if needed.

andrewsykim avatar Jan 20 '25 18:01 andrewsykim

We can consider an API like this as well:

spec:
  deletionPolicy:
    onSuccess: DeleteCluster
    onFailure: DeleteWorkers

andrewsykim avatar Jan 20 '25 18:01 andrewsykim

OK, let's update the API in v1.4.0.

kevin85421 avatar Jan 21 '25 21:01 kevin85421