Merge pull request #29588 from smarterclayton/init_container_quota

Automatic merge from submit-queue

Init container quota is inaccurate

Usage charged should be max of greater of init container or all regular
containers. Also, need to validate init container inputs

@derekwaynecarr

pkg/quota/evaluator/core/pods.go

@@ -79,8 +79,11 @@ func PodConstraintsFunc(required []api.ResourceName, object runtime.Object) erro
 	allErrs := field.ErrorList{}
 	fldPath := field.NewPath("spec").Child("containers")
 	for i, ctr := range pod.Spec.Containers {
-		idxPath := fldPath.Index(i)
+		allErrs = append(allErrs, validation.ValidateResourceRequirements(&ctr.Resources, fldPath.Index(i).Child("resources"))...)
-		allErrs = append(allErrs, validation.ValidateResourceRequirements(&ctr.Resources, idxPath.Child("resources"))...)
+	}
+	fldPath = field.NewPath("spec").Child("initContainers")
+	for i, ctr := range pod.Spec.InitContainers {
+		allErrs = append(allErrs, validation.ValidateResourceRequirements(&ctr.Resources, fldPath.Index(i).Child("resources"))...)
 	}
 	if len(allErrs) > 0 {
 		return allErrs.ToAggregate()
@@ -92,14 +95,10 @@ func PodConstraintsFunc(required []api.ResourceName, object runtime.Object) erro
 	requiredSet := quota.ToSet(required)
 	missingSet := sets.NewString()
 	for i := range pod.Spec.Containers {
-		requests := pod.Spec.Containers[i].Resources.Requests
+		enforcePodContainerConstraints(&pod.Spec.Containers[i], requiredSet, missingSet)
-		limits := pod.Spec.Containers[i].Resources.Limits
+	}
-		containerUsage := podUsageHelper(requests, limits)
+	for i := range pod.Spec.InitContainers {
-		containerSet := quota.ToSet(quota.ResourceNames(containerUsage))
+		enforcePodContainerConstraints(&pod.Spec.InitContainers[i], requiredSet, missingSet)
-		if !containerSet.Equal(requiredSet) {
-			difference := requiredSet.Difference(containerSet)
-			missingSet.Insert(difference.List()...)
-		}
 	}
 	if len(missingSet) == 0 {
 		return nil
@@ -107,6 +106,19 @@ func PodConstraintsFunc(required []api.ResourceName, object runtime.Object) erro
 	return fmt.Errorf("must specify %s", strings.Join(missingSet.List(), ","))
 }
 
+// enforcePodContainerConstraints checks for required resources that are not set on this container and
+// adds them to missingSet.
+func enforcePodContainerConstraints(container *api.Container, requiredSet, missingSet sets.String) {
+	requests := container.Resources.Requests
+	limits := container.Resources.Limits
+	containerUsage := podUsageHelper(requests, limits)
+	containerSet := quota.ToSet(quota.ResourceNames(containerUsage))
+	if !containerSet.Equal(requiredSet) {
+		difference := requiredSet.Difference(containerSet)
+		missingSet.Insert(difference.List()...)
+	}
+}
+
 // podUsageHelper can summarize the pod quota usage based on requests and limits
 func podUsageHelper(requests api.ResourceList, limits api.ResourceList) api.ResourceList {
 	result := api.ResourceList{}
@@ -144,10 +156,18 @@ func PodUsageFunc(object runtime.Object) api.ResourceList {
 	// when we have pod level cgroups, we can just read pod level requests/limits
 	requests := api.ResourceList{}
 	limits := api.ResourceList{}
+
 	for i := range pod.Spec.Containers {
 		requests = quota.Add(requests, pod.Spec.Containers[i].Resources.Requests)
 		limits = quota.Add(limits, pod.Spec.Containers[i].Resources.Limits)
 	}
+	// InitContainers are run sequentially before other containers start, so the highest
+	// init container resource is compared against the sum of app containers to determine
+	// the effective usage for both requests and limits.
+	for i := range pod.Spec.InitContainers {
+		requests = quota.Max(requests, pod.Spec.InitContainers[i].Resources.Requests)
+		limits = quota.Max(limits, pod.Spec.InitContainers[i].Resources.Limits)
+	}
 
 	return podUsageHelper(requests, limits)
 }

pkg/quota/evaluator/core/pods_test.go

@@ -0,0 +1,253 @@
+/*
+Copyright 2016 The Kubernetes 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 core
+
+import (
+	"testing"
+
+	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/api/resource"
+	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
+	"k8s.io/kubernetes/pkg/quota"
+)
+
+func TestPodConstraintsFunc(t *testing.T) {
+	testCases := map[string]struct {
+		pod      *api.Pod
+		required []api.ResourceName
+		err      string
+	}{
+		"init container resource invalid": {
+			pod: &api.Pod{
+				Spec: api.PodSpec{
+					InitContainers: []api.Container{{
+						Resources: api.ResourceRequirements{
+							Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("2m")},
+							Limits:   api.ResourceList{api.ResourceCPU: resource.MustParse("1m")},
+						},
+					}},
+				},
+			},
+			err: `spec.initContainers[0].resources.limits: Invalid value: "1m": must be greater than or equal to cpu request`,
+		},
+		"container resource invalid": {
+			pod: &api.Pod{
+				Spec: api.PodSpec{
+					Containers: []api.Container{{
+						Resources: api.ResourceRequirements{
+							Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("2m")},
+							Limits:   api.ResourceList{api.ResourceCPU: resource.MustParse("1m")},
+						},
+					}},
+				},
+			},
+			err: `spec.containers[0].resources.limits: Invalid value: "1m": must be greater than or equal to cpu request`,
+		},
+		"init container resource missing": {
+			pod: &api.Pod{
+				Spec: api.PodSpec{
+					InitContainers: []api.Container{{
+						Resources: api.ResourceRequirements{
+							Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("1m")},
+							Limits:   api.ResourceList{api.ResourceCPU: resource.MustParse("2m")},
+						},
+					}},
+				},
+			},
+			required: []api.ResourceName{api.ResourceMemory},
+			err:      `must specify memory`,
+		},
+		"container resource missing": {
+			pod: &api.Pod{
+				Spec: api.PodSpec{
+					Containers: []api.Container{{
+						Resources: api.ResourceRequirements{
+							Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("1m")},
+							Limits:   api.ResourceList{api.ResourceCPU: resource.MustParse("2m")},
+						},
+					}},
+				},
+			},
+			required: []api.ResourceName{api.ResourceMemory},
+			err:      `must specify memory`,
+		},
+	}
+	for testName, test := range testCases {
+		err := PodConstraintsFunc(test.required, test.pod)
+		switch {
+		case err != nil && len(test.err) == 0,
+			err == nil && len(test.err) != 0,
+			err != nil && test.err != err.Error():
+			t.Errorf("%s unexpected error: %v", testName, err)
+		}
+	}
+}
+
+func TestPodEvaluatorUsage(t *testing.T) {
+	kubeClient := fake.NewSimpleClientset()
+	evaluator := NewPodEvaluator(kubeClient)
+	testCases := map[string]struct {
+		pod   *api.Pod
+		usage api.ResourceList
+	}{
+		"init container CPU": {
+			pod: &api.Pod{
+				Spec: api.PodSpec{
+					InitContainers: []api.Container{{
+						Resources: api.ResourceRequirements{
+							Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("1m")},
+							Limits:   api.ResourceList{api.ResourceCPU: resource.MustParse("2m")},
+						},
+					}},
+				},
+			},
+			usage: api.ResourceList{
+				api.ResourceRequestsCPU: resource.MustParse("1m"),
+				api.ResourceLimitsCPU:   resource.MustParse("2m"),
+				api.ResourcePods:        resource.MustParse("1"),
+				api.ResourceCPU:         resource.MustParse("1m"),
+			},
+		},
+		"init container MEM": {
+			pod: &api.Pod{
+				Spec: api.PodSpec{
+					InitContainers: []api.Container{{
+						Resources: api.ResourceRequirements{
+							Requests: api.ResourceList{api.ResourceMemory: resource.MustParse("1m")},
+							Limits:   api.ResourceList{api.ResourceMemory: resource.MustParse("2m")},
+						},
+					}},
+				},
+			},
+			usage: api.ResourceList{
+				api.ResourceRequestsMemory: resource.MustParse("1m"),
+				api.ResourceLimitsMemory:   resource.MustParse("2m"),
+				api.ResourcePods:           resource.MustParse("1"),
+				api.ResourceMemory:         resource.MustParse("1m"),
+			},
+		},
+		"container CPU": {
+			pod: &api.Pod{
+				Spec: api.PodSpec{
+					Containers: []api.Container{{
+						Resources: api.ResourceRequirements{
+							Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("1m")},
+							Limits:   api.ResourceList{api.ResourceCPU: resource.MustParse("2m")},
+						},
+					}},
+				},
+			},
+			usage: api.ResourceList{
+				api.ResourceRequestsCPU: resource.MustParse("1m"),
+				api.ResourceLimitsCPU:   resource.MustParse("2m"),
+				api.ResourcePods:        resource.MustParse("1"),
+				api.ResourceCPU:         resource.MustParse("1m"),
+			},
+		},
+		"container MEM": {
+			pod: &api.Pod{
+				Spec: api.PodSpec{
+					Containers: []api.Container{{
+						Resources: api.ResourceRequirements{
+							Requests: api.ResourceList{api.ResourceMemory: resource.MustParse("1m")},
+							Limits:   api.ResourceList{api.ResourceMemory: resource.MustParse("2m")},
+						},
+					}},
+				},
+			},
+			usage: api.ResourceList{
+				api.ResourceRequestsMemory: resource.MustParse("1m"),
+				api.ResourceLimitsMemory:   resource.MustParse("2m"),
+				api.ResourcePods:           resource.MustParse("1"),
+				api.ResourceMemory:         resource.MustParse("1m"),
+			},
+		},
+		"init container maximums override sum of containers": {
+			pod: &api.Pod{
+				Spec: api.PodSpec{
+					InitContainers: []api.Container{
+						{
+							Resources: api.ResourceRequirements{
+								Requests: api.ResourceList{
+									api.ResourceCPU:    resource.MustParse("4"),
+									api.ResourceMemory: resource.MustParse("100M"),
+								},
+								Limits: api.ResourceList{
+									api.ResourceCPU:    resource.MustParse("8"),
+									api.ResourceMemory: resource.MustParse("200M"),
+								},
+							},
+						},
+						{
+							Resources: api.ResourceRequirements{
+								Requests: api.ResourceList{
+									api.ResourceCPU:    resource.MustParse("1"),
+									api.ResourceMemory: resource.MustParse("50M"),
+								},
+								Limits: api.ResourceList{
+									api.ResourceCPU:    resource.MustParse("2"),
+									api.ResourceMemory: resource.MustParse("100M"),
+								},
+							},
+						},
+					},
+					Containers: []api.Container{
+						{
+							Resources: api.ResourceRequirements{
+								Requests: api.ResourceList{
+									api.ResourceCPU:    resource.MustParse("1"),
+									api.ResourceMemory: resource.MustParse("50M"),
+								},
+								Limits: api.ResourceList{
+									api.ResourceCPU:    resource.MustParse("2"),
+									api.ResourceMemory: resource.MustParse("100M"),
+								},
+							},
+						},
+						{
+							Resources: api.ResourceRequirements{
+								Requests: api.ResourceList{
+									api.ResourceCPU:    resource.MustParse("2"),
+									api.ResourceMemory: resource.MustParse("25M"),
+								},
+								Limits: api.ResourceList{
+									api.ResourceCPU:    resource.MustParse("5"),
+									api.ResourceMemory: resource.MustParse("50M"),
+								},
+							},
+						},
+					},
+				},
+			},
+			usage: api.ResourceList{
+				api.ResourceRequestsCPU:    resource.MustParse("4"),
+				api.ResourceRequestsMemory: resource.MustParse("100M"),
+				api.ResourceLimitsCPU:      resource.MustParse("8"),
+				api.ResourceLimitsMemory:   resource.MustParse("200M"),
+				api.ResourcePods:           resource.MustParse("1"),
+				api.ResourceCPU:            resource.MustParse("4"),
+				api.ResourceMemory:         resource.MustParse("100M"),
+			},
+		},
+	}
+	for testName, testCase := range testCases {
+		actual := evaluator.Usage(testCase.pod)
+		if !quota.Equals(testCase.usage, actual) {
+			t.Errorf("%s expected: %v, actual: %v", testName, testCase.usage, actual)
+		}
+	}
+}

pkg/quota/resources.go

@@ -61,6 +61,26 @@ func LessThanOrEqual(a api.ResourceList, b api.ResourceList) (bool, []api.Resour
 	return result, resourceNames
 }
 
+// Max returns the result of Max(a, b) for each named resource
+func Max(a api.ResourceList, b api.ResourceList) api.ResourceList {
+	result := api.ResourceList{}
+	for key, value := range a {
+		if other, found := b[key]; found {
+			if value.Cmp(other) <= 0 {
+				result[key] = *other.Copy()
+				continue
+			}
+		}
+		result[key] = *value.Copy()
+	}
+	for key, value := range b {
+		if _, found := result[key]; !found {
+			result[key] = *value.Copy()
+		}
+	}
+	return result
+}
+
 // Add returns the result of a + b for each named resource
 func Add(a api.ResourceList, b api.ResourceList) api.ResourceList {
 	result := api.ResourceList{}

pkg/quota/resources_test.go

@@ -76,6 +76,41 @@ func TestEquals(t *testing.T) {
 	}
 }
 
+func TestMax(t *testing.T) {
+	testCases := map[string]struct {
+		a        api.ResourceList
+		b        api.ResourceList
+		expected api.ResourceList
+	}{
+		"noKeys": {
+			a:        api.ResourceList{},
+			b:        api.ResourceList{},
+			expected: api.ResourceList{},
+		},
+		"toEmpty": {
+			a:        api.ResourceList{api.ResourceCPU: resource.MustParse("100m")},
+			b:        api.ResourceList{},
+			expected: api.ResourceList{api.ResourceCPU: resource.MustParse("100m")},
+		},
+		"matching": {
+			a:        api.ResourceList{api.ResourceCPU: resource.MustParse("100m")},
+			b:        api.ResourceList{api.ResourceCPU: resource.MustParse("150m")},
+			expected: api.ResourceList{api.ResourceCPU: resource.MustParse("150m")},
+		},
+		"matching(reverse)": {
+			a:        api.ResourceList{api.ResourceCPU: resource.MustParse("150m")},
+			b:        api.ResourceList{api.ResourceCPU: resource.MustParse("100m")},
+			expected: api.ResourceList{api.ResourceCPU: resource.MustParse("150m")},
+		},
+	}
+	for testName, testCase := range testCases {
+		sum := Max(testCase.a, testCase.b)
+		if result := Equals(testCase.expected, sum); !result {
+			t.Errorf("%s expected: %v, actual: %v", testName, testCase.expected, sum)
+		}
+	}
+}
+
 func TestAdd(t *testing.T) {
 	testCases := map[string]struct {
 		a        api.ResourceList