dongjoon-hyun commented on code in PR #864:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/864#discussion_r4073869969
##########
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:
Thank you for catching this, and for the two-arm probe. Fixed in the latest
commit.
`applyAdmittedFlavors` now runs the check against the stored `Workload` as
you suggested, so both paths agree and the `IllegalArgumentException` lands in
the same `catch (Exception)` of the init steps:
```java
Map<String, KueuePodSetFlavor> flavors =
resolvePodSetFlavors(context.getClient(), workload);
checkNoNodeSelectorConflict(flavors, workload);
context.setKueuePodSetFlavors(flavors);
```
One deliberate difference from the admission path: the quota is not released
here. There the resource never started, so holding the quota would be wrong.
Here the driver or the master is running already and its pods still occupy what
the `Workload` reserved, so deleting it would make Kueue under-account the
usage this change exists to keep honest. The comment on the call states that,
and the new test asserts the `Workload` survives.
Added
`KueueWorkloadUtilsTest.admittedFlavorsConflictingWithTheNodeSelectorFail` in
the shape you described: an admitted `Workload` whose `executor` pod set
template asks for `pool: on-demand` against a flavor carrying `pool: spot`,
asserting the exception and `verify(context,
never()).setKueuePodSetFlavors(any())`.
Also fixed the sentence you flagged in `docs/spark_custom_resources.md`:
`SchedulingFailure` now covers both paths, while the `Workload` deletion is
qualified with the running driver or master.
Agreed on the boundary you drew. A resource spec edited after the admission
is not covered, since that pass skips `requestAdmission` and never re-validates
the pod sets hash either. That is the wider gap and belongs in its own change.
--
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]
