LimitRange calculation should only split Requests for Step Containers #4996
Conversation
tekton-robot
added
release-note
tekton-robot
added
the
size/L
|
/cc @lbernick |
skaegi
changed the title
|
/retest |
|
The following is the coverage report on the affected files.
|
This commit changes how resource requirements are calculated when one or more LimitRanges are present in the namespace where a TaskRun is. We now will only transform "step" containers and exclude all other app containers (e.g. sidecars and any other containers) when splitting "requests" from LimitRanges. Existing tests have been fixed to use "Step" containers and two additional tests have been added to verify behavior when a sidecar is present. Docs have been updated to reflect this change
skaegi
force-pushed
the
sidecarfix-limitrange
branch
from
828ecdd to
bbc549b
Compare
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lbernick 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 |
tekton-robot
added
the
approved
|
/test pull-tekton-pipeline-alpha-integration-tests |
|
/test pull-tekton-pipeline-integration-tests |
| - the LimitRange minimum resource requests | ||
| - the LimitRange default resource requests, divided among the app containers | ||
| - the LimitRange default resource requests (for init and Sidecar containers) or the LimitRange default resource requests divided by the number of Step containers (for Step containers) |
There was a problem hiding this comment.
thank you for clarifying this 👍
There was a problem hiding this comment.
Lee's new and improved wording not mine ;)
| nbContainers := len(p.Spec.Containers) | ||
| nbStepContainers := 0 | ||
| for _, c := range p.Spec.Containers { | ||
| if pod.IsContainerStep(c.Name) { |
There was a problem hiding this comment.
do we need to check for the running containers or any other specific state (not-running) of the container here? or the state of container does not matter at this point since they will be spawned with the configuration being determined here?
There was a problem hiding this comment.
alright, so this is only used in pod builder while creating a pod so the state is not relevant here ...
There was a problem hiding this comment.
The transform happens before the pod spec is applied so the state of the container does not matter at this point. We use the number of step containers to help choose the optimal request size at configuration generation time.
| defaultRequests := getDefaultAppContainerRequest(limitRange, nbContainers) | ||
| for i := range p.Spec.Containers { | ||
| for i, c := range p.Spec.Containers { | ||
| var defaultRequests = defaultContainerRequests |
There was a problem hiding this comment.
I hardly see this way of initializing and declaring a variable, interesting 🙃
| } | ||
| } | ||
| } | ||
| } | ||
| // return nil if the resource list is empty to avoid setting an empty defaultrequest |
There was a problem hiding this comment.
NIT: should this be done for the other two other get functions - getDefaultContainerRequest and getDefaultLimits? I havent looked at whether its needed or not but just wondering since we added this here.
There was a problem hiding this comment.
Glad you noticed ;) They actually already will return nil. The getStepContainerRequest creates an empty request list before trying to populate so this empty check to return nil makes it consistent with the others.
|
thank you @skaegi, I have one /lgtm |
Changes
This commit changes how resource requirements are calculated when one or more LimitRanges are present in the namespace where a TaskRun is.
We now will only transform "step" containers and exclude all other app containers (e.g. sidecars and any other containers) when splitting "requests" from LimitRanges.
Existing tests have been fixed to use "Step" containers and two additional tests have been added to verify behavior when a sidecar is present.
Docs have been updated to reflect this change
Cleanup:
resourcesrequirements for a container as this is not needed and adds clutter.Fixes #4978
/kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
(if there are no user facing changes, use release note "NONE")
Release Notes