Fix docs and remove redundant code for LimitRanges #5351
Conversation
Prior to this commit, LimitRange transformer behavior did not match our documentation. Our documentation stated that Tekton adjusts compute resources of containers to fit within LimitRange minimum requests and maximum limits. This is not true. Instead, Tekton divides LimitRange defaultRequests among Step containers, and considers the minimum only if the defaultRequest divided by the number of Steps would be less than the minimum. In addition, the LimitRange transformer does several things that would be done by Kubernetes anyway, which are removed by this commit. When a LimitRange is created, Kubernetes automatically sets defaults and defaultRequests to the max if a user did not specify them. Kubernetes will automatically apply these defaults and defaultRequests to any pods created. Therefore, there is no need for Tekton to set container limits to the default limits (or in fact, to consider limits at all). There's also no need for Tekton to consider defaults/defaultRequests for Sidecars and init containers, as we are happy to accept Kubernetes' default behavior here. The only resource requirements we need to modify are to split a LimitRange's defaultRequests among Steps that don't specify requests, so all other LimitRange-based transformations are removed in this commit. Note that Tekton makes no guarantees that a TaskRun's pod will be valid (i.e. requests <= limits). If a user configures a TaskRun in a way that is inconsistent with any LimitRanges, we allow Kubernetes to reject the resulting pod.
tekton-robot
added
kind/bug
tekton-robot
added
the
size/L
|
Closes #5278 |
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Instead, Tekton divides LimitRange defaultRequests among Step containers, and considers the minimum only if the defaultRequest divided by the number of Steps would be less than the minimum.
This one is important one in my view and shouldn't be removed. We do not need to do anything smart, but we did add this "support" to, at least, "try" to consume as little as possible, while still being valid.
We've been up and down with this support, but I am not sure reverting some of our decision is the best move — mainly, as of today, I "feel" we are doing probably the least amount of work that we could/should do — but it could make sense to be even less smart and just heavily document how to use LimitRange with Tekton.
I think one of the main "trick" here is that end-users might want to use the same LimitRange for Tekton workloads and their own workloads, which might not work for the best.
|
@vdemeester I'm a bit confused-- this PR isn't intended to cause any changes in behavior; I think I might have just misunderstood what the limitrange transformer does when I wrote these docs. I agree we don't want to be repeatedly changing this behavior.
Yeah that is tricky :/ |
Oh I see. Well I think it does, here. I don't think the LimitRange controller sets requests or limit to the specified min version in case a default is not present. |
I think a default will always be present-- k8s always creates the default and sets it to the max as in this example |
Ah, interesting 🙃, for some reason I didn't think that was the case 🤔 (maybe on some arcane cluster that maybe didn't have that behavior…). In that case, we might be fine with it 😅 |
|
@vdemeester would you mind taking another look at this? or are there any unresolved concerns about the behavior re: defaults? |
tekton-robot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jerop, 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 |
Changes
Prior to this commit, LimitRange transformer behavior did not match our documentation.
Our documentation stated that Tekton adjusts compute resources of containers to fit within
LimitRange minimum requests and maximum limits. This is not true. Instead, Tekton divides LimitRange
defaultRequests among Step containers, and considers the minimum only if the defaultRequest divided
by the number of Steps would be less than the minimum.
In addition, the LimitRange transformer does several things that would be done by Kubernetes anyway,
which are removed by this commit.
When a LimitRange is created, Kubernetes automatically sets defaults and defaultRequests to the max
if a user did not specify them. Kubernetes will automatically apply these defaults and defaultRequests
to any pods created. Therefore, there is no need for Tekton to set container limits to the default limits
(or in fact, to consider limits at all). There's also no need for Tekton to consider defaults/defaultRequests
for Sidecars and init containers, as we are happy to accept Kubernetes' default behavior here.
The only resource requirements we need to modify are to split a LimitRange's defaultRequests among Steps that
don't specify requests, so all other LimitRange-based transformations are removed in this commit.
Note that Tekton makes no guarantees that a TaskRun's pod will be valid (i.e. requests <= limits).
If a user configures a TaskRun in a way that is inconsistent with any LimitRanges, we allow Kubernetes
to reject the resulting pod.
/kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes