dongjoon-hyun commented on code in PR #892:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/892#discussion_r4112139314
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -220,15 +221,28 @@ private Optional<ReconcileProgress> holdForKueueAdmission(
* the case where the driver was created but the status update to
DriverRequested failed, so that
* a suspended application still completes its initialization instead of
being held with a live
* driver. See {@link SparkAppContext#getCurrentAttemptDriverPod()} for how
a pod left
- * from a previous attempt is told apart.
+ * from a previous attempt is told apart. The informer cache may not have
seen a driver created
+ * in the previous reconcile yet, so a driver missing from it is looked up
on the API server as
+ * well if the Kueue Workload is evicted or deactivated, since that Workload
would be released or
+ * held under the driver. Otherwise, the driver spec is not built for the
lookup.
*
* @param context The SparkAppContext for the application.
* @return True if the driver pod of the current attempt exists, false
otherwise.
* @throws KubernetesClientException if the lookup fails, so that a running
driver is not
* mistaken for one that was never requested.
*/
private boolean isDriverRequested(SparkAppContext context) {
- return context.getCurrentAttemptDriverPod().isPresent();
+ if (context.getCurrentAttemptDriverPod().isPresent()) {
+ return true;
+ }
+ Optional<Workload> workload = context.getCachedKueueWorkload();
+ if (workload.isEmpty()
Review Comment:
Thank you, @peter-toth. You're right. A driver is requested only after the
admission, and Kueue keeps the admission of an evicted or deactivated
`Workload`, so the lookup is now gated on `isAdmitted`. I added
`neverAdmittedKueueWorkloadDoesNotLookUpTheDriver` for a `Workload` deactivated
while pending, which verifies that the driver spec is not built for the lookup.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/v1beta2/WorkloadStatus.java:
##########
@@ -85,4 +86,19 @@ public boolean isFinished() {
"Finished".equalsIgnoreCase(c.getType())
&& "True".equalsIgnoreCase(c.getStatus()));
}
+
+ /**
+ * Checks if Kueue evicted the workload. Kueue keeps the "Admitted"
condition of an evicted
+ * workload until the controller of its resources stops them and releases
the quota.
+ *
+ * @return true if an "Evicted" condition exists with status "True", false
otherwise.
+ */
+ public boolean isEvicted() {
Review Comment:
Agreed. I removed `WorkloadStatus.isEvicted()` and its `WorkloadTest`
assertions, since `findEviction` has to return the `Condition` for the event
message anyway. The Javadoc of `CONDITION_EVICTED` no longer links to it.
--
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]
