You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dongjoon-hyun (via GitHub)" <gi...@apache.org> on 2026/09/24 20:05:30 UTC
[PR] [SPARK-59771] Release the Kueue `Workload` of `SparkApplication` only after its pods are gone [spark-kubernetes-operator]
dongjoon-hyun opened a new pull request, #885:
URL: https://github.com/apache/spark-kubernetes-operator/pull/885
### What changes were proposed in this pull request?
This PR aims to release the Kueue `Workload` of `SparkApplication` only after its driver and executor pods are gone, like `ClusterSuspendStep` does for `SparkCluster`.
- `AppCleanUpStep` deletes the driver resources first, keeps the application in its current state until no driver or executor pod remains, and then deletes the `Workload`. It still does so before a restart is scheduled.
- Only an application that holds Kueue quota waits. The wait is bounded by `forceTerminationGracePeriodMillis` from the start of the cleanup: the attempt stop, the deletion, or the retention expiry.
- `ReconcilerUtils.hasPods` extracts the remaining-pod check of `ClusterSuspendStep` so that `SparkAppContext` can reuse it.
### Why are the changes needed?
This is a follow-up of the following review comment.
- https://github.com/apache/spark-kubernetes-operator/pull/879#discussion_r4091942897
Previously, `AppCleanUpStep` released the `Workload` before it deleted the driver pod. So Kueue could admit another workload while the driver and executors were still terminating, and briefly overcommit the `ClusterQueue`.
### Does this PR introduce _any_ user-facing change?
Yes. A `SparkApplication` with a Kueue `Workload` now stays in its stopping state until its pods are gone. There is no change without the Kueue integration, which is disabled by default.
### How was this patch tested?
Pass the CIs with the newly added test cases in `AppCleanUpStepTest`, `SparkAppReconcilerTest`, and `SparkAppContextTest`.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 5.5
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59771] Release the Kueue `Workload` of `SparkApplication` only after its pods are gone [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #885:
URL: https://github.com/apache/spark-kubernetes-operator/pull/885#discussion_r4100732742
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppCleanUpStep.java:
##########
@@ -217,6 +225,117 @@ public ReconcileProgress reconcile(
}
}
+ /**
+ * Deletes the given resources and then releases the Kueue Workload of the application, once its
+ * driver and executor pods are gone. Kueue would admit another workload into the quota which the
+ * terminating pods still occupy. The deletion of each pod is observed by the pod informer, which
+ * reconciles again.
+ *
+ * <p>The Workload is released even if it was admitted before the queue label was removed. It is
+ * released before a restart is scheduled, since the Workload name is fixed per application and
+ * the next attempt would otherwise run on this admission. A restarted attempt is queued again
+ * with a new Workload only if the label is present.
+ *
+ * <p>A failed deletion is reported only after the Workload is released, unless pods remain, in
+ * which case it is retried with them. So a resource which cannot be deleted, e.g. a ConfigMap
+ * the operator may no longer delete, does not hold the quota once no pod uses it.
+ *
+ * @param context The SparkAppContext for the application.
+ * @param application The SparkApplication.
+ * @param resources The resources to delete.
+ * @return True if the Workload was released, false while the pods remain.
+ * @throws KubernetesClientException if a resource cannot be deleted and no pod remains.
+ */
+ private boolean releaseResources(
+ final SparkAppContext context,
+ final SparkApplication application,
+ final List<HasMetadata> resources) {
+ boolean forceDelete = enableForceDelete(application);
+ KubernetesClientException deleteFailure = null;
+ try {
Review Comment:
Thank you. You're right. Updated to catch per resource and keep the first failure with the others suppressed, so the driver pod is always asked to go. `everyResourceIsDeletedEvenIfAnotherFails` covers it.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppCleanUpStep.java:
##########
@@ -217,6 +225,117 @@ public ReconcileProgress reconcile(
}
}
+ /**
+ * Deletes the given resources and then releases the Kueue Workload of the application, once its
+ * driver and executor pods are gone. Kueue would admit another workload into the quota which the
+ * terminating pods still occupy. The deletion of each pod is observed by the pod informer, which
+ * reconciles again.
+ *
+ * <p>The Workload is released even if it was admitted before the queue label was removed. It is
+ * released before a restart is scheduled, since the Workload name is fixed per application and
+ * the next attempt would otherwise run on this admission. A restarted attempt is queued again
+ * with a new Workload only if the label is present.
+ *
+ * <p>A failed deletion is reported only after the Workload is released, unless pods remain, in
+ * which case it is retried with them. So a resource which cannot be deleted, e.g. a ConfigMap
+ * the operator may no longer delete, does not hold the quota once no pod uses it.
+ *
+ * @param context The SparkAppContext for the application.
+ * @param application The SparkApplication.
+ * @param resources The resources to delete.
+ * @return True if the Workload was released, false while the pods remain.
+ * @throws KubernetesClientException if a resource cannot be deleted and no pod remains.
+ */
+ private boolean releaseResources(
+ final SparkAppContext context,
+ final SparkApplication application,
+ final List<HasMetadata> resources) {
+ boolean forceDelete = enableForceDelete(application);
+ KubernetesClientException deleteFailure = null;
+ try {
+ for (HasMetadata resource : resources) {
+ ReconcilerUtils.deleteResourceIfExists(context.getClient(), resource, forceDelete);
+ }
+ } catch (KubernetesClientException e) {
+ deleteFailure = e;
+ }
+ if (isWaitingForPods(context, application)) {
+ if (deleteFailure != null) {
+ log.warn("Failed to delete the resources of the application, will retry.", deleteFailure);
+ }
+ return false;
+ }
+ KueueWorkloadUtils.releaseWorkload(context.getClient(), application);
Review Comment:
Good catch. Updated to call `deleteWorkloadOf` and keep the application in its state on a failure, so the next pass retries it before a restart is scheduled or the application terminates. Like a suspended `SparkCluster`, a persistent failure, e.g. a revoked access, holds the application until the access is restored, which the documentation now mentions. `failedWorkloadReleaseIsRetriedBeforeRestart` covers it.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppCleanUpStep.java:
##########
@@ -217,6 +225,117 @@ public ReconcileProgress reconcile(
}
}
+ /**
+ * Deletes the given resources and then releases the Kueue Workload of the application, once its
+ * driver and executor pods are gone. Kueue would admit another workload into the quota which the
+ * terminating pods still occupy. The deletion of each pod is observed by the pod informer, which
+ * reconciles again.
+ *
+ * <p>The Workload is released even if it was admitted before the queue label was removed. It is
+ * released before a restart is scheduled, since the Workload name is fixed per application and
+ * the next attempt would otherwise run on this admission. A restarted attempt is queued again
+ * with a new Workload only if the label is present.
+ *
+ * <p>A failed deletion is reported only after the Workload is released, unless pods remain, in
+ * which case it is retried with them. So a resource which cannot be deleted, e.g. a ConfigMap
+ * the operator may no longer delete, does not hold the quota once no pod uses it.
+ *
+ * @param context The SparkAppContext for the application.
+ * @param application The SparkApplication.
+ * @param resources The resources to delete.
+ * @return True if the Workload was released, false while the pods remain.
+ * @throws KubernetesClientException if a resource cannot be deleted and no pod remains.
+ */
+ private boolean releaseResources(
+ final SparkAppContext context,
+ final SparkApplication application,
+ final List<HasMetadata> resources) {
+ boolean forceDelete = enableForceDelete(application);
Review Comment:
Yes, the kubelet stops a force-deleted container within seconds, so I think the gap is acceptable. Changing when the driver is force deleted is out of the scope of this PR. I added a sentence to the javadoc of `getWaitForPodsDeadline` as you suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59771] Release the Kueue `Workload` of `SparkApplication` only after its pods are gone [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #885:
URL: https://github.com/apache/spark-kubernetes-operator/pull/885#issuecomment-5826914646
Merged to main
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59771] Release the Kueue `Workload` of `SparkApplication` only after its pods are gone [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #885:
URL: https://github.com/apache/spark-kubernetes-operator/pull/885#issuecomment-5822920421
Could you review this K8s operator PR when you have some time, @MaxGekk ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59771] Release the Kueue `Workload` of `SparkApplication` only after its pods are gone [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #885:
URL: https://github.com/apache/spark-kubernetes-operator/pull/885#issuecomment-5826090139
Thank you so much, @MaxGekk !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59771] Release the Kueue `Workload` of `SparkApplication` only after its pods are gone [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #885:
URL: https://github.com/apache/spark-kubernetes-operator/pull/885#issuecomment-5826169038
Thank you so much for the thorough review, @MaxGekk! All three comments are addressed in 9788f16.
- **Per-resource deletion**: `releaseResources` catches each failed deletion, keeping the first one and suppressing the others, so the driver pod is always asked to go even if a pre-resource cannot be deleted.
- **Retried `Workload` release**: The release uses `deleteWorkloadOf`, and a failure keeps the application in its state until the next pass succeeds. So neither the next attempt nor a terminated application keeps the previous admission. Like a suspended `SparkCluster`, a persistent failure, e.g. a revoked access, holds the application until the access is restored, and the documentation mentions it.
- **Force-deleted pods**: The javadoc of `getWaitForPodsDeadline` notes that the wait cannot observe a force-deleted pod while the kubelet stops its containers.
New test cases: `everyResourceIsDeletedEvenIfAnotherFails` and `failedWorkloadReleaseIsRetriedBeforeRestart`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59771] Release the Kueue `Workload` of `SparkApplication` only after its pods are gone [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #885: [SPARK-59771] Release the Kueue `Workload` of `SparkApplication` only after its pods are gone
URL: https://github.com/apache/spark-kubernetes-operator/pull/885
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59771] Release the Kueue `Workload` of `SparkApplication` only after its pods are gone [spark-kubernetes-operator]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #885:
URL: https://github.com/apache/spark-kubernetes-operator/pull/885#discussion_r4100312012
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppCleanUpStep.java:
##########
@@ -217,6 +225,117 @@ public ReconcileProgress reconcile(
}
}
+ /**
+ * Deletes the given resources and then releases the Kueue Workload of the application, once its
+ * driver and executor pods are gone. Kueue would admit another workload into the quota which the
+ * terminating pods still occupy. The deletion of each pod is observed by the pod informer, which
+ * reconciles again.
+ *
+ * <p>The Workload is released even if it was admitted before the queue label was removed. It is
+ * released before a restart is scheduled, since the Workload name is fixed per application and
+ * the next attempt would otherwise run on this admission. A restarted attempt is queued again
+ * with a new Workload only if the label is present.
+ *
+ * <p>A failed deletion is reported only after the Workload is released, unless pods remain, in
+ * which case it is retried with them. So a resource which cannot be deleted, e.g. a ConfigMap
+ * the operator may no longer delete, does not hold the quota once no pod uses it.
+ *
+ * @param context The SparkAppContext for the application.
+ * @param application The SparkApplication.
+ * @param resources The resources to delete.
+ * @return True if the Workload was released, false while the pods remain.
+ * @throws KubernetesClientException if a resource cannot be deleted and no pod remains.
+ */
+ private boolean releaseResources(
+ final SparkAppContext context,
+ final SparkApplication application,
+ final List<HasMetadata> resources) {
+ boolean forceDelete = enableForceDelete(application);
+ KubernetesClientException deleteFailure = null;
+ try {
+ for (HasMetadata resource : resources) {
+ ReconcilerUtils.deleteResourceIfExists(context.getClient(), resource, forceDelete);
+ }
+ } catch (KubernetesClientException e) {
+ deleteFailure = e;
+ }
+ if (isWaitingForPods(context, application)) {
+ if (deleteFailure != null) {
+ log.warn("Failed to delete the resources of the application, will retry.", deleteFailure);
+ }
+ return false;
+ }
+ KueueWorkloadUtils.releaseWorkload(context.getClient(), application);
Review Comment:
`releaseWorkload` swallows a failed DELETE and returns `false`, but `releaseResources` ignores the result and returns `true`, so the app moves on and the release is never tried again. The "garbage collected with its owner" fallback covers the deletion path, but not the other two:
- **`ScheduledToRestart`**: the next attempt calls `requestAdmission`, which finds the same fixed-name Workload with the same owner UID, still admitted, and returns `ADMITTED`. The new attempt then runs on the old admission, which the javadoc above says the release is there to prevent.
- **`ResourceReleased` without a restart**: the admitted Workload keeps its quota until the SparkApplication itself is deleted.
Could this call `deleteWorkloadOf` and, on a `KubernetesClientException`, return `false` (or rethrow) so the next pass retries? Since `deleteWorkloadOf` returns `false` when there is no Workload, retrying stays idempotent.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppCleanUpStep.java:
##########
@@ -217,6 +225,117 @@ public ReconcileProgress reconcile(
}
}
+ /**
+ * Deletes the given resources and then releases the Kueue Workload of the application, once its
+ * driver and executor pods are gone. Kueue would admit another workload into the quota which the
+ * terminating pods still occupy. The deletion of each pod is observed by the pod informer, which
+ * reconciles again.
+ *
+ * <p>The Workload is released even if it was admitted before the queue label was removed. It is
+ * released before a restart is scheduled, since the Workload name is fixed per application and
+ * the next attempt would otherwise run on this admission. A restarted attempt is queued again
+ * with a new Workload only if the label is present.
+ *
+ * <p>A failed deletion is reported only after the Workload is released, unless pods remain, in
+ * which case it is retried with them. So a resource which cannot be deleted, e.g. a ConfigMap
+ * the operator may no longer delete, does not hold the quota once no pod uses it.
+ *
+ * @param context The SparkAppContext for the application.
+ * @param application The SparkApplication.
+ * @param resources The resources to delete.
+ * @return True if the Workload was released, false while the pods remain.
+ * @throws KubernetesClientException if a resource cannot be deleted and no pod remains.
+ */
+ private boolean releaseResources(
+ final SparkAppContext context,
+ final SparkApplication application,
+ final List<HasMetadata> resources) {
+ boolean forceDelete = enableForceDelete(application);
+ KubernetesClientException deleteFailure = null;
+ try {
Review Comment:
The try/catch wraps the whole loop, so the first failed deletion skips every resource after it. On the `SchedulingFailure` path, `resourcesToRemove` is the pre-resources, then the driver pod, then the driver resources. If deleting a pre-resource ConfigMap fails, e.g. with a 403 (the case the javadoc below names), the driver pod is never asked to go. `isWaitingForPods` then finds the driver on every pass, each pass fails at the same ConfigMap, and after `forceTerminationGracePeriodMillis` the Workload is released while the driver is still running.
So the javadoc's "a resource which cannot be deleted ... does not hold the quota once no pod uses it" doesn't hold here, because the pod is never deleted. Would you catch per resource instead, keeping the first failure?
```java
for (HasMetadata resource : resources) {
try {
ReconcilerUtils.deleteResourceIfExists(context.getClient(), resource, forceDelete);
} catch (KubernetesClientException e) {
if (deleteFailure == null) {
deleteFailure = e;
} else {
deleteFailure.addSuppressed(e);
}
}
}
```
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppCleanUpStep.java:
##########
@@ -217,6 +225,117 @@ public ReconcileProgress reconcile(
}
}
+ /**
+ * Deletes the given resources and then releases the Kueue Workload of the application, once its
+ * driver and executor pods are gone. Kueue would admit another workload into the quota which the
+ * terminating pods still occupy. The deletion of each pod is observed by the pod informer, which
+ * reconciles again.
+ *
+ * <p>The Workload is released even if it was admitted before the queue label was removed. It is
+ * released before a restart is scheduled, since the Workload name is fixed per application and
+ * the next attempt would otherwise run on this admission. A restarted attempt is queued again
+ * with a new Workload only if the label is present.
+ *
+ * <p>A failed deletion is reported only after the Workload is released, unless pods remain, in
+ * which case it is retried with them. So a resource which cannot be deleted, e.g. a ConfigMap
+ * the operator may no longer delete, does not hold the quota once no pod uses it.
+ *
+ * @param context The SparkAppContext for the application.
+ * @param application The SparkApplication.
+ * @param resources The resources to delete.
+ * @return True if the Workload was released, false while the pods remain.
+ * @throws KubernetesClientException if a resource cannot be deleted and no pod remains.
+ */
+ private boolean releaseResources(
+ final SparkAppContext context,
+ final SparkApplication application,
+ final List<HasMetadata> resources) {
+ boolean forceDelete = enableForceDelete(application);
Review Comment:
A question rather than a request. `enableForceDelete` is anchored on the current state's `lastTransitionTime`, so deleting an app that has been running for longer than `forceTerminationGracePeriodMillis` deletes the driver with grace period 0 on the first pass. A grace-0 delete removes the Pod object from the API at once, so from then on `hasDriverOrExecutorPods` can't see the driver, while its container may still be stopping on the node.
With executors, the wait still holds them until they finish terminating, so the gap is small. For a driver-only app (e.g. one that never got executors), the Workload is released in the pass after the force delete. Is that acceptable because the kubelet stops a force-deleted container within seconds? If so, a sentence in the javadoc of `getWaitForPodsDeadline` saying that the wait cannot observe a force-deleted pod would help the next reader.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59771] Release the Kueue `Workload` of `SparkApplication` only after its pods are gone [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #885:
URL: https://github.com/apache/spark-kubernetes-operator/pull/885#issuecomment-5821654765
All tests passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org