chore(spark): align e2e harness with upstream Helm chart defaults - #777
chore(spark): align e2e harness with upstream Helm chart defaults#777adibmbrk wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
🎉 Welcome to the Kubeflow SDK! 🎉 Thanks for opening your first PR! We're happy to have you as part of our community 🚀 Here's what happens next:
Join the community:
Feel free to ask questions in the comments if you need any help or clarification! |
Drop the vendored SparkConnect CRD and hand-rolled RBAC in favor of the spark-operator chart's own CRD-upgrade hook and default permissions, so the e2e suite exercises the same setup a real Helm install produces and catches upstream drift. Extend the cluster watcher to also poll SparkApplication (not just SparkConnect), and expand the K8s version matrix to match upstream's tested versions. Signed-off-by: adibmbrk <adibmbrk@gmail.com>
419d275 to
76327a1
Compare
|
/assign @tariq-hasan |
|
/ok-to-test |
tariq-hasan
left a comment
There was a problem hiding this comment.
Hi @adibmbrk! Thanks for raising the PR. I have added a few comments.
| ) | ||
|
|
||
| success = succeeded and not failed | ||
| return success, logs, job_desc |
There was a problem hiding this comment.
We should not remove run_in_cluster.py, Dockerfile.spark-e2e-runner and connect_existing_session.py as in-cluster testing is important.
| @@ -1 +1 @@ | |||
| name: Spark Examples E2E Test | |||
There was a problem hiding this comment.
Let's repurpose this workflow file to follow the approach taken by the Trainer repo.
sdk/.github/workflows/test-e2e.yaml
Lines 1 to 82 in 3febcd7
I don't think we'd want a hand-crafted cluster creation setup for Spark SDK - we should delegate this process to the corresponding upstream spark operator project. The Makefile already supports cluster lifecycle management so let's centralize and reuse that logic instead of making the SDK own setup for a control plane it doesn't ship - given that this cluster setup is already part of the spark operator e2e and we'd want to ensure reproducibility on both the client-side and server-side layers as well as ensure consistency between the client-side components.
What this PR does / why we need it:
Makes the Spark E2E harness mirror a default
spark-operatorHelm install so it catches upstream drift instead of masking it:hook.upgradeCrd=truepre-install hook instead.ensure_sparkconnect_rbac) — it granted permissions beyond what the default chart install provides. Examples now run as subprocesses against the CI runner's kubeconfig instead of in-cluster Jobs, which no longer need bespoke ServiceAccount permissions.hack/Dockerfile.spark-e2e-runner,test/e2e/spark/run_in_cluster.py, theconnect_existing_session.pyexample, and theSPARK_E2E_RUN_IN_CLUSTER/SPARK_E2E_RUNNER_IMAGEenv vars/CI steps. This mode existed to let Job pods (running as thedefaultSA) connect via an in-cluster URL; removing thedefault-SA RBAC (per KEP-107: Improve Spark E2E Robustness #611) breaks that path, so rather than provision a new dedicated SA for it, in-cluster execution is removed and all examples now run as subprocesses against the ambient kubeconfig.SparkApplicationin addition toSparkConnectso batch-job test failures get the same diagnostics as session tests.1.32.11,1.33.7,1.34.3,1.35.0and theSPARK_OPERATOR_VERSIONfallback to2.5.0.Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes #611
Checklist: