dongjoon-hyun commented on code in PR #865:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/865#discussion_r4074944877
##########
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:
Thank you for the careful review, @peter-toth!
You are right, and I verified it: `BaseAppDriverObserver` sets
`DriverEvicted` inside the same `PodPhase.FAILED` branch as `Failed`, told
apart only by `status.reason`, so its driver pod is terminal and occupies no
capacity. Excluding it kept leaking exactly the quota this PR exists to release.
Addressed in c561d53 with the `FINISHED_KUEUE_STATES` set you suggested.
`success` stays `Succeeded == stateSummary`, so an evicted driver is finished
with the `Failed` reason. On the test side, `DriverEvicted` moved out of the
`cleanupWithRetainPolicyKeepsTheQuotaOfARetainedRunningDriver` loop into a new
`cleanupWithRetainPolicyFinishesTheWorkloadOfAnEvictedDriver`.
The other four exclusions stand: the three start timeouts can leave the
driver pod pending or running, and `SchedulingFailure` can leave live resources
behind, which is why the non-retain path recomputes and deletes them.
--
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]
