From 6704bcdaaffc9947905ba5426003fe408a5f1479 Mon Sep 17 00:00:00 2001 From: Daniel Helfand Date: Fri, 31 Jan 2020 21:51:00 -0500 Subject: [PATCH] check for limitrange minimum for taskrun container requests --- config/200-clusterrole.yaml | 2 +- go.mod | 3 +- pkg/pod/pod.go | 17 +++- pkg/pod/resource_request.go | 34 +++++++- pkg/pod/resource_request_test.go | 143 ++++++++++++++++++++++++++++++- 5 files changed, 191 insertions(+), 8 deletions(-) diff --git a/config/200-clusterrole.yaml b/config/200-clusterrole.yaml index 1ca30572693..7dd6dcdd57a 100644 --- a/config/200-clusterrole.yaml +++ b/config/200-clusterrole.yaml @@ -4,7 +4,7 @@ metadata: name: tekton-pipelines-admin rules: - apiGroups: [""] - resources: ["pods", "pods/log", "namespaces", "secrets", "events", "serviceaccounts", "configmaps", "persistentvolumeclaims"] + resources: ["pods", "pods/log", "namespaces", "secrets", "events", "serviceaccounts", "configmaps", "persistentvolumeclaims", "limitranges"] verbs: ["get", "list", "create", "update", "delete", "patch", "watch"] - apiGroups: ["apps"] resources: ["deployments"] diff --git a/go.mod b/go.mod index d1693b97772..2ee967e53a8 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.13 require ( cloud.google.com/go v0.47.0 // indirect + cloud.google.com/go/storage v1.0.0 contrib.go.opencensus.io/exporter/prometheus v0.1.0 // indirect contrib.go.opencensus.io/exporter/stackdriver v0.12.8 // indirect github.com/Azure/go-autorest/autorest/validation v0.2.0 // indirect @@ -44,7 +45,7 @@ require ( golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e // indirect golang.org/x/sys v0.0.0-20191210023423-ac6580df4449 // indirect golang.org/x/time v0.0.0-20191024005414-555d28b269f0 // indirect - google.golang.org/api v0.10.0 // indirect + google.golang.org/api v0.10.0 google.golang.org/appengine v1.6.5 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v2 v2.2.5 // indirect diff --git a/pkg/pod/pod.go b/pkg/pod/pod.go index 15511a872bf..ea978c6145d 100644 --- a/pkg/pod/pod.go +++ b/pkg/pod/pod.go @@ -119,8 +119,23 @@ func MakePod(images pipeline.Images, taskRun *v1alpha1.TaskRun, taskSpec v1alpha initContainers = append(initContainers, entrypointInit) volumes = append(volumes, toolsVolume, downwardVolume) + // Get LimitRange by name if present in ConfigMap tekton-limitrange-config. + // Otherwise, pass empty request to resolveResourceRequests. + limitRangeName := "" + limitRangeConfig, err := kubeclient.CoreV1().ConfigMaps(taskRun.Namespace).Get("tekton-limitrange-config", metav1.GetOptions{}) + if err == nil { + limitRangeName = limitRangeConfig.Data["default-limitrange-name"] + } + limitRange := &corev1.LimitRange{} + if limitRangeName != "" { + limitRange, err = kubeclient.CoreV1().LimitRanges(taskRun.Namespace).Get(limitRangeName, metav1.GetOptions{}) + if err != nil { + return nil, err + } + } + // Zero out non-max resource requests. - stepContainers = resolveResourceRequests(stepContainers) + stepContainers = resolveResourceRequests(stepContainers, limitRange) // Add implicit env vars. // They're prepended to the list, so that if the user specified any diff --git a/pkg/pod/resource_request.go b/pkg/pod/resource_request.go index 5cbbc2080c5..ac4a6cde3cc 100644 --- a/pkg/pod/resource_request.go +++ b/pkg/pod/resource_request.go @@ -21,6 +21,8 @@ import ( "k8s.io/apimachinery/pkg/api/resource" ) +var emptyLimitRange = &corev1.LimitRange{} +var emptyResourceQuantity = resource.Quantity{} var zeroQty = resource.MustParse("0") func allZeroQty() corev1.ResourceList { @@ -31,7 +33,7 @@ func allZeroQty() corev1.ResourceList { } } -func resolveResourceRequests(containers []corev1.Container) []corev1.Container { +func resolveResourceRequests(containers []corev1.Container, limitRange *corev1.LimitRange) []corev1.Container { max := allZeroQty() resourceNames := []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory, corev1.ResourceEphemeralStorage} maxIndicesByResource := make(map[corev1.ResourceName]int, len(resourceNames)) @@ -50,16 +52,42 @@ func resolveResourceRequests(containers []corev1.Container) []corev1.Container { } } + // Get limitrange minimum for container requests so they won't + // be zeroed out if minimum is specified in namespace + limitRangeItems := limitRange.Spec.Limits + min := allZeroQty() + if limitRange != emptyLimitRange { + for _, limitRangeItem := range limitRangeItems { + if limitRangeItem.Type == "Container" { + if limitRangeItem.Min != nil { + min = limitRangeItem.Min + } + break + } + } + } + + // Use zeroQty if request value is not set for min + if min[corev1.ResourceCPU] == emptyResourceQuantity { + min[corev1.ResourceCPU] = zeroQty + } + if min[corev1.ResourceMemory] == emptyResourceQuantity { + min[corev1.ResourceMemory] = zeroQty + } + if min[corev1.ResourceEphemeralStorage] == emptyResourceQuantity { + min[corev1.ResourceEphemeralStorage] = zeroQty + } + // Set all non max resource requests to 0. Leave max request at index // originally defined to account for limit of step. for i := range containers { if containers[i].Resources.Requests == nil { - containers[i].Resources.Requests = allZeroQty() + containers[i].Resources.Requests = min continue } for _, resourceName := range resourceNames { if maxIndicesByResource[resourceName] != i { - containers[i].Resources.Requests[resourceName] = zeroQty + containers[i].Resources.Requests[resourceName] = min[resourceName] } } } diff --git a/pkg/pod/resource_request_test.go b/pkg/pod/resource_request_test.go index b034cb00c99..099cdedc067 100644 --- a/pkg/pod/resource_request_test.go +++ b/pkg/pod/resource_request_test.go @@ -28,7 +28,7 @@ var resourceQuantityCmp = cmp.Comparer(func(x, y resource.Quantity) bool { return x.Cmp(y) == 0 }) -func TestResolveResourceRequests(t *testing.T) { +func TestResolveResourceRequests_No_LimitRange(t *testing.T) { for _, c := range []struct { desc string in, want []corev1.Container @@ -206,7 +206,146 @@ func TestResolveResourceRequests(t *testing.T) { }, } { t.Run(c.desc, func(t *testing.T) { - got := resolveResourceRequests(c.in) + got := resolveResourceRequests(c.in, &corev1.LimitRange{}) + if d := cmp.Diff(c.want, got, resourceQuantityCmp); d != "" { + t.Errorf("Diff(-want, +got): %s", d) + } + }) + } +} + +func TestResolveResourceRequests_LimitRange(t *testing.T) { + for _, c := range []struct { + desc string + in, want []corev1.Container + }{{ + desc: "three steps, no requests, apply minimum to all", + in: []corev1.Container{{}, {}, {}}, + want: []corev1.Container{{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("99Mi"), + corev1.ResourceEphemeralStorage: resource.MustParse("100m"), + }, + }, + }, { + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("99Mi"), + corev1.ResourceEphemeralStorage: resource.MustParse("100m"), + }, + }, + }, { + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("99Mi"), + corev1.ResourceEphemeralStorage: resource.MustParse("100m"), + }, + }, + }}, + }, { + desc: "three steps, no requests, apply minimum values when not max values", + in: []corev1.Container{{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("10"), + }, + }, + }, { + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("10Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("11Gi"), + }, + }, + }, { + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceEphemeralStorage: resource.MustParse("100Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("100Gi"), + }, + }, + }}, + want: []corev1.Container{{ + // ResourceCPU max request + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("10"), + corev1.ResourceMemory: resource.MustParse("99Mi"), + corev1.ResourceEphemeralStorage: resource.MustParse("100m"), + }, + }, + }, { + // ResourceMemory max request + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("10Gi"), + corev1.ResourceEphemeralStorage: resource.MustParse("100m"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("11Gi"), + }, + }, + }, { + // ResourceEphemeralStorage max request + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("99Mi"), + corev1.ResourceEphemeralStorage: resource.MustParse("100Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("100Gi"), + }, + }, + }}, + }, { + desc: "Only one step container with all request values filled out, no min values", + in: []corev1.Container{{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("10"), + corev1.ResourceMemory: resource.MustParse("10Gi"), + corev1.ResourceEphemeralStorage: resource.MustParse("100Gi"), + }, + }, + }}, + want: []corev1.Container{{ + // All max values set + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("10"), + corev1.ResourceMemory: resource.MustParse("10Gi"), + corev1.ResourceEphemeralStorage: resource.MustParse("100Gi"), + }, + }, + }}, + }, + } { + t.Run(c.desc, func(t *testing.T) { + got := resolveResourceRequests(c.in, + &corev1.LimitRange{ + Spec: corev1.LimitRangeSpec{ + Limits: []corev1.LimitRangeItem{ + { + Type: "Container", + Min: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("99Mi"), + corev1.ResourceEphemeralStorage: resource.MustParse("100m"), + }, + }, + }, + }, + }) if d := cmp.Diff(c.want, got, resourceQuantityCmp); d != "" { t.Errorf("Diff(-want, +got): %s", d) }