[Design Proposal] TEP-0094: Specifying Resource Requirements at Runtime #560

lbernick · 2021-11-15T16:32:54Z

Add design details and alternative solutions for updating the TaskRun
API to allow users to specify resource requirements
of Task Steps and Sidecars. These changes apply to both
one-shot TaskRuns and those launched via PipelineRuns.

/kind tep

skaegi · 2021-11-15T17:07:52Z

/assign

skaegi · 2021-11-15T17:08:43Z

/assign @wlynch
/assign @afrittoli
/assign @jerop

wlynch

Like the idea! Just a few follow ups.

teps/0094-configuring-resources-at-runtime.md

bobcatfish · 2021-11-16T00:29:47Z

/assign

bobcatfish

Thanks for the very thorough proposal @lbernick !!

My main thoughts:

I'd like to use named objects instead of a map of strings
I'd like to avoid introducing the * syntax if we can - using the named objects I think gives us a bit of flexibility here
I prefer the option where the feature is "container overrides" vs. just "resource overrides", but only supporting resource overloading for now

teps/0094-configuring-resources-at-runtime.md

bobcatfish · 2021-11-16T19:00:57Z

teps/0094-configuring-resources-at-runtime.md

+[Knative Serving conformance](https://github.com/knative/specs/blob/main/specs/serving/knative-api-specification-1.0.md#container)
+but not for
+[Tekton pipelines conformance](https://github.com/tektoncd/pipeline/blob/main/docs/api-spec.md#step).


bobcatfish · 2021-11-16T19:01:44Z

teps/0094-configuring-resources-at-runtime.md

+instead of having the `Container` API embedded.
+However, this would be a major API change at this point,
+for little gain compared to the proposed solution.


is it possible to elaborate a bit on the "little gain"?

nit, just spit balling

im also not clear on whether this change would solve the problem completely - maybe it makes sense in some cases to specify resource limits at authoring time as well and you'd still need to override them?

e.g. esp for scenarios within a particular organization, it might make more sense to be able to assume certain resource constraints

Added a few thoughts but I don't have many specific examples. It seems like resource requirements is the primary way this causes friction at the moment.

teps/0094-configuring-resources-at-runtime.md

bobcatfish · 2021-11-16T19:06:16Z

teps/0094-configuring-resources-at-runtime.md

+of modifying resource requirements of catalog `Task`s.
+While catalog `Task` owners can add resource requirement parameters to their `Task`s,
+this clutters `Task`s, and not all `Task`s may be updated.


nice analysis! 👍

teps/0094-configuring-resources-at-runtime.md

afrittoli

Nice work, thank you!

I have a couple of questions but nothing major / blocking.
About unnamed steps, I initially though indexes could be an option, but perhaps it's better to avoid that. What we could do it to recommend setting a step name for tasks in the catalog, to avoid have tasks in the catalog that cannot be fully tuned.

About overriding the catch-all setting, that's something we could also add later.

/approve

teps/0094-configuring-resources-at-runtime.md

vdemeester

Setting for all step is an interesting and tricky behavior to tackle, and might be confusing to some users. Applying 1CPU/1G to a Task that has 10 steps will mean the task will request at least 10CPU/10G (not counting init containers and/or sidecars). This will have to be very well documented that it is per-step and all steps are summed.

Overall looks good to me. I wonder however how much users want this vs a "Task resource request/limits", but I don't think this would get in the way of such a thing in the future.

vdemeester · 2021-11-22T16:34:47Z

teps/0094-configuring-resources-at-runtime.md

This has the similar problem that TaskRunSpec, … have in PipelineRun/Pipeline specs : you have to know the "shape" of the task you are using. The "small" problem with that is, your are tied to the Task definition. If the definition is updated and the step changed, you TaskRun / PipelineRun will fail.

vdemeester · 2021-11-22T16:43:20Z

teps/0094-configuring-resources-at-runtime.md

+abstractions from their implementation (a `Container`) and allows Tekton full control
+over what fields are specified at authoring time vs runtime.
+However, this would be a major API change at this point.


Note: this can (and should ?) be done carefully between v1beta1 and v1 if need be (by shrinking what we "show" to the world).

lbernick · 2021-11-22T19:55:35Z

Setting for all step is an interesting and tricky behavior to tackle, and might be confusing to some users. Applying 1CPU/1G to a Task that has 10 steps will mean the task will request at least 10CPU/10G (not counting init containers and/or sidecars). This will have to be very well documented that it is per-step and all steps are summed.

Thanks! definitely agree.

wlynch

LGTM, just some small naming bikeshedding

teps/0094-configuring-resources-at-runtime.md

jerop

thank you @lbernick 🎉

lbernick · 2021-12-20T16:22:15Z

/hold

I looked into tektoncd/pipeline#2986 (unrelated to the issues listed in this proposal) last week and realized that the way we address that issue may affect our design for runtime configuration for resources. I'm going to update the problem statement for this TEP to address that issue as well.

This commit expands the scope of TEP-0094 to cover the user experience of specifying resource requests and limits in Tasks. Focusing only on Step and Sidecar resource requirements may be too narrow of a scope for this TEP. This is largely motivated by tektoncd/pipeline#2986, because the solution to this problem may involve removing the ability to specify Step resource requests. It doesn't make sense to override Step resource requests in TaskRuns if users shouldn't be able to specify Step resource requests in the first place. The scope is also expanded to include parameterizing resource requests based on discussion in tektoncd#560, around treating resource requirement parameterization and runtime overrides as "both/and", rather than "either/or". Fixing Task's resource requirement UX may allow us to get parameterization for free.

lbernick · 2021-12-20T19:04:13Z

re-scoping in #588

lbernick · 2022-01-12T16:26:14Z

Going to re-open this proposal as the behavior that led me to rescope has already been addressed -- see #588 (comment) for more detail.

/hold cancel

jerop · 2022-01-24T17:07:10Z

/assign @wlynch

will follow up after API meeting

(ping @skaegi please take a look)

Add design details and alternative solutions for updating the `TaskRun` API to allow users to specify resource requirements of `Task` `Step`s and `Sidecar`s. These changes apply to both one-shot `TaskRun`s and those launched via `PipelineRun`s.

tekton-robot · 2022-01-25T17:25:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, bobcatfish, jerop, 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:

~~teps/OWNERS~~ [afrittoli,bobcatfish,jerop,vdemeester,wlynch]

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

bobcatfish · 2022-01-25T20:38:45Z

/lgtm

tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Nov 15, 2021

tekton-robot requested review from ncskier and PuneetPunamiya November 15, 2021 16:32

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 15, 2021

tekton-robot assigned skaegi Nov 15, 2021

tekton-robot assigned afrittoli, jerop and wlynch Nov 15, 2021

wlynch requested changes Nov 15, 2021

View reviewed changes

pritidesai reviewed Nov 15, 2021

View reviewed changes

teps/0094-configuring-resources-at-runtime.md Show resolved Hide resolved

lbernick force-pushed the resources branch from 53542ee to 2c1aaf9 Compare November 15, 2021 19:23

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

lbernick requested a review from wlynch November 15, 2021 19:26

tekton-robot assigned bobcatfish Nov 16, 2021

bobcatfish reviewed Nov 16, 2021

View reviewed changes

chmouel mentioned this pull request Nov 17, 2021

buildah: allow skipping push of built container tektoncd/catalog#849

Merged

10 tasks

lbernick force-pushed the resources branch from 2c1aaf9 to 8dba2c4 Compare November 17, 2021 17:36

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 Nov 17, 2021

lbernick force-pushed the resources branch from 8dba2c4 to 0b15619 Compare November 17, 2021 17:38

afrittoli reviewed Nov 22, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2021

vdemeester approved these changes Nov 22, 2021

View reviewed changes

lbernick force-pushed the resources branch 2 times, most recently from 886603f to e4a9ed3 Compare November 29, 2021 15:38

wlynch reviewed Dec 16, 2021

View reviewed changes

teps/0094-configuring-resources-at-runtime.md Outdated Show resolved Hide resolved

teps/0094-configuring-resources-at-runtime.md Outdated Show resolved Hide resolved

teps/0094-configuring-resources-at-runtime.md Outdated Show resolved Hide resolved

lbernick force-pushed the resources branch from 2a4f713 to 7c65da0 Compare December 16, 2021 20:31

jerop approved these changes Dec 20, 2021

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2021

lbernick mentioned this pull request Dec 20, 2021

[TEP-0094]: Rescope to Task resource UX #588

Closed

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2021

michaelsauter mentioned this pull request Jan 6, 2022

cluster tasks do not define resource limits opendevstack/ods-pipeline#372

Closed

lbernick force-pushed the resources branch from 7c65da0 to 236bdb8 Compare January 12, 2022 16:24

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2022

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

lbernick force-pushed the resources branch from 236bdb8 to 0e725a3 Compare January 13, 2022 20:57

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

skaegi removed their assignment Jan 24, 2022

lbernick force-pushed the resources branch from 0e725a3 to 21dcd67 Compare January 25, 2022 15:05

lbernick force-pushed the resources branch from 21dcd67 to e054130 Compare January 25, 2022 17:11

wlynch approved these changes Jan 25, 2022

View reviewed changes

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

tekton-robot merged commit 152667f into tektoncd:main Jan 25, 2022

lbernick deleted the resources branch March 3, 2022 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Design Proposal] TEP-0094: Specifying Resource Requirements at Runtime #560

[Design Proposal] TEP-0094: Specifying Resource Requirements at Runtime #560

lbernick commented Nov 15, 2021

skaegi commented Nov 15, 2021

skaegi commented Nov 15, 2021

wlynch left a comment

bobcatfish commented Nov 16, 2021

bobcatfish left a comment

bobcatfish Nov 16, 2021

bobcatfish Nov 16, 2021

bobcatfish Nov 16, 2021

lbernick Nov 17, 2021

bobcatfish Nov 16, 2021

afrittoli left a comment

vdemeester left a comment

vdemeester Nov 22, 2021

vdemeester Nov 22, 2021

lbernick commented Nov 22, 2021

wlynch left a comment

jerop left a comment

lbernick commented Dec 20, 2021

lbernick commented Dec 20, 2021

lbernick commented Jan 12, 2022

jerop commented Jan 24, 2022

tekton-robot commented Jan 25, 2022

bobcatfish commented Jan 25, 2022

[Design Proposal] TEP-0094: Specifying Resource Requirements at Runtime #560

[Design Proposal] TEP-0094: Specifying Resource Requirements at Runtime #560

Conversation

lbernick commented Nov 15, 2021

skaegi commented Nov 15, 2021

skaegi commented Nov 15, 2021

wlynch left a comment

Choose a reason for hiding this comment

bobcatfish commented Nov 16, 2021

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Nov 16, 2021

Choose a reason for hiding this comment

bobcatfish Nov 16, 2021

Choose a reason for hiding this comment

bobcatfish Nov 16, 2021

Choose a reason for hiding this comment

lbernick Nov 17, 2021

Choose a reason for hiding this comment

bobcatfish Nov 16, 2021

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester Nov 22, 2021

Choose a reason for hiding this comment

vdemeester Nov 22, 2021

Choose a reason for hiding this comment

lbernick commented Nov 22, 2021

wlynch left a comment

Choose a reason for hiding this comment

jerop left a comment

Choose a reason for hiding this comment

lbernick commented Dec 20, 2021

lbernick commented Dec 20, 2021

lbernick commented Jan 12, 2022

jerop commented Jan 24, 2022

tekton-robot commented Jan 25, 2022

bobcatfish commented Jan 25, 2022