LimitRange calculation to use only Requests for Step Container

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

docs/compute-resources.md

@@ -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.
@@ -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 or if a `Step` the result of dividing this value among the other `Step` containers
 
-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
@@ -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 |
@@ -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

pkg/internal/limitrange/transformer.go

@@ -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) {
+				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
+			if pod.IsContainerStep(c.Name) {
+				defaultRequests = defaultStepContainerRequests
+			}
+
 			if p.Spec.Containers[i].Resources.Requests == nil {
 				p.Spec.Containers[i].Resources.Requests = defaultRequests
 			} else {
@@ -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)
 				}
 			}
 		}
@@ -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 {
@@ -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
+	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 {