Skip to content

Conversation

jbhalodia-slack
Copy link
Contributor

@jbhalodia-slack jbhalodia-slack commented Feb 14, 2025

Purpose of this PR

Proposed changes:

  • If using a non-standard container name for Spark driver or executor container, assume that the first container in the list is the Spark container, which is what Spark assumes. See Rationale section for further explanation.

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

Current implementation only supports standard Spark container names. Driver container name must be spark-kubernetes-driver and executor container name must be spark-kubernetes-executor or executor. However, Pod template files can define multiple containers, and you can use the spark properties spark.kubernetes.driver.podTemplateContainerName and spark.kubernetes.executor.podTemplateContainerName to indicate which container should be used as a basis for the driver or executor. If not specified, or if the container name is not valid, Spark will assume that the first container in the list will be the driver or executor container [Source].

podTemplateContainerName config could be specified via Spec.SparkConf or in spark-defaults.conf, and in the webhook it's hard to determine if podTemplateContainerName is set via any one of the possible Spark configuration methods, so in the Webhook it is better to assume that if standard Spark container name is not found, the first container in the list is the driver or executor container.

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

@jbhalodia-slack
Copy link
Contributor Author

@ChenYi015 @jacobsalway @vara-bonthu Can I get a PR review?

Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Copy link
Contributor

[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

@google-oss-prow google-oss-prow bot merged commit 79264a4 into kubeflow:master Feb 20, 2025
16 checks passed
ChenYi015 pushed a commit that referenced this pull request Mar 21, 2025
Signed-off-by: jbhalodia-slack <[email protected]>
(cherry picked from commit 79264a4)
@ChenYi015 ChenYi015 mentioned this pull request Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants