peter-toth commented on code in PR #829: URL: https://github.com/apache/spark-kubernetes-operator/pull/829#discussion_r4027152218
########## 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: **Finding 6.** The description says the test "asserts each reaches its terminal state through the shared assertions in `tests/e2e/assertions/`". That holds for the SparkApplication, whose assertion ends at `ResourceReleased`. It does not hold here: this assertion ends at `RunningHealthy`, and `ClusterStateSummary.isTerminated()` counts only `ResourceReleased` (`spark-operator-api/src/main/java/org/apache/spark/k8s/operator/status/ClusterStateSummary.java:27-41`). The cluster step also asserts the worker StatefulSet reaches `readyReplicas: 1`, which the description leaves out. Something like "asserts the application reaches `ResourceReleased` and the cluster reaches `RunningHealthy` with its worker StatefulSet ready" would match. Description-only change. The repo squash-merges, so it becomes the commit body. ########## 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: Review Comment: **Finding 5.** This `catch:` prints the SparkApplication and any `Workload`, but not the pods, so it cannot show the one failure this cell has actually produced. `654c26d` failed here, and `K8s Integration Tests (1.37.0, kueue, kueue)` was the only red job in that run. `1af625c` fixed it by dropping the operator CPU request. That failure shape is a `FailedScheduling` event on the executor pod. `kubectl describe sparkapplication` shows events on the CR, not on the pod, and the `Workload` describe prints nothing when no `Workload` exists. Chainsaw leaves `--show-events` at kubectl's default of `true`, so one extra collector covers it. Driver and executor pods both carry `spark.operator/spark-app-name` (`spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/Utils.java:141-169`): ```yaml catch: - describe: apiVersion: spark.apache.org/v1 kind: SparkApplication namespace: default - describe: apiVersion: v1 kind: Pod namespace: default selector: spark.operator/spark-app-name=spark-job-kueue-test - podLogs: selector: spark.operator/spark-app-name=spark-job-kueue-test namespace: default - describe: apiVersion: kueue.x-k8s.io/v1beta2 kind: Workload namespace: default ``` Same shape for the cluster step at line 89, with `spark.operator/spark-cluster-name=qa`. I raised this in round 1 only to myself and dropped it, on the grounds that `state-transition` and `suspend` also collect nothing. I think that was the wrong read. `pi-with-comet`, `pi-java25` and `pi-with-gluten` all collect `podLogs`, and this group has the tightest CPU budget in the matrix, so it is the one most likely to need the evidence. ########## .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: **Finding 7.** `docs/operations.md:40-45` says that when `operatorRbac.kueue.enabled` is set, Kueue must also have `SparkApplication.v1.spark.apache.org` and `SparkCluster.v1.spark.apache.org` in its `integrations.externalFrameworks`. This job sets that flag (`tests/e2e/helm/kueue-config-values.yaml:26-28`) and installs the stock `manifests.yaml`, which registers neither. So CI now runs the configuration our own docs call incomplete. Nothing fails today, because no `Workload` is created either way. It matters for the step after this one. This group is the natural home for the positive case once `KueueWorkloadFactory` is wired in, and that case cannot pass without the registration. The `Helm Tests` install at line 306 has the same gap, so it is not a regression from this PR. This is the first job that runs real Spark resources under that config, though, so it is the one worth getting right. Patching the `kueue-manager-config` ConfigMap in `kueue-system` and restarting the controller before the `kubectl wait` would do it. A follow-up ticket is also fine if you would rather keep this PR to the test group, and I am happy to file it. -- 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]
