Use an Informer to list LimitRanges 🥼

Using a LimitRange lister here instead, so this doesn't end up hitting the real API server on each call. Taking into account a review : #4176 (comment). Signed-off-by: Vincent Demeester <[email protected]>
tektoncd · Sep 16, 2021 · b1375f4 · b1375f4
1 parent ff860df
commit b1375f4
Show file tree

Hide file tree

Showing 7 changed files with 212 additions and 28 deletions.
diff --git a/pkg/internal/limitrange/limitrange.go b/pkg/internal/limitrange/limitrange.go
@@ -20,23 +20,25 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/client-go/kubernetes"
+	limitrangeinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/limitrange"
 )
 
 func getVirtualLimitRange(ctx context.Context, namespace string, c kubernetes.Interface) (*corev1.LimitRange, error) {
-	limitRanges, err := c.CoreV1().LimitRanges(namespace).List(ctx, metav1.ListOptions{})
+	limitrangeInformer := limitrangeinformer.Get(ctx)
+	limitRanges, err := limitrangeInformer.Lister().LimitRanges(namespace).List(labels.Everything())
 	if err != nil {
 		return nil, err
 	}
-	var limitRange corev1.LimitRange
+	var limitRange *corev1.LimitRange
 	switch {
-	case len(limitRanges.Items) == 0:
+	case len(limitRanges) == 0:
 		// No LimitRange defined
 		break
-	case len(limitRanges.Items) == 1:
+	case len(limitRanges) == 1:
 		// One LimitRange defined
-		limitRange = limitRanges.Items[0]
+		limitRange = limitRanges[0]
 	default:
 		// Several LimitRange defined
 		// Create a virtual LimitRange with
@@ -45,8 +47,9 @@ func getVirtualLimitRange(ctx context.Context, namespace string, c kubernetes.In
 		// - 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.Items {
+		for _, lr := range limitRanges {
 			for _, item := range lr.Spec.Limits {
 				_, exists := m[item.Type]
 				if !exists {
@@ -74,7 +77,7 @@ func getVirtualLimitRange(ctx context.Context, namespace string, c kubernetes.In
 			}
 		}
 		// Handle Default and DefaultRequest
-		for _, lr := range limitRanges.Items {
+		for _, lr := range limitRanges {
 			for _, item := range lr.Spec.Limits {
 				// Default
 				m[item.Type].Default[corev1.ResourceCPU] = minOfBetween(m[item.Type].Default[corev1.ResourceCPU], item.Default[corev1.ResourceCPU], m[item.Type].Min[corev1.ResourceCPU], m[item.Type].Max[corev1.ResourceCPU])
@@ -90,7 +93,7 @@ func getVirtualLimitRange(ctx context.Context, namespace string, c kubernetes.In
 			limitRange.Spec.Limits = append(limitRange.Spec.Limits, v)
 		}
 	}
-	return &limitRange, nil
+	return limitRange, nil
 }
 
 func maxOf(a, b resource.Quantity) resource.Quantity {

diff --git a/pkg/internal/limitrange/transformer_test.go b/pkg/internal/limitrange/transformer_test.go
@@ -20,12 +20,17 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing"
+	"github.com/tektoncd/pipeline/test"
 	"github.com/tektoncd/pipeline/test/diff"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
-	fakek8s "k8s.io/client-go/kubernetes/fake"
+	// "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes"
+	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"
 )
 
 var resourceQuantityCmp = cmp.Comparer(func(x, y resource.Quantity) bool {
@@ -405,15 +410,16 @@ func TestTransformerOneContainer(t *testing.T) {
 		},
 	}} {
 		t.Run(tc.description, func(t *testing.T) {
-			kubeclient := fakek8s.NewSimpleClientset(
-				&corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "default"}},
-				&corev1.LimitRange{ObjectMeta: metav1.ObjectMeta{Name: "limitrange", Namespace: "default"},
+			ctx, kubeclient, cancel := setup(t,
+				[]corev1.ServiceAccount{{ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "default"}}},
+				[]corev1.LimitRange{{ObjectMeta: metav1.ObjectMeta{Name: "limitrange", Namespace: "default"},
 					Spec: corev1.LimitRangeSpec{
 						Limits: tc.limitranges,
 					},
-				},
+				}},
 			)
-			f := NewTransformer(context.Background(), "default", kubeclient)
+			defer cancel()
+			f := NewTransformer(ctx, "default", kubeclient)
 			got, err := f(&corev1.Pod{
 				Spec: tc.podspec,
 			})
@@ -817,15 +823,16 @@ func TestTransformerMultipleContainer(t *testing.T) {
 		},
 	}} {
 		t.Run(tc.description, func(t *testing.T) {
-			kubeclient := fakek8s.NewSimpleClientset(
-				&corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "default"}},
-				&corev1.LimitRange{ObjectMeta: metav1.ObjectMeta{Name: "limitrange", Namespace: "default"},
+			ctx, kubeclient, cancel := setup(t,
+				[]corev1.ServiceAccount{{ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "default"}}},
+				[]corev1.LimitRange{{ObjectMeta: metav1.ObjectMeta{Name: "limitrange", Namespace: "default"},
 					Spec: corev1.LimitRangeSpec{
 						Limits: tc.limitranges,
 					},
-				},
+				}},
 			)
-			f := NewTransformer(context.Background(), "default", kubeclient)
+			defer cancel()
+			f := NewTransformer(ctx, "default", kubeclient)
 			got, err := f(&corev1.Pod{
 				Spec: tc.podspec,
 			})
@@ -943,13 +950,12 @@ func TestTransformerOneContainerMultipleLimitRange(t *testing.T) {
 		},
 	}} {
 		t.Run(tc.description, func(t *testing.T) {
-			runtimeObjects := []runtime.Object{&corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "default"}}}
-			for _, l := range tc.limitranges {
-				l := l // because we use pointer, we need to descope this...
-				runtimeObjects = append(runtimeObjects, &l)
-			}
-			kubeclient := fakek8s.NewSimpleClientset(runtimeObjects...)
-			f := NewTransformer(context.Background(), "default", kubeclient)
+			ctx, kubeclient, cancel := setup(t,
+				[]corev1.ServiceAccount{{ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "default"}}},
+				tc.limitranges,
+			)
+			defer cancel()
+			f := NewTransformer(ctx, "default", kubeclient)
 			got, err := f(&corev1.Pod{
 				Spec: tc.podspec,
 			})
@@ -985,3 +991,27 @@ func cmpRequestsAndLimits(t *testing.T, want, got corev1.PodSpec) {
 		}
 	}
 }
+
+func setup(t *testing.T, serviceaccounts []corev1.ServiceAccount, limitranges []corev1.LimitRange) (context.Context, kubernetes.Interface, 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, kubeclient, cancel
+}
diff --git a/pkg/reconciler/taskrun/controller.go b/pkg/reconciler/taskrun/controller.go
@@ -34,6 +34,7 @@ import (
 	"github.com/tektoncd/pipeline/pkg/taskrunmetrics"
 	"k8s.io/client-go/tools/cache"
 	kubeclient "knative.dev/pkg/client/injection/kube/client"
+	limitrangeinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/limitrange"
 	filteredpodinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/pod/filtered"
 	"knative.dev/pkg/configmap"
 	"knative.dev/pkg/controller"
@@ -53,6 +54,7 @@ type ControllerConfiguration struct {
 // NewController instantiates a new controller.Impl from knative.dev/pkg/controller
 func NewController(namespace string, conf ControllerConfiguration) func(context.Context, configmap.Watcher) *controller.Impl {
 	return func(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
+		limitrangeinformer.Get(ctx)
 		logger := logging.FromContext(ctx)
 		kubeclientset := kubeclient.Get(ctx)
 		pipelineclientset := pipelineclient.Get(ctx)

diff --git a/test/controller.go b/test/controller.go
@@ -54,6 +54,7 @@ import (
 	"k8s.io/client-go/tools/record"
 	fakekubeclient "knative.dev/pkg/client/injection/kube/client/fake"
 	fakeconfigmapinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/fake"
+	_ "knative.dev/pkg/client/injection/kube/informers/core/v1/limitrange/fake"
 	fakefilteredpodinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/pod/filtered/fake"
 	fakeserviceaccountinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount/fake"
 	"knative.dev/pkg/controller"

diff --git a/vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/limitrange/fake/fake.go b/vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/limitrange/fake/fake.go
diff --git a/vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/limitrange/limitrange.go b/vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/limitrange/limitrange.go
diff --git a/vendor/modules.txt b/vendor/modules.txt