peter-toth commented on code in PR #823: URL: https://github.com/apache/spark-kubernetes-operator/pull/823#discussion_r4008502984
########## spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadFactory.java: ########## @@ -0,0 +1,462 @@ +/* + * 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.Locale; +import java.util.Map; + +import io.fabric8.kubernetes.api.model.Container; +import io.fabric8.kubernetes.api.model.ContainerBuilder; +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.fabric8.kubernetes.api.model.OwnerReference; +import io.fabric8.kubernetes.api.model.PodSpec; +import io.fabric8.kubernetes.api.model.PodSpecBuilder; +import io.fabric8.kubernetes.api.model.PodTemplateSpec; +import io.fabric8.kubernetes.api.model.PodTemplateSpecBuilder; +import io.fabric8.kubernetes.api.model.Quantity; +import io.fabric8.kubernetes.api.model.ResourceRequirements; +import io.fabric8.kubernetes.api.model.ResourceRequirementsBuilder; +import io.fabric8.kubernetes.api.model.apps.StatefulSet; +import lombok.extern.slf4j.Slf4j; + +import org.apache.spark.k8s.operator.Constants; +import org.apache.spark.k8s.operator.SparkApplication; +import org.apache.spark.k8s.operator.SparkCluster; +import org.apache.spark.k8s.operator.SparkClusterResourceSpec; +import org.apache.spark.k8s.operator.SparkClusterSubmissionWorker; +import org.apache.spark.k8s.operator.kueue.v1beta1.PodSet; +import org.apache.spark.k8s.operator.kueue.v1beta1.Workload; +import org.apache.spark.k8s.operator.kueue.v1beta1.WorkloadSpec; +import org.apache.spark.k8s.operator.reconciler.SparkClusterResourceSpecFactory; +import org.apache.spark.k8s.operator.spec.ApplicationSpec; +import org.apache.spark.k8s.operator.spec.BaseApplicationTemplateSpec; +import org.apache.spark.k8s.operator.utils.ModelUtils; +import org.apache.spark.k8s.operator.utils.ReconcilerUtils; +import org.apache.spark.k8s.operator.utils.StringUtils; +import org.apache.spark.network.util.JavaUtils; + +/** + * Factory for creating Kueue Workload resources from Spark custom resources. + * This factory supports both {@link SparkApplication} (driver and executor pod sets) + * and {@link SparkCluster} (master and worker pod sets). + */ +@Slf4j +@SuppressWarnings("PMD.GodClass") +public final class KueueWorkloadFactory { + + public static final String PODSET_DRIVER = "driver"; + public static final String PODSET_EXECUTOR = "executor"; + public static final String PODSET_MASTER = "master"; + public static final String PODSET_WORKER = "worker"; + + private static final String DEFAULT_CORES = "1"; + private static final String DEFAULT_MEMORY = "1g"; + private static final String DEFAULT_MIN_MEMORY_OVERHEAD = "384m"; + private static final double DEFAULT_MEMORY_OVERHEAD_FACTOR = 0.10; + private static final double NON_JVM_MEMORY_OVERHEAD_FACTOR = 0.40; + private static final int DEFAULT_EXECUTOR_INSTANCES = 2; + + private static final String NODE_SELECTOR_PREFIX = "spark.kubernetes.node.selector."; + + private KueueWorkloadFactory() {} + + /** + * Builds a Kueue Workload from a SparkApplication resource. + * + * @param app The SparkApplication. + * @return The constructed Kueue Workload. + */ + public static Workload buildWorkload(final SparkApplication app) { + ApplicationSpec spec = app.getSpec(); + Map<String, String> sparkConf = spec.getSparkConf(); + if ("true".equalsIgnoreCase(sparkConf.get("spark.dynamicAllocation.enabled"))) { + throw new UnsupportedOperationException( + "Kueue does not support SparkApplication with dynamic allocation " + + "(spark.dynamicAllocation.enabled=true) yet."); + } + // Like Spark's KubernetesClusterManager, `local[*]` runs the driver only without executors. + boolean driverOnly = + sparkConf.getOrDefault("spark.kubernetes.driver.master", "").startsWith("local"); + checkNoPodTemplateFile( + sparkConf, Constants.DRIVER_SPARK_TEMPLATE_FILE_PROP_KEY, spec.getDriverSpec()); + if (!driverOnly) { + checkNoPodTemplateFile( + sparkConf, Constants.EXECUTOR_SPARK_TEMPLATE_FILE_PROP_KEY, spec.getExecutorSpec()); + } + List<PodSet> podSets = + driverOnly + ? List.of(buildDriverPodSet(app, sparkConf)) + : List.of(buildDriverPodSet(app, sparkConf), buildExecutorPodSet(app, sparkConf)); + return buildWorkload(app, Constants.LABEL_SPARK_APPLICATION_NAME, spec.isSuspend(), podSets); + } + + /** + * Builds a Kueue Workload from a SparkCluster resource. + * + * @param cluster The SparkCluster. + * @return The constructed Kueue Workload. + */ + public static Workload buildWorkload(final SparkCluster cluster) { + // Use the same StatefulSets which the operator creates for the cluster. + SparkClusterResourceSpec resourceSpec = + SparkClusterResourceSpecFactory.buildResourceSpec( + cluster, new SparkClusterSubmissionWorker()); + if (resourceSpec.getHorizontalPodAutoscaler().isPresent()) { + throw new UnsupportedOperationException( + "Kueue does not support SparkCluster with HorizontalPodAutoscaler " + + "(minWorkers < maxWorkers) yet."); + } + List<PodSet> podSets = + List.of( + buildPodSet(PODSET_MASTER, resourceSpec.getMasterStatefulSet()), + buildPodSet(PODSET_WORKER, resourceSpec.getWorkerStatefulSet())); + return buildWorkload( + cluster, Constants.LABEL_SPARK_CLUSTER_NAME, cluster.getSpec().isSuspend(), podSets); + } + + /** + * A pod template file is fetched by Spark, not by the operator, so its node selectors, + * tolerations and extra containers cannot be reflected in the PodSet template. The file is + * ignored by Spark if the pod template is set in the SparkApplication spec. + */ + private static void checkNoPodTemplateFile( + final Map<String, String> sparkConf, + final String templateFileKey, + final BaseApplicationTemplateSpec roleSpec) { + if (sparkConf.containsKey(templateFileKey) + && (roleSpec == null || roleSpec.getPodTemplateSpec() == null)) { + throw new UnsupportedOperationException( + "Kueue does not support " + + templateFileKey + + " yet. Set the pod template in the SparkApplication spec instead."); + } + } + + private static Workload buildWorkload( + final HasMetadata owner, + final String nameLabelKey, + final boolean suspend, + final List<PodSet> podSets) { + Map<String, String> labels = new HashMap<>(); + if (owner.getMetadata().getLabels() != null) { + labels.putAll(owner.getMetadata().getLabels()); + } + labels.put(nameLabelKey, owner.getMetadata().getName()); + + OwnerReference ownerReference = ModelUtils.buildOwnerReferenceTo(owner); + ownerReference.setController(true); + + Workload workload = new Workload(); + workload.setMetadata( + new ObjectMetaBuilder() + .withName(getWorkloadName(owner)) + .withNamespace(owner.getMetadata().getNamespace()) + .withLabels(labels) + .withOwnerReferences(ownerReference) + .build()); + workload.setSpec( + WorkloadSpec.builder() + .queueName(getQueueName(owner)) + .active(!suspend) + .podSets(podSets) Review Comment: **Finding 24.** The Workload never carries a priority, so Kueue orders every Spark workload the same. `WorkloadSpec` has `priorityClassName` and `priority` — #822 added both and `WorkloadTest:74` exercises them — and this builder sets neither. Kueue reads `spec.priority` and nothing else: `priority.Priority(w)` is `ptr.Deref(w.Spec.Priority, constants.DefaultPriority)`, and the Workload webhook's `Default` only drops `minCount`s, it does not resolve a priority. So a driver pod template with `priorityClassName: high-priority` sits in the ClusterQueue at the default priority, and queue ordering and preemption inside the cohort cannot tell it apart from an unprioritized app. Kueue's own integrations do this in `jobframework.ExtractPriority`: the `kueue.x-k8s.io/priority-class` label wins, otherwise it takes `priorityClassName` from the first PodSet template that has one (`extractPriorityFromPodSets`) and resolves it through `GetPriorityFromPriorityClass`, which also picks up the cluster's `globalDefault` PriorityClass when nothing is named. Then it writes both fields back. The first half is local here — read `podSets.get(0).getTemplate().getSpec().getPriorityClassName()` and put it in `spec.priorityClassName`. The numeric `priority` needs a `PriorityClass` GET, so it needs a `KubernetesClient` this factory does not take, and that half fits the wiring PR better. I could not run Kueue here, so this is read off Kueue's source rather than observed. The experiment that settles it is a Workload created with `spec.priority` unset, queued behind a higher-priority one: if Spark's never preempts, it is at the default. If leaving it out is deliberate, it belongs in the description's Unsupported Features list next to `podTemplateFile` — today it fails silently. ########## spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadFactory.java: ########## @@ -0,0 +1,462 @@ +/* + * 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.Locale; +import java.util.Map; + +import io.fabric8.kubernetes.api.model.Container; +import io.fabric8.kubernetes.api.model.ContainerBuilder; +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.fabric8.kubernetes.api.model.OwnerReference; +import io.fabric8.kubernetes.api.model.PodSpec; +import io.fabric8.kubernetes.api.model.PodSpecBuilder; +import io.fabric8.kubernetes.api.model.PodTemplateSpec; +import io.fabric8.kubernetes.api.model.PodTemplateSpecBuilder; +import io.fabric8.kubernetes.api.model.Quantity; +import io.fabric8.kubernetes.api.model.ResourceRequirements; +import io.fabric8.kubernetes.api.model.ResourceRequirementsBuilder; +import io.fabric8.kubernetes.api.model.apps.StatefulSet; +import lombok.extern.slf4j.Slf4j; + +import org.apache.spark.k8s.operator.Constants; +import org.apache.spark.k8s.operator.SparkApplication; +import org.apache.spark.k8s.operator.SparkCluster; +import org.apache.spark.k8s.operator.SparkClusterResourceSpec; +import org.apache.spark.k8s.operator.SparkClusterSubmissionWorker; +import org.apache.spark.k8s.operator.kueue.v1beta1.PodSet; +import org.apache.spark.k8s.operator.kueue.v1beta1.Workload; +import org.apache.spark.k8s.operator.kueue.v1beta1.WorkloadSpec; +import org.apache.spark.k8s.operator.reconciler.SparkClusterResourceSpecFactory; +import org.apache.spark.k8s.operator.spec.ApplicationSpec; +import org.apache.spark.k8s.operator.spec.BaseApplicationTemplateSpec; +import org.apache.spark.k8s.operator.utils.ModelUtils; +import org.apache.spark.k8s.operator.utils.ReconcilerUtils; +import org.apache.spark.k8s.operator.utils.StringUtils; +import org.apache.spark.network.util.JavaUtils; + +/** + * Factory for creating Kueue Workload resources from Spark custom resources. + * This factory supports both {@link SparkApplication} (driver and executor pod sets) + * and {@link SparkCluster} (master and worker pod sets). + */ +@Slf4j +@SuppressWarnings("PMD.GodClass") +public final class KueueWorkloadFactory { + + public static final String PODSET_DRIVER = "driver"; + public static final String PODSET_EXECUTOR = "executor"; + public static final String PODSET_MASTER = "master"; + public static final String PODSET_WORKER = "worker"; + + private static final String DEFAULT_CORES = "1"; + private static final String DEFAULT_MEMORY = "1g"; + private static final String DEFAULT_MIN_MEMORY_OVERHEAD = "384m"; + private static final double DEFAULT_MEMORY_OVERHEAD_FACTOR = 0.10; + private static final double NON_JVM_MEMORY_OVERHEAD_FACTOR = 0.40; + private static final int DEFAULT_EXECUTOR_INSTANCES = 2; + + private static final String NODE_SELECTOR_PREFIX = "spark.kubernetes.node.selector."; + + private KueueWorkloadFactory() {} + + /** + * Builds a Kueue Workload from a SparkApplication resource. + * + * @param app The SparkApplication. + * @return The constructed Kueue Workload. + */ + public static Workload buildWorkload(final SparkApplication app) { + ApplicationSpec spec = app.getSpec(); + Map<String, String> sparkConf = spec.getSparkConf(); + if ("true".equalsIgnoreCase(sparkConf.get("spark.dynamicAllocation.enabled"))) { + throw new UnsupportedOperationException( + "Kueue does not support SparkApplication with dynamic allocation " + + "(spark.dynamicAllocation.enabled=true) yet."); + } + // Like Spark's KubernetesClusterManager, `local[*]` runs the driver only without executors. + boolean driverOnly = + sparkConf.getOrDefault("spark.kubernetes.driver.master", "").startsWith("local"); + checkNoPodTemplateFile( + sparkConf, Constants.DRIVER_SPARK_TEMPLATE_FILE_PROP_KEY, spec.getDriverSpec()); + if (!driverOnly) { + checkNoPodTemplateFile( + sparkConf, Constants.EXECUTOR_SPARK_TEMPLATE_FILE_PROP_KEY, spec.getExecutorSpec()); + } + List<PodSet> podSets = + driverOnly + ? List.of(buildDriverPodSet(app, sparkConf)) + : List.of(buildDriverPodSet(app, sparkConf), buildExecutorPodSet(app, sparkConf)); + return buildWorkload(app, Constants.LABEL_SPARK_APPLICATION_NAME, spec.isSuspend(), podSets); + } + + /** + * Builds a Kueue Workload from a SparkCluster resource. + * + * @param cluster The SparkCluster. + * @return The constructed Kueue Workload. + */ + public static Workload buildWorkload(final SparkCluster cluster) { + // Use the same StatefulSets which the operator creates for the cluster. + SparkClusterResourceSpec resourceSpec = + SparkClusterResourceSpecFactory.buildResourceSpec( + cluster, new SparkClusterSubmissionWorker()); + if (resourceSpec.getHorizontalPodAutoscaler().isPresent()) { + throw new UnsupportedOperationException( + "Kueue does not support SparkCluster with HorizontalPodAutoscaler " + + "(minWorkers < maxWorkers) yet."); + } + List<PodSet> podSets = + List.of( + buildPodSet(PODSET_MASTER, resourceSpec.getMasterStatefulSet()), + buildPodSet(PODSET_WORKER, resourceSpec.getWorkerStatefulSet())); + return buildWorkload( + cluster, Constants.LABEL_SPARK_CLUSTER_NAME, cluster.getSpec().isSuspend(), podSets); + } + + /** + * A pod template file is fetched by Spark, not by the operator, so its node selectors, + * tolerations and extra containers cannot be reflected in the PodSet template. The file is + * ignored by Spark if the pod template is set in the SparkApplication spec. + */ + private static void checkNoPodTemplateFile( + final Map<String, String> sparkConf, + final String templateFileKey, + final BaseApplicationTemplateSpec roleSpec) { + if (sparkConf.containsKey(templateFileKey) + && (roleSpec == null || roleSpec.getPodTemplateSpec() == null)) { Review Comment: **Finding 25.** This is a second copy of a rule `ModelUtils` already owns. `ModelUtils.overrideDriverTemplateEnabled` / `overrideExecutorTemplateEnabled` (`spark-operator-api/.../ModelUtils.java:132-148`) are exactly `roleSpec != null && roleSpec.getPodTemplateSpec() != null`, and they are what `SparkAppResourceSpecFactory.overrideDependencyConf` consults when it decides whether to overwrite the `podTemplateFile` key. The two agree today. If the precedence rule ever changes there, this guard diverges silently — it either rejects an app whose file the operator would have replaced, or admits one whose file Spark really does fetch. `ModelUtils` is already imported here for `buildOwnerReferenceTo`, so passing the boolean in is small: ```java checkNoPodTemplateFile( sparkConf, Constants.DRIVER_SPARK_TEMPLATE_FILE_PROP_KEY, ModelUtils.overrideDriverTemplateEnabled(spec)); if (!driverOnly) { checkNoPodTemplateFile( sparkConf, Constants.EXECUTOR_SPARK_TEMPLATE_FILE_PROP_KEY, ModelUtils.overrideExecutorTemplateEnabled(spec)); } ``` ```java /** * A pod template file is fetched by Spark, not by the operator, so its node selectors, * tolerations and extra containers cannot be reflected in the PodSet template. The operator * overwrites the file key when the pod template is set in the SparkApplication spec, so the * spec wins in that case. */ private static void checkNoPodTemplateFile( final Map<String, String> sparkConf, final String templateFileKey, final boolean specTemplateWins) { if (sparkConf.containsKey(templateFileKey) && !specTemplateWins) { ``` The Javadoc rewrite is part of the same edit. It is the operator, in `SparkAppResourceSpecFactory.getOrCreateLocalFileForSpec`, that replaces the key with a generated temp file — Spark itself reads whatever `spark.kubernetes.driver.podTemplateFile` says. -- 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]
