Rework MergePodSpecs logic #6262
Merged
+542
−153
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sovietaced
commented
Feb 21, 2025
Code Review Agent Run Status
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6262 +/- ##
==========================================
+ Coverage 36.86% 36.89% +0.03%
==========================================
Files 1318 1318
Lines 134773 134881 +108
==========================================
+ Hits 49683 49768 +85
- Misses 80758 80777 +19
- Partials 4332 4336 +4
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:
|
Signed-off-by: Jason Parraga <[email protected]>
Sovietaced
force-pushed
the
issue-6229-v2
branch
from
0e5fb84 to
b51b099
Compare
Signed-off-by: Jason Parraga <[email protected]>
Sovietaced
force-pushed
the
issue-6229-v2
branch
from
fed1f44 to
ee17165
Compare
Sovietaced
changed the title
Code Review Agent Run Status
|
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Sovietaced
commented
Feb 21, 2025
Sovietaced
commented
Feb 21, 2025
Code Review Agent Run #51076dActionable Suggestions - 0Additional Suggestions - 1
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
|
Can we make sure this also works with runtime pod templates? This is related to #6204. |
Signed-off-by: Jason Parraga <[email protected]>
Sovietaced
force-pushed
the
issue-6229-v2
branch
from
8365d33 to
20543d4
Compare
Code Review Agent Run #b4db69Actionable Suggestions - 1
Review Details
AI Code Review powered by |
flyte-bot
reviewed
Feb 22, 2025
| @@ -518,7 +518,7 @@ func buildWorkerPodTemplate(primaryContainer *v1.Container, basePodSpec *v1.PodS | |||
| } | |||
|
|
|||
| // Merges a ray head/worker node custom pod specs onto task's generated pod spec | |||
| func mergeCustomPodSpec(primaryContainer *v1.Container, podSpec *v1.PodSpec, k8sPod *core.K8SPod) (*v1.PodSpec, error) { | |||
| func mergeCustomPodSpec(podSpec *v1.PodSpec, k8sPod *core.K8SPod) (*v1.PodSpec, error) { | |||
There was a problem hiding this comment.
Update function calls to match signature
Consider updating function calls to mergeCustomPodSpec in buildHeadPodTemplate and buildWorkerPodTemplate to match the updated signature which no longer requires the primaryContainer parameter.
Code suggestion
Check the AI-generated fix before applying
- basePodSpec, err := mergeCustomPodSpec(primaryContainer, basePodSpec, spec.GetK8SPod())
+ basePodSpec, err := mergeCustomPodSpec(basePodSpec, spec.GetK8SPod())
@@ -503,1 +503,1 @@
- basePodSpec, err := mergeCustomPodSpec(primaryContainer, basePodSpec, spec.GetK8SPod())
+ basePodSpec, err := mergeCustomPodSpec(basePodSpec, spec.GetK8SPod())
Code Review Run #b4db69
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Signed-off-by: Jason Parraga <[email protected]>
Sovietaced
force-pushed
the
issue-6229-v2
branch
from
ba34cbc to
45811c3
Compare
Replied in the issue discussion |
Code Review Agent Run #b6b690Actionable Suggestions - 1
Review Details
AI Code Review powered by |
flyte-bot
reviewed
Feb 22, 2025
Signed-off-by: Jason Parraga <[email protected]>
Code Review Agent Run #9edcdbActionable Suggestions - 2
Review Details
AI Code Review powered by |
flyte-bot
reviewed
Feb 23, 2025
Comment on lines
+2394
to
+2406
| expectedError: errors.New("neither the basePodSpec or the overlayPodSpec can be nil"), | ||
| }, | ||
| InitContainers: []v1.Container{ | ||
| defaultInitContainerTemplate, | ||
| primaryInitContainerTemplate, | ||
| { | ||
| name: "nil base", | ||
| basePodSpec: nil, | ||
| overlayPodSpec: &v1.PodSpec{}, | ||
| expectedError: errors.New("neither the basePodSpec or the overlayPodSpec can be nil"), | ||
| }, | ||
| HostNetwork: true, | ||
| NodeSelector: map[string]string{ | ||
| "foo": "bar", | ||
| { | ||
| name: "nil base and overlay", | ||
| basePodSpec: nil, | ||
| overlayPodSpec: nil, | ||
| expectedError: errors.New("neither the basePodSpec or the overlayPodSpec can be nil"), |
There was a problem hiding this comment.
Consider more specific error messages
Consider using a more descriptive error message. The current message neither the basePodSpec or the overlayPodSpec can be nil could be clearer by specifying which parameter is nil. Maybe consider separate error messages for each case.
Code suggestion
Check the AI-generated fix before applying
- expectedError: errors.New("neither the basePodSpec or the overlayPodSpec can be nil"),
+ expectedError: errors.New("overlayPodSpec cannot be nil"),
@@ -2400,1 +2400,1 @@
- expectedError: errors.New("neither the basePodSpec or the overlayPodSpec can be nil"),
+ expectedError: errors.New("basePodSpec cannot be nil"),
@@ -2406,1 +2406,1 @@
- expectedError: errors.New("neither the basePodSpec or the overlayPodSpec can be nil"),
+ expectedError: errors.New("both basePodSpec and overlayPodSpec cannot be nil"),
Code Review Run #9edcdb
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
flyte-bot
reviewed
Feb 23, 2025
Comment on lines
+2529
to
+2531
| result, mergeErr := MergeOverlayPodSpecOntoBase(tt.basePodSpec, tt.overlayPodSpec) | ||
| assert.Equal(t, tt.expectedResult, result) | ||
| assert.Equal(t, tt.expectedError, mergeErr) |
There was a problem hiding this comment.
Consider adding more detailed merge assertions
Consider adding assertions to verify the container and init container merging behavior in more detail. The current assertions only check the final PodSpec result but don't explicitly verify that container properties are merged correctly.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| result, mergeErr := MergeOverlayPodSpecOntoBase(tt.basePodSpec, tt.overlayPodSpec) | |
| assert.Equal(t, tt.expectedResult, result) | |
| assert.Equal(t, tt.expectedError, mergeErr) | |
| result, mergeErr := MergeOverlayPodSpecOntoBase(tt.basePodSpec, tt.overlayPodSpec) | |
| assert.Equal(t, tt.expectedResult, result) | |
| assert.Equal(t, tt.expectedError, mergeErr) | |
| if result != nil { | |
| assert.Equal(t, tt.expectedResult.Containers, result.Containers, "Containers should be merged correctly") | |
| assert.Equal(t, tt.expectedResult.InitContainers, result.InitContainers, "InitContainers should be merged correctly") | |
| } |
Code Review Run #9edcdb
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
pingsutw
approved these changes
Feb 24, 2025
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tracking issue
Related to #6232
This pull request is a follow up to #6232 which introduced some regressions for pod templates with init containers.
Why are the changes needed?
We discovered a bug where init containers from pod templates were being injected into tasks which had no init containers. This is because of an out of order merge of the task pod spec onto the pod template spec.
What changes were proposed in this pull request?
Fundamentally,
MergePodSpecsis now used for two different use cases but it was originally designed for one:MergePodSpecswas originally designed to handle the first use case which gave it some strange behavior when it was used for the second case. Additionally, the signature ofMergePodSpecswas documented such that the base pod spec was passed in first and the templates were passed in second but when templates were used they were passed in first which is enormously confusing.This pull request reworks the two use cases into two different methods and hopes to clean up the semantics of the first use case. Lastly, it adds lots of new test coverage.
How was this patch tested?
Tested in production
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This PR refactors pod spec merging functionality by introducing separate methods (MergeBasePodSpecOntoTemplate and MergeOverlayPodSpecOntoBase) while simplifying mergeCustomPodSpec function. The implementation includes clearer error messages, improved template management, and simplified container merging logic. The changes fix bugs in init container handling and enhance pod spec merging semantics with more descriptive error messages for nil pod specs. The test suite provides comprehensive coverage for container handling, scheduler settings, and service account configurations.Unit tests added: True
Estimated effort to review (1-5, lower is better): 3