peter-toth commented on code in PR #895:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/895#discussion_r4119698614
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -236,15 +250,28 @@ private boolean isDriverRequested(SparkAppContext
context) {
if (context.getCurrentAttemptDriverPod().isPresent()) {
return true;
}
+ SparkApplication app = context.getResource();
+ boolean suspended = app.getSpec().isSuspend();
Optional<Workload> workload = context.getCachedKueueWorkload();
if (workload.isEmpty()
|| !KueueWorkloadUtils.isAdmitted(workload.get())
- || (KueueWorkloadUtils.findEviction(workload.get()).isEmpty()
+ || (!suspended
+ && KueueWorkloadUtils.findEviction(workload.get()).isEmpty()
&& !KueueWorkloadUtils.isDeactivated(workload.get()))) {
return false;
}
- Pod driverPod =
context.getClient().resource(context.getDriverPodSpec()).get();
- return driverPod != null && driverPod.getMetadata().getDeletionTimestamp()
== null;
+ Pod driverPodSpec;
+ try {
+ driverPodSpec = context.getDriverPodSpec();
+ } catch (Exception e) {
Review Comment:
**Finding 1.** This branch runs only after `getCurrentAttemptDriverPod()`
came back empty, and the whole reason this PR exists is that an empty cache
does not mean there is no driver. So "the spec could not be built" is answered
here with "no driver was requested", and `holdForSuspend` then deletes the
admitted `Workload` of a driver that is running.
Measured on `ea8b9997`, with the setup of
`suspendedAppWithDriverMissingFromCacheKeepsItsKueueWorkload` (admitted
`Workload`, a live driver pod carrying `driverLabels(app)` on the API server,
absent from the informer cache), varying only `getDriverPodSpec()`:
| `getDriverPodSpec()` | progress | `Workload` |
| --- | --- | --- |
| returns the spec (the PR's test) | `completeAndDefaultRequeue` | kept |
| throws `SparkException` | suspend hold, `PT30M` | **deleted** |
| throws `KubernetesClientException(503)` | suspend hold, `PT30M` |
**deleted** |
The reachable transient cause is row 2:
`KubernetesUtils.loadPodFromTemplate` downloads a remote
`spark.kubernetes.driver.podTemplateFile` and rethrows a failed download as
`SparkException("Could not load pod from template file.")`, so a flaky object
store is enough. Row 3 is the one that jars against the branch above - the
suspend handler catches `KubernetesClientException` and requeues precisely so
an unanswered question does not release the quota, and `catch (Exception e)`
intercepts that type first.
The javadoc's rationale ("rather than failing every reconcile while it keeps
the quota") holds only when no driver is running, and that is answerable
without the spec:
```java
} catch (Exception e) {
if (!suspended || hasLiveDriverPod(context, app)) {
throw e;
}
log.warn("Failed to build the driver spec of a suspended application,
holding it.", e);
return false;
}
```
with `hasLiveDriverPod` listing by the selector
`SparkAppContext.getDriverPodFromApi()` already uses:
```java
private boolean hasLiveDriverPod(SparkAppContext context, SparkApplication
app) {
return context.getClient().pods()
.inNamespace(app.getMetadata().getNamespace())
.withLabels(driverLabels(app))
.list()
.getItems()
.stream()
.anyMatch(p -> p.getMetadata().getDeletionTimestamp() == null);
}
```
I ran that: row 2 now propagates with the `Workload` kept and no event
published, row 3 reaches the suspend handler and requeues after `PT2M` with the
`Workload` kept, and every test in `AppInitStepTest` and `SparkAppContextTest`
still passes - including
`suspendedAppWithUnbuildableDriverSpecReleasesKueueWorkload`, which has no
driver pod and so still releases. The list runs only on the failure path.
The follow-up you described on
[r4118601516](https://github.com/apache/spark-kubernetes-operator/pull/895#discussion_r4118601516)
removes this failure mode too, by not building the spec for the lookup at all,
so closing it there instead is fine by me - the case is worth recording on the
ticket either way.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -223,9 +228,18 @@ private Optional<ReconcileProgress> holdForKueueAdmission(
* driver. See {@link SparkAppContext#getCurrentAttemptDriverPod()} for how
a pod left
* 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 but still admitted,
since that Workload
- * would be released or held under the driver, which is requested only after
the admission.
- * Otherwise, the driver spec is not built for the lookup.
+ * well if the Kueue Workload is still admitted and either the application
is suspended or the
+ * Workload is evicted or deactivated, since that Workload would be released
or held under the
+ * driver, which is requested only after the admission. Otherwise, the
driver spec is not built
+ * for the lookup. Like a cached pod, the one found there counts only if it
is a live driver pod,
+ * see {@link SparkAppContext#isLiveDriverPod}, so that a pod of another
application with the
+ * same name is not taken for the driver.
+ *
+ * <p>A driver spec which cannot be built for that lookup fails the
reconcile, except for a
+ * suspended application, which is held as it was before the lookup,
releasing the Workload
+ * rather than failing every reconcile while it keeps the quota. The spec
built for a cached
+ * driver, see {@link SparkAppContext#getCurrentAttemptDriverPod()}, always
fails the reconcile,
Review Comment:
**Finding 2.** "Fails the reconcile" covers two different outcomes, and the
new tests show both. A non-suspended application ends in `SchedulingFailure`,
written to the status by the `catch (Exception e)` at line 152
(`driverMissingFromCacheWithUnbuildableSpecFailsScheduling`). A suspended one
with a cached driver throws out of `reconcile` entirely, because the suspend
branch at line 78 catches only `KubernetesClientException` -
`suspendedAppWithCachedDriverAndUnbuildableSpecKeepsKueueWorkload` asserts
exactly that. The status then keeps saying `Submitted`, and the only record is
JOSDK's retry plus the one `ReconcileError` event from
`SparkAppReconciler.updateErrorStatus`. Worth naming which is which, since
`SchedulingFailure` is the outcome a reader would assume:
```java
* driver, see {@link SparkAppContext#getCurrentAttemptDriverPod()},
always fails the reconcile,
* since that driver may be running: a suspended application throws out of
the reconcile, which
* JOSDK retries, while any other one ends in SchedulingFailure.
```
--
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]
