peter-toth commented on code in PR #829:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/829#discussion_r4023682328
##########
.github/workflows/build_and_test.yml:
##########
@@ -231,6 +241,20 @@ jobs:
if: matrix.mode == 'selector'
run: |
chainsaw test --test-dir ./tests/e2e/${{ matrix.test-group }}
--parallel 1
+ - name: Run Spark K8s Operator on K8S with Kueue Enabled
+ if: matrix.mode == 'kueue'
+ run: |
+ eval $(minikube docker-env)
+ ./gradlew buildDockerImage
+ helm install spark --create-namespace -f \
+ build-tools/helm/spark-kubernetes-operator/values.yaml -f \
+ tests/e2e/helm/kueue-config-values.yaml \
Review Comment:
**Finding 1.** The description says the job "deploys the operator with
`tests/e2e/helm/helm-test-values/kueue/values.yaml`". That was true of
`654c26d`; `1af625c` changed it:
```
$ git diff 654c26d 1af625c -- .github/workflows/build_and_test.yml
- tests/e2e/helm/helm-test-values/kueue/values.yaml \
+ tests/e2e/helm/kueue-config-values.yaml \
```
Worth fixing rather than letting it ride, because the path in the
description still resolves: `tests/e2e/helm/helm-test-values/kueue/values.yaml`
is the file the `helm-tests / kueue` group uses, and it has no CPU override. A
reader who follows the description ends up in the wrong job. The repo
squash-merges, so the description becomes the commit body.
Description-only change. The new file and the switch to it are both right.
##########
tests/e2e/kueue/chainsaw-test.yaml:
##########
@@ -0,0 +1,98 @@
+#
+# 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: ../../../examples/pi.yaml
Review Comment:
**Finding 3.** This fixture is rejected by the factory before it can produce
a `Workload`, so the step's assertion stays inert even after the wiring lands.
`examples/pi.yaml` sets `spark.dynamicAllocation.enabled: "true"`, and
`KueueWorkloadFactory.java:93-97` throws on exactly that:
```java
if
("true".equalsIgnoreCase(sparkConf.get("spark.dynamicAllocation.enabled"))) {
throw new UnsupportedOperationException(
"Kueue does not support SparkApplication with dynamic allocation "
+ "(spark.dynamicAllocation.enabled=true) yet.");
}
```
So a future "queue everything" regression would surface here as a failed
reconcile, not as a `Workload`, and the `error:` step at line 37 would still
pass. The SparkCluster step does not have this problem:
`qa-cluster-with-one-worker.yaml` has `minWorkers == maxWorkers == 1`, so no
HPA is built and the guard at `KueueWorkloadFactory.java:129-133` lets it
through.
I checked the obvious swaps and neither fits this node, so this is not a
one-line change:
- `examples/pi-java25.yaml` has no dynamic allocation but then takes the
Spark default of 2 static executors, so driver + executors want 3 cores on a
3-core node that is already carrying the Kueue controller. That is the budget
`1af625c` just fought.
- `examples/pi-with-one-pod.yaml` requests 5 driver cores.
A small manifest in `tests/e2e/kueue/` pinning `spark.executor.instances:
"1"` would be eligible and would fit, at the cost of the "reuse the shipped
examples" property. Your call whether that is worth it now or when the factory
is wired.
##########
.github/workflows/build_and_test.yml:
##########
@@ -176,6 +179,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/v0.19.4/manifests.yaml
Review Comment:
**Finding 4.** `v0.19.4` now appears at line 185 and again at line 303 (the
`helm-tests` install from #827), with the same three-line block. A bump has to
touch both, and missing one leaves the two jobs testing different Kueue
versions without any signal.
The `if:` conditions differ, so the steps cannot be merged, but the version
can be one value:
```yaml
name: Build and test
env:
KUEUE_VERSION: v0.19.4
on:
```
and then `.../download/${{ env.KUEUE_VERSION }}/manifests.yaml` in both.
(The wait gate itself is right - Kueue registers a `NoopWebhook` at every
disabled integration's path, so nothing 404s under `failurePolicy: Fail`, and
`readyz` includes the webhook server's started-checker.)
##########
tests/e2e/kueue/chainsaw-test.yaml:
##########
@@ -0,0 +1,98 @@
+#
+# 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: ../../../examples/pi.yaml
+ - assert:
+ bindings:
+ - name: SPARK_APP_NAMESPACE
+ value: default
+ - name: SPARK_APPLICATION_NAME
+ value: pi
+ timeout: 10m
+ file: "../assertions/spark-application/spark-state-transition.yaml"
+ - error:
Review Comment:
**Finding 2.** Nothing in the operator can create a `Workload` today, so
this step and its twin at line 78 pass no matter what.
```
$ grep -rn "KueueWorkloadFactory" spark-operator/src/main
spark-operator/src/main/java/.../kueue/KueueWorkloadFactory.java:66:public
final class KueueWorkloadFactory {
$ grep -rn "LABEL_QUEUE_NAME" spark-operator/src/main
spark-operator-api/src/main
spark-operator-api/src/main/java/.../Constants.java:48: public static final
String LABEL_QUEUE_NAME = "kueue.x-k8s.io/queue-name";
```
The factory has no caller outside its own file and its unit tests, no
reconcile step under `reconciler/reconcilesteps/` mentions Kueue, and
`LABEL_QUEUE_NAME` has no reader. So the "Why" section's premise does not hold
yet: a regression that queued every resource is not currently reachable,
because there is no queueing code to regress.
That does not make the group worthless. Installing Kueue and still reaching
`ResourceReleased` and `RunningHealthy` is genuine coverage of Kueue's
pod/deployment/statefulset webhooks running at `failurePolicy: Fail` against
operator-created pods and statefulsets. That is what the job buys today, and it
is worth having.
Two ways to keep the record straight:
- land this after the wiring PR, so the assertion has something to guard
from day one; or
- keep it and say so, e.g. a comment above this step - `No operator code
path creates a Workload yet (SPARK-59490 is not wired in); this pins the
behaviour for when it is` - plus a matching sentence in the "Why" section.
--
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]
