From 684782d9b81951e7206946052fbd57206177d172 Mon Sep 17 00:00:00 2001 From: Lee Bernick Date: Thu, 16 Jun 2022 14:07:17 -0400 Subject: [PATCH] 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. --- .../limitrange/limitrange.go | 30 +- .../limitrange/limitrange_test.go | 317 ++++++++++++++++++ .../transformer.go | 128 +++---- .../transformer_test.go | 199 +---------- pkg/internal/limitrange/doc.go | 18 - pkg/reconciler/taskrun/taskrun.go | 4 +- 6 files changed, 422 insertions(+), 274 deletions(-) rename pkg/internal/{ => computeresources}/limitrange/limitrange.go (81%) create mode 100644 pkg/internal/computeresources/limitrange/limitrange_test.go rename pkg/internal/{limitrange => computeresources}/transformer.go (53%) rename pkg/internal/{limitrange => computeresources}/transformer_test.go (83%) delete mode 100644 pkg/internal/limitrange/doc.go diff --git a/pkg/internal/limitrange/limitrange.go b/pkg/internal/computeresources/limitrange/limitrange.go similarity index 81% rename from pkg/internal/limitrange/limitrange.go rename to pkg/internal/computeresources/limitrange/limitrange.go index a9858353de5..9c986ab07fb 100644 --- a/pkg/internal/limitrange/limitrange.go +++ b/pkg/internal/computeresources/limitrange/limitrange.go @@ -17,13 +17,26 @@ limitations under the License. package limitrange import ( + "context" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/labels" 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(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 @@ -38,12 +51,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 +100,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 +113,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 diff --git a/pkg/internal/computeresources/limitrange/limitrange_test.go b/pkg/internal/computeresources/limitrange/limitrange_test.go new file mode 100644 index 00000000000..19e347932f0 --- /dev/null +++ b/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(ctx, "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), + } +} diff --git a/pkg/internal/limitrange/transformer.go b/pkg/internal/computeresources/transformer.go similarity index 53% rename from pkg/internal/limitrange/transformer.go rename to pkg/internal/computeresources/transformer.go index 535beb958ab..2a87d7d1d05 100644 --- a/pkg/internal/limitrange/transformer.go +++ b/pkg/internal/computeresources/transformer.go @@ -14,11 +14,12 @@ See the License for the specific language governing permissions and limitations under the License. */ -package limitrange +package computeresources import ( "context" + "github.com/tektoncd/pipeline/pkg/internal/computeresources/limitrange" "github.com/tektoncd/pipeline/pkg/pod" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -27,84 +28,93 @@ import ( var resourceNames = []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory, corev1.ResourceEphemeralStorage} -func isZero(q resource.Quantity) bool { - return (&q).IsZero() -} - // NewTransformer returns a pod.Transformer that will modify limits if needed func NewTransformer(ctx context.Context, namespace string, lister corev1listers.LimitRangeLister) pod.Transformer { return func(p *corev1.Pod) (*corev1.Pod, error) { - limitRange, err := getVirtualLimitRange(namespace, lister) + limitRange, err := limitrange.GetVirtualLimitRange(ctx, namespace, lister) 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 - } + return transformPodBasedOnLimitRange(p, limitRange), 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 step containers in the Pod. - // This should help us find the smallest request to apply to containers - nbStepContainers := 0 - for _, c := range p.Spec.Containers { - if pod.IsContainerStep(c.Name) { - nbStepContainers++ - } +// transformPodBasedOnLimitRange modifies the pod's containers' resource requirements to meet the constraints of the LimitRange. +// The only supported type of LimitRange is "Container". +// For any container: +// - If the container has requests, they are set to the max of (requests, limitRange minimum). +// - If the container doesn't have requests, they are set to the max of (limitRange minimum, "default"), +// where "default" is the LimitRange defaultRequest (for init containers) or the LimitRange defaultRequest / # of app containers +// (for app containers). +// - If the container has limits, they are set to the min of (limits, limitRange maximum). +// - If the container doesn't have limits, they are set to the min of (limitRange maximum, limitRange default). +func transformPodBasedOnLimitRange(p *corev1.Pod, limitRange *corev1.LimitRange) *corev1.Pod { + // No LimitRange defined, nothing to transform, bail early we don't have anything to transform. + if limitRange == nil { + return p + } + + // 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 step containers in the Pod. + // This should help us find the smallest request to apply to containers + nbStepContainers := 0 + for _, c := range p.Spec.Containers { + if pod.IsContainerStep(c.Name) { + nbStepContainers++ } + } - // FIXME(#4230) maxLimitRequestRatio to support later - 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 = defaultContainerRequests - } else { - for _, name := range resourceNames { - setRequestsOrLimits(name, p.Spec.InitContainers[i].Resources.Requests, defaultContainerRequests) - } + // FIXME(#4230) maxLimitRequestRatio to support later + 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 = defaultContainerRequests + } else { + for _, name := range resourceNames { + 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 = defaultContainerLimits - } else { - for _, name := range resourceNames { - setRequestsOrLimits(name, p.Spec.InitContainers[i].Resources.Limits, defaultContainerLimits) - } + } + // We are trying to set the highest limits possible + if p.Spec.InitContainers[i].Resources.Limits == nil { + p.Spec.InitContainers[i].Resources.Limits = defaultContainerLimits + } else { + for _, name := range resourceNames { + setRequestsOrLimits(name, p.Spec.InitContainers[i].Resources.Limits, defaultContainerLimits) } } + } - for i, c := range p.Spec.Containers { - var defaultRequests = defaultContainerRequests - if pod.IsContainerStep(c.Name) { - defaultRequests = defaultStepContainerRequests - } + 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 { - for _, name := range resourceNames { - setRequestsOrLimits(name, p.Spec.Containers[i].Resources.Requests, defaultRequests) - } + 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 = defaultContainerLimits - } else { - for _, name := range resourceNames { - setRequestsOrLimits(name, p.Spec.Containers[i].Resources.Limits, defaultContainerLimits) - } + } + if p.Spec.Containers[i].Resources.Limits == nil { + p.Spec.Containers[i].Resources.Limits = defaultContainerLimits + } else { + for _, name := range resourceNames { + setRequestsOrLimits(name, p.Spec.Containers[i].Resources.Limits, defaultContainerLimits) } } - return p, nil } + return p } func setRequestsOrLimits(name corev1.ResourceName, dst, src corev1.ResourceList) { - if isZero(dst[name]) && !isZero(src[name]) { + if limitrange.IsZero(dst[name]) && !limitrange.IsZero(src[name]) { dst[name] = src[name] } } @@ -135,7 +145,7 @@ func getDefaultStepContainerRequest(limitRange *corev1.LimitRange, nbContainers result = takeTheMax(request, *resource.NewMilliQuantity(defaultRequest.MilliValue()/int64(nbContainers), defaultRequest.Format), min) } // only set non-zero request values - if !isZero(result) { + if !limitrange.IsZero(result) { r[name] = result } } diff --git a/pkg/internal/limitrange/transformer_test.go b/pkg/internal/computeresources/transformer_test.go similarity index 83% rename from pkg/internal/limitrange/transformer_test.go rename to pkg/internal/computeresources/transformer_test.go index 7b6ef0c53b3..68fd1bd30b3 100644 --- a/pkg/internal/limitrange/transformer_test.go +++ b/pkg/internal/computeresources/transformer_test.go @@ -13,22 +13,16 @@ 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 +package computeresources import ( - "context" "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" - 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 { @@ -396,22 +390,12 @@ func TestTransformerOneContainer(t *testing.T) { }, }} { t.Run(tc.description, func(t *testing.T) { - 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, - }, - }}, - ) - defer cancel() - f := NewTransformer(ctx, "default", fakelimitrangeinformer.Get(ctx).Lister()) - got, err := f(&corev1.Pod{ - Spec: tc.podspec, - }) - if err != nil { - t.Fatalf("Transformer failed: %v", err) - } + pod := corev1.Pod{Spec: tc.podspec} + limitRange := corev1.LimitRange{ObjectMeta: metav1.ObjectMeta{Name: "limitrange", Namespace: "default"}, + Spec: corev1.LimitRangeSpec{ + Limits: tc.limitranges, + }} + got := transformPodBasedOnLimitRange(&pod, &limitRange) // We only care about the request and limit, ignoring the rest of the spec cmpRequestsAndLimits(t, tc.want, got.Spec) }) @@ -979,145 +963,12 @@ func TestTransformerMultipleContainer(t *testing.T) { }, }} { t.Run(tc.description, func(t *testing.T) { - 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, - }, - }}, - ) - defer cancel() - f := NewTransformer(ctx, "default", fakelimitrangeinformer.Get(ctx).Lister()) - got, err := f(&corev1.Pod{ - Spec: tc.podspec, - }) - if err != nil { - t.Fatalf("Transformer failed: %v", err) - } - // We only care about the request and limit, ignoring the rest of the spec - cmpRequestsAndLimits(t, tc.want, got.Spec) - }) - } -} - -func TestTransformerOneContainerMultipleLimitRange(t *testing.T) { - for _, tc := range []struct { - description string - limitranges []corev1.LimitRange - podspec corev1.PodSpec - want corev1.PodSpec - }{{ - description: "limitRange with default requests and limits, min and max and no resources on containers", - limitranges: []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"), - }, - }}, - }, - }, { - ObjectMeta: metav1.ObjectMeta{Name: "limitrange2", Namespace: "default"}, - Spec: corev1.LimitRangeSpec{ - Limits: []corev1.LimitRangeItem{{ - Type: corev1.LimitTypeContainer, - Max: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("4"), - corev1.ResourceMemory: resource.MustParse("400Mi"), - corev1.ResourceEphemeralStorage: resource.MustParse("4Gi"), - }, - Min: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("800m"), - corev1.ResourceMemory: resource.MustParse("80Mi"), - corev1.ResourceEphemeralStorage: resource.MustParse("800Mi"), - }, - Default: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("3"), - corev1.ResourceMemory: resource.MustParse("300Mi"), - corev1.ResourceEphemeralStorage: resource.MustParse("3Gi"), - }, - DefaultRequest: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1500m"), - corev1.ResourceMemory: resource.MustParse("150Mi"), - corev1.ResourceEphemeralStorage: resource.MustParse("1.5Gi"), - }, - }}, - }, - }}, - podspec: corev1.PodSpec{ - InitContainers: []corev1.Container{{ - Name: "bar", - Image: "foo", - }}, - Containers: []corev1.Container{{ - Name: "step-foo", - Image: "baz", - }}, - }, - want: corev1.PodSpec{ - InitContainers: []corev1.Container{{ - Resources: corev1.ResourceRequirements{ - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("2"), - corev1.ResourceMemory: resource.MustParse("200Mi"), - corev1.ResourceEphemeralStorage: resource.MustParse("2Gi"), - }, - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1"), - corev1.ResourceMemory: resource.MustParse("100Mi"), - corev1.ResourceEphemeralStorage: resource.MustParse("1Gi"), - }, - }, - }}, - Containers: []corev1.Container{{ - Resources: corev1.ResourceRequirements{ - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("2"), - corev1.ResourceMemory: resource.MustParse("200Mi"), - corev1.ResourceEphemeralStorage: resource.MustParse("2Gi"), - }, - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1"), - corev1.ResourceMemory: resource.MustParse("100Mi"), - corev1.ResourceEphemeralStorage: resource.MustParse("1Gi"), - }, - }, - }}, - }, - }} { - t.Run(tc.description, func(t *testing.T) { - 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, - }) - if err != nil { - t.Fatalf("Transformer failed: %v", err) - } + pod := corev1.Pod{Spec: tc.podspec} + limitRange := corev1.LimitRange{ObjectMeta: metav1.ObjectMeta{Name: "limitrange", Namespace: "default"}, + Spec: corev1.LimitRangeSpec{ + Limits: tc.limitranges, + }} + got := transformPodBasedOnLimitRange(&pod, &limitRange) // We only care about the request and limit, ignoring the rest of the spec cmpRequestsAndLimits(t, tc.want, got.Spec) }) @@ -1163,27 +1014,3 @@ 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 -} diff --git a/pkg/internal/limitrange/doc.go b/pkg/internal/limitrange/doc.go deleted file mode 100644 index 05b2449826e..00000000000 --- a/pkg/internal/limitrange/doc.go +++ /dev/null @@ -1,18 +0,0 @@ -/* -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 diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index dffe5ccd97f..5965a6392e8 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -36,7 +36,7 @@ import ( listers "github.com/tektoncd/pipeline/pkg/client/listers/pipeline/v1beta1" resourcelisters "github.com/tektoncd/pipeline/pkg/client/resource/listers/resource/v1alpha1" "github.com/tektoncd/pipeline/pkg/internal/affinityassistant" - "github.com/tektoncd/pipeline/pkg/internal/limitrange" + "github.com/tektoncd/pipeline/pkg/internal/computeresources" podconvert "github.com/tektoncd/pipeline/pkg/pod" tknreconciler "github.com/tektoncd/pipeline/pkg/reconciler" "github.com/tektoncd/pipeline/pkg/reconciler/events" @@ -707,7 +707,7 @@ func (c *Reconciler) createPod(ctx context.Context, ts *v1beta1.TaskSpec, tr *v1 EntrypointCache: c.entrypointCache, } pod, err := podbuilder.Build(ctx, tr, *ts, - limitrange.NewTransformer(ctx, tr.Namespace, c.limitrangeLister), + computeresources.NewTransformer(ctx, tr.Namespace, c.limitrangeLister), affinityassistant.NewTransformer(ctx, tr.Annotations), ) if err != nil {