This is removed by the pre-commit

Overall LGTM, just thinking about if using StartupDeadlineSeconds or PreRunningDeadlineSeconds would be a more consistent naming as we use ActiveDeadlineSeconds for the active ttl.

Updated to PreRunningDeadlineSeconds in 90ab655

cc @rueian @andrewsykim to think about the naming, tks!

Naming seems fine. But I am wondering if we should expand the DeletionStrategy API instead to cover this case instead of adding a new field https://github.com/ray-project/kuberay/blob/master/ray-operator/apis/ray/v1/rayjob_types.go#L90-L190

If we use DeletionPolicy, we can only clean-up the resource and there’s no option to just fail it. Also the clean-up logic will appear in two different places, which can make it harder to maintain.

With the current method, we can fail the RayJob and let the Failed state handle the clean-up, which can be better as we can keep the clean-up logic in the same place.

WDYT?

Good point, the DeletionPolicy is for defining TTL based on whether job succeed / failed. This feature is to transition jobs to FAILED, so makes sense to keep it separate.

On second thought, is it possible to just introduce a new OnInitiailizing or OnWaiting to the DeletionPolicy API to add a TTL similar to this?

For me the better process here is: reach pre-running timeout -> transit to Failed -> clean-up in failed state.

I think adding OnInitiailizing or OnWaiting to DeletionPolicy would run into the same issue as DeletionPolicy is about how/when to clean up resources, not about job state transitions.

In this case, if we put a “fail-on-timeout” behavior into DeletionPolicy, it would blur the API semantics as DeletionPolicy should only handle the deletion. Therefor I’d prefer keeping PreRunningDeadlineSeconds separate from DeletionPolicy

Are you expecting we would support scenarios where someone sets ttlSecondsBeforeRunning but not shutdownAfterJobFinishes?

Yes! I would also want to place the clean-up code in the same place (all handle in completed phase).

makes sense,thanks!

PreRunningDeadlineExceeded not excluded from retry logic

Medium Severity

The checkBackoffLimitAndUpdateStatusIfNeeded function prevents retries when the reason is DeadlineExceeded, but the newly introduced PreRunningDeadlineExceeded reason is not similarly excluded. When a RayJob fails with PreRunningDeadlineExceeded and a BackoffLimit is configured, the job will be retried — inconsistent with how DeadlineExceeded is handled. The retry would just recreate a RayCluster likely to hit the same deadline again, causing unnecessary resource churn.

It's expected, PreRunningDeadlineExceeded is the deadline for pre-running step. When this pre-running deadline exceed, it may be caused by some temporarily issues (e.g. connection issue, resource not available now). We should make it retryable.

nit: we can add +kubebuilder:validation, so invalid values get rejected at admission time.

Updated in 11ec6a3

Tested this locally and seems that WithPreRunningDeadlineSeconds may require around 60 seconds to pass. Not sure if the 30 seconds in Buildkite is sufficient.

Fixed and update timeout to 60 sec in afda0ff