Repository navigation
[SPARK-60052] Suspend and requeue a running SparkApplication on Kueue eviction - #952
dongjoon-hyun wants to merge 2 commits into
Conversation
|
Could you review this PR when you have some time, @viirya ? |
There was a problem hiding this comment.
Thanks for the PR, @dongjoon-hyun!
This routes a Kueue eviction of a running SparkApplication through the existing spec.suspend release path. The next attempt starts with ApplicationStatus#resume, the same way ClusterSuspendStep handles a cluster. The eviction marker in the state message, the spec.suspend precedence, and the rule to keep the Workload until its backoff elapses or it is reactivated all match the cluster side. keepKueueWorkload runs only on the eviction path, and resumedAppReleasesDeactivatedWorkload pins that. I found nothing blocking.
Minor
- 1.
suspendHoldRequeueIntervalSecondsdescription: it still describes only thespec.suspendhold, while it now also paces the hold of an evicted app whoseWorkloadis deactivated. inline
| return Optional.empty(); | ||
| } | ||
| if (KueueWorkloadUtils.isDeactivated(workload.get())) { | ||
| return Optional.of(Duration.ofSeconds(SUSPEND_HOLD_REQUEUE_INTERVAL_SECONDS.getValue())); |
There was a problem hiding this comment.
Finding 1. The deactivated-Workload hold here is paced by suspendHoldRequeueIntervalSeconds. Its description (SparkOperatorConf.java:284, rendered into docs/config_properties.md) still covers only a spec.suspend hold, and says that hold "ends only when a user clears spec.suspend". This hold ends when the Workload is reactivated instead. ClusterSuspendStep.keepKueueWorkload has had the same gap since SPARK-59803, so one added sentence would cover both, e.g.:
It also paces a SparkApplication or SparkCluster which Kueue evicted while its Workload is deactivated. That hold ends when the Workload is reactivated, which also arrives as a watch event.
There was a problem hiding this comment.
Thank you for the review and approval, @peter-toth. You're right. I added the sentence to the description of suspendHoldRequeueIntervalSeconds, which now covers the hold of an evicted SparkApplication or SparkCluster whose Workload is deactivated, and regenerated config_properties.md.
|
Thank you so much, @peter-toth ! 😄 |
viirya
left a comment
There was a problem hiding this comment.
Thanks for the PR! Routing Kueue evictions through the existing suspension path looks reasonable and keeps application behavior consistent with clusters.
I checked the interaction with driver observation, resource cleanup, admission, and ApplicationStatus.resume(). Persisting Suspended before releasing resources, checking driver termination before suspension, and releasing the previous attempt’s resources before requesting a new admission provide a sound ordering. The precedence of spec.suspend and preservation of deactivated Workloads also look correct.
The added unit and end-to-end tests cover the main eviction and requeue scenarios. I found no blocking issues. The latest commit also addresses Peter’s comment about suspendHoldRequeueIntervalSeconds in both the configuration source and generated documentation.
One non-blocking maintainability suggestion below concerns using the state message as a persisted control marker.
| */ | ||
| private static boolean isSuspendedByEviction(SparkApplication app) { | ||
| String message = app.getStatus().getCurrentState().getMessage(); | ||
| return message != null && message.startsWith(APP_EVICTED_MESSAGE); |
There was a problem hiding this comment.
Non-blocking: this makes the full user-facing message prefix part of the persisted control protocol. A future wording change could cause an application suspended by an older version to be treated as manually suspended, skipping keepKueueWorkload() and deleting a deactivated Workload or bypassing its remaining backoff.
The compatibility requirement is documented, and this matches the cluster implementation, so I don’t think it needs to block this PR. Could we consider a structured suspension reason in a follow-up for both applications and clusters?
There was a problem hiding this comment.
Thank you for the review and approval, @viirya. I agree. The message prefix mirrors ClusterSuspendStep, but a structured suspension reason would be more robust for both SparkApplication and SparkCluster, so let me handle it in a new JIRA issue. One thing to consider there is that the live CRD prunes an unknown status field until it is upgraded, since Helm does not upgrade CRDs, so the message prefix may need to stay as a fallback.
|
Merged to main |
### What changes were proposed in this pull request? This PR adds `status.currentState.suspendReason` (`SpecSuspend` or `KueueEviction`), which is set on `Suspended` states, and uses it in `AppSuspendStep` and `ClusterSuspendStep` instead of the prefix of the state message. - `SuspendReason` and the nullable field in `BaseState`. The existing 3-arg constructor is kept. - `isSuspendedByEviction` only checks the reason of the current state. The cluster's walk through the history is removed. The stuck-pods state keeps the reason of the state before it. - A `Suspended` state without a reason is treated as `SpecSuspend`. - Regenerate the `v1` CRDs (`v1beta1` is frozen), and document the field in `docs/spark_custom_resources.md`. ### Why are the changes needed? The message prefix became a control protocol, as pointed out in the review of #952. If the wording changes, a resource suspended by an eviction by an older version is taken for a manual suspend, so `keepKueueWorkload()` is skipped and the remaining requeue backoff is ignored. No message-prefix fallback is needed. `Suspended` is not in 1.0.0, and the migration guide already requires replacing the CRDs for 1.1.0, which ships the new field. ### Does this PR introduce _any_ user-facing change? No for the released versions. `suspendReason` is a new field within the unreleased 1.1.0 changes, and the state messages are unchanged. ### How was this patch tested? Pass the CIs. `gradle build` passes locally, with new and updated tests in `ApplicationStatusTest`, `ClusterStatusTest`, `AppSuspendStepTest`, `ClusterSuspendStepTest` and `ClusterInitStepTest`. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Sonnet 5.5 Closes #954 from dongjoon-hyun/SPARK-60057. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
What changes were proposed in this pull request?
This PR aims to suspend a running
SparkApplicationwhose KueueWorkloadis evicted or deactivated, and to queue it again with a new attempt, like a runningSparkCluster.AppSuspendStepmoves such an application toSuspendedand releases its driver and executors. Then it deletes theWorkloadafter the requeue backoff, or keeps a deactivated one until it is reactivated.ApplicationStatus#resume, so it does not count as a restart.PodsReadyTimeoutis handled like any other eviction, andspec.suspendtakes precedence.AppKueueEvictionStepand theKueueEvictionIgnoredevent are removed.Why are the changes needed?
Currently, the driver and executors keep running after an eviction. So preemption, cohort quota reclamation and
waitForPodsReadydo not work, and a deactivatedWorkloadover-commits the quota.Does this PR introduce any user-facing change?
No for the released versions, because the Kueue integration is not released yet in 1.0.0 (2026-07-23).
How was this patch tested?
Pass the CIs with the newly added test cases.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 5.5