From ef13bead000ce8d48272dd899607ca89b7fcff5c Mon Sep 17 00:00:00 2001 From: Lee Bernick Date: Fri, 18 Feb 2022 10:44:22 -0700 Subject: [PATCH] Implement Step and Sidecar Overrides for TaskRun This implements TEP-0094: Configuring Resources at Runtime. StepOverrides and SidecarOverrides were added to the API in a previous commit. This commit allows them to override resource requirements specified in the Task spec. The same merging strategy that is currently used for stepTemplate is also used for step and sidecar overrides, except that only the resources field of the container is overridden, and unlike with templates, the overrides take precedence over the Task spec. --- docs/taskruns.md | 3 +- pkg/apis/pipeline/v1beta1/merge.go | 85 ++++ pkg/apis/pipeline/v1beta1/merge_test.go | 376 ++++++++++++++++++ .../pipeline/v1beta1/pipelinerun_types.go | 2 + pkg/pod/pod.go | 12 +- pkg/pod/pod_test.go | 211 ++++++++++ pkg/reconciler/pipelinerun/pipelinerun.go | 2 + .../pipelinerun/pipelinerun_test.go | 8 +- pkg/reconciler/taskrun/taskrun.go | 6 + pkg/reconciler/taskrun/validate_resources.go | 24 ++ .../taskrun/validate_resources_test.go | 71 ++++ 11 files changed, 794 insertions(+), 6 deletions(-) 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) + } + }) + } +}