Implement Step and Sidecar Overrides for TaskRun #4598

lbernick · 2022-02-18T18:40:13Z

Changes

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.

Closes #4326.

Tested E2E locally. Happy to split into separate PRs if this would be helpful.

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

[Feature] Implement Step and Sidecar Overrides for TaskRun

lbernick · 2022-02-18T18:41:21Z

/assign wlynch

Feel free to unassign yourself if you don't have time to review! Just assigning you since you reviewed the TEP pretty thoroughly.

tekton-robot · 2022-02-18T18:42:11Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/merge.go	72.4%	76.9%	4.5
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	86.0%	86.4%	0.3
pkg/pod/pod.go	89.5%	88.3%	-1.1
pkg/reconciler/taskrun/taskrun.go	80.5%	79.8%	-0.6
pkg/reconciler/taskrun/validate_resources.go	94.7%	95.5%	0.8

tekton-robot · 2022-02-18T18:53:06Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/merge.go	72.4%	76.9%	4.5
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	86.0%	86.4%	0.3
pkg/pod/pod.go	89.5%	88.3%	-1.1
pkg/reconciler/taskrun/taskrun.go	80.5%	79.8%	-0.6
pkg/reconciler/taskrun/validate_resources.go	94.7%	95.5%	0.8

tekton-robot · 2022-02-21T15:18:12Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/merge.go	72.4%	82.8%	10.3
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	86.0%	86.4%	0.3
pkg/pod/pod.go	89.5%	88.3%	-1.1
pkg/reconciler/taskrun/taskrun.go	80.5%	79.8%	-0.6
pkg/reconciler/taskrun/validate_resources.go	94.7%	95.5%	0.8

lbernick · 2022-02-24T15:38:24Z

/hold
Decided to split into multiple PRs. First is #4617.

lbernick · 2022-03-03T18:31:59Z

/hold cancel

tekton-robot · 2022-03-03T18:33:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	86.0%	86.4%	0.3
pkg/pod/pod.go	89.5%	88.3%	-1.1
pkg/reconciler/taskrun/taskrun.go	80.5%	79.8%	-0.6
pkg/reconciler/taskrun/validate_resources.go	94.7%	95.5%	0.8

ghost · 2022-03-03T20:49:43Z

/lgtm

jerop

thanks @lbernick - looks good, left a few comments

pkg/reconciler/taskrun/validate_resources.go

pkg/pod/pod_test.go

tekton-robot · 2022-03-04T15:12:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	86.0%	86.4%	0.3
pkg/pod/pod.go	89.5%	88.3%	-1.1
pkg/reconciler/taskrun/taskrun.go	80.5%	79.8%	-0.6
pkg/reconciler/taskrun/validate_resources.go	94.7%	95.7%	1.1

tekton-robot · 2022-03-04T15:27:10Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	86.0%	86.4%	0.3
pkg/pod/pod.go	89.5%	88.3%	-1.1
pkg/reconciler/taskrun/taskrun.go	80.5%	79.8%	-0.6
pkg/reconciler/taskrun/validate_resources.go	94.7%	95.7%	1.1

lbernick · 2022-03-04T15:52:57Z

/test pull-tekton-pipeline-integration-tests

lbernick · 2022-03-04T16:00:01Z

/test pull-tekton-pipeline-alpha-integration-tests

lbernick · 2022-03-07T16:37:33Z

@jerop @sbwsg mind taking another look when you have time?

ghost · 2022-03-07T16:46:17Z

/lgtm

Cheers @lbernick !

wlynch

mostly lgtm! just a few nits regarding exporting funcs.

wlynch · 2022-03-08T16:41:29Z

pkg/reconciler/taskrun/validate_resources_test.go

+	}}
+	for _, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			err := taskrun.ValidateOverrides(context.Background(), tc.ts, tc.trs)


Instead of exporting this func, can we move the test into the same package?

done. I left ValidateResolvedTaskResources exported as it's used in the pipelinerun reconciler to validate the param/resource bindings passed from the pipeline -> pipelinerun -> taskrun.

wlynch · 2022-03-08T16:51:33Z

pkg/pod/pod.go

+	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
+	}


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? 🤔

Discussed offline and reopened #1605 with comments.

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.

tekton-robot · 2022-03-08T20:03:45Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	86.0%	86.4%	0.3
pkg/pod/pod.go	89.5%	88.3%	-1.1
pkg/reconciler/taskrun/taskrun.go	80.5%	79.8%	-0.6
pkg/reconciler/taskrun/validate_resources.go	94.7%	95.7%	1.1

ghost · 2022-03-08T20:59:45Z

The workspace-in-sidecar example failed in the alpha integration run but is a known flake. Unfortunately no logs were printed but I think it's safe to retry.

/test pull-tekton-pipeline-alpha-integration-tests

/lgtm

lbernick · 2022-03-09T19:30:23Z

@wlynch would you mind taking another look at this?

wlynch

/lgtm

tekton-robot · 2022-03-11T10:33:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester, wlynch

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ghost · 2022-03-11T13:15:49Z

Integration run failed on a yaml example. Let's see if it repeats.

Workspace-in-sidecar failed in the alpha integration run, no logs because the step's container was waiting to start.

/test pull-tekton-pipeline-integration-tests

/test pull-tekton-pipeline-alpha-integration-tests

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 18, 2022

tekton-robot requested review from bobcatfish and vdemeester February 18, 2022 18:40

tekton-robot assigned wlynch Feb 18, 2022

lbernick force-pushed the resource-overrides branch from 98411f0 to ef13bea Compare February 18, 2022 18:51

lbernick force-pushed the resource-overrides branch from ef13bea to 3bba8e8 Compare February 21, 2022 15:15

afrittoli added this to the Pipelines v0.34 milestone Feb 22, 2022

lbernick mentioned this pull request Feb 24, 2022

Add functions to merge step/sidecar overrides #4617

Merged

5 tasks

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 24, 2022

lbernick force-pushed the resource-overrides branch from 3bba8e8 to 140f26d Compare March 3, 2022 18:31

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 3, 2022

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2022

tekton-robot assigned ghost Mar 3, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2022

jerop reviewed Mar 4, 2022

View reviewed changes

pkg/reconciler/taskrun/validate_resources.go Outdated Show resolved Hide resolved

pkg/pod/pod_test.go Show resolved Hide resolved

lbernick force-pushed the resource-overrides branch from 140f26d to 159f675 Compare March 4, 2022 15:10

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2022

lbernick force-pushed the resource-overrides branch from 159f675 to 9da9add Compare March 4, 2022 15:24

lbernick mentioned this pull request Mar 7, 2022

Not possible to define stepOverrides in a PipelineRun element taskRunSpecs #4653

Closed

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2022

wlynch requested changes Mar 8, 2022

View reviewed changes

lbernick mentioned this pull request Mar 8, 2022

Simplify and split apart MakePod #1605

Open

lbernick force-pushed the resource-overrides branch from 9da9add to c6ffaf3 Compare March 8, 2022 20:01

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2022

wlynch approved these changes Mar 9, 2022

View reviewed changes

vdemeester approved these changes Mar 11, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2022

tekton-robot merged commit a343e75 into tektoncd:main Mar 11, 2022

faust64 mentioned this pull request Jun 26, 2022

Handling parameter interpolation in fields not designed for it #1530

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Step and Sidecar Overrides for TaskRun #4598

Implement Step and Sidecar Overrides for TaskRun #4598

lbernick commented Feb 18, 2022

lbernick commented Feb 18, 2022

tekton-robot commented Feb 18, 2022

tekton-robot commented Feb 18, 2022

tekton-robot commented Feb 21, 2022

lbernick commented Feb 24, 2022

lbernick commented Mar 3, 2022

tekton-robot commented Mar 3, 2022

ghost commented Mar 3, 2022

jerop left a comment

tekton-robot commented Mar 4, 2022

tekton-robot commented Mar 4, 2022

lbernick commented Mar 4, 2022

lbernick commented Mar 4, 2022

lbernick commented Mar 7, 2022

ghost commented Mar 7, 2022

wlynch left a comment

wlynch Mar 8, 2022

lbernick Mar 8, 2022

wlynch Mar 8, 2022

lbernick Mar 8, 2022

tekton-robot commented Mar 8, 2022

ghost commented Mar 8, 2022

lbernick commented Mar 9, 2022

wlynch left a comment

tekton-robot commented Mar 11, 2022

ghost commented Mar 11, 2022

Implement Step and Sidecar Overrides for TaskRun #4598

Implement Step and Sidecar Overrides for TaskRun #4598

Conversation

lbernick commented Feb 18, 2022

Changes

Submitter Checklist

Release Notes

lbernick commented Feb 18, 2022

tekton-robot commented Feb 18, 2022

tekton-robot commented Feb 18, 2022

tekton-robot commented Feb 21, 2022

lbernick commented Feb 24, 2022

lbernick commented Mar 3, 2022

tekton-robot commented Mar 3, 2022

ghost commented Mar 3, 2022

jerop left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 4, 2022

tekton-robot commented Mar 4, 2022

lbernick commented Mar 4, 2022

lbernick commented Mar 4, 2022

lbernick commented Mar 7, 2022

ghost commented Mar 7, 2022

wlynch left a comment

Choose a reason for hiding this comment

wlynch Mar 8, 2022

Choose a reason for hiding this comment

lbernick Mar 8, 2022

Choose a reason for hiding this comment

wlynch Mar 8, 2022

Choose a reason for hiding this comment

lbernick Mar 8, 2022

Choose a reason for hiding this comment

tekton-robot commented Mar 8, 2022

ghost commented Mar 8, 2022

lbernick commented Mar 9, 2022

wlynch left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 11, 2022

ghost commented Mar 11, 2022