peter-toth commented on code in PR #892:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/892#discussion_r4111659418
##########
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:
**Finding 1.** This guard admits a `Workload` that was never admitted, and
on that path the fallback's answer makes the resource start rather than wait.
A driver can only exist on this path after `holdForAdmission` returned
`ADMITTED`, so `isAdmitted` is a necessary condition for the question the
fallback is asking. It is not checked. And `Evicted=True` with no `Admitted` is
a state Kueue produces routinely: the `!IsActive` branch of
`workload_controller.go` calls `workloadevict.Evict` unconditionally, so a
`Workload` deactivated while still pending gets `Evicted=True` with reason
`Deactivated` and no admission. That is the same state your own `holdsQuota`
guard in `requestAdmission` was written to recognise.
Measured in `AppInitStepTest`, with a labeled application whose `Workload`
has `active: false` and `Evicted=True/Deactivated` but no `Admitted` condition,
a pod carrying the driver name present and not terminating, and
`getCurrentAttemptDriverPod()` returning empty:
| arm | probe result | `driverMissingFromCacheKeepsItsEvictedKueueWorkload` |
|---|---|---|
| `AppInitStep.java` at base `f2720a4c` | `admitted=false`, state
`Submitted` | FAILS, so your test is effective |
| this PR | `admitted=false`, state **`DriverRequested`** | passes |
| this PR + the gate below | `admitted=false`, state `Submitted` | passes |
So the application reports `DriverRequested` while its `Workload` is
deactivated and Kueue counts nothing for it, which is the state the PR exists
to prevent. Base holds, so this arrives with the PR.
The reachability is the narrow part, and I want to be straight about it. The
fallback keeps only the pod's name and `deletionTimestamp`; it drops the
driver-label filter and the cache-candidate gate that
`SparkAppContext.getCurrentAttemptDriverPod()` documents as how a previous
attempt's pod is told apart ("The informer cache is only used to find a
candidate ... the candidate is verified against the API server"). So the pod it
finds is the current attempt's driver only by assumption. Reusing the driver
pod name across attempts, which that javadoc names as the case it guards, plus
a retained pod, is the concrete way to get a foreign pod under that name.
Gating on the admission closes it without touching the case you added:
```java
Optional<Workload> workload = context.getCachedKueueWorkload();
if (workload.isEmpty()
|| !KueueWorkloadUtils.isAdmitted(workload.get())
|| KueueWorkloadUtils.findEviction(workload.get()).isEmpty()
&& !KueueWorkloadUtils.isDeactivated(workload.get())) {
return false;
}
```
`driverMissingFromCacheKeepsItsEvictedKueueWorkload` still passes with it,
since that `Workload` is admitted, and `spark-operator`'s Kueue and
reconcile-step suites plus `checkstyleMain`, `pmdMain` and `spotlessCheck` are
green.
It buys a second thing. A `Workload` deactivated while pending is the common
shape of `kueuectl stop workload` on a queued resource, and that hold can last
hours. Without the gate, every reconcile in it runs
`context.getDriverPodSpec()`, which builds the whole driver submission spec,
plus one API `GET`, to answer a question whose answer cannot be yes. Your
javadoc already flags that cost ("Otherwise, the driver spec is not built for
the lookup"); the admission gate is what makes the scoping match it.
##########
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:
**Finding 2.** Nothing in `src/main` calls this, and it is a second copy of
a predicate the PR also writes in `KueueWorkloadUtils`.
`grep -rn "isEvicted" spark-operator/src` returns only
`WorkloadTest.java:143,161,164,167` plus the declaration here and the javadoc
reference at `KueueWorkloadUtils.java:83`. Every production read of the
eviction goes through `KueueWorkloadUtils.findEviction`, which matches
`CONDITION_EVICTED` with its own stream rather than calling this.
That inverts the convention the file already sets. `isAdmitted`,
`isQuotaReserved` and `isFinished` live on `WorkloadStatus`, and
`KueueWorkloadUtils.isAdmitted(Workload)` is a thin wrapper over
`status.isAdmitted()`. The `CONDITION_FINISHED` constant is not a
counterexample: it exists because `addFinishedCondition` *writes* that
condition, while the operator only ever reads `Evicted`.
The javadoc at `KueueWorkloadUtils.java:83` makes the split visible: "Type
of the Kueue condition of an evicted Workload, as {@link
WorkloadStatus#isEvicted}" points the reader at the method that is not the one
doing the work.
Two ways out, either is fine. Drop `isEvicted()` and its four `WorkloadTest`
assertions, and drop the `{@link}` from the constant's javadoc, since
`findEviction` needs the `Condition` for the event message anyway. Or move the
lookup down here, which keeps the predicate in one place:
```java
public Optional<Condition> findEviction() {
return conditions == null
? Optional.empty()
: conditions.stream()
.filter(
c ->
"Evicted".equalsIgnoreCase(c.getType())
&& "True".equalsIgnoreCase(c.getStatus()))
.findFirst();
}
public boolean isEvicted() {
return findEviction().isPresent();
}
```
with `KueueWorkloadUtils.findEviction(workload)` delegating to it for a null
status, the way `isAdmitted` already does.
--
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]
