feat: support driver and executor pod use different priority

[Kevinz857]

🛑 Important:

Please open an issue to discuss significant work before you start. We appreciate your contributions and don't want your efforts to go to waste!

For guidelines on how to contribute, please review the CONTRIBUTING.md document.

Purpose of this PR

This PR enhances the Spark Operator to support setting different priorities for driver and executor pods. This feature allows for more granular control over resource allocation and scheduling in Kubernetes clusters.

Proposed changes:

• Added support for specifying separate priorities for driver and executor pods in SparkApplication CRD
• Modified the controller logic to apply the specified priorities when creating driver and executor pods
• Updated relevant documentation to reflect the new feature

Change Category

Indicate the type of change by marking the applicable boxes:

• 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

In complex Kubernetes environments, there's often a need to prioritize Spark driver pods differently from executor pods. This change allows users to fine-tune their Spark application deployments, potentially improving overall cluster resource utilization and application performance.

Checklist

Before submitting your PR, please review the following:

• 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 feature introduces new fields in the SparkApplication CRD. Users will need to update their CRD definitions to take advantage of this new functionality. Backward compatibility is maintained for existing deployments.

[Kevinz857]

@agemooij @mhausenblas @ChenYi015 Pls help make a review, thank you

[ChenYi015]

/assign @vara-bonthu @jacobsalway @yuchaoran2011

[ChenYi015]

From my understanding, the priorityClassName in batchSchedulerOptions is utilized by Volcano's PodGroup, and it is unrelated to the priorityClass of Spark pods. Given this, it seems unnecessary to have the prioryClassSplitting option or the addDriverExecutorUnifyPriorityClassName function. WDYT?

[Kevinz857]

OK，prioryClassSplitting batchSchedulerOptions may be a bit inappropriate, let me clarify my usage scenario

The current business scenario has not yet used the ability of podGroup. The underlying K8s cluster is divided into multiple resource pools, and the SparkApplication task itself is defined with different priorities at the business level. Moreover, the resources of the underlying K8s cluster are limited. At this time, it is expected that high-priority executors can run first when resources are insufficient, so the PriorityClass priority preemption scheduling mechanism of K8s itself is used

But because the default PriorityClass of SparkApplication is in the batchSchedulerOptions, it means that the driver and executor under the same SparkApplication will be set to the same priority; but in the actual business scenario, there may be problems, such as we know that the driver is the controller of the executor, once the drvier node is preempted and expelled, the entire SparkApplication will fail, and the expected effect of the business is that the driver pod cannot be preempted with high priority, and the executor can be preempted with high priority, so that different priorities must be set for the driver and executor, such as setting the highest priority for the driver, and the executor can have a low priority, so that after the low-priority executor is expelled, it will wait for high priority After the level executor is executed and there are free resources, the executors of these previously expelled low-priority tasks will be created by the corresponding drvier pod

If this is the case, how should this priority be defined?

[ChenYi015]

@Kevinz857 Perhaps we can do as follows:

• If spec.batchSchedulerOptions.priorityClassName is defined, it will automatically be propagated to both driver and executor pods by default, as this is the current behavior so that we will not make a breaking change.
• Furthermore, if either spec.driver.priorityClassName or spec.executor.priorityClassName is specifically defined, it should take precedence over spec.batchSchedulerOptions.priorityClassName. In this way, we can eliminate the need to introduce spec.batchSchedulerOptions.prioryClassSplitting option as it is somewhat confusing. Also, users can define different priority classes for driver and executor pods.

[Kevinz857]

@ChenYi015 Very good suggestion, I can modify the code and resubmit MR according to this proposal.

[Kevinz857]

@ChenYi015 According to the latest proposal, if if either spec.driver.priorityClassName or spec.executor.priorityClassName is specifically defined, it should take precedence over spec.batchSchedulerOptions.priorityClassName.

Modified the code according to this proposal, please help review it when you have time, thanks

[ChenYi015]

Perhaps we could merge the logic of setPodPriorityClassName into addPriorityClassName.

[Kevinz857]

@ChenYi015 Already adapted merge the logic of setPodPriorityClassName into addPriorityClassName, thanks for ur suggestions

[ChenYi015]

@Kevinz857 Thanks for the contribution. LGTM. Will wait for another approval.
The CI workflow needs approval from a maintainer to run @vara-bonthu @yuchaoran2011
After the CI has run successfully, then this PR will be merged.
/hold

[jacobsalway]

I'll leave a better comment tomorrow on what the logic could look like, but just noting that unit tests aren't passing for me locally because of a nil pointer dereference on the app.Spec.{Driver|Executor}.PriorityClassName

[Kevinz857]

I'll leave a better comment tomorrow on what the logic could look like, but just noting that unit tests aren't passing for me locally because of a nil pointer dereference on the app.Spec.{Driver|Executor}.PriorityClassName

@jacobsalway Thanks for the prompt, the judgment of whether the pointer is nil has been added. Looking forward to better revision suggestions tomorrow

[ChenYi015]

I'll leave a better comment tomorrow on what the logic could look like.

@jacobsalway, please retake a look when you have time.

[ChenYi015]

@Kevinz857 The build-api-docs CI has failed, you need to make build-api-docs and commit the changes.

[Kevinz857]

Reading into how the Volcano PodGroup priority class interacts with priority classes set on the pods within that group. Maybe it makes sense for us to remove this conflict altogether by removing the propagation of the batch scheduler options priority class to the pod priority class

@jacobsalway Agree. The priority class of volcano pod group is unrelated to the ones of spark pods. But since this will cause a breaking change, we should denote it when we do the next release.

@ChenYi015 Removed the definition and reference of spec.batchSchedulerOptions priorityClassName, and unified the definition and use of priority by driver pod and executor pod. Pls have a look when u are free. Thanks a lot

[ChenYi015]

We need to add it back.

[ChenYi015]

Suggested change

	if util.IsDriverPod(pod) && *app.Spec.Driver.PriorityClassName != "" {
	priorityClassName = app.Spec.Driver.PriorityClassName
	} else if util.IsExecutorPod(pod) && *app.Spec.Executor.PriorityClassName != "" {
	priorityClassName = app.Spec.Executor.PriorityClassName
	}
	if util.IsDriverPod(pod) {
	priorityClassName = app.Spec.Driver.PriorityClassName
	} else if util.IsExecutorPod(pod) {
	priorityClassName = app.Spec.Executor.PriorityClassName
	}

This will simplify the code and avoid null pointer exception.

[vara-bonthu]

Thanks for the PR @Kevinz857 🙌🏼

/approve

@jacobsalway I know you left some comments. Are you happy with the PR?

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ChenYi015,vara-bonthu]

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

[ChenYi015]

/hold cancel
/lgtm