peter-toth commented on code in PR #864:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/864#discussion_r4073726749
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadUtils.java:
##########
@@ -218,13 +229,96 @@ public static Optional<ReconcileProgress>
holdForAdmission(
requested);
return Optional.of(ReconcileProgress.completeAndDefaultRequeue());
}
+ Map<String, KueuePodSetFlavor> flavors;
+ try {
+ flavors = resolvePodSetFlavors(context.getClient(),
admission.workload());
+ checkNoNodeSelectorConflict(flavors, desired);
+ } catch (KubernetesClientException e) {
+ return Optional.of(retryAfterFlavorReadFailure(context, e,
workloadName));
+ } catch (IllegalArgumentException e) {
+ // Like Kueue, a node selector conflict is permanent, so the quota is
released before the
+ // caller fails the resource, which this step does not reach again. The
release itself is
+ // retried, since a Workload left behind holds the quota of a resource
that never starts.
+ try {
+ deleteWorkloadOf(context.getClient(), context.getResource());
+ } catch (KubernetesClientException releaseFailure) {
+ return Optional.of(
+ retryAfterRequestFailure(
+ context,
+ releaseFailure,
+ "Failed to release the Kueue Workload of a rejected
admission"));
+ }
+ throw e;
+ }
EventUtils.normal(
context.getEventRecorder(),
EventUtils.REASON_KUEUE_ADMITTED,
"Kueue admitted Workload " + workloadName + ", requesting " +
requested + ".");
+ context.setKueuePodSetFlavors(flavors);
+ return Optional.empty();
+ }
+
+ /**
+ * Sets the flavors of the Workload which Kueue admitted before on the
context of a resource
+ * whose driver or master exists already. Such a reconcile applies the
secondary resources again
+ * without requesting the admission, so rebuilding them without the flavors
would remove the node
+ * selector and the tolerations which Kueue assigned to the pods.
+ *
+ * @param context The context of the resource to be reconciled.
+ * @return The progress to return while the flavors cannot be read, or empty
to proceed.
+ */
+ public static Optional<ReconcileProgress> applyAdmittedFlavors(final
BaseContext<?> context) {
+ HasMetadata resource = context.getResource();
+ String workloadName = KueueWorkloadFactory.getWorkloadName(resource);
+ Workload workload;
+ try {
+ workload =
+ context
+ .getClient()
+ .resources(Workload.class)
+ .inNamespace(resource.getMetadata().getNamespace())
+ .withName(workloadName)
+ .get();
+ } catch (KubernetesClientException e) {
+ return Optional.of(retryAfterRequestFailure(context, e, "Failed to read
the Kueue Workload"));
+ }
+ if (workload == null || !isAdmitted(workload)) {
+ // A Workload which is gone or evicted must not hold the resources which
are already running.
+ log.debug("The Kueue Workload {} is not admitted, applying no flavors.",
workloadName);
+ return Optional.empty();
+ }
+ try {
+ context.setKueuePodSetFlavors(resolvePodSetFlavors(context.getClient(),
workload));
Review Comment:
**Finding 1.** This path applies the flavors without
`checkNoNodeSelectorConflict`, which `holdForAdmission` runs two screens up.
`KueuePodSetFlavor.applyTo` states that as a precondition ("A node selector
conflict must be checked beforehand"), and `applyTo` merges with
`mergedNodeSelector.putAll(nodeSelector)`, so the flavor's label wins over the
pod's silently.
Both arms, on a `SparkCluster` whose master pod template asks for `pool:
userpool` and whose flavor carries `pool: cpu`:
```
PROBE-BEFORE >>> {pool=userpool}
PROBE-ARM-A-APPLY >>> {pool=cpu} // setKueuePodSetFlavors, as this
line does
PROBE-ARM-B-CHECK >>> IllegalArgumentException //
checkNoNodeSelectorConflict, same inputs
```
Arm B is what fails the resource with `SchedulingFailure` and releases the
quota. Arm A overwrites the user's placement with no event and no failure, and
for a cluster it lands: the `StatefulSet`s are server-side-applied on that
reconcile, so the worker pods move.
It also makes one sentence in `docs/spark_custom_resources.md` an
overstatement — "A node label that conflicts with the node selector of the pods
fails the resource with `SchedulingFailure` and deletes the `Workload`" holds
only for the admission path.
The stored `workload` already carries the pod set templates, so the check is
available here:
```java
try {
Map<String, KueuePodSetFlavor> flavors =
resolvePodSetFlavors(context.getClient(), workload);
checkNoNodeSelectorConflict(flavors, workload);
context.setKueuePodSetFlavors(flavors);
} catch (KubernetesClientException e) {
return Optional.of(retryAfterFlavorReadFailure(context, e,
workloadName));
}
```
and the `IllegalArgumentException` then propagates to the same `catch
(Exception)` in both init steps that the admission path relies on.
Where that stops: the stored `Workload`'s templates hold the selectors as of
admission, so it catches a `ResourceFlavor` edited afterwards but not a
resource spec edited afterwards. That second case is a wider gap than this line
— once the master exists the step skips `requestAdmission` entirely, so the
pod-sets hash is never re-validated either. I am not asking for that here, only
that the two flavor-applying paths agree.
Worth a test alongside it: the same shape as
`applyAdmittedFlavorsResolvesTheFlavorsOfTheAdmittedWorkload`, with a pod set
template carrying a conflicting `nodeSelector`, asserting
`IllegalArgumentException` and `verify(context,
never()).setKueuePodSetFlavors(any())`.
--
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]
