diff --git a/docs/taskruns.md b/docs/taskruns.md index fd5297bf508..4bcfc51e72c 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: @@ -450,6 +449,15 @@ spec: ``` `StepOverrides` and `SidecarOverrides` must include the `name` field and may include `resources`. No other fields can be overridden. +If the overridden `Task` uses a [`StepTemplate`](./tasks.md#specifying-a-step-template), configuration on +`Step` will take precedence over configuration in `StepTemplate`, and configuration in `StepOverride` will +take precedence over both. + +When merging resource requirements, different resource types are considered independently. +For example, if a `Step` configures both CPU and memory, and a `StepOverride` configures only memory, +the CPU values from the `Step` will be preserved. Requests and limits are also considered independently. +For example, if a `Step` configures a memory request and limit, and a `StepOverride` configures only a +memory request, the memory limit from the `Step` will be preserved. ### Specifying `LimitRange` values diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go index 9e01735b93f..31b64ef959b 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 0792f37a1c1..f085540c4f7 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 9e7d884b32f..a350784c69a 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -4451,6 +4451,8 @@ func TestReconcileAndPropagateCustomPipelineTaskRunSpec(t *testing.T) { "workloadtype": "tekton", }, }, + StepOverrides: []v1beta1.TaskRunStepOverride{{Name: "foo"}}, + SidecarOverrides: []v1beta1.TaskRunSidecarOverride{{Name: "bar"}}, }}, }, }} @@ -4487,8 +4489,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 cdc53f0c5a6..30bd470fdfd 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..a2ef55bd38c 100644 --- a/pkg/reconciler/taskrun/validate_resources.go +++ b/pkg/reconciler/taskrun/validate_resources.go @@ -25,6 +25,9 @@ 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" + + "github.com/hashicorp/go-multierror" ) func validateResources(requiredResources []v1beta1.TaskResource, providedResources map[string]*resourcev1alpha1.PipelineResource) error { @@ -168,3 +171,38 @@ 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 { + stepErr := validateStepOverrides(ctx, ts, trs) + sidecarErr := validateSidecarOverrides(ctx, ts, trs) + return multierror.Append(stepErr, sidecarErr).ErrorOrNil() +} + +func validateStepOverrides(ctx context.Context, ts *v1beta1.TaskSpec, trs *v1beta1.TaskRunSpec) error { + var err error + stepNames := sets.NewString() + for _, step := range ts.Steps { + stepNames.Insert(step.Name) + } + for _, stepOverride := range trs.StepOverrides { + if !stepNames.Has(stepOverride.Name) { + err = multierror.Append(err, fmt.Errorf("invalid StepOverride: No Step named %s", stepOverride.Name)) + } + } + return err +} + +func validateSidecarOverrides(ctx context.Context, ts *v1beta1.TaskSpec, trs *v1beta1.TaskRunSpec) error { + var err error + sidecarNames := sets.NewString() + for _, sidecar := range ts.Sidecars { + sidecarNames.Insert(sidecar.Name) + } + for _, sidecarOverride := range trs.SidecarOverrides { + if !sidecarNames.Has(sidecarOverride.Name) { + err = multierror.Append(err, fmt.Errorf("invalid SidecarOverride: No Sidecar named %s", sidecarOverride.Name)) + } + } + return err +} 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) + } + }) + } +}