Is the podTemplate expected to be merged?

[pritidesai]

Should the podTemplate merge when specified in pipelineRun.spec.podTemplate and pipelineRun.spec.taskRunSpecs[].taskPodTemplate?

Pipeline controller provides a way to specify podTemplate for any pipelineTask while creating pipelineRun through pipelineRun.spec.taskRunSpecs. It is also possible to specify podTemplate at the pipelineRun level through pipelineRun.spec.podTemplate.

This section on specifying taskRunSpecs explains how podTemplates specified in this way results in the overwrite, for example:

spec:
  podTemplate:
    securityContext:
      runAsUser: 1000
      runAsGroup: 2000
      fsGroup: 3000
  taskRunSpecs:
    - pipelineTaskName: build-task
      serviceAccountName: sa-for-build
      podTemplate:
        nodeSelector:
          disktype: ssd

Like it is described in the doc, with this pipelineRun, build-task will use the task specific podTemplate where the nodeSelector has disktype equal to ssd. But it looses any configuration from pipelineRun.spec.podTemplate and does not have the runAsUser, runAsGroup, or fsGroup set at all. I wrote a simple unit test making sure the pod templates were merged:

=== RUN   TestPipelineRun_GetTaskRunSpec/pipelineRun_Spec_podTemplate_and_taskRunSpec_pipelineTask_podTemplate
    pipelinerun_types_test.go:728: (-want, +got):   any(
        - 	pod.Template{
        - 		NodeSelector:    map[string]string{"diskType": "ssd"},
        - 		SecurityContext: s"&PodSecurityContext{SELinuxOptions:nil,RunAsUser:*1000,RunAsNonRoot:nil,SupplementalGroups:[],FSGroup:*3000,RunAsGroup:*2000,Sysct"...,
        - 	},
        + 	&pod.Template{NodeSelector: map[string]string{"diskType": "ssd"}},
          )

Should the following assignment be converted into MergePodTemplateWithDefault(task.TaskPodTemplate, s.TaskPodTemplate) such that the rest of the configuration in pipelineRun.spec.podTemplate?

pipeline/pkg/apis/pipeline/v1beta1/pipelinerun_types.go

Line 622 in d267743

s.TaskPodTemplate = task.TaskPodTemplate

Has anyone run into this kind of issue?

/kind question

[vdemeester]

@pritidesai I would say yes, it should. I thought it was already the case but it turns out it's only the case for the part that comes from the default (See #4057).

[pritidesai]

thanks @vdemeester 👍 shall we cherry pick fix for this issue into the LTS releases (0.41, 0.44, and 0.47)?

[dibyom]

This seems like a bug! Thanks for the catch @pritidesai