dongjoon-hyun commented on code in PR #829: URL: https://github.com/apache/spark-kubernetes-operator/pull/829#discussion_r4027226284
########## tests/e2e/kueue/chainsaw-test.yaml: ########## @@ -0,0 +1,102 @@ +# +# 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. +# + +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + name: spark-operator-kueue +spec: + namespace: default + steps: + - name: spark-application-without-queue-name-is-not-queued + try: + - apply: + file: spark-example.yaml + - assert: + bindings: + - name: SPARK_APP_NAMESPACE + value: default + - name: SPARK_APPLICATION_NAME + value: spark-job-kueue-test + timeout: 10m + file: "../assertions/spark-application/spark-state-transition.yaml" + # No operator code path creates a Workload yet: `KueueWorkloadFactory` has no caller + # outside its own unit tests. This pins the opt-out behavior for when it is wired in. + - error: + timeout: 10s + resource: + apiVersion: kueue.x-k8s.io/v1beta2 + kind: Workload + metadata: + namespace: default + catch: + - describe: + apiVersion: spark.apache.org/v1 + kind: SparkApplication + namespace: default + - describe: + apiVersion: kueue.x-k8s.io/v1beta2 + kind: Workload + namespace: default + finally: + - script: + timeout: 120s + content: | + kubectl delete sparkapplication spark-job-kueue-test --ignore-not-found=true + - name: spark-cluster-without-queue-name-is-not-queued + try: + - apply: + file: ../../../examples/qa-cluster-with-one-worker.yaml + - assert: + bindings: + - name: SPARK_APP_NAMESPACE + value: default + - name: SPARK_CLUSTER_NAME + value: qa + timeout: 10m + file: "../assertions/spark-cluster/spark-cluster-state-transition.yaml" Review Comment: Fixed in the description: it now says the application reaches `ResourceReleased` and the cluster reaches `RunningHealthy` with its worker StatefulSet ready. ########## .github/workflows/build_and_test.yml: ########## @@ -176,6 +182,13 @@ jobs: run: | kubectl get pods -A kubectl describe node + - name: Install Kueue + if: matrix.mode == 'kueue' + run: | + kubectl apply --server-side -f https://github.com/kubernetes-sigs/kueue/releases/download/${{ env.KUEUE_VERSION }}/manifests.yaml Review Comment: Thanks. I agree the docs and CI disagree, but I think the docs are the part to fix. In Kueue v0.19.4, `integrations.externalFrameworks` is only read on the job framework paths: - `pkg/controller/jobframework/reconciler.go:919` (walking up to a parent job) - `pkg/controller/jobframework/defaults.go:88,107` (queue-name and `WorkloadPriorityClass` defaulting) - `pkg/controller/jobs/pod/pod_webhook.go:269` (only builds a warning string) The `Workload` controller, the scheduler and the `Workload` webhook never look at the owner kind. The only owner check in `pkg/controller/core/workload_controller.go` is `isOrphanedWorkload`, and it only fires when `ownerReferences` is empty. The operator always sets a controller reference, so a `Workload` it creates is admitted without the registration. The future positive case should pass with the stock manifests. The registration still matters in one place. `defaultLocalQueueApplies` (`defaults.go:80-88`) has no feature gate. If a namespace has a `LocalQueue` named `default`, Kueue adds `queue-name: default` to any pod whose owner is not a registered kind. That includes the driver pod the operator creates, so the pod would be queued a second time. So the docs are right to recommend the registration, but it is not a hard prerequisite of `operatorRbac.kueue.enabled`. I'd keep CI as it is and follow up on `docs/operations.md:40-45` so it says why the registration is recommended. I'd appreciate it if you filed that follow-up. -- 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]
