peter-toth commented on code in PR #865:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/865#discussion_r4074857259
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppCleanUpStep.java:
##########
@@ -202,6 +203,40 @@ public ReconcileProgress reconcile(
}
}
+ /**
+ * Records the Kueue `Finished` condition on the Workload of an application
which terminated
+ * without releasing its resources. The Workload is retained with them, so
Kueue releases its
+ * quota on that condition instead of the deletion which the other paths
rely on. The
+ * application does not restart on this path, so no later attempt would find
the finished
+ * Workload.
+ *
+ * <p>Only an application whose driver reached a terminal phase is finished.
The other stopping
+ * states, such as the start timeouts and an eviction, leave a retained
driver running, and it
+ * would keep occupying the capacity which Kueue reclaims on the condition.
+ *
+ * @param context The SparkAppContext for the application.
+ * @param currentState The state which the application terminated with.
+ * @param terminationState The state which the application is updated to.
+ */
+ private void finishKueueWorkloadOfRetainedApp(
+ final SparkAppContext context,
+ final ApplicationState currentState,
+ final ApplicationState terminationState) {
+ ApplicationStateSummary stateSummary =
currentState.getCurrentStateSummary();
+ if (ApplicationStateSummary.Succeeded != stateSummary
Review Comment:
**Finding 1.** The justification for the exclusion list holds for four of
the five states it covers, but not for `DriverEvicted`.
`BaseAppDriverObserver.java:91-95` is where both come from:
```java
if (PodPhase.FAILED == PodPhase.getPhase(driverPod)) {
ApplicationState state = new ApplicationState(Failed,
DRIVER_FAILED_MESSAGE);
if ("Evicted".equalsIgnoreCase(status.getReason())) {
state = new ApplicationState(ApplicationStateSummary.DriverEvicted,
DRIVER_FAILED_MESSAGE);
}
```
So `DriverEvicted` *is* a `Failed` driver pod, distinguished only by
`status.reason`. The pod is in the terminal `Failed` phase and occupies no
capacity, exactly like the `Failed` state this guard does finish. "The other
stopping states leave a retained driver running, and it would keep occupying
the capacity which Kueue reclaims on the condition" is true for the three
timeouts, where the pod is still pending or running, and true for
`SchedulingFailure`, where a failure after the driver pod was created leaves it
live. It is not true for `DriverEvicted`.
The consequence is the leak this PR exists to close, on one sibling state.
`AppCleanUpStepTest.cleanupWithRetainPolicyKeepsTheQuotaOfARetainedRunningDriver`
pins it with `kueue.verifyNoInteractions()`, so for an evicted driver the
`Workload` is neither finished nor deleted, and by this method's own javadoc
the retain duration that would eventually release it is disabled by default.
Adding it to the guard is the whole fix, and it wants a named set rather
than a third `!=`:
```java
private static final Set<ApplicationStateSummary> FINISHED_KUEUE_STATES =
Set.of(
ApplicationStateSummary.Succeeded,
ApplicationStateSummary.Failed,
// An evicted driver is a Failed driver pod, see
BaseAppDriverObserver, so it occupies no
// capacity either. The timeouts and SchedulingFailure can leave a
live driver behind.
ApplicationStateSummary.DriverEvicted);
```
```java
if (!FINISHED_KUEUE_STATES.contains(stateSummary)) {
return;
}
```
`finishWorkload(..., success, ...)` then gets `false` for it, which is
right: Kueue's `Failed` reason is what an evicted driver is. The test change is
moving `DriverEvicted` out of that loop's list and into
`cleanupWithRetainPolicyFinishesKueueWorkloadAsSucceeded`'s sibling for the
failed reason.
--
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]
