From c2b670513c0e42e5befc8f1660602bec19e5cb40 Mon Sep 17 00:00:00 2001 From: derekwaynecarr Date: Tue, 31 Mar 2015 10:12:57 -0400 Subject: [PATCH] Introduce concept of a default limit --- examples/limitrange/invalid-pod.json | 4 +- examples/limitrange/limit-range.json | 6 +- examples/limitrange/v1beta3/invalid-pod.json | 12 +- examples/limitrange/v1beta3/limit-range.json | 6 +- pkg/api/types.go | 2 + pkg/api/v1beta1/conversion.go | 6 + pkg/api/v1beta1/types.go | 2 + pkg/api/v1beta2/conversion.go | 6 + pkg/api/v1beta2/types.go | 2 + pkg/api/v1beta3/types.go | 2 + pkg/kubectl/describe.go | 18 +- plugin/pkg/admission/limitranger/admission.go | 70 ++++- .../admission/limitranger/admission_test.go | 290 ++++++++---------- 13 files changed, 251 insertions(+), 175 deletions(-) diff --git a/examples/limitrange/invalid-pod.json b/examples/limitrange/invalid-pod.json index 2a4cdafd0bf..0d341c77faf 100644 --- a/examples/limitrange/invalid-pod.json +++ b/examples/limitrange/invalid-pod.json @@ -11,7 +11,9 @@ "id": "invalid-pod", "containers": [{ "name": "kubernetes-serve-hostname", - "image": "gcr.io/google_containers/serve_hostname" + "image": "gcr.io/google_containers/serve_hostname", + "cpu": 10, + "memory": 1048576 }] } } diff --git a/examples/limitrange/limit-range.json b/examples/limitrange/limit-range.json index 6e8906c5542..867a6ae269f 100644 --- a/examples/limitrange/limit-range.json +++ b/examples/limitrange/limit-range.json @@ -24,7 +24,11 @@ "min": { "memory": "1048576", "cpu": "0.25" - } + }, + "default": { + "memory": "1048576", + "cpu": "0.25" + } } ] } diff --git a/examples/limitrange/v1beta3/invalid-pod.json b/examples/limitrange/v1beta3/invalid-pod.json index 05dc3ad0300..36260f3728a 100644 --- a/examples/limitrange/v1beta3/invalid-pod.json +++ b/examples/limitrange/v1beta3/invalid-pod.json @@ -10,7 +10,13 @@ "spec": { "containers": [{ "name": "kubernetes-serve-hostname", - "image": "gcr.io/google_containers/serve_hostname" - }] - } + "image": "gcr.io/google_containers/serve_hostname", + "resources": { + "limits": { + "cpu": "10m", + "memory": "1Mi" + } + } + }] + } } diff --git a/examples/limitrange/v1beta3/limit-range.json b/examples/limitrange/v1beta3/limit-range.json index 64333656a68..58c92a4f9aa 100644 --- a/examples/limitrange/v1beta3/limit-range.json +++ b/examples/limitrange/v1beta3/limit-range.json @@ -26,7 +26,11 @@ "min": { "memory": "1Mi", "cpu": "250m" - } + }, + "default": { + "memory": "1Mi", + "cpu": "250m" + } } ] } diff --git a/pkg/api/types.go b/pkg/api/types.go index 10b6272a16d..066018e1504 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -1559,6 +1559,8 @@ type LimitRangeItem struct { Max ResourceList `json:"max,omitempty"` // Min usage constraints on this kind by resource name Min ResourceList `json:"min,omitempty"` + // Default usage constraints on this kind by resource name + Default ResourceList `json:"default,omitempty"` } // LimitRangeSpec defines a min/max usage limit for resources that match on kind diff --git a/pkg/api/v1beta1/conversion.go b/pkg/api/v1beta1/conversion.go index 31bea4d290b..a49569c9a1e 100644 --- a/pkg/api/v1beta1/conversion.go +++ b/pkg/api/v1beta1/conversion.go @@ -923,6 +923,9 @@ func init() { if err := s.Convert(&in.Min, &out.Min, 0); err != nil { return err } + if err := s.Convert(&in.Default, &out.Default, 0); err != nil { + return err + } return nil }, func(in *LimitRangeItem, out *newer.LimitRangeItem, s conversion.Scope) error { @@ -934,6 +937,9 @@ func init() { if err := s.Convert(&in.Min, &out.Min, 0); err != nil { return err } + if err := s.Convert(&in.Default, &out.Default, 0); err != nil { + return err + } return nil }, diff --git a/pkg/api/v1beta1/types.go b/pkg/api/v1beta1/types.go index a9e917b0320..2b26dbc5a96 100644 --- a/pkg/api/v1beta1/types.go +++ b/pkg/api/v1beta1/types.go @@ -1358,6 +1358,8 @@ type LimitRangeItem struct { Max ResourceList `json:"max,omitempty" description:"max usage constraints on this kind by resource name"` // Min usage constraints on this kind by resource name Min ResourceList `json:"min,omitempty" description:"min usage constraints on this kind by resource name"` + // Default usage constraints on this kind by resource name + Default ResourceList `json:"default,omitempty" description:"default values on this kind by resource name if omitted"` } // LimitRangeSpec defines a min/max usage limit for resources that match on kind diff --git a/pkg/api/v1beta2/conversion.go b/pkg/api/v1beta2/conversion.go index 0300c9c2467..3ad72cee180 100644 --- a/pkg/api/v1beta2/conversion.go +++ b/pkg/api/v1beta2/conversion.go @@ -854,6 +854,9 @@ func init() { if err := s.Convert(&in.Min, &out.Min, 0); err != nil { return err } + if err := s.Convert(&in.Default, &out.Default, 0); err != nil { + return err + } return nil }, func(in *LimitRangeItem, out *newer.LimitRangeItem, s conversion.Scope) error { @@ -865,6 +868,9 @@ func init() { if err := s.Convert(&in.Min, &out.Min, 0); err != nil { return err } + if err := s.Convert(&in.Default, &out.Default, 0); err != nil { + return err + } return nil }, diff --git a/pkg/api/v1beta2/types.go b/pkg/api/v1beta2/types.go index 09b5fa1efd2..ee02df16306 100644 --- a/pkg/api/v1beta2/types.go +++ b/pkg/api/v1beta2/types.go @@ -1420,6 +1420,8 @@ type LimitRangeItem struct { Max ResourceList `json:"max,omitempty" description:"max usage constraints on this kind by resource name"` // Min usage constraints on this kind by resource name Min ResourceList `json:"min,omitempty" description:"min usage constraints on this kind by resource name"` + // Default usage constraints on this kind by resource name + Default ResourceList `json:"default,omitempty" description:"default values on this kind by resource name if omitted"` } // LimitRangeSpec defines a min/max usage limit for resources that match on kind diff --git a/pkg/api/v1beta3/types.go b/pkg/api/v1beta3/types.go index 5640eb35873..79c94636811 100644 --- a/pkg/api/v1beta3/types.go +++ b/pkg/api/v1beta3/types.go @@ -1464,6 +1464,8 @@ type LimitRangeItem struct { Max ResourceList `json:"max,omitempty" description:"max usage constraints on this kind by resource name"` // Min usage constraints on this kind by resource name Min ResourceList `json:"min,omitempty" description:"min usage constraints on this kind by resource name"` + // Default usage constraints on this kind by resource name + Default ResourceList `json:"default,omitempty" description:"default values on this kind by resource name if omitted"` } // LimitRangeSpec defines a min/max usage limit for resources that match on kind diff --git a/pkg/kubectl/describe.go b/pkg/kubectl/describe.go index c8189642cdf..8a17d41d986 100644 --- a/pkg/kubectl/describe.go +++ b/pkg/kubectl/describe.go @@ -115,12 +115,13 @@ func (d *LimitRangeDescriber) Describe(namespace, name string) (string, error) { func describeLimitRange(limitRange *api.LimitRange) (string, error) { return tabbedString(func(out io.Writer) error { fmt.Fprintf(out, "Name:\t%s\n", limitRange.Name) - fmt.Fprintf(out, "Type\tResource\tMin\tMax\n") - fmt.Fprintf(out, "----\t--------\t---\t---\n") + fmt.Fprintf(out, "Type\tResource\tMin\tMax\tDefault\n") + fmt.Fprintf(out, "----\t--------\t---\t---\t---\n") for i := range limitRange.Spec.Limits { item := limitRange.Spec.Limits[i] maxResources := item.Max minResources := item.Min + defaultResources := item.Default set := map[api.ResourceName]bool{} for k := range maxResources { @@ -129,11 +130,15 @@ func describeLimitRange(limitRange *api.LimitRange) (string, error) { for k := range minResources { set[k] = true } + for k := range defaultResources { + set[k] = true + } for k := range set { // if no value is set, we output - maxValue := "-" minValue := "-" + defaultValue := "-" maxQuantity, maxQuantityFound := maxResources[k] if maxQuantityFound { @@ -145,8 +150,13 @@ func describeLimitRange(limitRange *api.LimitRange) (string, error) { minValue = minQuantity.String() } - msg := "%v\t%v\t%v\t%v\n" - fmt.Fprintf(out, msg, item.Type, k, minValue, maxValue) + defaultQuantity, defaultQuantityFound := defaultResources[k] + if defaultQuantityFound { + defaultValue = defaultQuantity.String() + } + + msg := "%v\t%v\t%v\t%v\t%v\n" + fmt.Fprintf(out, msg, item.Type, k, minValue, maxValue, defaultValue) } } return nil diff --git a/plugin/pkg/admission/limitranger/admission.go b/plugin/pkg/admission/limitranger/admission.go index af454ee45dd..f198b9f8d3e 100644 --- a/plugin/pkg/admission/limitranger/admission.go +++ b/plugin/pkg/admission/limitranger/admission.go @@ -35,7 +35,7 @@ import ( func init() { admission.RegisterPlugin("LimitRanger", func(client client.Interface, config io.Reader) (admission.Interface, error) { - return NewLimitRanger(client, PodLimitFunc), nil + return NewLimitRanger(client, Limit), nil }) } @@ -114,13 +114,67 @@ func Max(a int64, b int64) int64 { return b } -// PodLimitFunc enforces that a pod spec does not exceed any limits specified on the supplied limit range -func PodLimitFunc(limitRange *api.LimitRange, resourceName string, obj runtime.Object) error { - if resourceName != "pods" { - return nil +// Limit enforces resource requirements of incoming resources against enumerated constraints +// on the LimitRange. It may modify the incoming object to apply default resource requirements +// if not specified, and enumerated on the LimitRange +func Limit(limitRange *api.LimitRange, resourceName string, obj runtime.Object) error { + switch resourceName { + case "pods": + return PodLimitFunc(limitRange, obj.(*api.Pod)) } + return nil +} - pod := obj.(*api.Pod) +// defaultContainerResourceRequirements returns the default requirements for a container +// the requirement.Limits are taken from the LimitRange defaults (if specified) +// the requirement.Requests are taken from the LimitRange min (if specified) +func defaultContainerResourceRequirements(limitRange *api.LimitRange) api.ResourceRequirements { + requirements := api.ResourceRequirements{} + requirements.Limits = api.ResourceList{} + requirements.Requests = api.ResourceList{} + + for i := range limitRange.Spec.Limits { + limit := limitRange.Spec.Limits[i] + if limit.Type == api.LimitTypeContainer { + for k, v := range limit.Default { + value := v.Copy() + requirements.Limits[k] = *value + } + for k, v := range limit.Min { + value := v.Copy() + requirements.Requests[k] = *value + } + } + } + return requirements +} + +// mergePodResourceRequirements merges enumerated requirements with default requirements +func mergePodResourceRequirements(pod *api.Pod, defaultRequirements *api.ResourceRequirements) { + for i := range pod.Spec.Containers { + container := pod.Spec.Containers[i] + for k, v := range defaultRequirements.Limits { + _, found := container.Resources.Limits[k] + if !found { + container.Resources.Limits[k] = *v.Copy() + } + } + for k, v := range defaultRequirements.Requests { + _, found := container.Resources.Requests[k] + if !found { + container.Resources.Requests[k] = *v.Copy() + } + } + } +} + +// PodLimitFunc enforces resource requirements enumerated by the pod against +// the specified LimitRange. The pod may be modified to apply default resource +// requirements if not specified, and enumerated on the LimitRange +func PodLimitFunc(limitRange *api.LimitRange, pod *api.Pod) error { + + defaultResources := defaultContainerResourceRequirements(limitRange) + mergePodResourceRequirements(pod, &defaultResources) podCPU := int64(0) podMem := int64(0) @@ -190,11 +244,11 @@ func PodLimitFunc(limitRange *api.LimitRange, resourceName string, obj runtime.O switch minOrMax { case "Min": if observed < enforced { - return apierrors.NewForbidden(resourceName, pod.Name, err) + return apierrors.NewForbidden("pods", pod.Name, err) } case "Max": if observed > enforced { - return apierrors.NewForbidden(resourceName, pod.Name, err) + return apierrors.NewForbidden("pods", pod.Name, err) } } } diff --git a/plugin/pkg/admission/limitranger/admission_test.go b/plugin/pkg/admission/limitranger/admission_test.go index d86e839334d..559600ce101 100644 --- a/plugin/pkg/admission/limitranger/admission_test.go +++ b/plugin/pkg/admission/limitranger/admission_test.go @@ -17,27 +17,33 @@ limitations under the License. package limitranger import ( + "strconv" "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/resource" ) -func getResourceRequirements(cpu, memory string) api.ResourceRequirements { - res := api.ResourceRequirements{} - res.Limits = api.ResourceList{} +func getResourceList(cpu, memory string) api.ResourceList { + res := api.ResourceList{} if cpu != "" { - res.Limits[api.ResourceCPU] = resource.MustParse(cpu) + res[api.ResourceCPU] = resource.MustParse(cpu) } if memory != "" { - res.Limits[api.ResourceMemory] = resource.MustParse(memory) + res[api.ResourceMemory] = resource.MustParse(memory) } - return res } -func TestPodLimitFunc(t *testing.T) { - limitRange := &api.LimitRange{ +func getResourceRequirements(limits, requests api.ResourceList) api.ResourceRequirements { + res := api.ResourceRequirements{} + res.Limits = limits + res.Requests = requests + return res +} + +func validLimitRange() api.LimitRange { + return api.LimitRange{ ObjectMeta: api.ObjectMeta{ Name: "abc", }, @@ -45,177 +51,147 @@ func TestPodLimitFunc(t *testing.T) { Limits: []api.LimitRangeItem{ { Type: api.LimitTypePod, - Max: getResourceRequirements("200m", "4Gi").Limits, - Min: getResourceRequirements("50m", "2Mi").Limits, + Max: getResourceList("200m", "4Gi"), + Min: getResourceList("50m", "2Mi"), }, { - Type: api.LimitTypeContainer, - Max: getResourceRequirements("100m", "2Gi").Limits, - Min: getResourceRequirements("25m", "1Mi").Limits, + Type: api.LimitTypeContainer, + Max: getResourceList("100m", "2Gi"), + Min: getResourceList("25m", "1Mi"), + Default: getResourceList("50m", "5Mi"), }, }, }, } +} +func validPod(name string, numContainers int, resources api.ResourceRequirements) api.Pod { + pod := api.Pod{ + ObjectMeta: api.ObjectMeta{Name: name}, + Spec: api.PodSpec{}, + } + pod.Spec.Containers = make([]api.Container, 0, numContainers) + for i := 0; i < numContainers; i++ { + pod.Spec.Containers = append(pod.Spec.Containers, api.Container{ + Image: "foo:V" + strconv.Itoa(i), + Resources: resources, + }) + } + return pod +} + +func TestDefaultContainerResourceRequirements(t *testing.T) { + limitRange := validLimitRange() + expected := api.ResourceRequirements{ + Limits: getResourceList("50m", "5Mi"), + Requests: getResourceList("25m", "1Mi"), + } + + actual := defaultContainerResourceRequirements(&limitRange) + if !api.Semantic.DeepEqual(expected, actual) { + t.Errorf("actual.Limits != expected.Limits; %v != %v", actual.Limits, expected.Limits) + t.Errorf("actual.Requests != expected.Requests; %v != %v", actual.Requests, expected.Requests) + t.Errorf("expected != actual; %v != %v", expected, actual) + } +} + +func TestMergePodResourceRequirements(t *testing.T) { + limitRange := validLimitRange() + + // pod with no resources enumerated should get each resource from default + expected := getResourceRequirements(getResourceList("", ""), getResourceList("", "")) + pod := validPod("empty-resources", 1, expected) + defaultRequirements := defaultContainerResourceRequirements(&limitRange) + mergePodResourceRequirements(&pod, &defaultRequirements) + for i := range pod.Spec.Containers { + actual := pod.Spec.Containers[i].Resources + if !api.Semantic.DeepEqual(expected, actual) { + t.Errorf("pod %v, expected != actual; %v != %v", pod.Name, expected, actual) + } + } + + // pod with some resources enumerated should only merge empty + input := getResourceRequirements(getResourceList("", "512Mi"), getResourceList("", "")) + pod = validPod("limit-memory", 1, input) + expected = api.ResourceRequirements{ + Limits: api.ResourceList{ + api.ResourceCPU: defaultRequirements.Limits[api.ResourceCPU], + api.ResourceMemory: resource.MustParse("512Mi"), + }, + Requests: api.ResourceList{ + api.ResourceCPU: defaultRequirements.Requests[api.ResourceCPU], + api.ResourceMemory: defaultRequirements.Requests[api.ResourceMemory], + }, + } + mergePodResourceRequirements(&pod, &defaultRequirements) + for i := range pod.Spec.Containers { + actual := pod.Spec.Containers[i].Resources + if !api.Semantic.DeepEqual(expected, actual) { + t.Errorf("pod %v, expected != actual; %v != %v", pod.Name, expected, actual) + } + } +} + +func TestPodLimitFunc(t *testing.T) { + limitRange := validLimitRange() successCases := []api.Pod{ - { - ObjectMeta: api.ObjectMeta{Name: "foo"}, - Spec: api.PodSpec{ - Containers: []api.Container{ - { - Image: "foo:V1", - Resources: getResourceRequirements("100m", "2Gi"), - }, - { - Image: "boo:V1", - Resources: getResourceRequirements("100m", "2Gi"), - }, - }, - }, - }, - { - ObjectMeta: api.ObjectMeta{Name: "bar"}, - Spec: api.PodSpec{ - Containers: []api.Container{ - { - Image: "boo:V1", - Resources: getResourceRequirements("100m", "2Gi"), - }, - }, - }, - }, + validPod("foo", 2, getResourceRequirements(getResourceList("100m", "2Gi"), getResourceList("", ""))), + validPod("bar", 1, getResourceRequirements(getResourceList("100m", "2Gi"), getResourceList("", ""))), } errorCases := map[string]api.Pod{ - "min-container-cpu": { - ObjectMeta: api.ObjectMeta{Name: "foo"}, - Spec: api.PodSpec{ - Containers: []api.Container{ - { - Image: "boo:V1", - Resources: getResourceRequirements("25m", "2Gi"), - }, - }, - }, - }, - "max-container-cpu": { - ObjectMeta: api.ObjectMeta{Name: "foo"}, - Spec: api.PodSpec{ - Containers: []api.Container{ - { - Image: "boo:V1", - Resources: getResourceRequirements("110m", "1Gi"), - }, - }, - }, - }, - "min-container-mem": { - ObjectMeta: api.ObjectMeta{Name: "foo"}, - Spec: api.PodSpec{ - Containers: []api.Container{ - { - Image: "boo:V1", - Resources: getResourceRequirements("30m", "0"), - }, - }, - }, - }, - "max-container-mem": { - ObjectMeta: api.ObjectMeta{Name: "foo"}, - Spec: api.PodSpec{ - Containers: []api.Container{ - { - Image: "boo:V1", - Resources: getResourceRequirements("30m", "3Gi"), - }, - }, - }, - }, - "min-pod-cpu": { - ObjectMeta: api.ObjectMeta{Name: "foo"}, - Spec: api.PodSpec{ - Containers: []api.Container{ - { - Image: "boo:V1", - Resources: getResourceRequirements("40m", "2Gi"), - }, - }, - }, - }, - "max-pod-cpu": { - ObjectMeta: api.ObjectMeta{Name: "foo"}, - Spec: api.PodSpec{ - Containers: []api.Container{ - { - Image: "boo:V1", - Resources: getResourceRequirements("60m", "1Mi"), - }, - { - Image: "boo:V2", - Resources: getResourceRequirements("60m", "1Mi"), - }, - { - Image: "boo:V3", - Resources: getResourceRequirements("60m", "1Mi"), - }, - { - Image: "boo:V4", - Resources: getResourceRequirements("60m", "1Mi"), - }, - }, - }, - }, - "max-pod-memory": { - ObjectMeta: api.ObjectMeta{Name: "foo"}, - Spec: api.PodSpec{ - Containers: []api.Container{ - { - Image: "boo:V1", - Resources: getResourceRequirements("60m", "2Gi"), - }, - { - Image: "boo:V2", - Resources: getResourceRequirements("60m", "2Gi"), - }, - { - Image: "boo:V3", - Resources: getResourceRequirements("60m", "2Gi"), - }, - }, - }, - }, - "min-pod-memory": { - ObjectMeta: api.ObjectMeta{Name: "foo"}, - Spec: api.PodSpec{ - Containers: []api.Container{ - { - Image: "boo:V1", - Resources: getResourceRequirements("60m", "0"), - }, - { - Image: "boo:V2", - Resources: getResourceRequirements("60m", "0"), - }, - { - Image: "boo:V3", - Resources: getResourceRequirements("60m", "0"), - }, - }, - }, - }, + "min-container-cpu": validPod("foo", 1, getResourceRequirements(getResourceList("25m", "2Gi"), getResourceList("", ""))), + "max-container-cpu": validPod("foo", 1, getResourceRequirements(getResourceList("110m", "1Gi"), getResourceList("", ""))), + "min-container-mem": validPod("foo", 1, getResourceRequirements(getResourceList("30m", "0"), getResourceList("", ""))), + "max-container-mem": validPod("foo", 1, getResourceRequirements(getResourceList("30m", "3Gi"), getResourceList("", ""))), + "min-pod-cpu": validPod("foo", 1, getResourceRequirements(getResourceList("40m", "2Gi"), getResourceList("", ""))), + "max-pod-cpu": validPod("foo", 4, getResourceRequirements(getResourceList("60m", "1Mi"), getResourceList("", ""))), + "max-pod-memory": validPod("foo", 3, getResourceRequirements(getResourceList("60m", "2Gi"), getResourceList("", ""))), + "min-pod-memory": validPod("foo", 3, getResourceRequirements(getResourceList("60m", "0"), getResourceList("", ""))), } for i := range successCases { - err := PodLimitFunc(limitRange, "pods", &successCases[i]) + err := PodLimitFunc(&limitRange, &successCases[i]) if err != nil { t.Errorf("Unexpected error for valid pod: %v, %v", successCases[i].Name, err) } } for k, v := range errorCases { - err := PodLimitFunc(limitRange, "pods", &v) + err := PodLimitFunc(&limitRange, &v) if err == nil { t.Errorf("Expected error for %s", k) } } } + +func TestPodLimitFuncApplyDefault(t *testing.T) { + limitRange := validLimitRange() + testPod := validPod("foo", 1, getResourceRequirements(api.ResourceList{}, api.ResourceList{})) + err := PodLimitFunc(&limitRange, &testPod) + if err != nil { + t.Errorf("Unexpected error for valid pod: %v, %v", testPod.Name, err) + } + + for i := range testPod.Spec.Containers { + container := testPod.Spec.Containers[i] + memory := testPod.Spec.Containers[i].Resources.Limits.Memory().String() + cpu := testPod.Spec.Containers[i].Resources.Limits.Cpu().String() + switch container.Image { + case "boo:V1": + if memory != "100Mi" { + t.Errorf("Unexpected memory value %s", memory) + } + if cpu != "50m" { + t.Errorf("Unexpected cpu value %s", cpu) + } + case "foo:V1": + if memory != "2Gi" { + t.Errorf("Unexpected memory value %s", memory) + } + if cpu != "100m" { + t.Errorf("Unexpected cpu value %s", cpu) + } + } + } +}