peter-toth commented on code in PR #850:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/850#discussion_r4052619912
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadUtilsTest.java:
##########
@@ -220,6 +225,124 @@ void releaseWorkloadIgnoresFailures() {
Assertions.assertDoesNotThrow(() ->
KueueWorkloadUtils.releaseWorkload(client, owner()));
}
+ @Test
+ void resolvePodSetFlavorsMergesFlavorsOfEachPodSet() {
+ Toleration spot = toleration("spot");
+ Toleration gpu = toleration("gpu");
+ createFlavor("cpu-flavor", Map.of("pool", "cpu", "zone", "a"),
List.of(spot));
+ createFlavor("gpu-flavor", Map.of("pool", "gpu", "accelerator", "a100"),
List.of(spot, gpu));
+ // Like Kueue, a flavor assigned to several resources is applied once
+ Map<String, String> driverFlavors = Map.of("cpu", "cpu-flavor", "memory",
"cpu-flavor");
+ // Like Kueue, a later flavor overwrites a node label, which is in the
resource name order
+ Map<String, String> executorFlavors =
+ Map.of("cpu", "cpu-flavor", "nvidia.com/gpu", "gpu-flavor");
Review Comment:
**Finding 1.** This `Map.of` is what feeds the ordering the assertion
checks, and `Map.of`'s iteration order is salted per JVM run, so the test only
catches a loss of the sort about half the time.
I removed the `new TreeMap<>(...)` from `resolvePodSetFlavors` (leaving `new
LinkedHashSet<>(assignment.getFlavors().values())`) and ran
`KueueWorkloadUtilsTest` eight times in fresh JVMs: it failed on 5 and **passed
on 3**. A probe printing `Map.of("cpu", ..., "nvidia.com/gpu", ...).keySet()`
over six runs gave `[cpu, nvidia.com/gpu]` four times and `[nvidia.com/gpu,
cpu]` twice, since `java.util.ImmutableCollections.SALT` is derived from
`System.nanoTime()` at class init.
To be clear, this is not a flaky test. The real code sorts, so CI stays
green either way. It is the regression-catching power that is a coin flip, and
the sort is the PR's one deliberate divergence from Kueue, whose
`FromAssignment` ranges over the map and so has no defined order at all.
A `LinkedHashMap` in the reverse of resource-name order pins it. Without the
sort the iteration is then gpu-before-cpu, `pool` comes out `cpu`, and the
assertion fails on every run:
```java
Map<String, String> executorFlavors = new LinkedHashMap<>();
executorFlavors.put("nvidia.com/gpu", "gpu-flavor");
executorFlavors.put("cpu", "cpu-flavor");
```
`driverFlavors` on line 235 can stay a `Map.of`, since both of its values
are the same flavor.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadUtils.java:
##########
@@ -127,6 +137,97 @@ public static AdmissionResult requestAdmission(
return AdmissionResult.PENDING;
}
+ /**
+ * Resolves the node selector and tolerations of the ResourceFlavors which
Kueue assigned to each
+ * pod set of the admitted Workload, in the same way as Kueue built-in
integrations. The flavors
Review Comment:
**Finding 2.** "in the same way as Kueue built-in integrations" does not
hold for Topology Aware Scheduling, and TAS is on by default in the version you
ported from.
`podset.FromAssignment` does two more things, before the flavor loop this PR
mirrors:
```go
if features.Enabled(features.TopologyAwareScheduling) &&
assignment.TopologyAssignment != nil {
if podSet.TopologyRequest == nil || (...) {
info.Annotations[kueue.PodSetUnconstrainedTopologyAnnotation] = "true"
}
info.SchedulingGates = append(info.SchedulingGates,
corev1.PodSchedulingGate{
Name: kueue.TopologySchedulingGate,
})
}
```
`pkg/features/kube_features.go` in v0.19.4 lists `TopologyAwareScheduling:
{{0.9, false, Alpha}, {0.14, true, Beta}}`, so it is enabled unless an admin
turns it off, and it applies to any `ResourceFlavor` with `spec.topologyName`.
Without the scheduling gate the pods go straight to kube-scheduler, which
ignores the topology domains Kueue assigned. That is wrong placement with no
error anywhere. The operator also cannot notice today: the curated model has no
`topologyName` on `ResourceFlavorSpec` and no `topologyAssignment` on
`PodSetAssignment`, so both are invisible here.
Nothing is broken yet, since nothing calls this. Two asks:
- Now: scope the claim, e.g. "resolves the node labels and tolerations like
Kueue's `podset.FromAssignment`. The TAS scheduling gate and annotation are not
handled."
- Before this gets wired up: fail loudly rather than mis-place. Adding
`topologyName` to `ResourceFlavorSpec` and throwing
`UnsupportedOperationException` when a resolved flavor has it matches how
`KueueWorkloadFactory.buildWorkload` already rejects dynamic allocation, HPA
and pod template files, and the init steps already turn that into
`SchedulingFailure`.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueuePodSetFlavor.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.spark.k8s.operator.kueue;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+
+import io.fabric8.kubernetes.api.model.PodSpec;
+import io.fabric8.kubernetes.api.model.Toleration;
+
+/**
+ * The node selector and tolerations of the ResourceFlavors which Kueue
assigned to a pod set.
+ * Like Kueue built-in integrations, they are applied to the pods of the pod
set.
+ *
+ * @param nodeSelector The merged `nodeLabels` of the ResourceFlavors.
+ * @param tolerations The merged `tolerations` of the ResourceFlavors.
+ */
+public record KueuePodSetFlavor(Map<String, String> nodeSelector,
List<Toleration> tolerations) {
+
+ private static final String OPERATOR_EQUAL = "Equal";
+
+ /**
+ * Adds the node selector and tolerations to the given pod spec. A node
selector conflict must be
+ * checked beforehand, see {@link KueueWorkloadUtils#resolvePodSetFlavors}.
+ *
+ * @param podSpec The pod spec to be modified in place.
+ */
+ public void applyTo(final PodSpec podSpec) {
Review Comment:
**Finding 3.** The precondition the javadoc states is checked against a
different object than the one `applyTo` mutates, so a caller that gets it wrong
silently overwrites the user's node selector instead of failing.
`resolvePodSetFlavors` passes `podSetNodeSelector(desired, ...)` to
`checkNoNodeSelectorConflict`, i.e. the node selector of the **Workload's** pod
set template. `applyTo` then merges into whatever `PodSpec` it is handed, and
`mergedNodeSelector.putAll(nodeSelector)` on line 53 lets the flavor win
unconditionally. Kueue never has that split: `podset.Merge` runs
`utilmaps.HaveConflict` on the very `spec.NodeSelector` it is about to
overwrite, and returns an error.
For a `SparkApplication` the two are produced by independent code. The
Workload's driver pod set template gets its node selector from
`KueueWorkloadFactory.decorateNodeSelector`
(`spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadFactory.java:526`),
a reimplementation of Spark's rules, while the real driver pod spec comes from
`SparkAppSubmissionWorker` via Spark's own `BasicDriverFeatureStep`. They agree
today. The check would not notice if they ever stopped agreeing.
This record already hosts the toleration half of Kueue's merge
(`addTolerations`, `isSameToleration`), so the node-selector half fits beside
them. Move `checkNoNodeSelectorConflict` here and call it from both places:
```java
public void applyTo(final PodSpec podSpec) {
Map<String, String> mergedNodeSelector = new HashMap<>();
if (podSpec.getNodeSelector() != null) {
checkNoNodeSelectorConflict(podSpec.getNodeSelector(), nodeSelector);
mergedNodeSelector.putAll(podSpec.getNodeSelector());
}
mergedNodeSelector.putAll(nodeSelector);
```
The pod set name only feeds the message, so `resolvePodSetFlavors` can keep
passing it and the `applyTo` path can use a name-free variant. Keeping the
early check in `resolvePodSetFlavors` is still worth it, so a conflict is
reported before anything is created.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadUtils.java:
##########
@@ -56,6 +64,8 @@ public final class KueueWorkloadUtils {
*/
public static final Duration STALE_WORKLOAD_REQUEUE_INTERVAL =
Duration.ofSeconds(5);
+ private static final int HTTP_NOT_FOUND = 404;
Review Comment:
**Finding 4.** The module already takes this from the JDK:
`ReconcilerUtils.java:27` and `ProbeService.java:22` both do `import static
java.net.HttpURLConnection.HTTP_NOT_FOUND`. `Constants.HTTP_TOO_MANY_REQUESTS`
exists only because the JDK has no constant for 429, so a new one for 404 is
the odd case out.
```java
import static java.net.HttpURLConnection.HTTP_NOT_FOUND;
```
and drop the field.
--
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]
