diff --git a/docs/taskruns.md b/docs/taskruns.md index fd5297bf508..3ef2f1c9651 100644 --- a/docs/taskruns.md +++ b/docs/taskruns.md @@ -404,10 +404,9 @@ and reasons. ### Overriding Task Steps and Sidecars **([alpha only](https://github.com/tektoncd/pipeline/blob/main/docs/install.md#alpha-features))** -**Warning: This feature is still under development and is not yet functional. Do not use it.** A TaskRun can specify `StepOverrides` or `SidecarOverrides` to override Step or Sidecar -configuration specified in a Task. +configuration specified in a Task. Only named Steps and Sidecars may be overridden. For example, given the following Task definition: diff --git a/pkg/apis/pipeline/v1beta1/merge.go b/pkg/apis/pipeline/v1beta1/merge.go index 5c7b0ba3b14..554c333d534 100644 --- a/pkg/apis/pipeline/v1beta1/merge.go +++ b/pkg/apis/pipeline/v1beta1/merge.go @@ -89,3 +89,88 @@ func MergeStepsWithStepTemplate(template *v1.Container, steps []Step) ([]Step, e } return steps, nil } + +// MergeStepsWithOverrides takes a possibly nil list of overrides and a +// list of steps, merging each of the steps with the overrides' resource requirements, if +// it's not nil, and returning the resulting list. +func MergeStepsWithOverrides(steps []Step, overrides []TaskRunStepOverride) ([]Step, error) { + stepNameToOverride := make(map[string]TaskRunStepOverride, len(overrides)) + for _, o := range overrides { + stepNameToOverride[o.Name] = o + } + for i, s := range steps { + o, found := stepNameToOverride[s.Name] + if !found { + continue + } + mergedResources, err := mergeResourcesWithOverride(&s.Resources, &o.Resources) + if err != nil { + return nil, err + } + steps[i].Container.Resources = *mergedResources + } + return steps, nil +} + +// MergeSidecarsWithOverrides takes a possibly nil list of overrides and a +// list of sidecars, merging each of the sidecars with the overrides' resource requirements, if +// it's not nil, and returning the resulting list. +func MergeSidecarsWithOverrides(sidecars []Sidecar, overrides []TaskRunSidecarOverride) ([]Sidecar, error) { + sidecarNameToOverride := make(map[string]TaskRunSidecarOverride, len(overrides)) + for _, o := range overrides { + sidecarNameToOverride[o.Name] = o + } + for i, s := range sidecars { + o, found := sidecarNameToOverride[s.Name] + if !found { + continue + } + mergedResources, err := mergeResourcesWithOverride(&s.Resources, &o.Resources) + if err != nil { + return nil, err + } + sidecars[i].Container.Resources = *mergedResources + } + return sidecars, nil +} + +func mergeResourcesWithOverride(resources, overrides *v1.ResourceRequirements) (*v1.ResourceRequirements, error) { + if overrides == nil { + return resources, nil + } + resourcesAsJSON, err := json.Marshal(resources) + if err != nil { + return nil, err + } + overridesAsJSON, err := json.Marshal(overrides) + if err != nil { + return nil, err + } + emptyJSON, err := json.Marshal(&v1.ResourceRequirements{}) + if err != nil { + return nil, err + } + // Treat resources as "template", since we want overrides to override the value in the resources + patchSchema, err := strategicpatch.NewPatchMetaFromStruct(resources) + if err != nil { + return nil, err + } + patch, err := strategicpatch.CreateThreeWayMergePatch(emptyJSON, overridesAsJSON, resourcesAsJSON, patchSchema, true) + if err != nil { + return nil, err + } + + // Actually apply the merge patch to the template JSON. + mergedAsJSON, err := strategicpatch.StrategicMergePatchUsingLookupPatchMeta(resourcesAsJSON, patch, patchSchema) + if err != nil { + return nil, err + } + + // Unmarshal the merged JSON to a Container pointer, and return it. + merged := &v1.ResourceRequirements{} + err = json.Unmarshal(mergedAsJSON, merged) + if err != nil { + return nil, err + } + return merged, nil +} diff --git a/pkg/apis/pipeline/v1beta1/merge_test.go b/pkg/apis/pipeline/v1beta1/merge_test.go index 7bf5c3b4940..03f5041a64b 100644 --- a/pkg/apis/pipeline/v1beta1/merge_test.go +++ b/pkg/apis/pipeline/v1beta1/merge_test.go @@ -117,3 +117,379 @@ func TestMergeStepsWithStepTemplate(t *testing.T) { }) } } + +func TestMergeStepOverrides(t *testing.T) { + tcs := []struct { + name string + steps []Step + stepOverrides []TaskRunStepOverride + want []Step + wantErr bool + }{{ + name: "no overrides", + steps: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }}, + want: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }}, + }, { + name: "not all steps overridden", + steps: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }, { + Container: corev1.Container{ + Name: "bar", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }}, + stepOverrides: []TaskRunStepOverride{{ + Name: "bar", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }}, + want: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }, { + Container: corev1.Container{ + Name: "bar", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + }, { + name: "override memory but not CPU", + steps: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("1Gi"), + corev1.ResourceCPU: resource.MustParse("100m"), + }, + }, + }, + }}, + stepOverrides: []TaskRunStepOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }}, + want: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("2Gi"), + }, + }, + }, + }}, + }, { + name: "override request but not limit", + steps: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + stepOverrides: []TaskRunStepOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + }, + }}, + want: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + }, { + name: "override request and limit", + steps: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + stepOverrides: []TaskRunStepOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + }, + }}, + want: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + }, + }, + }}, + }, { + // We don't make any effort to reject overrides that would result in invalid pods; + // instead, we let k8s reject the resulting pod. + name: "new request > old limit", + steps: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + stepOverrides: []TaskRunStepOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + }, + }}, + want: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + }} + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + steps, err := MergeStepsWithOverrides(tc.steps, tc.stepOverrides) + if (err != nil) != tc.wantErr { + t.Errorf("expected error: %t, but got error %s", tc.wantErr, err) + } + if d := cmp.Diff(tc.want, steps); d != "" { + t.Errorf("merged steps don't match, diff: %s", diff.PrintWantGot(d)) + } + }) + } +} + +func TestMergeSidecarOverrides(t *testing.T) { + tcs := []struct { + name string + sidecars []Sidecar + sidecarOverrides []TaskRunSidecarOverride + want []Sidecar + wantErr bool + }{{ + name: "no overrides", + sidecars: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }}, + want: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }}, + }, { + name: "not all sidecars overridden", + sidecars: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }, { + Container: corev1.Container{ + Name: "bar", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }}, + sidecarOverrides: []TaskRunSidecarOverride{{ + Name: "bar", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }}, + want: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }, { + Container: corev1.Container{ + Name: "bar", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + }, { + name: "override memory but not CPU", + sidecars: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("1Gi"), + corev1.ResourceCPU: resource.MustParse("100m"), + }, + }, + }, + }}, + sidecarOverrides: []TaskRunSidecarOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }}, + want: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("2Gi"), + }, + }, + }, + }}, + }, { + name: "override request but not limit", + sidecars: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + sidecarOverrides: []TaskRunSidecarOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + }, + }}, + want: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + }, { + name: "override request and limit", + sidecars: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + sidecarOverrides: []TaskRunSidecarOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + }, + }}, + want: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + }, + }, + }}, + }, { + // We don't make any effort to reject overrides that would result in invalid pods; + // instead, we let k8s reject the resulting pod. + name: "new request > old limit", + sidecars: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + sidecarOverrides: []TaskRunSidecarOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + }, + }}, + want: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + }} + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + sidecars, err := MergeSidecarsWithOverrides(tc.sidecars, tc.sidecarOverrides) + if (err != nil) != tc.wantErr { + t.Errorf("expected error: %t, but got error %s", tc.wantErr, err) + } + if d := cmp.Diff(tc.want, sidecars); d != "" { + t.Errorf("merged sidecars don't match, diff: %s", diff.PrintWantGot(d)) + } + }) + } +} diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go index eecc8f59496..43616cf1552 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go @@ -534,6 +534,8 @@ func (pr *PipelineRun) GetTaskRunSpec(pipelineTaskName string) PipelineTaskRunSp if task.TaskServiceAccountName != "" { s.TaskServiceAccountName = task.TaskServiceAccountName } + s.StepOverrides = task.StepOverrides + s.SidecarOverrides = task.SidecarOverrides } } return s diff --git a/pkg/pod/pod.go b/pkg/pod/pod.go index 984142abc0a..c1ed7f12b02 100644 --- a/pkg/pod/pod.go +++ b/pkg/pod/pod.go @@ -138,13 +138,21 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec if err != nil { return nil, err } + steps, err = v1beta1.MergeStepsWithOverrides(steps, taskRun.Spec.StepOverrides) + if err != nil { + return nil, err + } + sidecars, err := v1beta1.MergeSidecarsWithOverrides(taskSpec.Sidecars, taskRun.Spec.SidecarOverrides) + if err != nil { + return nil, err + } // Convert any steps with Script to command+args. // If any are found, append an init container to initialize scripts. if alphaAPIEnabled { - scriptsInit, stepContainers, sidecarContainers = convertScripts(b.Images.ShellImage, b.Images.ShellImageWin, steps, taskSpec.Sidecars, taskRun.Spec.Debug) + scriptsInit, stepContainers, sidecarContainers = convertScripts(b.Images.ShellImage, b.Images.ShellImageWin, steps, sidecars, taskRun.Spec.Debug) } else { - scriptsInit, stepContainers, sidecarContainers = convertScripts(b.Images.ShellImage, "", steps, taskSpec.Sidecars, nil) + scriptsInit, stepContainers, sidecarContainers = convertScripts(b.Images.ShellImage, "", steps, sidecars, nil) } if scriptsInit != nil { initContainers = append(initContainers, *scriptsInit) diff --git a/pkg/pod/pod_test.go b/pkg/pod/pod_test.go index 06c6dbd4d3c..70c6f85d34e 100644 --- a/pkg/pod/pod_test.go +++ b/pkg/pod/pod_test.go @@ -791,6 +791,217 @@ _EOF_ }), ActiveDeadlineSeconds: &defaultActiveDeadlineSeconds, }, + }, { + desc: "with stepOverrides", + ts: v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{Container: corev1.Container{ + Name: "step1", + Image: "image", + Command: []string{"cmd"}, + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("10Gi"), + }, + }, + }}}, + }, + trs: v1beta1.TaskRunSpec{ + StepOverrides: []v1beta1.TaskRunStepOverride{{ + Name: "step1", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("6"), + corev1.ResourceMemory: resource.MustParse("5Gi"), + }, + }, + }}, + }, + want: &corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyNever, + InitContainers: []corev1.Container{placeToolsInit, tektonDirInit(images.EntrypointImage, []v1beta1.Step{ + {Container: corev1.Container{Name: "step1"}}, + })}, + Containers: []corev1.Container{{ + Name: "step-step1", + Image: "image", + Command: []string{"/tekton/bin/entrypoint"}, + Args: []string{ + "-wait_file", + "/tekton/downward/ready", + "-wait_file_content", + "-post_file", + "/tekton/run/0/out", + "-termination_path", + "/tekton/termination", + "-step_metadata_dir", + "/tekton/run/0/status", + "-entrypoint", + "cmd", + "--", + }, + VolumeMounts: append([]corev1.VolumeMount{binROMount, runMount(0, false), downwardMount, { + Name: "tekton-creds-init-home-0", + MountPath: "/tekton/creds", + }}, implicitVolumeMounts...), + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("6"), + corev1.ResourceMemory: resource.MustParse("5Gi"), + }, + }, + TerminationMessagePath: "/tekton/termination", + }}, + Volumes: append(implicitVolumes, binVolume, runVolume(0), downwardVolume, corev1.Volume{ + Name: "tekton-creds-init-home-0", + VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{Medium: corev1.StorageMediumMemory}}, + }), + ActiveDeadlineSeconds: &defaultActiveDeadlineSeconds, + }, + }, { + desc: "with stepOverrides and stepTemplate", + ts: v1beta1.TaskSpec{ + StepTemplate: &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("10Gi"), + }, + }, + }, + Steps: []v1beta1.Step{{Container: corev1.Container{ + Name: "step1", + Image: "image", + Command: []string{"cmd"}, + }}}, + }, + trs: v1beta1.TaskRunSpec{ + StepOverrides: []v1beta1.TaskRunStepOverride{{ + Name: "step1", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("6"), + corev1.ResourceMemory: resource.MustParse("5Gi"), + }, + }, + }}, + }, + want: &corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyNever, + InitContainers: []corev1.Container{placeToolsInit, tektonDirInit(images.EntrypointImage, []v1beta1.Step{ + {Container: corev1.Container{Name: "step1"}}, + })}, + Containers: []corev1.Container{{ + Name: "step-step1", + Image: "image", + Command: []string{"/tekton/bin/entrypoint"}, + Args: []string{ + "-wait_file", + "/tekton/downward/ready", + "-wait_file_content", + "-post_file", + "/tekton/run/0/out", + "-termination_path", + "/tekton/termination", + "-step_metadata_dir", + "/tekton/run/0/status", + "-entrypoint", + "cmd", + "--", + }, + VolumeMounts: append([]corev1.VolumeMount{binROMount, runMount(0, false), downwardMount, { + Name: "tekton-creds-init-home-0", + MountPath: "/tekton/creds", + }}, implicitVolumeMounts...), + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("6"), + corev1.ResourceMemory: resource.MustParse("5Gi"), + }, + }, + TerminationMessagePath: "/tekton/termination", + }}, + Volumes: append(implicitVolumes, binVolume, runVolume(0), downwardVolume, corev1.Volume{ + Name: "tekton-creds-init-home-0", + VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{Medium: corev1.StorageMediumMemory}}, + }), + ActiveDeadlineSeconds: &defaultActiveDeadlineSeconds, + }, + }, { + desc: "with sidecarOverrides", + ts: v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{Container: corev1.Container{ + Name: "primary-name", + Image: "primary-image", + Command: []string{"cmd"}, // avoid entrypoint lookup. + }}}, + Sidecars: []v1beta1.Sidecar{{ + Container: corev1.Container{ + Name: "sc-name", + Image: "sidecar-image", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("10Gi"), + }, + }, + }, + }}, + }, + trs: v1beta1.TaskRunSpec{ + SidecarOverrides: []v1beta1.TaskRunSidecarOverride{{ + Name: "sc-name", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("6"), + corev1.ResourceMemory: resource.MustParse("5Gi"), + }, + }, + }}, + }, + wantAnnotations: map[string]string{}, + want: &corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyNever, + InitContainers: []corev1.Container{placeToolsInit, tektonDirInit(images.EntrypointImage, []v1beta1.Step{{Container: corev1.Container{Name: "primary-name"}}})}, + Containers: []corev1.Container{{ + Name: "step-primary-name", + Image: "primary-image", + Command: []string{"/tekton/bin/entrypoint"}, + Args: []string{ + "-wait_file", + "/tekton/downward/ready", + "-wait_file_content", + "-post_file", + "/tekton/run/0/out", + "-termination_path", + "/tekton/termination", + "-step_metadata_dir", + "/tekton/run/0/status", + "-entrypoint", + "cmd", + "--", + }, + VolumeMounts: append([]corev1.VolumeMount{binROMount, runMount(0, false), downwardMount, { + Name: "tekton-creds-init-home-0", + MountPath: "/tekton/creds", + }}, implicitVolumeMounts...), + TerminationMessagePath: "/tekton/termination", + }, { + Name: "sidecar-sc-name", + Image: "sidecar-image", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("6"), + corev1.ResourceMemory: resource.MustParse("5Gi"), + }, + }, + }}, + Volumes: append(implicitVolumes, binVolume, runVolume(0), downwardVolume, corev1.Volume{ + Name: "tekton-creds-init-home-0", + VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{Medium: corev1.StorageMediumMemory}}, + }), + ActiveDeadlineSeconds: &defaultActiveDeadlineSeconds, + }, }, { desc: "step with script and stepTemplate", ts: v1beta1.TaskSpec{ diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index cfecb4a0f66..b72e9a506df 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -749,6 +749,8 @@ func (c *Reconciler) createTaskRun(ctx context.Context, rprt *resources.Resolved ServiceAccountName: taskRunSpec.TaskServiceAccountName, Timeout: getTimeoutFunc(ctx, pr, rprt, c.Clock), PodTemplate: taskRunSpec.TaskPodTemplate, + StepOverrides: taskRunSpec.StepOverrides, + SidecarOverrides: taskRunSpec.SidecarOverrides, }} if rprt.ResolvedTaskResources.TaskName != "" { diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 39d9d2825f5..e27d485a8cd 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -4438,6 +4438,8 @@ func TestReconcileAndPropagateCustomPipelineTaskRunSpec(t *testing.T) { "workloadtype": "tekton", }, }, + StepOverrides: []v1beta1.TaskRunStepOverride{{Name: "foo"}}, + SidecarOverrides: []v1beta1.TaskRunSidecarOverride{{Name: "bar"}}, }}, }, }} @@ -4474,8 +4476,10 @@ func TestReconcileAndPropagateCustomPipelineTaskRunSpec(t *testing.T) { "workloadtype": "tekton", }, }, - Resources: &v1beta1.TaskRunResources{}, - Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}, + Resources: &v1beta1.TaskRunResources{}, + Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}, + StepOverrides: []v1beta1.TaskRunStepOverride{{Name: "foo"}}, + SidecarOverrides: []v1beta1.TaskRunSidecarOverride{{Name: "bar"}}, }, } diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index ad0f796d9a4..5b400777934 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -358,6 +358,12 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1 } } + if err := ValidateOverrides(ctx, taskSpec, &tr.Spec); err != nil { + logger.Errorf("TaskRun %q step or sidecar overrides are invalid: %v", tr.Name, err) + tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) + return nil, nil, controller.NewPermanentError(err) + } + // Initialize the cloud events if at least a CloudEventResource is defined // and they have not been initialized yet. // FIXME(afrittoli) This resource specific logic will have to be replaced diff --git a/pkg/reconciler/taskrun/validate_resources.go b/pkg/reconciler/taskrun/validate_resources.go index 213b5763a9f..0013ec23610 100644 --- a/pkg/reconciler/taskrun/validate_resources.go +++ b/pkg/reconciler/taskrun/validate_resources.go @@ -25,6 +25,7 @@ import ( resourcev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1" "github.com/tektoncd/pipeline/pkg/list" "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" + "k8s.io/apimachinery/pkg/util/sets" ) func validateResources(requiredResources []v1beta1.TaskResource, providedResources map[string]*resourcev1alpha1.PipelineResource) error { @@ -168,3 +169,26 @@ func validateTaskSpecRequestResources(ctx context.Context, taskSpec *v1beta1.Tas return nil } + +// ValidateOverrides validates that all stepOverrides map to valid steps, and likewise for sidecarOverrides +func ValidateOverrides(ctx context.Context, ts *v1beta1.TaskSpec, trs *v1beta1.TaskRunSpec) error { + stepNames := sets.NewString() + for _, step := range ts.Steps { + stepNames.Insert(step.Name) + } + for _, stepOverride := range trs.StepOverrides { + if !stepNames.Has(stepOverride.Name) { + return fmt.Errorf("invalid StepOverride: No Step named %s", stepOverride.Name) + } + } + sidecarNames := sets.NewString() + for _, sidecar := range ts.Sidecars { + sidecarNames.Insert(sidecar.Name) + } + for _, sidecarOverride := range trs.SidecarOverrides { + if !sidecarNames.Has(sidecarOverride.Name) { + return fmt.Errorf("invalid SidecarOverride: No Sidecar named %s", sidecarOverride.Name) + } + } + return nil +} diff --git a/pkg/reconciler/taskrun/validate_resources_test.go b/pkg/reconciler/taskrun/validate_resources_test.go index 4818b3f25da..e20b8900654 100644 --- a/pkg/reconciler/taskrun/validate_resources_test.go +++ b/pkg/reconciler/taskrun/validate_resources_test.go @@ -422,3 +422,74 @@ func TestValidateResolvedTaskResources_InvalidResources(t *testing.T) { }) } } + +func TestValidateOverrides(t *testing.T) { + tcs := []struct { + name string + ts *v1beta1.TaskSpec + trs *v1beta1.TaskRunSpec + wantErr bool + }{{ + name: "valid stepOverrides", + ts: &v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{ + Container: corev1.Container{ + Name: "step1", + }, + }, { + Container: corev1.Container{ + Name: "step2", + }, + }}, + }, + trs: &v1beta1.TaskRunSpec{ + StepOverrides: []v1beta1.TaskRunStepOverride{{ + Name: "step1", + }}, + }, + }, { + name: "valid sidecarOverrides", + ts: &v1beta1.TaskSpec{ + Sidecars: []v1beta1.Sidecar{{ + Container: corev1.Container{ + Name: "step1", + }, + }, { + Container: corev1.Container{ + Name: "step2", + }, + }}, + }, + trs: &v1beta1.TaskRunSpec{ + SidecarOverrides: []v1beta1.TaskRunSidecarOverride{{ + Name: "step1", + }}, + }, + }, { + name: "invalid stepOverrides", + ts: &v1beta1.TaskSpec{}, + trs: &v1beta1.TaskRunSpec{ + StepOverrides: []v1beta1.TaskRunStepOverride{{ + Name: "step1", + }}, + }, + wantErr: true, + }, { + name: "invalid sidecarOverrides", + ts: &v1beta1.TaskSpec{}, + trs: &v1beta1.TaskRunSpec{ + SidecarOverrides: []v1beta1.TaskRunSidecarOverride{{ + Name: "step1", + }}, + }, + wantErr: true, + }} + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + err := taskrun.ValidateOverrides(context.Background(), tc.ts, tc.trs) + if (err != nil) != tc.wantErr { + t.Errorf("expected err: %t, but got err %s", tc.wantErr, err) + } + }) + } +}