Suspend/Resume feature on SparkApplication

[everpeace]

Purpose of this PR

This PR implements Suspend/Resume operation on SparkApplication.

Proposed changes:

• Add Suspend: true|false field in SparkApplicationSpec
• State Transition:
  • Suspend: ** --Suspend: true--> SUSPENDING --> SUSPENDED (**=any non-Terminal Application State)
  • Resume: SUSPENDING | SUSPENDED --Suspend: false--> RESUMING --> SUBMITTED

Fixes #2290

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

Supporting Suspend/Resume feature could make Kueue integration easier.

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

[Kevinz857]

@everpeace Maybe u can resolve this conflict so that this PR can continue to move forward？

we also think about use kueue with sparkoperator , depend this suspend state feature . It would be better if you could push further down ~

[everpeace]

@Kevinz857 Hi, thanks for your comment!! I rebaed my PR.

[everpeace]

we also think about use kueue with sparkoperator , depend this suspend state feature . It would be better if you could push further down ~

My original SparkApplication integration PR in Kueue(kubernetes-sigs/kueue#4032) was actually closed because

• Kueue already has the design doc (Pod Groups: Driver contains information about remaining Pods kubernetes-sigs/kueue#1656) how to support spark workloads (PodGroups)
• my original PR does not follow the design doc.

However, I already opened a PR for this which will be needed to support SparkApplication integration in Kueue: kubernetes-sigs/kueue#4102.

[ChenYi015]

/assign @jacobsalway @vara-bonthu @ImpSy

[ChenYi015]

@Kevinz857 Would you mind help reviewing the PR?

[Kevinz857]

@Kevinz857 Would you mind help reviewing the PR?

@ChenYi015 Very happy, I will review this PR together in detail later.

[Kevinz857]

@everpeace Suggestions for Improvement

1. Controller logic abstraction
   Consider extracting the suspension check (if app.Spec.Suspend != nil && *app.Spec.Suspend) into a helper function like isSuspended(app) for clarity and potential reuse.
   Also, using a named constant for the event string ("SparkApplicationSuspended") would improve consistency and readability.

2. Pod termination check reusability
   The logic that checks for Spark driver/executor pod termination before resuming could be encapsulated in a helper function (e.g. areSparkPodsTerminated(app)) to make the code more modular and easier to test.
   Optionally, adding retry limits or exponential backoff can help avoid indefinite retries in corner cases.

3. Field documentation enhancement
   In sparkapplication_types.go, the Suspend field comment could be more descriptive, such as:

// Suspend indicates whether the SparkApplication should be suspended.
// When true, the controller skips submitting the Spark job.

4. Test enhancement ideas

Add unit tests for the suspension logic to ensure the controller skips job submission when .spec.suspend is true.
Consider validating whether the correct Kubernetes events (Suspended/Resumed) are emitted using a fake event recorder.

@ChenYi015 If have time, we can take a look together to see if these suggestions are suitable

@everpeace Or if there are any better suggestions, we can discuss them together

[google-oss-prow]

[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 ask for approval from jacobsalway. For more information see the Kubernetes Code Review Process.

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

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

[everpeace]

@Kevinz857 Thanks for your review.

Consider extracting the suspension check (if app.Spec.Suspend != nil && *app.Spec.Suspend) into a helper function like isSuspended(app) for clarity and potential reuse.

hmmm. SparkApplicationSpec.Suspend is not a pointer. So, I can't see any motivation for it.

spark-operator/api/v1beta2/sparkapplication_types.go

Line 44 in 4d6dd1d

Suspend bool `json:"suspend"`

Perhaps, you saw ScheduledSparkApplication's suspension check:

spark-operator/internal/controller/scheduledsparkapplication/controller.go

Lines 109 to 112 in 4d6dd1d

	if scheduledApp.Spec.Suspend != nil && *scheduledApp.Spec.Suspend {
	return ctrl.Result{}, nil
	}

Could you elaborate on your suggestion if I missed something?

Also, using a named constant for the event string ("SparkApplicationSuspended") would improve consistency and readability.

hmmm. I think I already did.

spark-operator/pkg/common/event.go

Lines 33 to 38 in 4d6dd1d

	EventSparkApplicationSuspending = "SparkApplicationSuspending"
	EventSparkApplicationSuspended = "SparkApplicationSuspended"
	EventSparkApplicationResuming = "SparkApplicationResuming"
	)

Could you elaborate on your suggestion if I missed something?

Field documentation enhancement
In sparkapplication_types.go, the Suspend field comment could be more descriptive, such as:

Thanks. Fixed in b3f8e8c

Test enhancement ideas
Add unit tests for the suspension logic to ensure the controller skips job submission when .spec.suspend is true.

I think it's already covered:

spark-operator/internal/controller/sparkapplication/controller_test.go

Lines 579 to 584 in 4d6dd1d

	When("reconciling a new SparkApplication with Suspend=True", func() {
	BeforeEach(func() {
	app.Spec.Suspend = true
	Expect(k8sClient.Create(ctx, app)).To(Succeed())
	})
	AfterEach(func() {

Could you elaborate on your suggestion if I missed something?

Consider validating whether the correct Kubernetes events (Suspended/Resumed) are emitted using a fake event recorder.

IIUC, the current SparkApplication unit tests(internal/controller/sparkapplication/controller_test.go) do not assert emitted events at all. Although I fully agree your suggestion(we should assert emitted events in unit tests), I think the enhancement should be done in another PR so that all the unit tests assert emitted events. Or, should this PR cover event assertions only for suspend/resume in this PR?? If so, I would be happy to handle it.

[Kevinz857]

@everpeace Thanks for your contribution !

@ChenYi015 I think the current submission can be merged. This commit will not have a destructive impact on the current core functionality.