fix: stop adding SparkApplication owner reference to executor pods

[KR-Ravindra]

Purpose of this PR

Proposed changes:

• Stop adding the SparkApplication as a second (non-controller) ownerReference on executor pod templates at submission time, so that executor pods are garbage-collected as soon as their controller owner (the driver pod) is deleted.
• Update the expectations in TestExecutorPodTemplateContents and TestSubmitPodTemplates so executor templates carry only user-supplied owner references; driver templates keep the controller reference to the SparkApplication (TestDriverPodTemplateContents is unchanged).

Problem

Since #2670 every executor pod has two owners: the driver pod (controller: true, set by Spark itself) and the SparkApplication (controller unset, added by the operator). Kubernetes garbage collection only deletes a dependent once all of its owners are gone, so deleting the driver pod (manually, or via spec.suspend=true, e.g. when Kueue preempts the workload) leaves the executor pods behind until the SparkApplication itself is deleted. Across Kueue requeue cycles this accumulates orphaned executor pods.

Root cause

internal/controller/sparkapplication/submission.go:1199-1209 (buildExecutorPodTemplate) appends util.GetOwnerReference(app) with Controller/BlockOwnerDeletion cleared to the executor pod template. That extra owner is what blocks the cascade deletion from the driver pod. The webhook path (internal/webhook/sparkpod_defaulter.go addOwnerReference) already returns early for non-driver pods, so submission is the only place that adds it.

Fix

Remove the executor ownerReference block in buildExecutorPodTemplate. Executor pods keep their controller reference to the driver pod, and the driver pod keeps its controller reference to the SparkApplication, so deleting the SparkApplication still cascades to the driver and then to the executors, while deleting only the driver now cascades to the executors as well. This follows the approach suggested in the issue discussion (drop the executor's SparkApplication reference and rely on GC cascade deletion instead of adding operator-managed deletion of executor pods).

The original motivation for the extra reference was to let other controllers (e.g. Kueue) recognize executor pods as children of the SparkApplication. Kueue's ownership walk (FindAncestorJobManagedByKueue) and its IsOwnerManagedByKueueForObject check both use metav1.GetControllerOf, i.e. only controller references, and its SparkApplication integration identifies executor pods by the spark-role label and reads readiness from status.executorState, so the non-controller reference is not consumed there.

How tested

Unit tests (no cluster required) plus an e2e assertion in the existing suspend/resume test.

Before the fix, with the updated expectations, three subtests fail. Two are shown below; the third, TestSubmitPodTemplates/sends_default_templates_with_owner_ref_when_not_specified (internal/controller/sparkapplication/rest_submission_test.go:203), fails the same way because the submitted executor template still carries the SparkApplication owner reference (Empty assertion on OwnerReferences) and is only absent from the output because of the -run filter. The "pod template already has owner references" case is unchanged and passes both before and after, because user-supplied owner references are preserved as-is.

$ go test ./internal/controller/sparkapplication/ -run 'TestExecutorPodTemplateContents|TestDriverPodTemplateContents' -count=1
--- FAIL: TestExecutorPodTemplateContents (0.00s)
    --- FAIL: TestExecutorPodTemplateContents/with_pod_template (0.00s)
        submission_test.go:645:
            	Diff:
            	--- Expected
            	+++ Actual
            	@@ -6,2 +6,6 @@
            	     uid: ""
            	+  - apiVersion: sparkoperator.k8s.io/v1beta2
            	+    kind: SparkApplication
            	+    name: test
            	+    uid: 2d2d2c08-fcc9-4e41-9d93-13540605f24e
            	 spec:
    --- FAIL: TestExecutorPodTemplateContents/no_pod_template (0.00s)
        submission_test.go:645:
            	Diff:
            	--- Expected
            	+++ Actual
            	@@ -1,2 +1,7 @@
            	-metadata: {}
            	+metadata:
            	+  ownerReferences:
            	+  - apiVersion: sparkoperator.k8s.io/v1beta2
            	+    kind: SparkApplication
            	+    name: test
            	+    uid: 2d2d2c08-fcc9-4e41-9d93-13540605f24e
            	 spec:
FAIL
FAIL	github.com/kubeflow/spark-operator/v2/internal/controller/sparkapplication	0.031s

After the fix:

$ go test ./internal/controller/sparkapplication/ -run 'TestExecutorPodTemplateContents|TestDriverPodTemplateContents' -count=1 -v
--- PASS: TestDriverPodTemplateContents (0.00s)
--- PASS: TestExecutorPodTemplateContents (0.00s)
ok  	github.com/kubeflow/spark-operator/v2/internal/controller/sparkapplication	0.030s

$ KUBEBUILDER_ASSETS=... go test ./internal/controller/sparkapplication/... ./internal/webhook/... -count=1
ok  	github.com/kubeflow/spark-operator/v2/internal/controller/sparkapplication
ok  	github.com/kubeflow/spark-operator/v2/internal/webhook

gofmt -l, go vet, go build ./... and golangci-lint run ./internal/controller/sparkapplication/... are clean.

e2e (added on request of the reviewers): the spark-pi: Suspend and Resume case in test/e2e/sparkapplication_test.go now waits for an executor pod to exist before suspending and, once the application reaches Suspended, asserts that no pod labelled sparkoperator.k8s.io/app-name=spark-pi remains (driver or executor). Without this fix the executor pod survives the driver deletion because the SparkApplication is still an owner, so the assertion fails; with the fix the executors are cascade-deleted with the driver. gofmt -l and go vet ./... in test/e2e are clean; the CI e2e matrix runs it.

Links

• Issue: Executor pods are not cascade-deleted when driver pod is deleted #2961
• Change that introduced the executor owner reference: Set ControllerReference on driver pod and non-Controller OwnerReference on executor pod at submission time #2670
• Related Kueue report: SparkApplication framework: PodsReady ignores executor state, and Stop leaves orphan executor pods across requeue cycles kubernetes-sigs/kueue#11675

Change Category

• Bugfix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that could affect existing functionality)
• Documentation update

Rationale

Covered above. Executor pods created by an operator built before this change keep their existing owner references; only newly submitted applications are affected.

Checklist

• I have conducted a self-review of my own code.
• I have updated documentation accordingly.
• I have added tests that prove my changes are effective or that my feature works.
• Existing unit tests pass locally with my changes.

Additional Notes

This change was prepared with an AI agent operated by KR-Ravindra, who reviewed and tested it.

[github-actions]

🎉 Welcome to the Kubeflow Spark Operator! 🎉

Thanks for opening your first PR! We're happy to have you as part of our community 🚀

Here's what happens next:

• If you haven't already, please check out our Contributing Guide for repo-specific guidelines and the Kubeflow Contributor Guide for general community standards.
• Our team will review your PR soon!

Join the community:

• Slack: Join our #kubeflow-spark-on-kubernetes Slack channel.

Feel free to ask questions in the comments if you need any help or clarification!
Thanks again for contributing to Kubeflow! 🙏

[KR-Ravindra]

Round 1 self-review (AI agent operated by me; I review every revision). Marking ready for review because:

1. Root cause verified on master: buildExecutorPodTemplate (submission.go:1199-1209) appends a non-controller SparkApplication owner reference to every executor template; the webhook's addOwnerReference returns early for non-driver pods, so this is the only source. Kubernetes GC keeps a dependent while any owner exists, so the driver pod's deletion cannot cascade.
2. This is the approach a reviewer suggested on Executor pods are not cascade-deleted when driver pod is deleted #2961 (drop the executor reference and rely on GC cascade, instead of operator-managed executor deletion).
3. The Kueue claim in the body checks out: FindAncestorJobManagedByKueue walks metav1.GetControllerOf (kueue reconciler.go:933), and the SparkApplication integration selects executors by the spark-role label, so the non-controller reference is not consumed there.
4. slices / reflect stay imported for the driver template path (:1175); DCO, FOSSA and readthedocs pass.

Open question for maintainers: #2670 added the reference for Kueue; if any other consumer relies on it, the alternative is an explicit executor cleanup on driver deletion.

[Copilot]

🔵 Needs a closer look

The change alters Kubernetes ownership/garbage-collection semantics with cross-controller (Kueue) implications that warrant final human verification.

Pull request overview

This PR fixes a garbage-collection bug (issue #2961) where executor pods were not cascade-deleted when only the driver pod was removed (e.g. on spec.suspend=true via Kueue preemption). The root cause was a second, non-controller ownerReference pointing at the SparkApplication that the operator added to executor pod templates at submission time (introduced in #2670). Since Kubernetes GC only deletes a dependent once all owners are gone, this blocked the cascade from the driver pod. The fix removes that extra owner reference so executors rely solely on their controller reference to the driver pod.

Changes:

• Remove the block in buildExecutorPodTemplate that appended a non-controller SparkApplication owner reference to executor pod templates, replacing it with an explanatory comment.
• Update TestExecutorPodTemplateContents and TestSubmitPodTemplates so executor templates carry only user-supplied owner references.

File summaries

File	Description
internal/controller/sparkapplication/submission.go	Drops the executor-template SparkApplication owner reference; driver template still gets its controller reference.
internal/controller/sparkapplication/submission_test.go	Removes the expected SparkApplication owner reference from executor template expectations; user-supplied refs preserved.
internal/controller/sparkapplication/rest_submission_test.go	Asserts executor pod template now carries no owner references.

I verified that the slices, reflect, and metav1 imports remain in use by buildDriverPodTemplate, the appNonControllerOwnerReference test helper is still referenced by the "already has owner references" case, the webhook's addOwnerReference returns early for non-driver pods (so the reference is not re-added), and GetOwnerReference still sets Controller: true for the driver template. No issues were found.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[RobuRishabh]

Hi @KR-Ravindra thanks for the contribution, the PR looks good to me
@tariq-hasan need a suggestion here unit tests are enough for merge but how about add a e2e test here that submits an app, deletes the driver, and asserts executors disappear would lock in the fix or do you think its over-engineering.

and a quick question if anyone has answer to it, Is there any other tool (not Kueue, not spark-operator) that depends on executors having a direct SparkApplication owner ref?

[RobuRishabh]

/ok-to-test

[KR-Ravindra]

@RobuRishabh thanks for the review. Happy to add the e2e test if @tariq-hasan wants it: a case in test/e2e/sparkapplication_test.go that submits an application, deletes the driver pod once the executors are running, and asserts the executor pods are gone within the GC timeout. It is a few dozen lines on top of the existing helpers, so I would not call it over-engineering. Say the word and I will push it to this PR.

On the ownerRef question: executor pods keep the owner reference Spark itself sets (the driver pod, controller: true), so anything that walks ownerReferences (kubectl tree, the Argo CD resource tree) still reaches the executors through the driver. Only the direct executor -> SparkApplication edge goes away. I am not aware of a tool that depends on that direct edge; the operator's own executor accounting is label-based (sparkoperator.k8s.io/app-name), and Spark's own cleanup uses the driver reference.

[tariq-hasan]

I'm thinking we might be able to add assertions here to test the change.

spark-operator/test/e2e/sparkapplication_test.go

Lines 456 to 461 in cfc9cba

	By("Waiting for SparkApplication to Suspended")
	Eventually(func(g Gomega) {
	app := &v1beta2.SparkApplication{}
	g.Expect(k8sClient.Get(ctx, key, app)).To(Succeed())
	g.Expect(app.Status.AppState.State).To(Equal(v1beta2.ApplicationStateSuspended))
	}).WithTimeout(3 * time.Minute).Should(Succeed())

For the use cases I'm thinking we do not really have a clearly defined one: #2670 (comment). We can always revisit if we do see actual use cases that require this owner reference.

@vjanelle @vikas-saxena02 Would appreciate further thoughts on this.

[KR-Ravindra]

@tariq-hasan done in 2b4fb02: the suspend/resume e2e case now waits for an executor pod to exist before suspending and, after the app reaches Suspended, asserts that no pod labelled sparkoperator.k8s.io/app-name=spark-pi remains. On master that assertion fails because the executor pod survives the driver deletion; with this change the executors are cascade-deleted with the driver. Could you take another look once the e2e matrix has run?

[RobuRishabh]

Thanks @KR-Ravindra for the contribution.
/lgtm

[vikas-saxena02]

I'm thinking we might be able to add assertions here to test the change.

spark-operator/test/e2e/sparkapplication_test.go

Lines 456 to 461 in cfc9cba

	By("Waiting for SparkApplication to Suspended")
	Eventually(func(g Gomega) {
	app := &v1beta2.SparkApplication{}
	g.Expect(k8sClient.Get(ctx, key, app)).To(Succeed())
	g.Expect(app.Status.AppState.State).To(Equal(v1beta2.ApplicationStateSuspended))
	}).WithTimeout(3 * time.Minute).Should(Succeed())

For the use cases I'm thinking we do not really have a clearly defined one: #2670 (comment). We can always revisit if we do see actual use cases that require this owner reference.

@vjanelle @vikas-saxena02 Would appreciate further thoughts on this.

Agree with dropping the reference — I think this is the right direction, not just the
expedient one, and I'd be happy to see this merge as-is.

Executors already carry the owner reference Spark itself sets (driver pod,
controller: true). #2670 added a second edge the operator invented, and since GC only
reaps a dependent once every owner is gone, that edge structurally prevents any partial
cascade. Removing it puts us back on the upstream ownership graph, which is the one Spark
maintains for us rather than one we have to keep correct ourselves.

Worth noting the fix is wider than the reported bug: every path that removes the driver
without removing the SparkApplication was leaking executors — suspend/Kueue preemption,
restartPolicy retries, and node-lifecycle deletion of the driver pod.

@tariq-hasan there are three things I'd like on the record, but none of them are things I
think we should load onto this PR — @KR-Ravindra has already turned around two rounds
quickly and each of these needs research beyond the change itself. Your call whether they
become follow-up issues or you'd rather fold any of them in here:

1. Orphan-policy deletes. After this change, an orphan-policy delete of the driver
   (kubectl delete pod <driver> --cascade=orphan, or a controller doing the same) leaves
   executors with no owner at all, where previously the SparkApplication reference would
   still reap them. Niche, and I'd accept it as the cost of the fix. If it ever surfaces,
   the mitigation is a finalizer-based sweep on SparkApplication deletion keyed on
   sparkoperator.k8s.io/app-name, which the operator already tracks executors by.

2. Kueue confirmation. Set ControllerReference on driver pod and non-Controller OwnerReference on executor pod at submission time #2670 was merged for Kueue specifically, so I'd rather have
   that closed out by the Kueue side on SparkApplication framework: PodsReady ignores executor state, and Stop leaves orphan executor pods across requeue cycles kubernetes-sigs/kueue#11675 than inferred from
   their source. FindAncestorJobManagedByKueue walking GetControllerOf reads correctly
   to me, but that's a cross-project conversation and shouldn't be a contributor's job to
   chase.

3. A GC-specific e2e case. The assertion added to suspend/resume does catch the
   current bug, but it passes whether executors were removed by GC cascade or by
   operator-managed deletion — so if the suspend path is ever reimplemented as explicit
   cleanup, it stays green while this regresses silently. A dedicated case (submit, wait
   for executors Running, delete only the driver pod, Eventually assert executors
   gone) pins the semantics Executor pods are not cascade-deleted when driver pod is deleted #2961 is actually about.

Happy to take (1) and (2) as follow-ups myself if that's useful.

[KR-Ravindra]

@vikas-saxena02 thanks, that matches my understanding of the ownership graph. On (3): agreed, the suspend assertion cannot tell a GC cascade from explicit cleanup. I can add the dedicated case (submit, wait for executors Running, delete only the driver pod, Eventually assert the executors are gone) to this PR or as a follow-up PR after this merges, whichever @tariq-hasan prefers. Taking (1) and (2) as follow-up issues sounds right to me.

[vjanelle]

An orphan cascade delete intentionally means ownership is being broken, do we need to handle that?

…

On Wed, Sep 9, 2026 at 5:07 PM KR Ravindra ***@***.***> wrote: *KR-Ravindra* left a comment (kubeflow/spark-operator#3152) <#3152 (comment)> @vikas-saxena02 <https://github.com/vikas-saxena02> thanks, that matches my understanding of the ownership graph. On (3): agreed, the suspend assertion cannot tell a GC cascade from explicit cleanup. I can add the dedicated case (submit, wait for executors Running, delete only the driver pod, Eventually assert the executors are gone) to this PR or as a follow-up PR after this merges, whichever @tariq-hasan <https://github.com/tariq-hasan> prefers. Taking (1) and (2) as follow-up issues sounds right to me. — Reply to this email directly, view it on GitHub <#3152?email_source=notifications&email_token=AAFTDSNA4J7NXUL6DRLKLN35OHWELA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNRRGA2TINJTG4Z2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5610545373>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFTDSK5MGZGOJH2YGCMDE35OHWELAVCNFSNUABFKJSXA33TNF2G64TZHMYTCNRRGY2TCOBYHNEXG43VMU5TKMZYGEZDEMBZGUZKC5QC> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[KR-Ravindra]

@vjanelle I read it the same way. --cascade=orphan is an explicit request to strip the dependents' owner references, so executors ending up ownerless is the documented outcome of that call, and it is also what happened before #2670, when executors only carried the driver reference Spark sets. I don't think this PR should special-case it. If the label-based sweep vikas described is wanted later, it fits a follow-up.

[vjanelle]

Also consider clusters with 10s of thousands of executors, if it's a one time scan maybe, but not continuous.

[KR-Ravindra]

@vjanelle agreed. If that sweep is ever built it should be a one-time pass triggered by the SparkApplication deletion (finalizer), listing by the sparkoperator.k8s.io/app-name label once, not a continuous scan. Nothing in this PR adds any scanning; it only removes the owner reference.

[vikas-saxena02]

An orphan cascade delete intentionally means ownership is being broken, do we need to handle that?
…
On Wed, Sep 9, 2026 at 5:07 PM KR Ravindra @.> wrote: KR-Ravindra left a comment (kubeflow/spark-operator#3152) <#3152 (comment)> @vikas-saxena02 https://github.com/vikas-saxena02 thanks, that matches my understanding of the ownership graph. On (3): agreed, the suspend assertion cannot tell a GC cascade from explicit cleanup. I can add the dedicated case (submit, wait for executors Running, delete only the driver pod, Eventually assert the executors are gone) to this PR or as a follow-up PR after this merges, whichever @tariq-hasan https://github.com/tariq-hasan prefers. Taking (1) and (2) as follow-up issues sounds right to me. — Reply to this email directly, view it on GitHub <#3152?email_source=notifications&email_token=AAFTDSNA4J7NXUL6DRLKLN35OHWELA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNRRGA2TINJTG4Z2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5610545373>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFTDSK5MGZGOJH2YGCMDE35OHWELAVCNFSNUABFKJSXA33TNF2G64TZHMYTCNRRGY2TCOBYHNEXG43VMU5TKMZYGEZDEMBZGUZKC5QC . You are receiving this because you were mentioned.Message ID: @.>

I may be overthinking but it could be due user mistakes, happy to drop this if other think the same way!!

[KR-Ravindra]

@vikas-saxena02 works for me. If a real user-mistake case turns up later, the one-time label-based sweep is the fix and can go in as a follow-up.

[vikas-saxena02]

@vikas-saxena02 works for me. If a real user-mistake case turns up later, the one-time label-based sweep is the fix and can go in as a follow-up.

I actually had this at a client side when one of my guys, did so based on chatgpt advise :D

[vikas-saxena02]

@KR-Ravindra the changes look good to me

[vara-bonthu]

/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vara-bonthu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [vara-bonthu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment