WIP: Revamp how pipeline supports Limitranges

Up until now, pipeline support for LimitRange is rather limited and
confusing and can lead to inconsistency:
- It is not applied to `InitContainers`
- It zero-out user requests to keep the max and assign to one step,
  which can lead to invalid `Pod` definition.
- It uses only `Min` and never reads `Default` and `DefaultRequest`
- It also doesn't support `MaxLimitRequestRatio`

This commits aims to fix that by adding more support to LimitRange.
Note that, to understand some of the choice, some assumption on how
LimitRange works is required.

On `Pod` Containers:
- *Requests* are not enforced. If the node has more resource available than the request, the
  container can use it.
- *Limits* on the other hand, are a hard stop. A container going over the limit, will be
  killed.

It is thus important to get `Requests` right, to allow scheduling.

Requests and limits can come from both Containers and Init Containers.
- For init containers, the max of each type is taken
- For containers, it sums all requests/limits for each containers

This means, if you got the following:
- initContainer1 : 1 CPU, 100m memory
- initContainer2 : 2 CPU, 200m memory
- container1 : 1 CPU, 50m memory
- container2 : 2 CPU, 250m memory
- container3 : 3 CPU, 500m memory

The computation will be:
- CPU : 2 (max init containers) + 6 (sum of containers) = 8 CPU
- Memory: 200m (max init containers) + 800m (sum of containers) =
1000m (1G)

LimitRange enforce (mutates the `Pod`) some Limits and Requests (using
`Default` and `DefaultRequest`) and validate those (`Min`, `Max` and
`MaxLimitRequestRatio`). They are applied by namespace, and it is
possible to have multiple `LimitRange` in a namespace.

The way Limits and Requests works in Kubernetes is because it is assumed that all
containers run in parallel (which they do — except in tekton with some hack), and init
container run before, each one after the others.

That assumption — running in parallel — is not really true in Tekton. They do all start
together (because there is no way around this) *but* the /entrypoint hack/ is making sure they
actually run in sequence and thus there is always only one container that is actually
consuming some resource at the same time.

This means, we need to handle limits, request and LimitRanges in a /non-standard/ way. Let's
try to define that. Tekton needs to take into account all the aspect of the LimitRange :
the min/max as well as the default. If there is no default, but there is min/max, Tekton
need then to *set* a default value that is between the min/max. If we set the value too low,
the Pod won't be able to be created, similar if we set the value too high. *But* those
values are set on *containers*, so we *have to* do our own computation to know what request to
put on each containers. To add to the complexity here, we also need to support
`MaxLimitRequestRatio`, which is just adding complexity on top of something complex. That
said, ideally, if we take the default correctly, we should be able to have support for
`MaxLimitRequestRatio` for free.

This commits tries to add support for this, by computing the minimum
request to apply that satisfy the `LimitRange`(s), applying them to
`Containers` as well `InitContainers`.

Note: If there is multiple `LimitRange` in the namespace, Tekton tries
to make the best out of it *but* if they are conflicting with each
other (a `Max` on one that is smaller than the `Min` on the other),
its the user responsability.

Signed-off-by: Vincent Demeester <[email protected]>

internal/builder/v1beta1/pod.go

@@ -18,7 +18,6 @@ package builder
 
 import (
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -123,15 +122,6 @@ func PodContainer(name, image string, ops ...ContainerOp) PodSpecOp {
 		c := &corev1.Container{
 			Name:  name,
 			Image: image,
-			// By default, containers request zero resources. Ops
-			// can override this.
-			Resources: corev1.ResourceRequirements{
-				Requests: corev1.ResourceList{
-					corev1.ResourceCPU:              resource.MustParse("0"),
-					corev1.ResourceMemory:           resource.MustParse("0"),
-					corev1.ResourceEphemeralStorage: resource.MustParse("0"),
-				},
-			},
 		}
 		for _, op := range ops {
 			op(c)

internal/builder/v1beta1/pod_test.go

@@ -85,15 +85,9 @@ func TestPod(t *testing.T) {
 			ServiceAccountName: "sa",
 			RestartPolicy:      corev1.RestartPolicyNever,
 			Containers: []corev1.Container{{
-				Name:  "nop",
-				Image: "nop:latest",
-				Resources: corev1.ResourceRequirements{
-					Requests: corev1.ResourceList{
-						corev1.ResourceCPU:              resource.MustParse("0"),
-						corev1.ResourceMemory:           resource.MustParse("0"),
-						corev1.ResourceEphemeralStorage: resource.MustParse("0"),
-					},
-				},
+				Name:      "nop",
+				Image:     "nop:latest",
+				Resources: corev1.ResourceRequirements{},
 			}},
 			InitContainers: []corev1.Container{{
 				Name:       "basic",

pkg/pod/limitrange/doc.go

@@ -0,0 +1,17 @@
+/*
+Copyright 2020 The Tekton Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+// Package limitrange defines logic for supporting Kubernetes LimitRange for the specific Tekton use cases
+package limitrange

pkg/pod/limitrange/limitrange.go

@@ -0,0 +1,41 @@
+/*
+Copyright 2020 The Tekton Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+package limitrange
+
+import (
+	"context"
+	"fmt"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes"
+)
+
+func getVirtualLimitRange(ctx context.Context, namespace string, c kubernetes.Interface) (*corev1.LimitRange, error) {
+	limitRanges, err := c.CoreV1().LimitRanges(namespace).List(ctx, metav1.ListOptions{})
+	if err != nil {
+		return nil, err
+	}
+	// No LimitRange defined
+	if len(limitRanges.Items) == 0 {
+		return nil, nil
+	}
+	if len(limitRanges.Items) == 1 {
+		return &limitRanges.Items[0], nil
+	}
+	// FIXME(vdemester) support this
+	return nil, fmt.Errorf("Multiple limitRanges not supported yet")
+}

pkg/pod/limitrange/transformer.go

@@ -0,0 +1,167 @@
+/*
+Copyright 2020 The Tekton Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+package limitrange
+
+import (
+	"context"
+
+	"github.com/tektoncd/pipeline/pkg/pod"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/client-go/kubernetes"
+)
+
+var emptyResourceQuantity = resource.Quantity{}
+
+// var zeroQty = resource.MustParse("0")
+var resourceNames = []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory, corev1.ResourceEphemeralStorage}
+
+func isZero(q resource.Quantity) bool {
+	return (&q).IsZero()
+}
+
+func NewTransformer(ctx context.Context, namespace string, clientset kubernetes.Interface) pod.Transformer {
+	return func(p *corev1.Pod) (*corev1.Pod, error) {
+		limitRange, err := getVirtualLimitRange(ctx, namespace, clientset)
+		if err != nil {
+			return p, err
+		}
+		// No LimitRange defined, nothing to transform, bail early we don't have anything to transform.
+		if limitRange == nil {
+			return p, nil
+		}
+
+		// 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.
+		// This should help us find the smallest request to apply to containers
+		// We are adding +1 to the number of container to take into account the init containers.
+		// The reason to use +1 only is that, k8s tread request on all init container as one (getting the max of all)
+		nbContainers := len(p.Spec.Containers) + 1
+		// maxLimitRequestRatios := getMaxLimitRequestRatios(limitRange) // FIXME(vdemeester) to support later
+		defaultRequests := getDefaultRequest(limitRange, nbContainers)
+		// defaultLimits := getDefaultLimits(limitRange, defaultRequests, maxLimitRequestRatios)
+		defaultLimits := getDefaultLimits(limitRange)
+
+		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 = defaultRequests
+			} else {
+				for _, name := range resourceNames {
+					setRequestsOrLimits(name, p.Spec.InitContainers[i].Resources.Requests, defaultRequests)
+				}
+			}
+			// We are trying to set the highest limits possible
+			if p.Spec.InitContainers[i].Resources.Limits == nil {
+				p.Spec.InitContainers[i].Resources.Limits = defaultLimits
+			} else {
+				for _, name := range resourceNames {
+					setRequestsOrLimits(name, p.Spec.InitContainers[i].Resources.Limits, defaultLimits)
+				}
+			}
+		}
+
+		for i := range p.Spec.Containers {
+			if p.Spec.Containers[i].Resources.Requests == nil {
+				p.Spec.Containers[i].Resources.Requests = defaultRequests
+			} else {
+				for _, name := range resourceNames {
+					setRequestsOrLimits(name, p.Spec.Containers[i].Resources.Requests, defaultRequests)
+				}
+			}
+			if p.Spec.Containers[i].Resources.Limits == nil {
+				p.Spec.Containers[i].Resources.Limits = defaultLimits
+			} else {
+				for _, name := range resourceNames {
+					setRequestsOrLimits(name, p.Spec.Containers[i].Resources.Limits, defaultLimits)
+				}
+			}
+		}
+		return p, nil
+	}
+}
+
+func setRequestsOrLimits(name corev1.ResourceName, dst, src corev1.ResourceList) {
+	if isZero(dst[name]) && !isZero(src[name]) {
+		dst[name] = src[name]
+	}
+}
+
+func getDefaultRequest(limitRange *corev1.LimitRange, nbContainers int) corev1.ResourceList {
+	// Support only Type Container to start with
+	// FIXME(vdemeester) figure out what to do with Pod type
+	var r corev1.ResourceList = map[corev1.ResourceName]resource.Quantity{}
+	for _, item := range limitRange.Spec.Limits {
+		// Only support LimitTypeContainer
+		if item.Type == corev1.LimitTypeContainer {
+			for _, name := range resourceNames {
+				var defaultRequest resource.Quantity
+				var min resource.Quantity
+				request := r[name]
+				if item.DefaultRequest != nil {
+					defaultRequest = item.DefaultRequest[name]
+				}
+				if item.Min != nil {
+					min = item.Min[name]
+				}
+				r[name] = takeTheMax(request, *resource.NewMilliQuantity(defaultRequest.MilliValue()/int64(nbContainers), defaultRequest.Format), min)
+			}
+		}
+	}
+	return r
+}
+
+func takeTheMax(r, d, m resource.Quantity) resource.Quantity {
+	var q resource.Quantity = r
+	if d.Cmp(q) > 0 {
+		q = d
+	}
+	if m.Cmp(q) > 0 {
+		q = m
+	}
+	return q
+}
+
+func getDefaultLimits(limitRange *corev1.LimitRange) corev1.ResourceList {
+	// Support only Type Container to start with
+	// FIXME(vdemeester) figure out what to do with Pod type
+	var l corev1.ResourceList
+	for _, item := range limitRange.Spec.Limits {
+		if item.Type == corev1.LimitTypeContainer {
+			if item.Default != nil {
+				l = item.Default
+			} else if item.Max != nil {
+				l = item.Max
+			}
+		}
+	}
+	return l
+}
+
+func getMaxLimitRequestRatios(limitRange *corev1.LimitRange) corev1.ResourceList {
+	// Support only Type Container to start with
+	// FIXME(vdemeester) figure out what to do with Pod type
+	var l corev1.ResourceList
+	for _, item := range limitRange.Spec.Limits {
+		if item.Type == corev1.LimitTypeContainer {
+			if item.MaxLimitRequestRatio != nil {
+				l = item.MaxLimitRequestRatio
+			}
+		}
+	}
+	return l
+}