[FIX] Error when using different primary container name in driver and executor #6363
Conversation
Signed-off-by: machichima <nary12321@gmail.com>
Signed-off-by: machichima <nary12321@gmail.com>
Signed-off-by: machichima <nary12321@gmail.com>
Code Review Agent Run #6af42fActionable Suggestions - 0Review Details
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6363 +/- ##
==========================================
+ Coverage 58.48% 58.49% +0.01%
==========================================
Files 940 940
Lines 71567 71571 +4
==========================================
+ Hits 41857 41868 +11
+ Misses 26526 26521 -5
+ Partials 3184 3182 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Changelist by BitoThis pull request implements the following key changes.
|
| if templateContainer.Name != container.Name { | ||
| continue | ||
| } | ||
| } else { |
There was a problem hiding this comment.
This change seems okay to me. Can we have the same behavior for init containers to keep it consistent?
| @@ -35,7 +35,7 @@ const unsignedSIGKILL = 247 | |||
| const defaultContainerTemplateName = "default" | |||
| const defaultInitContainerTemplateName = "default-init" | |||
| const primaryContainerTemplateName = "primary" | |||
| const primaryInitContainerTemplateName = "primary-init" | |||
| const PrimaryInitContainerTemplateName = "primary-init" | |||
There was a problem hiding this comment.
These are reserved container names, I don't think they should be exported and used in the spark plugin logic
I'm not sure I understand the issue. The way that the overlay is intended to work is that it matches on container names and does not honor any magic container names. Are you unable to specify the custom driver and selector pod specs with container names that match the primary driver or executor containers? If those container names are not deterministic and you wanted to support some magic values like What are you actually trying to do with the custom pod specs? |
|
In Spark plugin, user can set driver and executor pod with Using magic value like Also, as only setting primary_container_name is supported, I need to pass the reserved init container name ( Example, @task(
task_config=Spark(
driver_pod=PodTemplate(...),
8000
executor_pod=PodTemplate(...),
...
),
...
) |
Signed-off-by: machichima <nary12321@gmail.com>
Code Review Agent Run #1adb75Actionable Suggestions - 0Review Details
|
Why do you need to set the primary container name in the pod template at all? If the goal is to customize the driver or executor pods all you need to do is pass the pod spec with a container name that matches the target container name. |
Yes, we don’t necessarily need to set the It’s more of an additional feature, not strictly necessary, but an option for flexibility. |
That makes me think, what is the value in being able to specify the But still I'm curious what is your end goal with being able to configure it? What do you expect it to do that simply configuring a container in the pod spec to match the existing driver or executor container name won't do? |
|
What do you think @pingsutw |
Tracking issue
Why are the changes needed?
There's a bug in Spark after this PR: #6262. For example, When setting primary container name in driver pod named "driver-primary", the error of
invalid TaskSpecification, container [driver-primary] not definedis raised.What changes were proposed in this pull request?
Use
MergeBasePodSpecOntoTemplatethat enables settingprimaryContainerNameandprimaryInitContainerName.How was this patch tested?
Unittest is added to check if setting primary container name that's not "primary" in driver and executor, spark can perform as expected.
Labels
Please add one or more of the following labels to categorize your PR:
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This PR fixes a critical bug related to different primary container names in Spark driver and executor pods causing 'container not defined' errors. It refactors MergeBasePodSpecOntoTemplate to improve conditional logic, adds an else branch for container name comparisons, and consolidates init container merging procedures.Unit tests added: True
Estimated effort to review (1-5, lower is better): 2