dongjoon-hyun commented on code in PR #850:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/850#discussion_r4055233410
##########
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:
Thank you for catching this, @peter-toth. `executorFlavors` is now a
`LinkedHashMap` in the reverse resource-name order, as you suggested. I
verified it the same way you did: with the `TreeMap` sort removed from
`resolvePodSetFlavors`, the test now fails on every run (3 of 3 in fresh JVMs)
instead of the coin flip. `driverFlavors` stays 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:
##########
@@ -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:
Thank you, fixed. It now uses `import static
java.net.HttpURLConnection.HTTP_NOT_FOUND` like `ReconcilerUtils` and
`ProbeService`, and the private field is gone.
##########
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:
Thank you for the detailed pointer to `podset.FromAssignment`. You are right
that Topology Aware Scheduling is not covered here. I would like to handle it
in the follow-up which wires these flavors into the pods, so that the
`topologyName` field on `ResourceFlavorSpec`, the loud rejection, and the
javadoc wording land together in one consistent change rather than a partial
one here.
##########
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:
Good point, the conflict check and the mutation do look at different objects
today. Since `applyTo` has no production caller yet, I would like to address
this in the same follow-up that wires it into the driver/executor pod specs,
where the real `PodSpec` shows up and the check can be validated end to end.
--
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]
