feat: garbage-collect terminated SparkApplications via operator default TTL by dineshkumar181094 · Pull Request #3042 · kubeflow/spark-operator

dineshkumar181094 · 2026-07-13T20:03:59Z

Purpose of this PR

Terminated SparkApplication objects (COMPLETED/FAILED) are only garbage-collected when the user sets spec.timeToLiveSeconds. When that field is unset, the object — and its driver pod could — linger indefinitely. This is a real hazard when driver pods carry sidecars (logging agents, service-mesh proxies) that do not reliably exit after the primary Spark container finishes: at scale the lingering pods cause pod-IP exhaustion and resource crunch.

This PR adds an operator-configurable default TTL that is applied to terminated SparkApplications only when the user has not set their own timeToLiveSeconds. It is applied purely as a runtime fallback for the cleanup decision — the user's object is never mutated — and it is off by default, gated behind a new alpha feature gate.

Proposed changes:

Add a new alpha feature gate DefaultTimeToLive (default false).
Add a --default-time-to-live-seconds controller flag (default 0 = disabled) plumbed through to the reconciler Options.
Add util.EffectiveTimeToLiveSeconds, which resolves the TTL governing cleanup: user spec wins; else the operator default when the gate is enabled and the value is > 0; else nil ("never expire"). The pure util.IsExpired is left unchanged.
Use the resolved effective TTL in reconcileTerminatedSparkApplication (applies to both COMPLETED and FAILED), with an info log emitted when the operator default is the source.
Expose the setting via the Helm chart: controller.defaultTimeToLiveSeconds value, a DefaultTimeToLive entry in controller.featureGates, the rendered flag in the controller deployment, chart unit tests, and regenerated chart docs.

Change Category

Feature (non-breaking change which adds functionality)
Documentation update

Rationale

The behavior is opt-in and non-destructive: an explicit user TTL always wins, the object's spec is never rewritten, and two independent guards (feature gate enabled and a positive value) must both hold for the fallback to activate. This makes the upgrade path a no-op for existing clusters while giving operators a cluster-wide safety net against resource exhaustion.

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

The feature gate follows the repo's existing alpha→beta→GA promotion model in pkg/features.
Tests: unit coverage for EffectiveTimeToLiveSeconds, a controller integration test verifying deletion of a terminated app past the default TTL (gate on, no spec TTL), and Helm chart tests asserting the flag renders only when the value is set.

Signed-off-by: dineshkumar181094 <dineshkumar181094@gmail.com>

…lications Signed-off-by: dineshkumar181094 <dineshkumar181094@gmail.com>

Signed-off-by: dineshkumar181094 <dineshkumar181094@gmail.com>

google-oss-prow · 2026-07-13T20:04:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andreyvelich for approval. For more information see the Kubernetes Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

Copilot

Pull request overview

Adds an opt-in, operator-configurable default TTL that applies to terminated SparkApplications only when the user has not set spec.timeToLiveSeconds, guarded by a new alpha feature gate.

Changes:

Introduces DefaultTimeToLive feature gate and a --default-time-to-live-seconds controller flag plumbed into reconciler options.
Adds util.EffectiveTimeToLiveSeconds and switches terminated-app cleanup to use the resolved “effective TTL”, with logging when the operator default is applied.
Exposes the configuration through the Helm chart (values, rendered args, chart tests, and README docs).

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pkg/util/sparkapplication.go	Adds effective TTL resolver used for cleanup decisions (now feature-gate aware).
pkg/util/sparkapplication_test.go	Adds unit tests for effective TTL resolution behavior.
pkg/features/features.go	Registers new `DefaultTimeToLive` alpha feature gate.
pkg/features/features_test.go	Verifies the new gate is registered and off by default.
internal/controller/sparkapplication/controller.go	Applies effective TTL during terminated-app reconciliation and deletion scheduling.
internal/controller/sparkapplication/controller_test.go	Adds integration test for deletion via operator default TTL.
cmd/operator/controller/start.go	Adds `--default-time-to-live-seconds` flag and wires it into reconciler options.
charts/spark-operator-chart/values.yaml	Adds chart value and feature gate entry for default TTL.
charts/spark-operator-chart/tests/controller/deployment_test.yaml	Adds chart unit tests for flag rendering behavior.
charts/spark-operator-chart/templates/controller/deployment.yaml	Renders `--default-time-to-live-seconds` controller arg based on values.
charts/spark-operator-chart/README.md	Documents the new chart value and feature gate entry.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


 	return false
 }

+// EffectiveTimeToLiveSeconds returns the TTL (in seconds) that should govern cleanup


+			app := &v1beta2.SparkApplication{}
+			if err := k8sClient.Get(ctx, key, app); err != nil && errors.IsNotFound(err) {
+				app = &v1beta2.SparkApplication{
+					ObjectMeta: metav1.ObjectMeta{Name: appName, Namespace: appNamespace},
+					Spec: v1beta2.SparkApplicationSpec{
+						MainApplicationFile: ptr.To("local:///dummy.jar"),
+					},
+				}
+				v1beta2.SetSparkApplicationDefaults(app)
+				Expect(k8sClient.Create(ctx, app)).To(Succeed())
+
+				app.Status.AppState.State = v1beta2.ApplicationStateCompleted
+				app.Status.TerminationTime = metav1.NewTime(time.Now().Add(-2 * time.Minute))
+				Expect(k8sClient.Status().Update(ctx, app)).To(Succeed())
+			}


+        {{- if .Values.controller.defaultTimeToLiveSeconds }}
+        - --default-time-to-live-seconds={{ .Values.controller.defaultTimeToLiveSeconds }}
+        {{- end }}


Address code review feedback on the operator default-TTL feature: - Unify expiry into util.IsExpiredWithTTL, the single primitive that takes an explicit (possibly defaulted) TTL; IsExpired delegates to it. Treat a non-positive spec.timeToLiveSeconds as 'expire immediately' everywhere, so an explicit user value always wins over the operator default and behavior is unchanged from before this feature. - Move the expiry decision out of the controller into util; the reconciler now calls util.IsExpiredWithTTL with the effective TTL. - Fix a flaky controller test: drive the app into COMPLETED with a past TerminationTime unconditionally (get-or-create) so the precondition holds even when the object already exists. - Render --default-time-to-live-seconds only when the value is > 0 so a negative value no longer emits the flag; add a chart test for it. Signed-off-by: dineshkumar181094 <dineshkumar181094@gmail.com>

dineshkumar181094 · 2026-07-14T06:39:05Z

@RobuRishabh / @Tabrizian can you please take a look at this feature.

dineshkumar181094 · 2026-07-16T16:27:40Z

@nabuskey can you please take a look at this feature and this pr.

tariq-hasan

Hi @dineshkumar181094! I have added a few comments.

tariq-hasan · 2026-07-17T07:59:27Z

+  # -- Default Time-To-Live (in seconds) applied to terminated SparkApplications that do not
+  # set spec.timeToLiveSeconds. Requires the DefaultTimeToLive feature gate to be enabled.
+  # 0 (default) disables it.
+  defaultTimeToLiveSeconds: 0


Would there be some way to link the dependency between controller.defaultTimeToLiveSeconds and the DefaultTimetoLive entry in the featuresGates list?

At this time passing defaultTimeToLiveSeconds while having the feature turned off is essentially a silent no-op on the Helm install (as well as controller startup).

Maybe we can use the pattern from the REST submitter service here as well.

Valid point I have update the helm chart and the controller code as well to handle this case.

tariq-hasan · 2026-07-17T08:07:51Z

 			if features.Enabled(features.RestSubmitter) {
 				if submitterServiceURL == "" {
 					return fmt.Errorf("--submitter-service-url is required when RestSubmitter feature gate is enabled")
 				}
 				if submitterMaxRetries < 1 {
 					return fmt.Errorf("invalid value %d for --submitter-max-retries, must be at least 1", submitterMaxRetries)
 				}
 				if submitterTLSEnabled {
 					if submitterTLSCertFile == "" || submitterTLSKeyFile == "" || submitterTLSCAFile == "" {
 						return fmt.Errorf("--submitter-tls-enabled requires --submitter-tls-cert-file, --submitter-tls-key-file, and --submitter-tls-ca-file")
 					}
 				}
 			}


Similar to how we do it for the REST submission flags do we want to have validation for the operator default TTL - erroring when the flag is non-positive but the feature gate is on and logging a warning at the very least when the flag is set but the feature gate is turned off?

Added a soft warning if user turn on the feature but feature is disabled.

tariq-hasan · 2026-07-17T08:58:29Z

+		logger.Info("Applying operator default TTL to terminated SparkApplication",
+			"ttlSeconds", *effectiveTTLSeconds, "state", app.Status.AppState.State)
+	}


I'm thinking probably better to move this logging to when the actual deletion occurs so it does not need to fire on every reconcile of the terminated app.

Indeed move the log line into the actual deletion logic.

tariq-hasan · 2026-07-17T11:16:26Z

 	// Otherwise, requeue the application after (TTL - survival) seconds.
 	return ctrl.Result{RequeueAfter: ttl - survival}, nil
 }



The terminal reconciliation loop is now using a repurposed concept of effective TTL - either user-provided or operator-defaulted - but the event filter is still using just the user-provided TTL if there is one.

Should we update the event filter as well to be consistent on the changes?

Yeah Now the Expired definition has changed. That part in event filter was considering no-op in case the app version is same and app has not expired and no retry just to avoid reconciliation.
But now the app expired definition has changed with the default operator side it make sense to update there so if the user has not put ttl and operator is applying controller side ttl and it should not result in no-op and start the deletion logic instead.

Address further review feedback on the operator default-TTL feature: - Gate the Helm-rendered --default-time-to-live-seconds flag on both the DefaultTimeToLive feature gate (new spark-operator.defaultTimeToLive.enabled helper, mirroring the RestSubmitter one) and a positive value, so the chart no longer emits a flag the controller would ignore at runtime. - Validate the flag at controller startup: error in PreRunE when the gate is enabled but the value is non-positive, and warn (after the logger is set up) when the value is set but the gate is disabled. - Log the operator-default provenance only when the app is actually deleted, instead of on every reconcile of a terminated app. - Make the event filter's resync guard use the effective TTL so an app expired via the operator default is re-admitted for deletion, consistent with the controller's cleanup decision; thread defaultTimeToLiveSeconds through the event filter and add coverage. Signed-off-by: dineshkumar181094 <dineshkumar181094@gmail.com>

dineshkumar181094 · 2026-07-17T15:32:35Z

Hi @dineshkumar181094! I have added a few comments.

@tariq-hasan Thank you for review all points are valid addressed all points.

dineshkumar181094 added 5 commits July 13, 2026 22:26

feat: add DefaultTimeToLive alpha feature gate

f62e891

Signed-off-by: dineshkumar181094 <dineshkumar181094@gmail.com>

feat: add EffectiveTimeToLiveSeconds TTL resolution helper

b22ea35

Signed-off-by: dineshkumar181094 <dineshkumar181094@gmail.com>

feat: add --default-time-to-live-seconds controller flag

a09851b

Signed-off-by: dineshkumar181094 <dineshkumar181094@gmail.com>

feat: apply operator default TTL when reconciling terminated SparkApp…

e970edd

…lications Signed-off-by: dineshkumar181094 <dineshkumar181094@gmail.com>

feat: expose default TTL via Helm chart

c1a6742

Signed-off-by: dineshkumar181094 <dineshkumar181094@gmail.com>

Copilot AI review requested due to automatic review settings July 13, 2026 20:04

google-oss-prow Bot requested review from ImpSy and RobuRishabh July 13, 2026 20:04

google-oss-prow Bot added the size/L label Jul 13, 2026

Copilot started reviewing on behalf of dineshkumar181094 July 13, 2026 20:04 View session

dineshkumar181094 changed the title ~~Feat/default ttl~~ feat: garbage-collect terminated SparkApplications via operator default TTL Jul 13, 2026

Copilot AI reviewed Jul 13, 2026

View reviewed changes

dineshkumar181094 marked this pull request as draft July 13, 2026 20:22

google-oss-prow Bot added the do-not-merge/work-in-progress label Jul 13, 2026

dineshkumar181094 marked this pull request as ready for review July 14, 2026 06:38

google-oss-prow Bot removed the do-not-merge/work-in-progress label Jul 14, 2026

tariq-hasan reviewed Jul 17, 2026

View reviewed changes

Uh oh!

Conversation

dineshkumar181094 commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this PR

Change Category

Rationale

Checklist

Additional Notes

Uh oh!

google-oss-prow Bot commented Jul 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

dineshkumar181094 commented Jul 14, 2026

Uh oh!

dineshkumar181094 commented Jul 16, 2026

Uh oh!

tariq-hasan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dineshkumar181094 commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dineshkumar181094 commented Jul 13, 2026 •

edited

Loading