[TEP-0104]: Support Task-level resource limits #703
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,15 @@ | ||
| --- | ||
| status: implementable | ||
| title: Task-level Resource Requirements | ||
| creation-date: '2022-04-08' | ||
| last-updated: '2022-05-11' | ||
| authors: | ||
| - '@lbernick' | ||
| - '@vdemeester' | ||
| - '@jerop' | ||
| --- | ||
|
|
||
| # TEP-0104: Task-level Resource Requirements | ||
|
|
||
| <!-- toc --> | ||
| - [Summary](#summary) | ||
|
|
@@ -22,7 +22,6 @@ authors: | |
| - [Existing Strategies for Controlling Resource Consumption](#existing-strategies-for-controlling-resource-consumption) | ||
| - [Proposal](#proposal) | ||
| - [API Changes](#api-changes) | ||
| - [Applying Task Resources to Containers](#applying-task-resources-to-containers) | ||
| - [Sidecars](#sidecars) | ||
| - [Interaction with Step resource requirements](#interaction-with-step-resource-requirements) | ||
|
|
@@ -67,6 +66,8 @@ In addition, if a pod exceeds its memory requests, it may be evicted from the no | |
| For more information, see [“Resource Management for Pods and Containers”][management]. | ||
| Resource requirements are also used to determine a pod’s quality of service, which affect how likely it is to be scheduled or evicted. | ||
|
|
||
| Resource requirements [can't be updated after pods are created][update]. | ||
|
|
||
| ### Resource requirements in Tekton | ||
| Tekton [Steps][step] correspond to containers, and resource requirements can be specified on a per-Step basis. | ||
| Step resource requirements can be specified via [Task.StepTemplate][stepTemplate], [Task.Steps][step], | ||
|
|
@@ -78,7 +79,7 @@ as close to the user’s configuration as possible, subject to the [minimum/maxi | |
| TaskRuns are rejected if there is no configuration that meets these constraints. | ||
|
|
||
| ## Goals | ||
| - Task-level resource requirements are configurable at authoring time (i.e. on Task) and runtime (i.e. on TaskRun). | ||
| - Authoring time configuration is provided for consistency with existing functionality (Task.Step.Resources). | ||
| - The reasons for runtime configuration are discussed in more detail in [TEP-0094][tep-0094]. | ||
|
|
||
|
|
@@ -102,9 +103,8 @@ depending on its inputs. In addition, LimitRanges don’t distinguish between Te | |
|
|
||
| ### API Changes | ||
|
|
||
| Augment the Task and TaskRun APIs with a resources field that allows the user to configure the resource requirements | ||
| of a Task. An example Task is as follows. | ||
|
|
||
| ``` | ||
| apiVersion: tekton.dev/v1beta1 | ||
|
|
@@ -118,63 +118,74 @@ spec: | |
| resources: | ||
| requests: | ||
| memory: 1Gi | ||
| limits: | ||
| memory: 2Gi | ||
| ``` | ||
|
|
||
| This field should also be added to [PipelineRun.TaskRunSpecs][taskRunSpecs]. | ||
|
|
||
| **Note**: Currently, `Task.Resources` is used for PipelineResources. Appropriate naming for this field is still | ||
| under discussion. | ||
|
|
||
| ### Applying Task Resources to Containers | ||
|
|
||
| #### Requests | ||
|
|
||
| As mentioned in [Resource Requirements in Kubernetes](#resource-requirements-in-kubernetes), | ||
| the effective resource requests of a pod are the sum of the resource requests of its containers, | ||
| and this value is used to determine the resources reserved by the kubelet when scheduling a pod. | ||
| Therefore, when a user configures a resource request for a Task, that request should be applied to one container only in the pod. | ||
| (See [Interaction with LimitRanges](#interaction-with-limitranges) for caveats.) | ||
| All other containers will not have resource requests. This will result in a pod with an effective resource request | ||
| that is the same as that of the Task, and will be scheduled correctly. This is similar to | ||
| Tekton’s [handling of resource requests pre v0.28.0][pre-v28], where the maximum resource request of all containers | ||
| was applied to only one container, and the rest were left without resource requests. | ||
|
|
||
| #### Limits | ||
|
|
||
| Because Kubernetes considers [containers without resource limits to have higher limits than those with limits configured][effective], | ||
| configuration for limits is different than configuration for requests. There are several options for how Task-level resource limits | ||
| could be implemented: | ||
| - If the task resource limit is applied to only one container, the pod will not have an effective limit | ||
| due to the other containers without limits. This defeats the purpose of the feature. | ||
| - If the task limit is spread out among containers, a task where one step is more resource intensive | ||
| than all the others could get oomkilled or throttled. | ||
| - If the task limit is applied to each container, the pod has a much higher effective limit than desired. | ||
|
|
||
| However, the effective resource limit of a pod are not used for scheduling (see | ||
| [How Pods with resource requests are scheduled][scheduling] and [How Kubernetes applies resource requests and limits][enforcement]). | ||
| Instead, container limits are enforced by the container runtime. | ||
|
There was a problem hiding this comment. Question: What happens when the container runtime enforces the limit? Do we currently handle that error somehow? Or does the taskrun just time out? There was a problem hiding this comment. working on the implementation, perhaps can help with some my understanding... task-level limits will be applied on each step, so each step/container gets enforced with the required limits. Although the task/pod-level will have a larger summed up limits amount(as from each step), the container runtime still follows the limits over the whole process -- due to the sequentially executed steps under Tekton context. some lines I updated in docs, perhaps help a bit also There was a problem hiding this comment. over-limits (task-level) will be checked in a helper func under reconcile logic and throw an error to fail TaskRun an example like the current over-limits checks for step-level as There was a problem hiding this comment. Discussed in tektoncd/pipeline#4930 |
||
|
|
||
| This means that applying the task resource limits to each container in the pod will result in a pod with higher effective limits than | ||
| desired, but which prevents any individual Step from exceeding configured limits, as is likely desired. | ||
|
|
||
| If Task-level limits are set, Tekton should apply the smallest possible resource request to each container. | ||
| This is because containers with limits but not requests automatically have their requests set to their limits, | ||
| which would result in a pod with much higher effective requests than desired. | ||
|
|
||
| ### Sidecars | ||
|
|
||
| Sidecar containers run in parallel with Steps, meaning that their resource requests and limits | ||
| should actually be summed with Steps’ resource requirements. In the case of Task-level limits, it is not clear how to | ||
| distribute the limit between a Sidecar and Steps, since they run at the same time. Therefore, the Task-level resource limit | ||
| should be interpreted as the limit only for Steps, and Sidecar limits should be set separately. For consistency, Task-level | ||
| requests should also be interpreted as requests for Steps only. | ||
| Users should be able to specify both Task resource requirements and Sidecar resource requirements. | ||
|
|
||
| ### Interaction with Step resource requirements | ||
|
|
||
| Because Tekton will handle the logic for the combined resource requests of a Task or TaskRun, | ||
| users should not be able to specify resource requests for both the Task or TaskRun and individual Steps. | ||
| This means: | ||
| - If a Task defines [StepTemplate.Resources][stepTemplate] or [Step.Resources][step]: | ||
lbernick marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - If the Task also defines Resources, the Task will be rejected by the admission webhook. | ||
| - If the corresponding TaskRun defines Resources, the value from the TaskRun will apply | ||
| and the value from the Task will be ignored. | ||
| - If a Task or TaskRun defines Resources, the admission webhook should reject TaskRuns | ||
| that also define [StepOverrides.Resources][stepOverride]. | ||
|
|
||
| Users should not be able to mix and match Step resource requirements and Task resource requirements, even for different | ||
| types of compute resources (e.g. CPU, memory). | ||
|
|
||
| ### Interaction with LimitRanges | ||
|
|
||
|
|
@@ -192,8 +203,93 @@ If there is no container configuration that satisfies the LimitRange, the TaskRu | |
| - Tekton pods currently have a [burstable quality of service class][qos], which will not change as a result of this implementation. | ||
| - We should consider updating our catalog Task guidelines with guidance around when to specify resource requirements. | ||
|
|
||
| ### Future Work | ||
|
|
||
| We should consider deprecating `Task.Step.Resources`, `Task.StepTemplate.Resources`, and `TaskRun.StepOverrides`. | ||
| Specifying resource requirements for individual Steps is confusing and likely too granular for many CI/CD workflows. | ||
|
|
||
| We could also consider support for both Task-level and Step-level resource requirements if the requirements are for different types | ||
|
There was a problem hiding this comment. @austinzhao-go updated the tep to clarify |
||
| of compute resources (for example, specifying CPU request at the Step level and memory request at the Task level). However, | ||
| this functionality will not be supported by the initial implementation of this proposal; it can be added later if desired. | ||
|
|
||
| ## Examples | ||
|
|
||
| ### Example with requests only | ||
|
|
||
| ``` | ||
| apiVersion: tekton.dev/v1beta1 | ||
| kind: Task | ||
| metadata: | ||
| name: my-task | ||
| spec: | ||
| steps: | ||
| - name: step-1 | ||
| - name: step-2 | ||
| - name: step-3 | ||
| resources: | ||
| requests: | ||
| cpu: 1.5 | ||
| ``` | ||
|
|
||
| | Step name | CPU request | CPU limit | | ||
| | --------- | ----------- | --------- | | ||
| | step-1 | N/A | N/A | | ||
| | step-2 | N/A | N/A | | ||
| | step-3 | 1.5 | N/A | | ||
|
|
||
| ### Example with limits only | ||
|
|
||
| ``` | ||
| apiVersion: tekton.dev/v1beta1 | ||
| kind: Task | ||
| metadata: | ||
| name: my-task | ||
| spec: | ||
| steps: | ||
| - name: step-1 | ||
| - name: step-2 | ||
| - name: step-3 | ||
| resources: | ||
| limits: | ||
| cpu: 2 | ||
| ``` | ||
|
|
||
| | Step name | CPU request | CPU limit | | ||
| | --------- | ----------------- | --------- | | ||
| | step-1 | smallest possible | 2 | | ||
| | step-2 | smallest possible | 2 | | ||
| | step-3 | smallest possible | 2 | | ||
|
|
||
| (Here, the smallest possible request is based on both what the kubelet will allow, and the minimum values allowed by any LimitRange.) | ||
|
|
||
| ### Example with both requests and limits | ||
|
|
||
| ``` | ||
| apiVersion: tekton.dev/v1beta1 | ||
| kind: Task | ||
| metadata: | ||
| name: my-task | ||
| spec: | ||
| steps: | ||
| - name: step-1 | ||
| - name: step-2 | ||
| - name: step-3 | ||
| resources: | ||
| requests: | ||
| cpu: 1.5 | ||
| limits: | ||
| cpu: 2 | ||
| ``` | ||
|
|
||
| | Step name | CPU request | CPU limit | | ||
| | --------- | ----------------- | --------- | | ||
| | step-1 | smallest possible | 2 | | ||
| | step-2 | smallest possible | 2 | | ||
| | step-3 | 1.5 | 2 | | ||
|
|
||
| (Here, the smallest possible request is based on both what the kubelet will allow, and the minimum values allowed by any LimitRange.) | ||
|
|
||
|
|
||
| ### Example with Sidecar | ||
|
|
||
| ``` | ||
|
|
@@ -207,6 +303,11 @@ spec: | |
| - name: step-2 | ||
| sidecars: | ||
| - name: sidecar-2 | ||
| resources: | ||
| requests: | ||
| cpu: 750m | ||
| limits: | ||
| cpu: 1 | ||
| resources: | ||
| requests: | ||
| cpu: 1.5 | ||
|
|
@@ -218,7 +319,7 @@ The resulting pod would have the following containers: | |
| | ----------------- | ----------- | --------- | | ||
| | step-1 | N/A | N/A | | ||
| | step-2 | 1.5 | N/A | | ||
| | sidecar-1 | 750m | 1 | | ||
|
|
||
| ### Example with LimitRange | ||
|
|
||
|
|
@@ -302,7 +403,7 @@ The resulting pod would have the following containers: | |
| If the “Resources.Requests” field were present on the Task instead of the TaskRun, | ||
| the Task would be rejected. | ||
|
|
||
| ### Example with Step resource requests overridden by TaskRun | ||
|
|
||
| ``` | ||
| apiVersion: tekton.dev/v1beta1 | ||
|
|
@@ -376,35 +477,6 @@ spec: | |
|
|
||
| This TaskRun would be rejected. | ||
|
|
||
| ### Example with both CPU and memory | ||
|
|
||
| ``` | ||
|
|
@@ -415,26 +487,21 @@ metadata: | |
| spec: | ||
| steps: | ||
| - name: step-1 | ||
| - name: step-2 | ||
| resources: | ||
| requests: | ||
| cpu: 1.5 | ||
| memory: 500Mi | ||
| limits: | ||
| memory: 1Gi | ||
| ``` | ||
|
|
||
| The resulting pod would have the following containers: | ||
|
|
||
| | Step name | CPU request | CPU limit | Memory request | Memory limit | | ||
| | --------- | ----------- | --------- | ----------------- | ------------ | | ||
| | step-1 | N/A | N/A | 500Mi | 1Gi | | ||
| | step-2 | 1.5 | N/A | smallest possible | 1Gi | | ||
lbernick marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ## Alternatives | ||
|
|
||
|
|
@@ -466,6 +533,8 @@ reverting to pre-0.28.0 behavior. | |
| (for how resource requests are currently handled) | ||
|
|
||
| [resources]: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ | ||
| [scheduling]: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#how-pods-with-resource-requests-are-scheduled | ||
| [enforcement]: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#how-pods-with-resource-limits-are-run | ||
| [effective]: https://kubernetes.io/docs/concepts/workloads/pods/init-containers/#resources | ||
| [update]: https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#resources | ||
| [init]: https://kubernetes.io/docs/concepts/workloads/pods/init-containers/ | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
a check for the augment on the existing
Task.spec.resourcesfield which containsinputsandoutputsforTaskResourcesthinking perhaps the context will be a bit diff - as
inputs/outputsfor running the actual build task while the addedlimits/requestsfor running the beneath Pod. but after checking other existingresourcesfields, likeTask.Steps.*etc, think still should be a better choice to keep the same field name and add thelimitsandrequests, as for a consistent naming on the same funcs.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.
Yeah, unfortunately
resourcesis an overloaded word here. However, I agree that it makes sense to use it in this context, because it is the terminology Kubernetes uses to describe compute resources, and luckily PipelineResources will be going away which should lessen the confusion.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.
Hmmm...that's unfortunate...would the limits and requests fields be added under the current
resourcesfield?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.
ah I see the problem, this is definitely annoying but I'm hoping naming bikeshedding won't block this PR-- maybe we could call it "compute" or something
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.
an updates from implementation... tentatively I keep the added
requestsandlimitsunder the mentionedresourcesfield for this momenthttps://github.com/tektoncd/pipeline/pull/4877/files?diff=unified&w=0#diff-f67e6d3007cea84a7a2e6301beb54c39b96b75ac9e57d888cbf5cc86c57510f3R77
thinking these reasons:
resourcesfield, likeTask.spec.step.resources,Task.spec.stepTemplate.resourcesThere 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.
It's definitely less than ideal but I think it's ok given that we'll be removing pipelineresources (and better than the alternatives). thanks for the update Austin!
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.
note: we discussed this briefly during last week API working group, and even though the
resourcesfield (PipelineResource) is going away, re-using the same field name is going to create a lot of confusion. We may have to use a more explicit field here even (resource_requirementor something)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.
Hm yeah I think this needs more discussion. How would you feel about merging this PR discussing behavior changes, and I'll open a subsequent one specifically for naming? I added a note here mentioning the name conflict with pipelineresources.