community icon indicating copy to clipboard operation
community copied to clipboard

[TEP-0119] Simplifying compute resources API

Open lbernick opened this issue 3 years ago • 1 comments

This commit proposes API changes to provide a more consistent user experience for compute resources.

/kind tep

lbernick avatar Aug 16 '22 16:08 lbernick

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: To complete the pull request process, please ask for approval from lbernick after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

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

tekton-robot avatar Aug 16 '22 16:08 tekton-robot

API WG 08/22

summary: deprecating step-level compute resources and renaming fields

/assign @dibyom

jerop avatar Aug 22 '22 16:08 jerop

@lbernick can you help me with a reference link on how Task-level computeResources are translated into the resources for each individual step. I'd like to make sure that multi-step tasks work which consist of steps that need near to no resources but also a step (or multiple) that require many resources. An example is https://github.com/tektoncd/catalog/blob/main/task/buildpacks/0.5/buildpacks.yaml: while the prepare and results steps are simple bash scripts that need a few CPU millicores and near to no memory; there is the Buildpacks create step which will need much more.

SaschaSchwarze0 avatar Aug 22 '22 16:08 SaschaSchwarze0

I'd like to make sure that multi-step tasks work which consist of steps that need near to no resources but also a step (or multiple) that require many resources. An example is https://github.com/tektoncd/catalog/blob/main/task/buildpacks/0.5/buildpacks.yaml: while the prepare and results steps are simple bash scripts that need a few CPU millicores and near to no memory; there is the Buildpacks create step which will need much more.

@SaschaSchwarze0 great question! the full design proposal is here. TL;DR: Task-level requests are divided among containers so that the kubelet reserves the correct amount of resources for the pod during scheduling, and task-level limits are applied as-is to each container so that the container runtime enforces the correct limits. I'd think for the use case you've described it would make sense to set the task-level requirements to the requirements of the most compute intensive step.

lbernick avatar Aug 23 '22 13:08 lbernick

I'd like to make sure that multi-step tasks work which consist of steps that need near to no resources but also a step (or multiple) that require many resources. An example is https://github.com/tektoncd/catalog/blob/main/task/buildpacks/0.5/buildpacks.yaml: while the prepare and results steps are simple bash scripts that need a few CPU millicores and near to no memory; there is the Buildpacks create step which will need much more.

@SaschaSchwarze0 great question! the full design proposal is here. TL;DR: Task-level requests are divided among containers so that the kubelet reserves the correct amount of resources for the pod during scheduling, and task-level limits are applied as-is to each container so that the container runtime enforces the correct limits. I'd think for the use case you've described it would make sense to set the task-level requirements to the requirements of the most compute intensive step.

I don't think that works @lbernick assuming I correctly understand Kubernetes. Let's assume the simple case of having two steps. I am on task-level assigning 2 CPU cores and 2 GB memory. If I understand it correctly, then we will have the following on the two containers:

  • requests: 1 CPU core and 1 GB memory
  • limits: 2 CPU cores and 2 GB memory

This means that one container has guaranteed 1 GB memory and can take up to 2 if the node has unrequested capacity. But this is not guaranteed as the node can simply be full.

Let's assume we have a node which has only two cores and 2 GB memory. By chance this node has no pods and now is taken for this TaskRun. Now none of the two containers will ever be able to use more than 1 CPU core nor more than 1 GB memory. The reason is that all resources of the node are requested and therefore reserved. Kubernetes will not give resources that are reserved by one container to another one.

SaschaSchwarze0 avatar Aug 23 '22 13:08 SaschaSchwarze0

Let's assume the simple case of having two steps. I am on task-level assigning 2 CPU cores and 2 GB memory. If I understand it correctly, then we will have the following on the two containers:

  • requests: 1 CPU core and 1 GB memory
  • limits: 2 CPU cores and 2 GB memory

This means that one container has guaranteed 1 GB memory and can take up to 2 if the node has unrequested capacity. But this is not guaranteed as the node can simply be full.

Let's assume we have a node which has only two cores and 2 GB memory. By chance this node has no pods and now is taken for this TaskRun. Now none of the two containers will ever be able to use more than 1 CPU core nor more than 1 GB memory. The reason is that all resources of the node are requested and therefore reserved. Kubernetes will not give resources that are reserved by one container to another one.

Thanks for this example! Thinking about how you could ensure the buildpacks create step has enough resources reserved, you could specify task level limits of 2 and task level requests of 4 -- however it's confusing to have requests > limits, and this will also reserve too much compute for the steps that don't need it. What you're describing sounds like a good reason to keep taskRun.spec.stepOverrides[].resources (maybe renamed to taskRun.spec.stepSpecs[].computeResources).

Do you have any reason to keep task.spec.steps[].resources, or do you think your needs are met by taskRun.spec.stepOverrides[].resources ?

lbernick avatar Aug 23 '22 13:08 lbernick

Do you have any reason to keep task.spec.steps[].resources, or do you think your needs are met by taskRun.spec.stepOverrides[].resources ?

This is a good question. The reason why I engaged here was to make sure that the scenario of having different resource needs in steps of a Tekton Task can still be specified. As I am seeing most things from Shipwright perspective where we're only using standalone TaskRuns, then either is good I think.

But let's look at the scenario just from Tekton perspective. I had grabbed the Buildpacks sample Task. It has three steps. The question is who knows how many resources those steps will consume. Is it the author of the Task or the person who runs it and therefore creates the TaskRun ? And I think the answer is "it depends". For the simple prepare step and the simple results step, the Task author can define their low resources requirements and those should be fine for all TaskRuns. The create step is then different because there the real work happens. If you run Buildpacks to simply package a set of Python scripts, then maybe 0.5 CPU cores and 256 M memory are enough. But if you run Buildpacks on a Go source where it must compile a lot of dependencies and the application(s), then 0.5 CPU cores would mean it compiles forever and 256 M memory means it goes OOM. You'll rather want as much CPU as possible and could require several GB of memory to get it through. If the task author would have to define the final resources to capture all possible builds, it would be a waste of resources because (s)he would have to chose really high values. So, one would probably want to define a lower set of resources that can handle 80 % of the scenarioes in the Task, but TaskRun users will want to override it if they build a really large application.

SaschaSchwarze0 avatar Aug 23 '22 14:08 SaschaSchwarze0

Thanks @SaschaSchwarze0. I might have been a bit too eager to remove some of these fields :) It seems like the best option might be to keep these fields around for now and wait until we have more feedback on some of these alpha features, especially since there do seem to be use cases for all of them. I still think we might want to rename some of these fields for consistency's sake, but I'll open a separate PR for that on TEP-0096 (it doesn't need to be its own TEP).

lbernick avatar Aug 25 '22 14:08 lbernick

Proposed in https://github.com/tektoncd/community/pull/798 -- @dibyom @vdemeester @SaschaSchwarze0 PTAL

lbernick avatar Aug 25 '22 14:08 lbernick