fix(spark): pass driver/executor pod specs through as the pod template - #7822
fix(spark): pass driver/executor pod specs through as the pod template#7822pingsutw wants to merge 2 commits into
Conversation
The custom driver/executor pod spec was merged onto the pod spec Flyte
builds. That merge matches containers by name, and the container Flyte
generates is named after the task execution ID, so any container the user
named something else (the operator's own spark-kubernetes-{driver,executor},
or "primary") was silently dropped. Setting primary_container_name to such a
container failed the task outright with "invalid TaskSpecification, container
[...] not defined".
Hand the pod spec to the spark operator verbatim as spec.{driver,executor}
.template instead, and drop the primary_container_name override. The operator
patches the template onto the container it generates, which is the container
users are actually naming.
Signed-off-by: Kevin Su <pingsutw@gmail.com>
Claude-Session: https://claude.ai/code/session_01SAATDRHXajHapuojwQ4SMR
There was a problem hiding this comment.
Pull request overview
This PR fixes Spark driver/executor pod customization by passing user-provided driver/executor PodSpecs directly to the Spark Operator via spec.{driver,executor}.template, instead of merging them into Flyte’s generated pod spec (which dropped container-level overrides due to unpredictable Flyte container naming).
Changes:
- Updated SparkApplication construction so
createSparkPodSpeccan use a separate custom pod spec for the pod template (falling back to Flyte’s generated pod spec when none is provided). - Removed the
primary_container_nameoverride path so user container naming no longer breaks task validation. - Added/updated unit tests to validate custom pod spec passthrough, legacy-CRD behavior when template support is absent, and affinity “dilution” behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| flyteplugins/go/tasks/plugins/k8s/spark/spark.go | Pass custom driver/executor pod specs through as Spark Operator pod templates; stop merging into Flyte’s pod spec. |
| flyteplugins/go/tasks/plugins/k8s/spark/spark_test.go | Add/adjust tests covering custom pod spec template passthrough and legacy/template-gating behaviors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Kevin Su <pingsutw@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
flyteplugins/go/tasks/plugins/k8s/spark/spark.go:270
- This uses pluginmachinery/utils.UnmarshalStructToObj, which is marked deprecated in flyteplugins/go/tasks/pluginmachinery/utils/marshal_utils.go. Consider switching to flytestdlib/utils.UnmarshalStructToObj instead and dropping the staticcheck suppression, so new code paths aren't built on deprecated helpers.
if executorPod.GetPodSpec() != nil {
err = utils.UnmarshalStructToObj(executorPod.GetPodSpec(), &customPodSpec) //nolint: staticcheck
if err != nil {
return nil, errors.Errorf(errors.BadTaskSpecification,
"Unable to unmarshal executor pod spec [%v], Err: [%v]", executorPod.GetPodSpec(), err.Error())
}
flyteplugins/go/tasks/plugins/k8s/spark/spark.go:186
- The comment says the custom driver/executor pod spec is passed through "verbatim", but the implementation intentionally applies some defaulting (e.g. EnableServiceLinks when unset). Consider tightening the wording to avoid implying a strict byte-for-byte passthrough.
// the operator treats explicit fields as overrides of the template. The user's driver/executor
// pod spec is passed through verbatim so the operator can patch it onto the container it
// generates (`spark-kubernetes-driver`/`spark-kubernetes-executor`); Flyte's own container
// names never match those, so merging it here would drop it on the floor.
flyteplugins/go/tasks/plugins/k8s/spark/spark.go:223
- This uses pluginmachinery/utils.UnmarshalStructToObj, which is marked deprecated in flyteplugins/go/tasks/pluginmachinery/utils/marshal_utils.go. Consider switching to flytestdlib/utils.UnmarshalStructToObj instead and dropping the staticcheck suppression, so new code paths aren't built on deprecated helpers.
This issue also appears on line 265 of the same file.
if driverPod.GetPodSpec() != nil {
err = utils.UnmarshalStructToObj(driverPod.GetPodSpec(), &customPodSpec) //nolint: staticcheck
if err != nil {
return nil, errors.Errorf(errors.BadTaskSpecification,
"Unable to unmarshal driver pod spec [%v], Err: [%v]", driverPod.GetPodSpec(), err.Error())
}
Why are the changes needed?
A Spark task that customizes its driver or executor pod cannot do so today, and in the
common case it fails outright:
createDriverSpec/createExecutorSpecmerged the custom pod spec onto the pod spec Flytebuilds with
MergeOverlayPodSpecOntoBase. That merge matches containers by name, anditerates the base containers only — the container Flyte generates is named after the task
execution ID (
<TaskAction name>-<attempt>in v2,container_helper.goBuildRawContainer),which no user can predict. So:
ephemeral-storagerequestabove never reaches the pod), and
primary_container_namethen overrode the primary container name, and the followingGetContainer(podSpec, primaryContainerName)failed the task.Naming the container after the operator's
spark-kubernetes-driver/spark-kubernetes-executor— the names the Spark operator itself documents for podtemplates — hits exactly the same wall.
What changes were proposed in this pull request?
Hand the custom pod spec to the Spark operator verbatim as
spec.{driver,executor}.templateinstead of merging it into Flyte's pod spec, and drop theprimary_container_nameoverride. The operator patches the template onto the container itgenerates, which is the container users are naming in the first place.
createSparkPodSpecnow takes the custom pod spec separately and uses it forspec.Template, falling back to the pod spec Flyte builds when the task sets no customdriver/executor pod (unchanged behavior for the vast majority of tasks).
SparkPodSpecfields (affinity, tolerations, node selector, image, env, …)are still derived from the pod spec Flyte builds, so platform scheduling constraints stay
intact — the operator treats explicit fields as overrides of the template, so a custom
affinity in the template can no longer dilute the non-interruptible node-selector
requirement.
Tradeoff, called out explicitly: the template is now the only path a custom
driver/executor pod spec travels. On a cluster whose
SparkApplicationCRD predatesspec.driver.template(or with theenable-pod-template: falsekill switch), the custompod spec no longer applies at all, where previously its pod-level fields were merged into
the legacy
SparkPodSpecfields. Container-level fields never worked on that path anyway.TestBuildResourceCustomPodSpecDroppedWithoutTemplateSupportpins this behavior.How was this patch tested?
go test ./go/tasks/plugins/k8s/spark/— green. Tests added/updated:TestBuildResourceCustomPodSpecPassthrough— driver pod namedspark-kubernetes-driverand executor pod named
primary, both with anephemeral-storagerequest and a matchingprimary_container_name. Asserts the build succeeds and both containers reachspec.{driver,executor}.templateverbatim, while image/env still come from Flyte's podspec. Fails on
mainwithinvalid TaskSpecification, container [spark-kubernetes-driver] not defined(verified).TestBuildResourceCustomPodSpecDroppedWithoutTemplateSupport— documents the tradeoffabove on a legacy CRD.
TestBuildResourceSparkExecutorAffinityDilution— updated: the custom affinity now landson the template, and every node selector term on the explicit
Affinityfield stillcarries the non-interruptible requirement.
TestBuildResourcePodTemplateGating(existing) covers the no-custom-pod-spec fallback.Labels
fixed
Check all the applicable boxes
https://claude.ai/code/session_01SAATDRHXajHapuojwQ4SMR