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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
