Skip to content

LimitRange calculation should only split Requests for Step Containers #4996

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 17, 2022
Merged

LimitRange calculation should only split Requests for Step Containers #4996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions docs/compute-resources.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ Therefore, the pod will have no effective CPU limit.
Tekton allows users to specify resource requirements of [`Steps`](./tasks.md#defining-steps),
which run sequentially. However, the pod's effective resource requirements are still the
sum of its containers' resource requirements. This means that when specifying resource
requirements for `Step`containers, they must be treated as if they are running in parallel.
requirements for `Step` containers, they must be treated as if they are running in parallel.

Tekton adjusts `Step` resource requirements to comply with [LimitRanges](#limitrange-support).
[ResourceQuotas](#resourcequota-support) are not currently supported.
Expand All @@ -68,26 +68,26 @@ in a `Task's` pod, unless there is a LimitRange present in the namespace.
(Tekton doesn't allow users to configure init containers for a `Task`.)
Tekton supports LimitRange minimum, maximum, and default resource requirements for containers,
but does not support LimitRange ratios between requests and limits ([#4230](https://github.com/tektoncd/pipeline/issues/4230)).
LimitRange types other than "Container" are not supported.
LimitRange types other than "Container" are not considered for purposes of resource requirements.

### Requests

If a `Step` does not have requests defined, the resulting container's requests are the larger of:
If a container does not have requests defined, the resulting container's requests are the larger of:
- 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)
Copy link
Member

@pritidesai pritidesai Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for clarifying this 👍

Copy link
Contributor Author

@skaegi skaegi Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lee's new and improved wording not mine ;)


If a `Step` has requests defined, the resulting container's requests are the larger of:
- the `Step's` requests
If a container has requests defined, the resulting container's requests are the larger of:
- the container's requests
- the LimitRange minimum resource requests

### Limits

If a `Step` does not have limits defined, the resulting container's limits are the smaller of:
If a container does not have limits defined, the resulting container's limits are the smaller of:
- the LimitRange maximum resource limits
- the LimitRange default resource limits

If a `Step` has limits defined, the resulting container's limits are the smaller of:
- the `Step's` limits
If a container has limits defined, the resulting container's limits are the smaller of:
- the container's limits
- the LimitRange maximum resource limits

### Examples
Expand Down Expand Up @@ -116,13 +116,26 @@ A `Task` with 2 `Steps` and no resources specified would result in a pod with th

| Container | CPU request | CPU limit |
| ------------ | ----------- | --------- |
| container 1 | 500m | 2 |
| container 1 | 500m | 2 |
| container 2 | 500m | 2 |

Here, the default CPU request was divided among app containers, and this value was used since it was greater
Here, the default CPU request was divided among the step containers, and this value was used since it was greater
than the minimum request specified by the LimitRange.
The CPU limits are 2 for each container, as this is the default limit specifed in the LimitRange.

If we had a `Task` with 2 `Steps` and 1 `Sidecar` with no resources specified would result in a pod with the following containers:

| Container | CPU request | CPU limit |
| ------------ | ----------- | --------- |
| container 1 | 500m | 2 |
| container 2 | 500m | 2 |
| container 3 | 1 | 2 |

For the first two containers, the default CPU request was divided among the step containers, and this value was used since it was greater
than the minimum request specified by the LimitRange. The third container is a sidecar and since it is not a step container gets the full
default CPU request of 1. AS before the CPU limits are 2 for each container, as this is the default limit specifed in the LimitRange.


Now, consider a `Task` with the following `Step`s:

| Step | CPU request | CPU limit |
Expand Down Expand Up @@ -221,7 +234,7 @@ The maximum of the "min" values is the output "min" value.

Kubernetes allows users to define [ResourceQuotas](https://kubernetes.io/docs/concepts/policy/resource-quotas/),
which restrict the maximum resource requests and limits of all pods running in a namespace.
`TaskRuns` can't currently be created in a namespace with ResourceQuotas
`TaskRuns` can't currently be created in a namespace with ResourceQuotas without siginificant caveats.
([#2933](https://github.com/tektoncd/pipeline/issues/2933)).

# References
Expand Down
58 changes: 40 additions & 18 deletions pkg/internal/limitrange/transformer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,33 +45,45 @@ func NewTransformer(ctx context.Context, namespace string, lister corev1listers.

// The assumption here is that the min, max, default, ratio have already been
// computed if there is multiple LimitRange to satisfy the most (if we can).
// Count the number of containers (that we know) in the Pod.
// Count the number of step containers in the Pod.
// This should help us find the smallest request to apply to containers
nbContainers := len(p.Spec.Containers)
nbStepContainers := 0
for _, c := range p.Spec.Containers {
if pod.IsContainerStep(c.Name) {
Copy link
Member

@pritidesai pritidesai Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@pritidesai pritidesai Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright, so this is only used in pod builder while creating a pod so the state is not relevant here ...

Copy link
Contributor Author

@skaegi skaegi Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

nbStepContainers++
}
}

// FIXME(#4230) maxLimitRequestRatio to support later
defaultLimits := getDefaultLimits(limitRange)
defaultInitRequests := getDefaultInitContainerRequest(limitRange)
defaultContainerLimits := getDefaultLimits(limitRange)
defaultContainerRequests := getDefaultContainerRequest(limitRange)
defaultStepContainerRequests := getDefaultStepContainerRequest(limitRange, nbStepContainers)

for i := range p.Spec.InitContainers {
// We are trying to set the smallest requests possible
if p.Spec.InitContainers[i].Resources.Requests == nil {
p.Spec.InitContainers[i].Resources.Requests = defaultInitRequests
p.Spec.InitContainers[i].Resources.Requests = defaultContainerRequests
} else {
for _, name := range resourceNames {
setRequestsOrLimits(name, p.Spec.InitContainers[i].Resources.Requests, defaultInitRequests)
setRequestsOrLimits(name, p.Spec.InitContainers[i].Resources.Requests, defaultContainerRequests)
}
}
// We are trying to set the highest limits possible
if p.Spec.InitContainers[i].Resources.Limits == nil {
p.Spec.InitContainers[i].Resources.Limits = defaultLimits
p.Spec.InitContainers[i].Resources.Limits = defaultContainerLimits
} else {
for _, name := range resourceNames {
setRequestsOrLimits(name, p.Spec.InitContainers[i].Resources.Limits, defaultLimits)
setRequestsOrLimits(name, p.Spec.InitContainers[i].Resources.Limits, defaultContainerLimits)
}
}
}

defaultRequests := getDefaultAppContainerRequest(limitRange, nbContainers)
for i := range p.Spec.Containers {
for i, c := range p.Spec.Containers {
var defaultRequests = defaultContainerRequests
Copy link
Member

@pritidesai pritidesai Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hardly see this way of initializing and declaring a variable, interesting 🙃

if pod.IsContainerStep(c.Name) {
defaultRequests = defaultStepContainerRequests
}

if p.Spec.Containers[i].Resources.Requests == nil {
p.Spec.Containers[i].Resources.Requests = defaultRequests
} else {
Expand All @@ -80,10 +92,10 @@ func NewTransformer(ctx context.Context, namespace string, lister corev1listers.
}
}
if p.Spec.Containers[i].Resources.Limits == nil {
p.Spec.Containers[i].Resources.Limits = defaultLimits
p.Spec.Containers[i].Resources.Limits = defaultContainerLimits
} else {
for _, name := range resourceNames {
setRequestsOrLimits(name, p.Spec.Containers[i].Resources.Limits, defaultLimits)
setRequestsOrLimits(name, p.Spec.Containers[i].Resources.Limits, defaultContainerLimits)
}
}
}
Expand All @@ -97,9 +109,9 @@ func setRequestsOrLimits(name corev1.ResourceName, dst, src corev1.ResourceList)
}
}

// Returns the default requests to use for each app container, determined by dividing the LimitRange default requests
// among the app containers, and applying the LimitRange minimum if necessary
func getDefaultAppContainerRequest(limitRange *corev1.LimitRange, nbContainers int) corev1.ResourceList {
// Returns the default requests to use for each step container, determined by dividing the LimitRange default requests
// among the step containers, and applying the LimitRange minimum if necessary
func getDefaultStepContainerRequest(limitRange *corev1.LimitRange, nbContainers int) corev1.ResourceList {
// Support only Type Container to start with
var r corev1.ResourceList = map[corev1.ResourceName]resource.Quantity{}
for _, item := range limitRange.Spec.Limits {
Expand All @@ -115,19 +127,29 @@ func getDefaultAppContainerRequest(limitRange *corev1.LimitRange, nbContainers i
if item.Min != nil {
min = item.Min[name]
}

var result resource.Quantity
if name == corev1.ResourceMemory || name == corev1.ResourceEphemeralStorage {
r[name] = takeTheMax(request, *resource.NewQuantity(defaultRequest.Value()/int64(nbContainers), defaultRequest.Format), min)
result = takeTheMax(request, *resource.NewQuantity(defaultRequest.Value()/int64(nbContainers), defaultRequest.Format), min)
} else {
r[name] = takeTheMax(request, *resource.NewMilliQuantity(defaultRequest.MilliValue()/int64(nbContainers), defaultRequest.Format), min)
result = takeTheMax(request, *resource.NewMilliQuantity(defaultRequest.MilliValue()/int64(nbContainers), defaultRequest.Format), min)
}
// only set non-zero request values
if !isZero(result) {
r[name] = result
}
}
}
}
// return nil if the resource list is empty to avoid setting an empty defaultrequest
Copy link
Member

@pritidesai pritidesai Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@skaegi skaegi Jun 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if len(r) == 0 {
return nil
}
return r
}

// Returns the default requests to use for each init container, determined by the LimitRange default requests and minimums
func getDefaultInitContainerRequest(limitRange *corev1.LimitRange) corev1.ResourceList {
func getDefaultContainerRequest(limitRange *corev1.LimitRange) corev1.ResourceList {
// Support only Type Container to start with
var r corev1.ResourceList
for _, item := range limitRange.Spec.Limits {
Expand Down
Loading