Skip to content

Commit

Permalink
Use an Informer to list LimitRanges 🥼
Browse files Browse the repository at this point in the history 
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 <vdemeest@redhat.com>
  • Loading branch information
@vdemeester @tekton-robot
vdemeester authored and tekton-robot committed Sep 16, 2021
1 parent ff860df commit adaae90
Show file tree
Hide file tree
Showing 12 changed files with 233 additions and 63 deletions.
7 changes: 0 additions & 7 deletions internal/builder/v1beta1/pod.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package builder

import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

Expand Down Expand Up @@ -124,9 +123,6 @@ func PodContainer(name, image string, ops ...ContainerOp) PodSpecOp {
c := &corev1.Container{
Name: name,
Image: image,
Resources: corev1.ResourceRequirements{
Requests: map[corev1.ResourceName]resource.Quantity{},
},
}
for _, op := range ops {
op(c)
Expand All @@ -143,9 +139,6 @@ func PodInitContainer(name, image string, ops ...ContainerOp) PodSpecOp {
Name: name,
Image: image,
Args: []string{},
Resources: corev1.ResourceRequirements{
Requests: map[corev1.ResourceName]resource.Quantity{},
},
}
for _, op := range ops {
op(c)
Expand Down
3 changes: 0 additions & 3 deletions internal/builder/v1beta1/pod_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,6 @@ func TestPod(t *testing.T) {
Containers: []corev1.Container{{
Name: "nop",
Image: "nop:latest",
Resources: corev1.ResourceRequirements{
Requests: map[corev1.ResourceName]resource.Quantity{},
},
}},
InitContainers: []corev1.Container{{
Name: "basic",
Expand Down
23 changes: 12 additions & 11 deletions pkg/internal/limitrange/limitrange.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,23 @@ import (

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
"k8s.io/apimachinery/pkg/labels"
corev1listers "k8s.io/client-go/listers/core/v1"
)

func getVirtualLimitRange(ctx context.Context, namespace string, c kubernetes.Interface) (*corev1.LimitRange, error) {
limitRanges, err := c.CoreV1().LimitRanges(namespace).List(ctx, metav1.ListOptions{})
func getVirtualLimitRange(ctx context.Context, namespace string, lister corev1listers.LimitRangeLister) (*corev1.LimitRange, error) {
limitRanges, err := 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
Expand All @@ -45,8 +45,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 {
Expand Down Expand Up @@ -74,7 +75,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])
Expand All @@ -90,7 +91,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 {
Expand Down
6 changes: 3 additions & 3 deletions pkg/internal/limitrange/transformer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"github.com/tektoncd/pipeline/pkg/pod"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/client-go/kubernetes"
corev1listers "k8s.io/client-go/listers/core/v1"
)

var resourceNames = []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory, corev1.ResourceEphemeralStorage}
Expand All @@ -30,9 +30,9 @@ func isZero(q resource.Quantity) bool {
return (&q).IsZero()
}

func NewTransformer(ctx context.Context, namespace string, clientset kubernetes.Interface) pod.Transformer {
func NewTransformer(ctx context.Context, namespace string, lister corev1listers.LimitRangeLister) pod.Transformer {
return func(p *corev1.Pod) (*corev1.Pod, error) {
limitRange, err := getVirtualLimitRange(ctx, namespace, clientset)
limitRange, err := getVirtualLimitRange(ctx, namespace, lister)
if err != nil {
return p, err
}
Expand Down
66 changes: 47 additions & 19 deletions pkg/internal/limitrange/transformer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,15 @@ 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"
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 {
Expand Down Expand Up @@ -405,15 +408,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, 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", fakelimitrangeinformer.Get(ctx).Lister())
got, err := f(&corev1.Pod{
Spec: tc.podspec,
})
Expand Down Expand Up @@ -817,15 +821,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, 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", fakelimitrangeinformer.Get(ctx).Lister())
got, err := f(&corev1.Pod{
Spec: tc.podspec,
})
Expand Down Expand Up @@ -943,13 +948,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, cancel := setup(t,
[]corev1.ServiceAccount{{ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "default"}}},
tc.limitranges,
)
defer cancel()
f := NewTransformer(ctx, "default", fakelimitrangeinformer.Get(ctx).Lister())
got, err := f(&corev1.Pod{
Spec: tc.podspec,
})
Expand Down Expand Up @@ -985,3 +989,27 @@ func cmpRequestsAndLimits(t *testing.T, want, got corev1.PodSpec) {
}
}
}

func setup(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
}
3 changes: 3 additions & 0 deletions pkg/reconciler/taskrun/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -61,6 +62,7 @@ func NewController(namespace string, conf ControllerConfiguration) func(context.
clusterTaskInformer := clustertaskinformer.Get(ctx)
podInformer := filteredpodinformer.Get(ctx, v1beta1.ManagedByLabelKey)
resourceInformer := resourceinformer.Get(ctx)
limitrangeInformer := limitrangeinformer.Get(ctx)
configStore := config.NewStore(logger.Named("config-store"), taskrunmetrics.MetricsOnStore(logger))
configStore.WatchConfigs(cmw)

Expand All @@ -77,6 +79,7 @@ func NewController(namespace string, conf ControllerConfiguration) func(context.
taskLister: taskInformer.Lister(),
clusterTaskLister: clusterTaskInformer.Lister(),
resourceLister: resourceInformer.Lister(),
limitrangeLister: limitrangeInformer.Lister(),
cloudEventClient: cloudeventclient.Get(ctx),
metrics: taskrunmetrics.Get(ctx),
entrypointCache: entrypointCache,
Expand Down
4 changes: 3 additions & 1 deletion pkg/reconciler/taskrun/taskrun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import (
k8serrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
corev1Listers "k8s.io/client-go/listers/core/v1"
"knative.dev/pkg/apis"
"knative.dev/pkg/controller"
"knative.dev/pkg/kmeta"
Expand All @@ -71,6 +72,7 @@ type Reconciler struct {
taskLister listers.TaskLister
clusterTaskLister listers.ClusterTaskLister
resourceLister resourcelisters.PipelineResourceLister
limitrangeLister corev1Listers.LimitRangeLister
cloudEventClient cloudevent.CEClient
entrypointCache podconvert.EntrypointCache
metrics *taskrunmetrics.Recorder
Expand Down Expand Up @@ -704,7 +706,7 @@ func (c *Reconciler) createPod(ctx context.Context, tr *v1beta1.TaskRun, rtr *re
EntrypointCache: c.entrypointCache,
OverrideHomeEnv: shouldOverrideHomeEnv,
}
pod, err := podbuilder.Build(ctx, tr, *ts, limitrange.NewTransformer(ctx, tr.Namespace, c.KubeClientSet))
pod, err := podbuilder.Build(ctx, tr, *ts, limitrange.NewTransformer(ctx, tr.Namespace, c.limitrangeLister))
if err != nil {
return nil, fmt.Errorf("translating TaskSpec to Pod: %w", err)
}
Expand Down
Loading

0 comments on commit adaae90

Please sign in to comment.