added cleanup k8s service *-ui-svc (#2984)#2985
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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves SparkApplication termination cleanup to prevent orphaned UI/driver-ingress Services (and their Kubernetes-injected env vars) from lingering, while also preserving operator-required labels when users provide additional labels.
Changes:
- Merge user-provided driver-ingress Service labels into operator labels instead of replacing them.
- On termination, delete Spark UI Service/Ingress and driver-ingress Services to avoid resource/env-var leakage (#2984).
- Add controller tests asserting Services are deleted for Completed/Failed applications.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| internal/controller/sparkapplication/driveringress.go | Merge user labels into existing Service labels to preserve operator labels. |
| internal/controller/sparkapplication/controller.go | Add cleanup + validation for UI/ingress + driver-ingress Services and fallback UI Service deletion name. |
| internal/controller/sparkapplication/controller_test.go | Add tests ensuring UI + driver-ingress Services are removed on terminal states. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Merge any user-provided labels on top of the operator labels rather than replacing | ||
| // them, so that the SparkApplication name label that owner-reference GC, selectors and | ||
| // cleanup rely on is always preserved. | ||
| for key, value := range serviceLabels { | ||
| service.Labels[key] = value |
| if err := r.client.DeleteAllOf( | ||
| ctx, | ||
| &corev1.Service{}, | ||
| client.InNamespace(app.Namespace), | ||
| client.MatchingLabels(map[string]string{common.LabelSparkAppName: app.Name}), | ||
| client.GracePeriodSeconds(0), | ||
| ); err != nil && !errors.IsNotFound(err) { | ||
| return err |
There was a problem hiding this comment.
SparkAppName are generated. Little/No chance to conflict with other services on delete
| // Validate whether the driver-ingress services have been deleted. Their names are not | ||
| // recorded in the status, so they are matched by the SparkApplication name label. | ||
| services := &corev1.ServiceList{} | ||
| if err := r.client.List(ctx, services, | ||
| client.InNamespace(app.Namespace), | ||
| client.MatchingLabels(map[string]string{common.LabelSparkAppName: app.Name}), | ||
| ); err != nil { | ||
| return false | ||
| } | ||
| if len(services.Items) > 0 { | ||
| return false | ||
| } |
| // Fall back to the deterministic default name so the service is not orphaned if the | ||
| // status update that recorded WebUIServiceName was lost (e.g. failed right after submission). | ||
| if svcName == "" { | ||
| return nil | ||
| svcName = util.GetDefaultUIServiceName(app) | ||
| } |
| Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, &corev1.Service{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: uiServiceKey.Name, Namespace: appNamespace}, | ||
| }))).To(Succeed()) |
|
I did use Claude with the following prompt: Then interactively answered serveral times "yes" for security action executions, I am NOT a "go" developper, and I do not now enough on the internals of this project "spark-operator" |
|
Here is the full conversation thinking result text from Claude Code ╭─── Claude Code v2.1.169 ─────────────────────────────────────────────────────────────────────────────────────────────╮ Fable 5 is now available with the latest version of Claude Code! Run "claude update" to update to v2.1.170+ ▎ Opus 4.8 is here! Now defaults to high effort · /effort xhigh for your hardest tasks ❯ analyze the creation and deletion of kubernetes "service" objects from this kubernetes operator for Crd of "SparkApplication". In particular, see issue #2984, and suggest how to implement the code that cleanup correctly resources when the sparkApplication is COMPLETED or FAILED I'll analyze the Service object lifecycle in this Spark operator. Let me start by examining the issue and the relevant code. Searched for 1 pattern Searched for 2 patterns Let me read the key functions: deleteSparkReand theGetDefaultUIServiceName/GetOwnerReference util functions. Read 2 files Now I have the core picture. The key issue: deleteWebUIService relies on app.Status.DriverInfo.WebUIServiceName being Searched for 1 pattern, read 1 file Now let me look at the critical section — where the UI service is created (around line 340-375) and how the status is Read 1 file I found a key part of the bug. Notice at line 377-394: driver ingress services created from DriverIngressConfiguration Read 1 file This confirms the core bug. Look at lines 569-605 (reconcileSucceedingSparkApplication) and 607-649
So on the terminal COMPLETED/FAILED states, y happens later — let me confirm by checkingthe COMPLETED/FAILED reconcile handlers (line ~760) and the dispatcher. Let me also look at Read 1 file Now the key function: cleanUpOnTermination (ead it and deleteDriverIngressResources (line ~1273-1310). Read 1 file Now let me find cleanUpOnTermination: Searched for 1 pattern This is the smoking gun. cleanUpOnTermination only does batch-scheduler cleanup — it does NOT delete the web UI The only mechanism keeping them from leaking is the Kubernetes OwnerReference garbage collection (set in Let me verify there's no MatchingLabels-basehe driver pod IS being deleted on termination (to understand why the -svc gets cleaned but -ui-svc does not, per the reporter). Let me look at what deletes the Searched for 1 pattern, read 1 file My analysis is confirmed. deleteSparkResourcrvice + deleteWebUIIngress) is invoked atlines 296 (CR deletion), 437 (failed-submission retry), 550 (invalidating), 585 (succeeding-with-retry), 631 I now have a complete and precise understanding. Let me give the analysis and the concrete implementation Analysis: Service lifecycle in the SparkAppl How Services are created When a SparkApplication transitions to SUBMIApplication (controller.go:332) creates up to three kinds of Service objects: ┌───────────────────────────┬─────────────────────────────────────────────┬────────────────────────────────────────┐ Every Service is created with an OwnerReference to the SparkApplication (GetOwnerReference, sparkapplication.go:279, How Services are supposed to be deleted deleteSparkResources (controller.go:1159) de UI service (deleteWebUIService:1200), andthe web UI ingress (deleteWebUIIngress:1224). Crucially, deleteWebUIService deletes by name from status: svcName := app.Status.DriverInfo.WebUIServiceName The bug (issue #2984)
func (r *Reconciler) cleanUpOnTermination(ctx context.Context, _, newApp *v1beta2.SparkApplication) error { So for a completed/failed app that is kept afires), the only thing that ever removes the-ui-svc is Kubernetes owner-reference garbage collection — which fires only when the SparkApplication CR itself is
▎ Note on the reporter's observation that -snot: the driver headless service *-svc iscreated by the Spark driver itself (spark-submit / Kubernetes scheduler backend) and is owned by the driver pod, so Recommended implementation The cleanest fix is to make terminal cleanup actually delete the network resources, and make deletion robust to a A. Delete UI service + ingress + driver-ingrExtend cleanUpOnTermination (the only placethe terminal path cleans up): func (r *Reconciler) cleanUpOnTermination(ctx context.Context, _, newApp *v1beta2.SparkApplication) error { ▎ I deliberately do not add the driver-pod dmpleted/failed driver pod is intentional(logs/inspection, and it's governed separately). Only the network objects that cause the leak are removed. B. Make deletion label-based so it works even when status is empty. The current name-from-status approach silently func (r *Reconciler) deleteWebUIService(ctx context.Context, app *v1beta2.SparkApplication) error { And add the missing cleanup for DriverIngres no recorded name, so they must be deleted by label): func (r *Reconciler) deleteDriverIngressServices(ctx context.Context, app *v1beta2.SparkApplication) error { (Driver-ingress services are created with GetResourceLabels → LabelSparkAppName, so this matches them; the -ui-svc C. Record the driver-ingress service names in status (optional but recommended) so validateSparkResourceDeletion Things to verify / decide
Would you like me to:
I'd recommend option 2 — it pins the regressceed. ✻ Brewed for 1m 55s ※ recap: Goal: fix issue #2984, where Spark UIpplication completes or fails. I've finishedanalyzing the code and identified the cause. Next action: implement the cleanup fix, ideally starting with a failing ❯ add a failing test first then implement A B Let me examine the existing test suite to ma failing test and implement A/B/C. Read 1 file Let me look at the test that handles the COMhe helpers (createDriverPod, etc.), plus anexisting test that reconciles a completed app, to match the pattern exactly. Read 1 file Let me see the helper functions (createDriverPod, getDriverNamespacedName, createExecutorPod) at the end of the test Read 1 file Now let me check how the reconcile dispatches by state — to confirm a COMPLETED app routes to Searched for 1 pattern Confirmed: COMPLETED → reconcileCompletedSpainatedSparkApplication → cleanUpOnTermination (no service deletion). Now let me write the failing test. I'll add a new Context for reconciling a Completed SparkApplication that has a UI Searched for 1 pattern Good. With no TimeToLiveSeconds, the completed app won't be deleted/expired, so we exercise exactly the cleanup path. Read 1 file Now I'll add the failing test. I'll insert a new Context block before the final }) of the top-level Describe (line Read 1 file Now let me look at the common label constants to construct the services with proper labels, and confirm the UI service Searched for 1 pattern Now I'll write the failing test. I'll add a and a driver-ingress service with ownerreference, then a Context that reconciles a completed app and expects both services + ingress to be gone. ● Update(internal\controller\sparkapplication\controller_test.go) Context("When reconciling a terminated SparkApplication with a Spark UI service", func() { ound)) ound)) func getDriverNamespacedName(appName string, appNamespace string) types.NamespacedName { Now let me run the new test to confirm it fails (the bug exists). Let me check how tests are run in this repo first. Searched for 1 pattern These ginkgo controller tests require an envtest binary (KUBEBUILDER_ASSETS). Let me check whether envtest is already Ran 1 shell command No envtest binaries and no go on PATH in this installed somewhere on this Windowsmachine. Ran 1 shell command Go isn't installed (or not in a standard location) on this machine, and envtest binaries aren't present — so I can't Let me confirm NewReconciler returns *Reconciler (exported) so my newReconciler helper compiles. Searched for 1 pattern, read 1 file NewReconciler returns *Reconciler — my helper is correct. The test is sound. Now let me implement A/B/C. First, A + B: modify cleanUpOnTermination and deleteWebUIService, and add Now implementing B — make deleteWebUIService fall back to the deterministic name, and add deleteDriverIngressServices: ● Update(internal\controller\sparkapplication\controller.go) return nil // deleteDriverIngressServices delions services created for the func (r *Reconciler) deleteWebUIIngress(ctx context.Context, app *v1beta2.SparkApplication) error { Now A — wire the deletions into cleanUpOnTermination: ● Update(internal\controller\sparkapplication\controller.go) // Delete the network resources (Spark UI service/ingress and driver-ingress services) that Now C — record the driver-ingress service names in status so validateSparkResourceDeletion can verify them, and so Searched for 1 pattern For C, adding a new status field would require an API change + make generate + CRD regeneration — heavier than needed, Let me update validateSparkResourceDeletion to also verify that no driver-ingress services remain. Read 1 file ● Update(internal\controller\sparkapplication\controller.go) // Validate whether the driver-ieted. Their names are not return true Now let me verify common is already imported in controller.go (it's used extensively, e.g. common.LabelSparkAppName Searched for 1 pattern common is imported, and client.InNamespace/MatchingLabels/GracePeriodSeconds are all already used (lines 1116, plnew code). Lines 1237-1239 are my deleteDriv326 are the validation List. Let me also verify the driver-ingress servickAppName. They're created viacreateDriverIngressService with Labels: util.GetResourceLabels(app) (driveringress.go:339), and GetResourceLabels sets Read 1 file This is an important edge case: if a user sets custom serviceLabels on a DriverIngressConfiguration (or This is a pre-existing latent issue (the label that owner-GC and selectors rely on gets clobbered), but it directly Read 1 file ● Update(internal\controller\sparkapplication\driveringress.go) if len(serviceLabels) != 0 { if len(serviceAnnotations) != 0 { Now there's a test in driveringress_test.go that may assert the old replace-behavior. Let me check. Searched for 1 pattern Let me look at how this test verifies the actual created Service object's labels — specifically whether it asserts Read 1 file The test reads ingressConfig.serviceLabels — that's the SparkService.serviceLabels field, which is populated from the Now let me re-read my full set of changes to confirm correctness and that the new test will pass. Let me view the Read 1 file All changes are in place. Now let me try to un. Let me check WSL/bash since theenvironment lists Bash as available. Ran 1 shell command Go is not available in this environment (neither PowerShell nor Bash), and there are no envtest binaries, so I cannot Let me verify there are no leftover/unused rriverIngressServiceKey is used in both Itblocks — yes; uiServiceKey populated in reconcileTerminatedState and used in assertions/AfterEach — yes). One thing to double check: in AfterEach, I delete leftover services by name even on the happy path where they're Let me also verify the test file compiles co1beta2.ApplicationState is a struct with aState field (used in reconcileTerminatedState(state v1beta2.ApplicationState) then state.State). Searched for 1 pattern ApplicationState{State: ...} is correct. Actually, my helper takes a whole ApplicationState just to use .State — let ● Update(internal\controller\sparkapplication\controller_test.go) ● Update(internal\controller\sparkapplication ● Update(internal\controller\sparkapplication\controller_test.go) ● Update(internal\controller\sparkapplication Let me confirm ApplicationStateType is the right type name (used above) and that the helper function name doesn't Searched for 1 pattern All correct. Let me do a final read of the ce sure it's coherent end-to-end. Read 1 file The test block is coherent. One small stale-nt says "then returns the key of the createdUI service" but it returns nothing. Let me correct it. ● Update(internal\controller\sparkapplication\controller_test.go) ● Update(internal\controller\sparkapplication\controller_test.go) Added 1 line, removed 1 line
C — verification: validateSparkResourceDeletarkAppName and fails if any service remains,so the retry-readiness check covers driver-ingress services too. I deliberately avoided a new status field (would Not yet verified Want me to stage these into a branch/commit, or adjust the DeleteAllOf selector to additionally scope by LabelSubmissionID for a tighter match? ✻ Crunched for 4m 21s |
tariq-hasan
left a comment
tariq-hasan
left a comment
There was a problem hiding this comment.
Hi @Arnaud-Nauwynck! I have added a few comments. Thanks!
|
|
||
| if err := r.deleteWebUIIngress(ctx, app); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
We should delete all the operator-managed services since validateSparkResourceDeletion validates that all of these have been removed. Otherwise we will have a deadlock in the reconciliation loop.
| if err := r.deleteWebUIIngress(ctx, app); err != nil { | |
| return err | |
| } | |
| if err := r.deleteDriverIngressServices(ctx, app); err != nil { | |
| return err | |
| } | |
| return nil |
| return nil | ||
| svcName = util.GetDefaultUIServiceName(app) | ||
| } | ||
| logger.Info("Deleting Spark web UI service", "service", svcName) |
There was a problem hiding this comment.
It's probably best to put a guard on the logging since this would be redundant for a terminated app that keeps being requeued until TTL.
|
@Arnaud-Nauwynck The commits need to be signed as well to pass the DCO check. Detailed instructions to pass the check: https://github.com/kubeflow/spark-operator/pull/2985/checks?check_run_id=80835640557. |
Purpose of this PR
This PR is for cleaning up the thousands of "*-ui-svc" kubernetes zombi services that still exist after a SparkApplication is COMPLETED or FAILED
Proposed changes:
Change Category
Rationale
Having clusters in PROD with thousands of SparkApplications launched every day produces blocking ERRORs "argument list too long".
It forced my company to implement strange workarounds: "automatic sparkApp remover", and split into multiple clusters and multiple namespaces ...
A better solution is highly recommended
Checklist
Additional Notes
Code was "pair-progamming" with Claude Code.