peter-toth commented on code in PR #883:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/883#discussion_r4097217357
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadUtils.java:
##########
@@ -385,18 +385,25 @@ public static ReconcileProgress retryAfterRequestFailure(
* the admission. The resource now starts without Kueue, so Kueue would
otherwise admit the
* Workload later into quota which nothing uses. An admitted Workload is
kept, since the
* resources it was admitted for may be running already, and it is released
with them like the
- * Workload of a queued resource. Like the admission request, a failed
release is retried before
- * the resources are requested.
+ * Workload of a queued resource. Its flavors are applied again like {@link
+ * #applyAdmittedFlavors}, since the secondary resources are applied again
in this reconcile.
+ * Like the admission request, a failed release is retried before the
resources are requested.
*
* @param context The context of the resource without a queue name label.
- * @return The progress to return while the release fails, or empty to
proceed.
+ * @return The progress to return while the release or the read of the
flavors fails, or empty to
+ * proceed.
+ * @throws IllegalArgumentException if a node label of the flavors of an
admitted Workload
+ * conflicts with the node selector of a pod set, like {@link
#applyAdmittedFlavors}.
*/
public static Optional<ReconcileProgress> releaseDequeuedWorkload(
final BaseContext<?> context) {
Optional<Workload> workload = context.getCachedKueueWorkload();
- if (workload.isEmpty() || isAdmitted(workload.get())) {
+ if (workload.isEmpty()) {
return Optional.empty();
}
+ if (isAdmitted(workload.get())) {
+ return applyAdmittedFlavors(context);
Review Comment:
**Finding 1.** This call drops the precondition the callee documents.
`applyAdmittedFlavors` opens with "Sets the flavors of the Workload which Kueue
admitted before on the context of a resource whose driver or master exists
already" (`KueueWorkloadUtils.java:289-292`). On the labeled path
`isDriverRequested` / `isMasterRequested` enforces it. Here nothing does. So
the flavors reach a resource whose driver or master was never created, and so
do the hold and the throw that reading them can produce.
Measured in `AppInitStepTest`. Both rows are the same probe pair, with the
control being the identical tests and only `KueueWorkloadUtils.java` reverted
to `5c2ccf6a`. The app is unlabeled, its `Workload` is admitted, and no driver
pod exists.
| trigger | base `5c2ccf6a` | this PR |
|---|---|---|
| the flavor of the admitted `Workload` was deleted or renamed | driver
created | no driver, `PT2M` requeue, repeats for as long as the flavor is
missing |
| a flavor node label conflicts with the pod node selector | driver created,
`DriverRequested` | no driver, `SchedulingFailure`, `Workload` kept |
Both triggers are the documented ones. `retryAfterFlavorReadFailure` says so
itself: "a flavor which an admitted Workload references is also missing when it
was deleted or renamed". A 403 on the cluster-scoped `ResourceFlavor`s lands in
the same branch, which is the missing `operatorRbac.clusterRole.create` the
docs already warn about. Kueue admits a `Workload` whether or not the operator
can read the flavors, so "admitted, but never started because the flavors
cannot be read" is reachable. Removing the queue label was the way out of it.
It no longer is.
The second row is worse for a `SparkCluster` than for an application.
`SchedulingFailure` is terminal there, since `SparkClusterReconciler:195`
routes it to `ClusterUnknownStateStep`. And unlike the labeled path,
`applyAdmittedFlavors` deliberately does not release the `Workload` on a
conflict, because it assumes the pods are running. On this path they are not. I
measured `workloadKept=true` with `driverPodCreated=false`, so the quota is
held with nothing using it.
The PR's own tests advertise the gate that is missing. In
`kueueFlavorsAreAppliedAgainWhenTheDriverExists(true)` the
`when(mockContext.getCurrentAttemptDriverPod()).thenReturn(Optional.of(driverPodSpec))`
stub is inert, as is `when(mockClient.resource(masterStatefulSetSpec).get())`
in the cluster arm. I ran the same scenario with no driver pod and
`getCurrentAttemptDriverPod()` left unstubbed, and `setKueuePodSetFlavors` was
still called with the flavor.
The fix is to let the caller answer the precondition. A supplier keeps the
"a resource which was never queued costs no request" property, because it only
runs once there is an admitted cached `Workload`.
```java
public static Optional<ReconcileProgress> releaseDequeuedWorkload(
final BaseContext<?> context, final BooleanSupplier requested) {
Optional<Workload> workload = context.getCachedKueueWorkload();
if (workload.isEmpty()) {
return Optional.empty();
}
if (isAdmitted(workload.get())) {
// Only resources which are applied again would lose the flavors. A
driver or master which
// was never created starts without Kueue, since the label is gone, so
a flavor which cannot
// be read must not hold it back.
return requested.getAsBoolean() ? applyAdmittedFlavors(context) :
Optional.empty();
}
```
In both init steps the call moves inside the existing `try`, so the lookup's
`KubernetesClientException` is reported exactly as it is today.
```java
boolean labeled = KueueWorkloadFactory.hasQueueName(app);
if (labeled && !KUEUE_ENABLED.getValue()) {
KueueWorkloadUtils.warnQueueNameIgnored(context);
return Optional.empty();
}
try {
if (!labeled) {
return KueueWorkloadUtils.releaseDequeuedWorkload(
context, () -> isDriverRequested(context));
}
if (isDriverRequested(context)) {
```
I applied that, plus `() -> true` at the four `KueueWorkloadUtilsTest` call
sites, and ran it. Both probes go back to creating the driver.
`:spark-operator:test`, `checkstyleMain`, `pmdMain`, `spotbugsMain` and
`spotlessCheck` are all green, including both `queueLabelRemoved=true` arms. A
`false` arm on `admittedWorkloadOfDequeuedResourceIsKept` would pin the new
branch.
If you would rather keep the call unconditional, two things need to follow
it. `applyAdmittedFlavors` has to stop claiming the driver-or-master
precondition in its javadoc. And `docs/spark_custom_resources.md` should say
that an unlabeled resource is held too until the flavors can be read, since
that bullet currently reads as being about queued resources only.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadUtils.java:
##########
@@ -385,18 +385,25 @@ public static ReconcileProgress retryAfterRequestFailure(
* the admission. The resource now starts without Kueue, so Kueue would
otherwise admit the
* Workload later into quota which nothing uses. An admitted Workload is
kept, since the
* resources it was admitted for may be running already, and it is released
with them like the
- * Workload of a queued resource. Like the admission request, a failed
release is retried before
- * the resources are requested.
+ * Workload of a queued resource. Its flavors are applied again like {@link
+ * #applyAdmittedFlavors}, since the secondary resources are applied again
in this reconcile.
+ * Like the admission request, a failed release is retried before the
resources are requested.
*
* @param context The context of the resource without a queue name label.
- * @return The progress to return while the release fails, or empty to
proceed.
+ * @return The progress to return while the release or the read of the
flavors fails, or empty to
+ * proceed.
+ * @throws IllegalArgumentException if a node label of the flavors of an
admitted Workload
+ * conflicts with the node selector of a pod set, like {@link
#applyAdmittedFlavors}.
*/
public static Optional<ReconcileProgress> releaseDequeuedWorkload(
Review Comment:
**Finding 2.** The name now covers one of two branches. The admitted branch
this PR adds releases nothing, so `releaseDequeuedWorkload` reads as a release
at both call sites, `AppInitStep:191` and `ClusterInitStep:228`. The javadoc
explains both branches, but the call site is where a reader meets the name
first. `handleDequeuedWorkload` or `reconcileDequeuedWorkload` would cover the
pair.
--
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]
