fix: merge podTemplates filed more specific, such as tolerations, nodeselector and so on. #7851
Conversation
tekton-robot
added
kind/bug
tekton-robot
added
the
size/L
|
Hi @Ma-YuXin. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
tekton-robot
added
the
needs-ok-to-test
|
The following is the coverage report on the affected files.
|
|
/ok-to-test |
tekton-robot
added
ok-to-test
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/retest |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chitrangpatel, vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| // mergeMaps takes two maps with the same key and value types | ||
| // and merges them, giving priority to the items in the override map. | ||
| func mergeMaps[K comparable, V any](base, overrides map[K]V) map[K]V { | ||
| result := make(map[K]V) |
There was a problem hiding this comment.
may be we can just add the overwrite to the base and done with it, instead of creating new and return that
There was a problem hiding this comment.
I don't think overwriting the base is safe. The base/default can be reused. We should only change tpl.
There was a problem hiding this comment.
Thanks for this PR.
There are two places where template get merged:
- config with
PipelineRun.PodTemplate, viaSetDefaults, into thePipelineRunspec PipelineRun.PodTemplatewithPipelineRun.TaskRunSpecs[].PodTemplateviaGetTaskRunSpecinto theTaskRunitself
This PR updates only the latter, but for consistency I think it should update both. WDYT?
|
@afrittoli's commend still needs to be addressed. Lets move this to the next release unless it can be done now. |
|
@Ma-YuXin thanks for this PR! Are you able to continue working on this? |
Changes
fixes: #7127
Background:
In the original implementation, template replacement is simply replacing pipelineRun.spec.podTemplate with pipelineRun.spec.taskRunSpecs[].taskPodTemplate,the specific description can be seen here:Issue #6846.
To solve the problem #6846. In pr #6862, the function MergePodTemplateWithDefault is called to merge podtemplate, In this function, two parameters named tpl and defaultTpl are passed in. It only merges volumes and envs, the judgment logic of the remaining fields is if the same field is set on both templates, the value from tpl will overwrite the value from defaultTpl, which leads to the occurrence of problem #7127
Solution:
Here I created a new function named ExpandedMergePodTemplateWithDefault, which can merge fields such as NodeSelector, Tolerations ,ImagePullSecrets and so on. If there is a conflict when these fields are merged, we will use the value in the field corresponding to the incoming parameter with high priority (similar to the implementation of the original function, that is, the second default of the incoming parameter is higher priority). The reason why it is not modified on the original function is that the original function is referenced in multiple places. If it is modified directly, it may cause unknown problems.
In this pr, I replaced the merge function called in function GetTaskRunSpec with my newly defined function and wrote a test function for my newly defined function and added multiple test cases to the original TestPipelineRun_GetTaskRunSpec test function
/kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes