Repository navigation
[SPARK-59559] Add kueue E2E test for resources without kueue.x-k8s.io/queue-name - #829
dongjoon-hyun wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Thanks for the PR, @dongjoon-hyun!
The new kueue mode installs Kueue v0.19.4, deploys the operator with the opt-in RBAC from #827, and runs a SparkApplication and a SparkCluster to terminal state while asserting no Workload appears. The reconciliation half is real coverage: it proves Kueue's pod and statefulset webhooks do not break the operator. The Workload half is not, because KueueWorkloadFactory has no caller in src/main yet, so nothing can create a Workload whatever the label says. The description also still names the values file the first commit used.
Blocking
- 1. Description names the wrong values file: it says the job deploys with
tests/e2e/helm/helm-test-values/kueue/values.yaml, but1af625cswitched that totests/e2e/helm/kueue-config-values.yaml. The named file exists and belongs to a different job, so the description points somewhere real and wrong. [inline:.github/workflows/build_and_test.yml:251]
Non-blocking
- 2. The
error: Workloadsteps cannot fail:KueueWorkloadFactoryandConstants.LABEL_QUEUE_NAMEhave no consumer insrc/main, so no operator code path creates aWorkload. Both steps pass unconditionally today, and would keep passing under any mutation of the label check they are meant to guard. [inline:tests/e2e/kueue/chainsaw-test.yaml:37] - 3.
examples/pi.yamlis not a Kueue-eligible fixture:buildWorkloadthrows onspark.dynamicAllocation.enabled=true, which that example sets. So even once the factory is wired, the SparkApplication step still cannot observe aWorkload- the regression would surface as a reconcile failure instead. The SparkCluster step is fine. [inline:tests/e2e/kueue/chainsaw-test.yaml:28]
Minor
- 4. Kueue
v0.19.4is hardcoded in two places: lines 185 and 303, so a bump needs both or the two jobs silently test different versions. [inline:.github/workflows/build_and_test.yml:185]
| ./gradlew buildDockerImage | ||
| helm install spark --create-namespace -f \ | ||
| build-tools/helm/spark-kubernetes-operator/values.yaml -f \ | ||
| tests/e2e/helm/kueue-config-values.yaml \ |
There was a problem hiding this 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.
There was a problem hiding this comment.
Fixed. The description now points to tests/e2e/helm/kueue-config-values.yaml and explains why it lowers the operator CPU request.
| value: pi | ||
| timeout: 10m | ||
| file: "../assertions/spark-application/spark-state-transition.yaml" | ||
| - error: |
There was a problem hiding this 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.
There was a problem hiding this comment.
Agreed, thanks. I kept the group and said so: 2955b4d adds a comment above both error: steps, and the "Why" section now states that no operator code path creates a Workload yet, so today the value is running operator-created pods and StatefulSets under Kueue's failurePolicy: Fail webhooks.
| - name: spark-application-without-queue-name-is-not-queued | ||
| try: | ||
| - apply: | ||
| file: ../../../examples/pi.yaml |
There was a problem hiding this 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:
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.yamlhas 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 budget1af625cjust fought.examples/pi-with-one-pod.yamlrequests 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.
There was a problem hiding this comment.
Good catch. 2955b4d switches the application step to a local tests/e2e/kueue/spark-example.yaml with no dynamic allocation and spark.executor.instances: "1", so it passes the factory guard. The CI node reports 4 allocatable cores, so the budget is 750m (system) + 500m (Kueue) + 100m (operator) + 1000m (driver) + 1000m (executor) = 3350m. The cluster step keeps examples/qa-cluster-with-one-worker.yaml since it passes the HPA guard, as you noted.
| - 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 |
There was a problem hiding this 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:
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.)
There was a problem hiding this comment.
Done in 2955b4d: KUEUE_VERSION: v0.19.4 is now a workflow-level env, and both installs use ${{ env.KUEUE_VERSION }}.
|
Thank you for the thorough review, @peter-toth. All four findings are addressed in 2955b4d. |
There was a problem hiding this comment.
Re-checked through 2955b4d — findings 1, 2, 3, 4 resolved, nothing regressed.
The new tests/e2e/kueue/spark-example.yaml clears every guard in buildWorkload: no dynamic allocation, no spark.kubernetes.driver.master, no pod-template file key. qa-cluster-with-one-worker.yaml still clears the HPA guard at minWorkers == maxWorkers. The env hoist resolves correctly, since Helm Tests (1.37.0, kueue) is green on this head. KueueWorkloadFactory is still unwired on apache/main as of d383957, so the documented framing in the "Why" section is still accurate. Three small things left, none about the design.
Non-blocking
- 5.
catch:cannot show why a pod failed (late catch): the one failure this cell has produced was654c26d, where the executor did not fit on the node. That surfaces as aFailedSchedulingevent on the pod, which neither collector here prints. [inline:tests/e2e/kueue/chainsaw-test.yaml:46]
Minor
- 6. Description calls the cluster's state terminal (late catch): the SparkCluster assertion stops at
RunningHealthy, whichClusterStateSummarydoes not count as terminated. [inline:tests/e2e/kueue/chainsaw-test.yaml:71] - 7. CI's Kueue install skips the prerequisite our own docs state (late catch):
docs/operations.md:40-45requiresintegrations.externalFrameworksregistration wheneveroperatorRbac.kueue.enabledis set, and this job sets it. [inline:.github/workflows/build_and_test.yml:188]
| kind: Workload | ||
| metadata: | ||
| namespace: default | ||
| catch: |
There was a problem hiding this 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):
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: defaultSame 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.
There was a problem hiding this comment.
Agreed, thanks. dd39c64 adds a pod describe and podLogs to both catch: blocks, selecting on spark.operator/spark-app-name=spark-job-kueue-test and spark.operator/spark-cluster-name=qa, so a FailedScheduling event like the one in 654c26d will show up. chainsaw lint test (v0.2.15, same as CI) reports the file as valid.
| - name: SPARK_CLUSTER_NAME | ||
| value: qa | ||
| timeout: 10m | ||
| file: "../assertions/spark-cluster/spark-cluster-state-transition.yaml" |
There was a problem hiding this 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.
There was a problem hiding this comment.
Fixed in the description: it now says the application reaches ResourceReleased and the cluster reaches RunningHealthy with its worker StatefulSet ready.
| - 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 |
There was a problem hiding this 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.
There was a problem hiding this 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 andWorkloadPriorityClassdefaulting)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.
There was a problem hiding this comment.
Filed SPARK-59578. I checked defaultLocalQueueApplies against v0.19.4 and it is ungated, unlike ApplyDefaultWorkloadPriorityClass right below it, so the doc reword is the right fix. Keeping CI as is.
What changes were proposed in this pull request?
This PR adds a
kueueE2E test group that verifies Spark resources without akueue.x-k8s.io/queue-namelabel are left alone when Kueue is installed and the operator holds theopt-in Kueue RBAC rules.
tests/e2e/kueue/chainsaw-test.yaml. It appliestests/e2e/kueue/spark-example.yamlandexamples/qa-cluster-with-one-worker.yaml, asserts through the shared assertions intests/e2e/assertions/that the application reachesResourceReleasedand the cluster reachesRunningHealthywith its worker StatefulSet ready, and asserts that noWorkloadis created. Theapplication manifest is local rather than a shipped example because
KueueWorkloadFactoryrejectsdynamic allocation, so an example using it could never produce a
Workload.K8s Integration Testsjob as a newkueuemode, which installsKueue v0.19.4 (released on
2026-09-10) and deploys the operator with
tests/e2e/helm/kueue-config-values.yaml. That filelowers the operator CPU request to
0.1, like the dynamic config values already do, because theKueue controller requests 500m on top of it and otherwise no executor fits on the test node.
KUEUE_VERSIONso that this job and theHelm Testsinstall cannot drift apart.Why are the changes needed?
Kueue support is opt-in. No operator code path creates a
Workloadyet, so today this group's realvalue is showing that the operator's pods and StatefulSets keep working with Kueue's
failurePolicy: Failwebhooks installed. TheWorkloadassertions pin the opt-out behavior forwhen
KueueWorkloadFactoryis wired into reconciliation.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Pass the CIs.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 5