fix: pod_override existing init_containers#62284
Conversation
Previously, init containers were simply appended when using pod_override, leading to duplicates when the same container name existed in both base and client specs. Now init containers are properly reconciled by name, matching the behavior of regular containers - same-name containers are merged while others are appended. Fixes apache#61074
|
Thanks for the contribution! To better understand the fix, can you elaborate a bit on the reason you added this? I would assume if there are multiple init containers with the same name this is rather a conflict and might rather expect to raise an alarm. Merging might be OK but also feels like a source of problems and surprises if you do not know about. And to think about consistency, the same could also apply to volumes in just two lines below. |
|
Note: Reading the bug ticket, is maybe a hook needed to influence how init container and parameters are produced compared to assume a correct merge of properties is made? |
Keep the original extend_object_field call for init_containers to avoid mutating client_spec and producing empty lists instead of None. Only apply name-based reconciliation when both specs have init_containers.
|
Thanks for the review @jscheffl! The motivation comes from issue #61074: when using the Kubernetes executor in multi-namespace mode with git-sync, the Helm chart configures the same git-sync init container for all components. But a worker only needs a specific submodule, not the full repo. With This aligns with how regular containers already work in When names don't match, containers are still appended (same behavior as before). Name-based merge only kicks in when both base and client have an init container with the same name. Regarding volumes — good point about consistency. However, volumes don't have the same "override by identity" semantic since they are referenced by name from Regarding the hook approach: a hook would be a more flexible solution long-term, but it would be a much larger change involving new hook points, configuration, and documentation. This PR follows the existing pattern already established by I also pushed a fix for the CI failures — the original code was mutating |
jscheffl
left a comment
There was a problem hiding this comment.
Okay, arguments are convicing. Reading other code a second time.
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
* fix: merge init containers by name in pod_override Previously, init containers were simply appended when using pod_override, leading to duplicates when the same container name existed in both base and client specs. Now init containers are properly reconciled by name, matching the behavior of regular containers - same-name containers are merged while others are appended. Fixes apache#61074 * fix: preserve extend_object_field deepcopy for init_containers Keep the original extend_object_field call for init_containers to avoid mutating client_spec and producing empty lists instead of None. Only apply name-based reconciliation when both specs have init_containers.
* fix: merge init containers by name in pod_override Previously, init containers were simply appended when using pod_override, leading to duplicates when the same container name existed in both base and client specs. Now init containers are properly reconciled by name, matching the behavior of regular containers - same-name containers are merged while others are appended. Fixes apache#61074 * fix: preserve extend_object_field deepcopy for init_containers Keep the original extend_object_field call for init_containers to avoid mutating client_spec and producing empty lists instead of None. Only apply name-based reconciliation when both specs have init_containers.
* fix: merge init containers by name in pod_override Previously, init containers were simply appended when using pod_override, leading to duplicates when the same container name existed in both base and client specs. Now init containers are properly reconciled by name, matching the behavior of regular containers - same-name containers are merged while others are appended. Fixes apache#61074 * fix: preserve extend_object_field deepcopy for init_containers Keep the original extend_object_field call for init_containers to avoid mutating client_spec and producing empty lists instead of None. Only apply name-based reconciliation when both specs have init_containers.
Summary
Fixes init container reconciliation in
PodGenerator.reconcile_specs()to properly merge init containers by name instead of simply appending them. This aligns init container handling with the existing behavior for regular containers.Issue
Fixes #61074
Changes
reconcile_init_containers()method to merge init containers by name, matching the pattern used for regular containersreconcile_specs()to use the new method instead ofextend_object_field()volume_mounts,env,env_from,ports, andvolume_devicesare extended; other properties are merged with client values taking precedenceTesting
test_reconcile_specs_init_containers_same_nameverifying that init containers with the same name are properly merged