dongjoon-hyun commented on code in PR #823: URL: https://github.com/apache/spark-kubernetes-operator/pull/823#discussion_r4008566905
########## 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: Thank you. I agree that the priority should be propagated, but I'd like to defer both fields together to the wiring PR rather than setting `priorityClassName` alone here. Kueue's Workload CRD has a CEL rule on `WorkloadSpec` (`apis/kueue/v1beta1/workload_types.go`): ``` // +kubebuilder:validation:XValidation:rule="has(self.priorityClassName) ? has(self.priority) : true", message="priority should not be nil when priorityClassName is set" ``` So a Workload with `priorityClassName` but no `priority` is rejected by the API server, and resolving `priority` needs a `PriorityClass` lookup with a `KubernetesClient`, which this factory doesn't take. I listed it under "Deferred Features" in the PR description. ########## 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: Fixed in ab7cfb8. `checkNoPodTemplateFile` takes the result of `ModelUtils.overrideDriverTemplateEnabled` / `overrideExecutorTemplateEnabled` now, and the Javadoc is corrected as you suggested. -- 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]
