Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Step and Sidecar Overrides for TaskRun #4598

Merged
merged 1 commit into from
Mar 11, 2022
Merged

Implement Step and Sidecar Overrides for TaskRun #4598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions docs/taskruns.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down Expand Up @@ -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

Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/pipeline/v1beta1/pipelinerun_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 10 additions & 2 deletions pkg/pod/pod.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Comment on lines +141 to +148
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a TODO above to move the MergeStepsWithStepTemplate to this package (presumably to unexport it) - should we just do this from the get-go with these funcs? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline and reopened #1605 with comments.


// 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)
Expand Down
211 changes: 211 additions & 0 deletions pkg/pod/pod_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
}, {
lbernick marked this conversation as resolved.
Show resolved Hide resolved
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{
Expand Down
2 changes: 2 additions & 0 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 != "" {
Expand Down
8 changes: 6 additions & 2 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4451,6 +4451,8 @@ func TestReconcileAndPropagateCustomPipelineTaskRunSpec(t *testing.T) {
"workloadtype": "tekton",
},
},
StepOverrides: []v1beta1.TaskRunStepOverride{{Name: "foo"}},
SidecarOverrides: []v1beta1.TaskRunSidecarOverride{{Name: "bar"}},
}},
},
}}
Expand Down Expand Up @@ -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"}},
},
}

Expand Down
6 changes: 6 additions & 0 deletions pkg/reconciler/taskrun/taskrun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
38 changes: 38 additions & 0 deletions pkg/reconciler/taskrun/validate_resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Loading