Refactor LimitRange package

This commit separates logic for calculating a "virtual" limitrange from all limitranges
present in a namespace from logic for setting pod compute resources based on the virtual limitrange,
and adds tests. This makes it much easier to add tests for scenarios with multiple limitranges
and multiple containers, because each component can be tested separately.

No functional changes.

pkg/internal/limitrange/limitrange.go → pkg/internal/computeresources/limitrange/limitrange.go

@@ -23,7 +23,18 @@ import (
 	corev1listers "k8s.io/client-go/listers/core/v1"
 )
 
-func getVirtualLimitRange(namespace string, lister corev1listers.LimitRangeLister) (*corev1.LimitRange, error) {
+// GetVirtualLimitRange returns a pointer to a single LimitRange representing the most restrictive
+// requirements of all LimitRanges present in the namespace, or a nil pointer if there are no LimitRanges.
+// This LimitRange meets the following constraints:
+// - Its max is the smallest max of all the LimitRanges
+// - Its min is the largest min of all the LimitRanges
+// - Its maxLimitRequestRatio is the smallest maxLimitRequestRatio of all the LimitRanges
+// - Its default is the smallest default of any of the LimitRanges that fits within the minimum and maximum
+// - Its defaultRequest is the smallest defaultRequest of any of the LimitRanges that fits within the minimum and maximum
+//
+// This function isn't guaranteed to return a LimitRange with consistent constraints.
+// For example, the minimum could be greater than the maximum.
+func GetVirtualLimitRange(namespace string, lister corev1listers.LimitRangeLister) (*corev1.LimitRange, error) {
 	limitRanges, err := lister.LimitRanges(namespace).List(labels.Everything())
 	if err != nil {
 		return nil, err
@@ -38,12 +49,6 @@ func getVirtualLimitRange(namespace string, lister corev1listers.LimitRangeListe
 		limitRange = limitRanges[0]
 	default:
 		// Several LimitRange defined
-		// Create a virtual LimitRange with
-		// - Maximum of min values
-		// - Minimum of max values
-		// - Default that "fits" into min/max taken above
-		// - Default request that "fits" into min/max taken above
-		// - Smallest ratio (aka the most restrictive one)
 		limitRange = &corev1.LimitRange{}
 		m := map[corev1.LimitType]corev1.LimitRangeItem{}
 		for _, lr := range limitRanges {
@@ -93,6 +98,11 @@ func getVirtualLimitRange(namespace string, lister corev1listers.LimitRangeListe
 	return limitRange, nil
 }
 
+// IsZero returns true if the resource quantity has a zero value
+func IsZero(q resource.Quantity) bool {
+	return (&q).IsZero()
+}
+
 func maxOf(a, b resource.Quantity) resource.Quantity {
 	if (&a).Cmp(b) > 0 {
 		return a
@@ -101,14 +111,14 @@ func maxOf(a, b resource.Quantity) resource.Quantity {
 }
 
 func minOf(a, b resource.Quantity) resource.Quantity {
-	if isZero(a) || (&a).Cmp(b) > 0 {
+	if IsZero(a) || (&a).Cmp(b) > 0 {
 		return b
 	}
 	return a
 }
 
 func minOfBetween(a, b, min, max resource.Quantity) resource.Quantity {
-	if isZero(a) || (&a).Cmp(b) > 0 {
+	if IsZero(a) || (&a).Cmp(b) > 0 {
 		return b
 	}
 	return a

pkg/internal/computeresources/limitrange/limitrange_test.go

@@ -0,0 +1,317 @@
+/*
+Copyright 2022 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_test
+
+import (
+	"context"
+	"strconv"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/tektoncd/pipeline/pkg/internal/computeresources/limitrange"
+	ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing"
+	"github.com/tektoncd/pipeline/test"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	fakekubeclient "knative.dev/pkg/client/injection/kube/client/fake"
+	fakelimitrangeinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/limitrange/fake"
+	fakeserviceaccountinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount/fake"
+)
+
+func setupTestData(t *testing.T, serviceaccounts []corev1.ServiceAccount, limitranges []corev1.LimitRange) (context.Context, func()) {
+	ctx, _ := ttesting.SetupFakeContext(t)
+	ctx, cancel := context.WithCancel(ctx)
+	kubeclient := fakekubeclient.Get(ctx)
+	// LimitRange
+	limitRangeInformer := fakelimitrangeinformer.Get(ctx)
+	kubeclient.PrependReactor("*", "limitranges", test.AddToInformer(t, limitRangeInformer.Informer().GetIndexer()))
+	for _, tl := range limitranges {
+		if _, err := kubeclient.CoreV1().LimitRanges(tl.Namespace).Create(ctx, &tl, metav1.CreateOptions{}); err != nil {
+			t.Fatal(err)
+		}
+	}
+	// ServiceAccount
+	serviceAccountInformer := fakeserviceaccountinformer.Get(ctx)
+	kubeclient.PrependReactor("*", "serviceaccounts", test.AddToInformer(t, serviceAccountInformer.Informer().GetIndexer()))
+	for _, ts := range serviceaccounts {
+		if _, err := kubeclient.CoreV1().ServiceAccounts(ts.Namespace).Create(ctx, &ts, metav1.CreateOptions{}); err != nil {
+			t.Fatal(err)
+		}
+	}
+	kubeclient.ClearActions()
+	return ctx, cancel
+}
+
+func TestGetVirtualLimitRange(t *testing.T) {
+	limitRange1 := corev1.LimitRange{
+		ObjectMeta: metav1.ObjectMeta{Name: "limitrange1", Namespace: "default"},
+		Spec: corev1.LimitRangeSpec{
+			Limits: []corev1.LimitRangeItem{{
+				Type: corev1.LimitTypeContainer,
+				Max: corev1.ResourceList{
+					corev1.ResourceCPU:              resource.MustParse("3"),
+					corev1.ResourceMemory:           resource.MustParse("300Mi"),
+					corev1.ResourceEphemeralStorage: resource.MustParse("3Gi"),
+				},
+				Min: corev1.ResourceList{
+					corev1.ResourceCPU:              resource.MustParse("700m"),
+					corev1.ResourceMemory:           resource.MustParse("70Mi"),
+					corev1.ResourceEphemeralStorage: resource.MustParse("700Mi"),
+				},
+				Default: corev1.ResourceList{
+					corev1.ResourceCPU:              resource.MustParse("2"),
+					corev1.ResourceMemory:           resource.MustParse("200Mi"),
+					corev1.ResourceEphemeralStorage: resource.MustParse("2Gi"),
+				},
+				DefaultRequest: corev1.ResourceList{
+					corev1.ResourceCPU:              resource.MustParse("1"),
+					corev1.ResourceMemory:           resource.MustParse("100Mi"),
+					corev1.ResourceEphemeralStorage: resource.MustParse("1Gi"),
+				},
+			}}}}
+
+	for _, tc := range []struct {
+		description string
+		limitranges []corev1.LimitRange
+		want        *corev1.LimitRange
+	}{{
+		description: "No limitRange",
+		want:        nil,
+	}, {
+		description: "Single LimitRange",
+		limitranges: []corev1.LimitRange{limitRange1},
+		want:        &limitRange1,
+	}, {
+		description: "multiple LimitRanges; min only",
+		limitranges: []corev1.LimitRange{{
+			ObjectMeta: metav1.ObjectMeta{Name: "limitrange1", Namespace: "default"},
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type: corev1.LimitTypeContainer,
+					Min:  createResourceList(7),
+				}}}}, {
+			ObjectMeta: metav1.ObjectMeta{Name: "limitrange2", Namespace: "default"},
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type: corev1.LimitTypeContainer,
+					Min:  createResourceList(8),
+				}}}},
+		},
+		want: &corev1.LimitRange{
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type: corev1.LimitTypeContainer,
+					Min:  createResourceList(8),
+				}}}},
+	}, {
+		description: "multiple LimitRanges; max only",
+		limitranges: []corev1.LimitRange{{
+			ObjectMeta: metav1.ObjectMeta{Name: "limitrange1", Namespace: "default"},
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type: corev1.LimitTypeContainer,
+					Max:  createResourceList(7),
+				}}}}, {
+			ObjectMeta: metav1.ObjectMeta{Name: "limitrange2", Namespace: "default"},
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type: corev1.LimitTypeContainer,
+					Max:  createResourceList(8),
+				}}}},
+		},
+		want: &corev1.LimitRange{
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type: corev1.LimitTypeContainer,
+					Max:  createResourceList(7),
+				}}}},
+	}, {
+		description: "multiple LimitRanges; maxLimitRequestRatio only",
+		limitranges: []corev1.LimitRange{{
+			ObjectMeta: metav1.ObjectMeta{Name: "limitrange1", Namespace: "default"},
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type:                 corev1.LimitTypeContainer,
+					MaxLimitRequestRatio: createResourceList(7),
+				}}}}, {
+			ObjectMeta: metav1.ObjectMeta{Name: "limitrange2", Namespace: "default"},
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type:                 corev1.LimitTypeContainer,
+					MaxLimitRequestRatio: createResourceList(8),
+				}}}},
+		},
+		want: &corev1.LimitRange{
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type:                 corev1.LimitTypeContainer,
+					MaxLimitRequestRatio: createResourceList(7),
+				}}}},
+	}, {
+		description: "multiple LimitRanges; default only",
+		limitranges: []corev1.LimitRange{{
+			ObjectMeta: metav1.ObjectMeta{Name: "limitrange1", Namespace: "default"},
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type:    corev1.LimitTypeContainer,
+					Default: createResourceList(7),
+				}}}}, {
+			ObjectMeta: metav1.ObjectMeta{Name: "limitrange2", Namespace: "default"},
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type:    corev1.LimitTypeContainer,
+					Default: createResourceList(8),
+				}}}},
+		},
+		want: &corev1.LimitRange{
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type:    corev1.LimitTypeContainer,
+					Default: createResourceList(7),
+				}}}},
+	}, {
+		description: "multiple LimitRanges; defaultRequest only",
+		limitranges: []corev1.LimitRange{{
+			ObjectMeta: metav1.ObjectMeta{Name: "limitrange1", Namespace: "default"},
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type:           corev1.LimitTypeContainer,
+					DefaultRequest: createResourceList(7),
+				}}}}, {
+			ObjectMeta: metav1.ObjectMeta{Name: "limitrange2", Namespace: "default"},
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type:           corev1.LimitTypeContainer,
+					DefaultRequest: createResourceList(8),
+				}}}},
+		},
+		want: &corev1.LimitRange{
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type:           corev1.LimitTypeContainer,
+					DefaultRequest: createResourceList(7),
+				}}}},
+	}, {
+		description: "multiple LimitRanges; all fields",
+		limitranges: []corev1.LimitRange{{
+			ObjectMeta: metav1.ObjectMeta{Name: "limitrange1", Namespace: "default"},
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type:                 corev1.LimitTypeContainer,
+					Min:                  createResourceList(3),
+					Max:                  createResourceList(10),
+					MaxLimitRequestRatio: createResourceList(3),
+					Default:              createResourceList(8),
+					DefaultRequest:       createResourceList(7),
+				}}}}, {
+			ObjectMeta: metav1.ObjectMeta{Name: "limitrange2", Namespace: "default"},
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type:                 corev1.LimitTypeContainer,
+					Min:                  createResourceList(1),
+					Max:                  createResourceList(5),
+					MaxLimitRequestRatio: createResourceList(2),
+					Default:              createResourceList(5),
+					DefaultRequest:       createResourceList(3),
+				}}}},
+		},
+		want: &corev1.LimitRange{
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type:                 corev1.LimitTypeContainer,
+					Min:                  createResourceList(3),
+					Max:                  createResourceList(5),
+					MaxLimitRequestRatio: createResourceList(2),
+					Default:              createResourceList(5),
+					DefaultRequest:       createResourceList(3),
+				}}}},
+	}, {
+		description: "multiple contradictory LimitRanges",
+		limitranges: []corev1.LimitRange{{
+			ObjectMeta: metav1.ObjectMeta{Name: "limitrange1", Namespace: "default"},
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type: corev1.LimitTypeContainer,
+					Min:  createResourceList(6),
+					Max:  createResourceList(10),
+				}}}}, {
+			ObjectMeta: metav1.ObjectMeta{Name: "limitrange2", Namespace: "default"},
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type: corev1.LimitTypeContainer,
+					Min:  createResourceList(1),
+					Max:  createResourceList(5),
+				}}}},
+		},
+		want: &corev1.LimitRange{
+			Spec: corev1.LimitRangeSpec{
+				Limits: []corev1.LimitRangeItem{{
+					Type: corev1.LimitTypeContainer,
+					Min:  createResourceList(6),
+					Max:  createResourceList(5),
+				}}}},
+	}} {
+		t.Run(tc.description, func(t *testing.T) {
+			ctx, cancel := setupTestData(t,
+				[]corev1.ServiceAccount{{ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "default"}}},
+				tc.limitranges,
+			)
+			defer cancel()
+			lister := fakelimitrangeinformer.Get(ctx).Lister()
+			got, err := limitrange.GetVirtualLimitRange("default", lister)
+			if err != nil {
+				t.Errorf("Unexpected error %s", err)
+			}
+			if d := cmp.Diff(tc.want, got, resourceQuantityCmp, equateEmpty()); d != "" {
+				t.Errorf("Unexpected output for virtual limit range: %s", d)
+			}
+		})
+	}
+}
+
+var resourceQuantityCmp = cmp.Comparer(func(x, y resource.Quantity) bool {
+	return x.Cmp(y) == 0
+})
+
+func equateAlways(_, _ interface{}) bool { return true }
+
+func equateEmpty() cmp.Option {
+	return cmp.FilterValues(func(x, y corev1.ResourceList) bool { return isEmpty(x) && isEmpty(y) }, cmp.Comparer(equateAlways))
+}
+
+func isEmpty(x corev1.ResourceList) bool {
+	if len(x) == 0 {
+		return true
+	}
+	for _, q := range x {
+		if !q.IsZero() {
+			return false
+		}
+	}
+	return true
+}
+
+func createResourceList(multiple int) corev1.ResourceList {
+	cpuStr := strconv.Itoa(multiple*100) + "m"
+	memStr := strconv.Itoa(multiple*10) + "Mi"
+	ephemeralStr := strconv.Itoa(multiple*100) + "Mi"
+	return corev1.ResourceList{
+		corev1.ResourceCPU:              resource.MustParse(cpuStr),
+		corev1.ResourceMemory:           resource.MustParse(memStr),
+		corev1.ResourceEphemeralStorage: resource.MustParse(ephemeralStr),
+	}
+}