-
Notifications
You must be signed in to change notification settings - Fork 221
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.
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
TEP-0094: Specifying resource requirements at runtime [Problem Statement] #556
Conversation
/kind tep |
/assign @skaegi |
/assign |
50487c6
to
4babc84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple minor thoughts from me but looks great overall!
/approve
This TEP addresses tektoncd/pipeline#4080 and tektoncd/pipeline#4326 by proposing new configuration to TaskRuns and PipelineTaskRuns that can override any Step resource requirements specified in a Task or PipelineTask.
|
||
Currently, users can specify resource requirements in a `Task` definition, | ||
via the `Resources` field of each `Step`, `StepTemplate`, or `Sidecar`. However, there is currently no support | ||
for modifying these requirements in a `TaskRun` or `PipelineTaskRun`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does PipelineTaskRun
mean here? does it refer to PipelineTask
? Does it essentially mean modifying requirements in a TaskRun
and/or PipelineTask
with having reference to a catalog?
Or is it referring to PipelineRun
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a run of a PipelineTask: https://github.com/tektoncd/pipeline/blob/main/pkg/apis/pipeline/v1beta1/pipelinerun_types.go#L468-L481
The idea is to be able to change resource requirements for a Pipeline within a PipelineRun.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For PipelineTaskRun
- I'm guessing this will be the place in the API? https://tekton.dev/docs/pipelines/pipelineruns/#specifying-taskrunspecs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could say in TaskRun
(issued from PipelineRuns or one-shot) or something like this, but it's a nit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to the nit, maybe for a future update - i think it's important to be cautious about introducing new terminology. "pipeline task" is already a bit confusing since this isn't a Tekton type, PipelineTaskRun
looks like something I'd expect to be a Tekton type (esp mentioned alongside TaskRun
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, I will change this in the subsequent pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @lbernick
It seems like a valid use case, and users ask for it.
/approve
|
||
Currently, users can specify resource requirements in a `Task` definition, | ||
via the `Resources` field of each `Step`, `StepTemplate`, or `Sidecar`. However, there is currently no support | ||
for modifying these requirements in a `TaskRun` or `PipelineTaskRun`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For PipelineTaskRun
- I'm guessing this will be the place in the API? https://tekton.dev/docs/pipelines/pipelineruns/#specifying-taskrunspecs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit, but looking good 👍🏼
|
||
Currently, users can specify resource requirements in a `Task` definition, | ||
via the `Resources` field of each `Step`, `StepTemplate`, or `Sidecar`. However, there is currently no support | ||
for modifying these requirements in a `TaskRun` or `PipelineTaskRun`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could say in TaskRun
(issued from PipelineRuns or one-shot) or something like this, but it's a nit.
/approve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @lbernick 🎉
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli, bobcatfish, jerop, skaegi, vdemeester 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:
Approvers can indicate their approval by writing |
/lgtm |
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This TEP addresses tektoncd/pipeline#4080 and tektoncd/pipeline#4326 by
proposing new configuration to TaskRuns and PipelineTaskRuns that can
override any Step resource requirements specified in a Task or PipelineTask.