Skip to content

[k8s] [config] merge initContainers by name #5247

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? No Sign in to your account

Merged
merged 4 commits into from
Apr 16, 2025

Conversation

SeungjinYang
Copy link
Collaborator

@SeungjinYang SeungjinYang commented Apr 16, 2025

Currently, if the same initContainers field is specified on both client and server, the merged initContainers field ends up containing duplicate values and job launch would fail.

Screenshot 2025-04-16 at 12 16 22 PM `initContainers` user merge strategy with a mergeKey of `name` according to https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/

We actually already have a logic for dealing with list merges with a mergekey of name - so I just use that.

I am starting to have a bigger question of how much longer we want to be maintaining this custom merge logic for kubernetes, but for now, this works and will solve some immediate user pains.

Tested (run the relevant ones):

  • Code formatting: install pre-commit (auto-check on commit) or bash format.sh
  • Manual test: specify the same initContainers on both client and server and verify no duplicate is created after config merge.

@SeungjinYang SeungjinYang requested review from aylei and cg505 April 16, 2025 18:40
@romilbhardwaj
Copy link
Collaborator

Looks like the name key in init containers is required. Can we dedup the merged list based on name?

@SeungjinYang
Copy link
Collaborator Author

SeungjinYang commented Apr 16, 2025

reference for myself:
https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/
tl;dr chatGPT seems to have lied, initContainers does use merge with merge key of name
Screenshot 2025-04-16 at 12 16 22 PM

edit: also we already have a way to handle this case, so I'm just going to freeload off of that

@SeungjinYang SeungjinYang force-pushed the k8s-merge-initContainers branch from 12cc835 to 346d852 Compare April 16, 2025 20:05
@SeungjinYang SeungjinYang force-pushed the k8s-merge-initContainers branch from 346d852 to 63d93b3 Compare April 16, 2025 20:05
@SeungjinYang SeungjinYang marked this pull request as ready for review April 16, 2025 20:09
@SeungjinYang SeungjinYang changed the title [k8s] [config] override initContainers wholesale [k8s] [config] merge initContainers by name Apr 16, 2025
Copy link
Collaborator

@cg505 cg505 left a comment

Choose a reason for hiding this comment

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

can we add a test?

@SeungjinYang SeungjinYang merged commit f5bca34 into master Apr 16, 2025
21 checks passed
@SeungjinYang SeungjinYang deleted the k8s-merge-initContainers branch April 16, 2025 22:46
No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants