Merge pull request #12600 from derekwaynecarr/resource_quota_request

Update ResourceQuota for resource requirements requests

pkg/api/resource/quantity.go

@@ -309,6 +309,14 @@ func (q *Quantity) Add(y Quantity) error {
 	return nil
 }
 
+func (q *Quantity) Sub(y Quantity) error {
+	if q.Format != y.Format {
+		return fmt.Errorf("format mismatch: %v vs. %v", q.Format, y.Format)
+	}
+	q.Amount.Sub(q.Amount, y.Amount)
+	return nil
+}
+
 // MarshalJSON implements the json.Marshaller interface.
 func (q Quantity) MarshalJSON() ([]byte, error) {
 	return []byte(`"` + q.String() + `"`), nil

pkg/controller/resourcequota/resource_quota_controller.go

@@ -17,6 +17,7 @@ limitations under the License.
 package resourcequotacontroller
 
 import (
+	"fmt"
 	"time"
 
 	"github.com/golang/glog"
@@ -163,18 +164,6 @@ func (rm *ResourceQuotaController) syncResourceQuota(quota api.ResourceQuota) (e
 		switch k {
 		case api.ResourcePods:
 			value = resource.NewQuantity(int64(len(filteredPods)), resource.DecimalSI)
-		case api.ResourceMemory:
-			val := int64(0)
-			for _, pod := range filteredPods {
-				val = val + PodMemory(pod).Value()
-			}
-			value = resource.NewQuantity(int64(val), resource.DecimalSI)
-		case api.ResourceCPU:
-			val := int64(0)
-			for _, pod := range filteredPods {
-				val = val + PodCPU(pod).MilliValue()
-			}
-			value = resource.NewMilliQuantity(int64(val), resource.DecimalSI)
 		case api.ResourceServices:
 			items, err := rm.kubeClient.Services(usage.Namespace).List(labels.Everything())
 			if err != nil {
@@ -205,6 +194,10 @@ func (rm *ResourceQuotaController) syncResourceQuota(quota api.ResourceQuota) (e
 				return err
 			}
 			value = resource.NewQuantity(int64(len(items.Items)), resource.DecimalSI)
+		case api.ResourceMemory:
+			value = PodsRequests(filteredPods, api.ResourceMemory)
+		case api.ResourceCPU:
+			value = PodsRequests(filteredPods, api.ResourceCPU)
 		}
 
 		// ignore fields we do not understand (assume another controller is tracking it)
@@ -224,7 +217,73 @@ func (rm *ResourceQuotaController) syncResourceQuota(quota api.ResourceQuota) (e
 	return nil
 }
 
-// PodCPU computes total cpu usage of a pod
+// PodsRequests returns sum of each resource request for each pod in list
+// If a given pod in the list does not have a request for the named resource, we log the error
+// but still attempt to get the most representative count
+func PodsRequests(pods []*api.Pod, resourceName api.ResourceName) *resource.Quantity {
+	var sum *resource.Quantity
+	for i := range pods {
+		pod := pods[i]
+		podQuantity, err := PodRequests(pod, resourceName)
+		if err != nil {
+			// log the error, but try to keep the most accurate count possible in log
+			// rationale here is that you may have had pods in a namespace that did not have
+			// explicit requests prior to adding the quota
+			glog.Infof("No explicit request for resource, pod %s/%s, %s", pod.Namespace, pod.Name, resourceName)
+		} else {
+			if sum == nil {
+				sum = podQuantity
+			} else {
+				sum.Add(*podQuantity)
+			}
+		}
+	}
+	// if list is empty
+	if sum == nil {
+		q := resource.MustParse("0")
+		sum = &q
+	}
+	return sum
+}
+
+// PodRequests returns sum of each resource request across all containers in pod
+func PodRequests(pod *api.Pod, resourceName api.ResourceName) (*resource.Quantity, error) {
+	if !PodHasRequests(pod, resourceName) {
+		return nil, fmt.Errorf("Each container in pod %s/%s does not have an explicit request for resource %s.", pod.Namespace, pod.Name, resourceName)
+	}
+	var sum *resource.Quantity
+	for j := range pod.Spec.Containers {
+		value, _ := pod.Spec.Containers[j].Resources.Requests[resourceName]
+		if sum == nil {
+			sum = value.Copy()
+		} else {
+			err := sum.Add(value)
+			if err != nil {
+				return sum, err
+			}
+		}
+	}
+	// if list is empty
+	if sum == nil {
+		q := resource.MustParse("0")
+		sum = &q
+	}
+	return sum, nil
+}
+
+// PodHasRequests verifies that each container in the pod has an explicit request that is non-zero for a named resource
+func PodHasRequests(pod *api.Pod, resourceName api.ResourceName) bool {
+	for j := range pod.Spec.Containers {
+		value, valueSet := pod.Spec.Containers[j].Resources.Requests[resourceName]
+		if !valueSet || value.Value() == int64(0) {
+			return false
+		}
+	}
+	return true
+}
+
+// PodCPU computes total cpu limit across all containers in pod
+// TODO: Remove this once the mesos scheduler becomes request aware
 func PodCPU(pod *api.Pod) *resource.Quantity {
 	val := int64(0)
 	for j := range pod.Spec.Containers {
@@ -233,29 +292,8 @@ func PodCPU(pod *api.Pod) *resource.Quantity {
 	return resource.NewMilliQuantity(int64(val), resource.DecimalSI)
 }
 
-// IsPodCPUUnbounded returns true if the cpu use is unbounded for any container in pod
+// PodMemory computes total memory limit across all containers in a pod
-func IsPodCPUUnbounded(pod *api.Pod) bool {
+// TODO: Remove this once the mesos scheduler becomes request aware
-	for j := range pod.Spec.Containers {
-		container := pod.Spec.Containers[j]
-		if container.Resources.Limits.Cpu().MilliValue() == int64(0) {
-			return true
-		}
-	}
-	return false
-}
-
-// IsPodMemoryUnbounded returns true if the memory use is unbounded for any container in pod
-func IsPodMemoryUnbounded(pod *api.Pod) bool {
-	for j := range pod.Spec.Containers {
-		container := pod.Spec.Containers[j]
-		if container.Resources.Limits.Memory().Value() == int64(0) {
-			return true
-		}
-	}
-	return false
-}
-
-// PodMemory computes the memory usage of a pod
 func PodMemory(pod *api.Pod) *resource.Quantity {
 	val := int64(0)
 	for j := range pod.Spec.Containers {

pkg/controller/resourcequota/resource_quota_controller_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package resourcequotacontroller
 
 import (
+	"strconv"
 	"testing"
 
 	"k8s.io/kubernetes/pkg/api"
@@ -25,19 +26,39 @@ import (
 	"k8s.io/kubernetes/pkg/util"
 )
 
-func getResourceRequirements(cpu, memory string) api.ResourceRequirements {
+func getResourceList(cpu, memory string) api.ResourceList {
-	res := api.ResourceRequirements{}
+	res := api.ResourceList{}
-	res.Limits = 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 getResourceRequirements(requests, limits api.ResourceList) api.ResourceRequirements {
+	res := api.ResourceRequirements{}
+	res.Requests = requests
+	res.Limits = limits
+	return res
+}
+
+func validPod(name string, numContainers int, resources api.ResourceRequirements) *api.Pod {
+	pod := &api.Pod{
+		ObjectMeta: api.ObjectMeta{Name: name, Namespace: "test"},
+		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 TestFilterQuotaPods(t *testing.T) {
 	pods := []api.Pod{
 		{
@@ -105,7 +126,7 @@ func TestSyncResourceQuota(t *testing.T) {
 				Status:     api.PodStatus{Phase: api.PodRunning},
 				Spec: api.PodSpec{
 					Volumes:    []api.Volume{{Name: "vol"}},
-					Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}},
+					Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements(getResourceList("100m", "1Gi"), getResourceList("", ""))}},
 				},
 			},
 			{
@@ -113,7 +134,7 @@ func TestSyncResourceQuota(t *testing.T) {
 				Status:     api.PodStatus{Phase: api.PodRunning},
 				Spec: api.PodSpec{
 					Volumes:    []api.Volume{{Name: "vol"}},
-					Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}},
+					Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements(getResourceList("100m", "1Gi"), getResourceList("", ""))}},
 				},
 			},
 			{
@@ -121,7 +142,7 @@ func TestSyncResourceQuota(t *testing.T) {
 				Status:     api.PodStatus{Phase: api.PodFailed},
 				Spec: api.PodSpec{
 					Volumes:    []api.Volume{{Name: "vol"}},
-					Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}},
+					Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements(getResourceList("100m", "1Gi"), getResourceList("", ""))}},
 				},
 			},
 		},
@@ -144,7 +165,7 @@ func TestSyncResourceQuota(t *testing.T) {
 			},
 			Used: api.ResourceList{
 				api.ResourceCPU:    resource.MustParse("200m"),
-				api.ResourceMemory: resource.MustParse("2147483648"),
+				api.ResourceMemory: resource.MustParse("2Gi"),
 				api.ResourcePods:   resource.MustParse("2"),
 			},
 		},
@@ -177,7 +198,6 @@ func TestSyncResourceQuota(t *testing.T) {
 			t.Errorf("Usage Used: Key: %v, Expected: %v, Actual: %v", k, expectedValue, actualValue)
 		}
 	}
-
 }
 
 func TestSyncResourceQuotaSpecChange(t *testing.T) {
@@ -269,62 +289,151 @@ func TestSyncResourceQuotaNoChange(t *testing.T) {
 	}
 }
 
-func TestIsPodCPUUnbounded(t *testing.T) {
+func TestPodHasRequests(t *testing.T) {
-	pod := api.Pod{
+	type testCase struct {
-		ObjectMeta: api.ObjectMeta{Name: "pod-running"},
+		pod            *api.Pod
-		Status:     api.PodStatus{Phase: api.PodRunning},
+		resourceName   api.ResourceName
-		Spec: api.PodSpec{
+		expectedResult bool
-			Volumes:    []api.Volume{{Name: "vol"}},
+	}
-			Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "0")}},
+	testCases := []testCase{
+		{
+			pod:            validPod("request-cpu", 2, getResourceRequirements(getResourceList("100m", ""), getResourceList("", ""))),
+			resourceName:   api.ResourceCPU,
+			expectedResult: true,
+		},
+		{
+			pod:            validPod("no-request-cpu", 2, getResourceRequirements(getResourceList("", ""), getResourceList("", ""))),
+			resourceName:   api.ResourceCPU,
+			expectedResult: false,
+		},
+		{
+			pod:            validPod("request-zero-cpu", 2, getResourceRequirements(getResourceList("0", ""), getResourceList("", ""))),
+			resourceName:   api.ResourceCPU,
+			expectedResult: false,
+		},
+		{
+			pod:            validPod("request-memory", 2, getResourceRequirements(getResourceList("", "2Mi"), getResourceList("", ""))),
+			resourceName:   api.ResourceMemory,
+			expectedResult: true,
+		},
+		{
+			pod:            validPod("no-request-memory", 2, getResourceRequirements(getResourceList("", ""), getResourceList("", ""))),
+			resourceName:   api.ResourceMemory,
+			expectedResult: false,
+		},
+		{
+			pod:            validPod("request-zero-memory", 2, getResourceRequirements(getResourceList("", "0"), getResourceList("", ""))),
+			resourceName:   api.ResourceMemory,
+			expectedResult: false,
 		},
 	}
-	if IsPodCPUUnbounded(&pod) {
+	for _, item := range testCases {
-		t.Errorf("Expected false")
+		if actual := PodHasRequests(item.pod, item.resourceName); item.expectedResult != actual {
-	}
+			t.Errorf("Pod %s for resource %s expected %v actual %v", item.pod.Name, item.resourceName, item.expectedResult, actual)
-	pod = api.Pod{
+		}
-		ObjectMeta: api.ObjectMeta{Name: "pod-running"},
-		Status:     api.PodStatus{Phase: api.PodRunning},
-		Spec: api.PodSpec{
-			Volumes:    []api.Volume{{Name: "vol"}},
-			Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("0", "0")}},
-		},
-	}
-	if !IsPodCPUUnbounded(&pod) {
-		t.Errorf("Expected true")
-	}
-
-	pod.Spec.Containers[0].Resources = api.ResourceRequirements{}
-	if !IsPodCPUUnbounded(&pod) {
-		t.Errorf("Expected true")
 	}
 }
 
-func TestIsPodMemoryUnbounded(t *testing.T) {
+func TestPodRequests(t *testing.T) {
-	pod := api.Pod{
+	type testCase struct {
-		ObjectMeta: api.ObjectMeta{Name: "pod-running"},
+		pod            *api.Pod
-		Status:     api.PodStatus{Phase: api.PodRunning},
+		resourceName   api.ResourceName
-		Spec: api.PodSpec{
+		expectedResult string
-			Volumes:    []api.Volume{{Name: "vol"}},
+		expectedError  bool
-			Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("0", "1Gi")}},
+	}
+	testCases := []testCase{
+		{
+			pod:            validPod("request-cpu", 2, getResourceRequirements(getResourceList("100m", ""), getResourceList("", ""))),
+			resourceName:   api.ResourceCPU,
+			expectedResult: "200m",
+			expectedError:  false,
+		},
+		{
+			pod:            validPod("no-request-cpu", 2, getResourceRequirements(getResourceList("", ""), getResourceList("", ""))),
+			resourceName:   api.ResourceCPU,
+			expectedResult: "",
+			expectedError:  true,
+		},
+		{
+			pod:            validPod("request-zero-cpu", 2, getResourceRequirements(getResourceList("0", ""), getResourceList("", ""))),
+			resourceName:   api.ResourceCPU,
+			expectedResult: "",
+			expectedError:  true,
+		},
+		{
+			pod:            validPod("request-memory", 2, getResourceRequirements(getResourceList("", "500Mi"), getResourceList("", ""))),
+			resourceName:   api.ResourceMemory,
+			expectedResult: "1000Mi",
+			expectedError:  false,
+		},
+		{
+			pod:            validPod("no-request-memory", 2, getResourceRequirements(getResourceList("", ""), getResourceList("", ""))),
+			resourceName:   api.ResourceMemory,
+			expectedResult: "",
+			expectedError:  true,
+		},
+		{
+			pod:            validPod("request-zero-memory", 2, getResourceRequirements(getResourceList("", "0"), getResourceList("", ""))),
+			resourceName:   api.ResourceMemory,
+			expectedResult: "",
+			expectedError:  true,
 		},
 	}
-	if IsPodMemoryUnbounded(&pod) {
+	for _, item := range testCases {
-		t.Errorf("Expected false")
+		actual, err := PodRequests(item.pod, item.resourceName)
-	}
+		if item.expectedError != (err != nil) {
-	pod = api.Pod{
+			t.Errorf("Unexpected error result for pod %s for resource %s expected error %v got %v", item.pod.Name, item.resourceName, item.expectedError, err)
-		ObjectMeta: api.ObjectMeta{Name: "pod-running"},
+		}
-		Status:     api.PodStatus{Phase: api.PodRunning},
+		if item.expectedResult != "" && (item.expectedResult != actual.String()) {
-		Spec: api.PodSpec{
+			t.Errorf("Expected %s, Actual %s, pod %s for resource %s", item.expectedResult, actual.String(), item.pod.Name, item.resourceName)
-			Volumes:    []api.Volume{{Name: "vol"}},
+		}
-			Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("0", "0")}},
+	}
-		},
+}
-	}
+
-	if !IsPodMemoryUnbounded(&pod) {
+func TestPodsRequests(t *testing.T) {
-		t.Errorf("Expected true")
+	type testCase struct {
-	}
+		pods           []*api.Pod
-
+		resourceName   api.ResourceName
-	pod.Spec.Containers[0].Resources = api.ResourceRequirements{}
+		expectedResult string
-	if !IsPodMemoryUnbounded(&pod) {
+	}
-		t.Errorf("Expected true")
+	testCases := []testCase{
+		{
+			pods: []*api.Pod{
+				validPod("request-cpu-1", 1, getResourceRequirements(getResourceList("100m", ""), getResourceList("", ""))),
+				validPod("request-cpu-2", 1, getResourceRequirements(getResourceList("1", ""), getResourceList("", ""))),
+			},
+			resourceName:   api.ResourceCPU,
+			expectedResult: "1100m",
+		},
+		{
+			pods: []*api.Pod{
+				validPod("no-request-cpu-1", 1, getResourceRequirements(getResourceList("", ""), getResourceList("", ""))),
+				validPod("no-request-cpu-2", 1, getResourceRequirements(getResourceList("", ""), getResourceList("", ""))),
+			},
+			resourceName:   api.ResourceCPU,
+			expectedResult: "",
+		},
+		{
+			pods: []*api.Pod{
+				validPod("request-zero-cpu-1", 1, getResourceRequirements(getResourceList("0", ""), getResourceList("", ""))),
+				validPod("request-zero-cpu-1", 1, getResourceRequirements(getResourceList("0", ""), getResourceList("", ""))),
+			},
+			resourceName:   api.ResourceCPU,
+			expectedResult: "",
+		},
+		{
+			pods: []*api.Pod{
+				validPod("request-memory-1", 1, getResourceRequirements(getResourceList("", "500Mi"), getResourceList("", ""))),
+				validPod("request-memory-2", 1, getResourceRequirements(getResourceList("", "1Gi"), getResourceList("", ""))),
+			},
+			resourceName:   api.ResourceMemory,
+			expectedResult: "1524Mi",
+		},
+	}
+	for _, item := range testCases {
+		actual := PodsRequests(item.pods, item.resourceName)
+		if item.expectedResult != "" && (item.expectedResult != actual.String()) {
+			t.Errorf("Expected %s, Actual %s, pod %s for resource %s", item.expectedResult, actual.String(), item.pods[0].Name, item.resourceName)
+		}
 	}
 }

plugin/pkg/admission/resourcequota/admission.go

@@ -190,52 +190,69 @@ func IncrementUsage(a admission.Attributes, status *api.ResourceQuotaStatus, cli
 			}
 		}
 	}
-	// handle memory/cpu constraints, and any diff of usage based on memory/cpu on updates
-	if a.GetResource() == "pods" && (set[api.ResourceMemory] || set[api.ResourceCPU]) {
-		pod := obj.(*api.Pod)
-		deltaCPU := resourcequotacontroller.PodCPU(pod)
-		deltaMemory := resourcequotacontroller.PodMemory(pod)
-		// if this is an update, we need to find the delta cpu/memory usage from previous state
-		if a.GetOperation() == admission.Update {
-			oldPod, err := client.Pods(a.GetNamespace()).Get(pod.Name)
-			if err != nil {
-				return false, err
-			}
-			oldCPU := resourcequotacontroller.PodCPU(oldPod)
-			oldMemory := resourcequotacontroller.PodMemory(oldPod)
-			deltaCPU = resource.NewMilliQuantity(deltaCPU.MilliValue()-oldCPU.MilliValue(), resource.DecimalSI)
-			deltaMemory = resource.NewQuantity(deltaMemory.Value()-oldMemory.Value(), resource.DecimalSI)
-		}
 
-		hardMem, hardMemFound := status.Hard[api.ResourceMemory]
+	if a.GetResource() == "pods" {
-		if hardMemFound {
+		for _, resourceName := range []api.ResourceName{api.ResourceMemory, api.ResourceCPU} {
-			if set[api.ResourceMemory] && resourcequotacontroller.IsPodMemoryUnbounded(pod) {
+
-				return false, fmt.Errorf("Limited to %s memory, but pod has no specified memory limit", hardMem.String())
+			// ignore tracking the resource if its not in the quota document
+			if !set[resourceName] {
+				continue
 			}
-			used, usedFound := status.Used[api.ResourceMemory]
+
+			hard, hardFound := status.Hard[resourceName]
+			if !hardFound {
+				continue
+			}
+
+			// if we do not yet know how much of the current resource is used, we cannot accept any request
+			used, usedFound := status.Used[resourceName]
 			if !usedFound {
-				return false, fmt.Errorf("Quota usage stats are not yet known, unable to admit resource until an accurate count is completed.")
+				return false, fmt.Errorf("Unable to admit pod until quota usage stats are calculated.")
 			}
-			if used.Value()+deltaMemory.Value() > hardMem.Value() {
+
-				return false, fmt.Errorf("Limited to %s memory", hardMem.String())
+			// the amount of resource being requested, or an error if it does not make a request that is tracked
+			pod := obj.(*api.Pod)
+			delta, err := resourcequotacontroller.PodRequests(pod, resourceName)
+
+			if err != nil {
+				return false, fmt.Errorf("Must make a non-zero request for %s since it is tracked by quota.", resourceName)
+			}
+
+			// if this operation is an update, we need to find the delta usage from the previous state
+			if a.GetOperation() == admission.Update {
+				oldPod, err := client.Pods(a.GetNamespace()).Get(pod.Name)
+				if err != nil {
+					return false, err
+				}
+
+				// if the previous version of the resource made a resource request, we need to subtract the old request
+				// from the current to get the actual resource request delta.  if the previous version of the pod
+				// made no request on the resource, then we get an err value.  we ignore the err value, and delta
+				// will just be equal to the total resource request on the pod since there is nothing to subtract.
+				oldRequest, err := resourcequotacontroller.PodRequests(oldPod, resourceName)
+				if err == nil {
+					err = delta.Sub(*oldRequest)
+					if err != nil {
+						return false, err
+					}
+				}
+			}
+
+			newUsage := used.Copy()
+			newUsage.Add(*delta)
+
+			// make the most precise comparison possible
+			newUsageValue := newUsage.Value()
+			hardUsageValue := hard.Value()
+			if newUsageValue <= resource.MaxMilliValue && hardUsageValue <= resource.MaxMilliValue {
+				newUsageValue = newUsage.MilliValue()
+				hardUsageValue = hard.MilliValue()
+			}
+
+			if newUsageValue > hardUsageValue {
+				return false, fmt.Errorf("Unable to admit pod without exceeding quota for resource %s.  Limited to %s but require %s to succeed.", resourceName, hard.String(), newUsage.String())
 			} else {
-				status.Used[api.ResourceMemory] = *resource.NewQuantity(used.Value()+deltaMemory.Value(), resource.DecimalSI)
+				status.Used[resourceName] = *newUsage
-				dirty = true
-			}
-		}
-		hardCPU, hardCPUFound := status.Hard[api.ResourceCPU]
-		if hardCPUFound {
-			if set[api.ResourceCPU] && resourcequotacontroller.IsPodCPUUnbounded(pod) {
-				return false, fmt.Errorf("Limited to %s CPU, but pod has no specified cpu limit", hardCPU.String())
-			}
-			used, usedFound := status.Used[api.ResourceCPU]
-			if !usedFound {
-				return false, fmt.Errorf("Quota usage stats are not yet known, unable to admit resource until an accurate count is completed.")
-			}
-			if used.MilliValue()+deltaCPU.MilliValue() > hardCPU.MilliValue() {
-				return false, fmt.Errorf("Limited to %s CPU", hardCPU.String())
-			} else {
-				status.Used[api.ResourceCPU] = *resource.NewMilliQuantity(used.MilliValue()+deltaCPU.MilliValue(), resource.DecimalSI)
 				dirty = true
 			}
 		}

plugin/pkg/admission/resourcequota/admission_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package resourcequota
 
 import (
+	"strconv"
 	"testing"
 
 	"k8s.io/kubernetes/pkg/admission"
@@ -24,21 +25,42 @@ import (
 	"k8s.io/kubernetes/pkg/api/resource"
 	"k8s.io/kubernetes/pkg/client/unversioned/cache"
 	"k8s.io/kubernetes/pkg/client/unversioned/testclient"
+	"k8s.io/kubernetes/pkg/controller/resourcequota"
 )
 
-func getResourceRequirements(cpu, memory string) api.ResourceRequirements {
+func getResourceList(cpu, memory string) api.ResourceList {
-	res := api.ResourceRequirements{}
+	res := api.ResourceList{}
-	res.Limits = 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 getResourceRequirements(requests, limits api.ResourceList) api.ResourceRequirements {
+	res := api.ResourceRequirements{}
+	res.Requests = requests
+	res.Limits = limits
+	return res
+}
+
+func validPod(name string, numContainers int, resources api.ResourceRequirements) *api.Pod {
+	pod := &api.Pod{
+		ObjectMeta: api.ObjectMeta{Name: name, Namespace: "test"},
+		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 TestAdmissionIgnoresDelete(t *testing.T) {
 	namespace := "default"
 	handler := createResourceQuota(&testclient.Fake{}, nil)
@@ -64,38 +86,118 @@ func TestAdmissionIgnoresSubresources(t *testing.T) {
 
 	indexer.Add(quota)
 
-	newPod := &api.Pod{
+	newPod := validPod("123", 1, getResourceRequirements(getResourceList("100m", "2Gi"), getResourceList("", "")))
-		ObjectMeta: api.ObjectMeta{Name: "123", Namespace: quota.Namespace},
+	err := handler.Admit(admission.NewAttributesRecord(newPod, "Pod", newPod.Namespace, newPod.Name, "pods", "", admission.Create, nil))
-		Spec: api.PodSpec{
-			Volumes:    []api.Volume{{Name: "vol"}},
-			Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "2Gi")}},
-		}}
-
-	err := handler.Admit(admission.NewAttributesRecord(newPod, "Pod", newPod.Namespace, "123", "pods", "", admission.Create, nil))
 	if err == nil {
 		t.Errorf("Expected an error because the pod exceeded allowed quota")
 	}
 
-	err = handler.Admit(admission.NewAttributesRecord(newPod, "Pod", newPod.Namespace, "123", "pods", "subresource", admission.Create, nil))
+	err = handler.Admit(admission.NewAttributesRecord(newPod, "Pod", newPod.Namespace, newPod.Name, "pods", "subresource", admission.Create, nil))
 	if err != nil {
 		t.Errorf("Did not expect an error because the action went to a subresource: %v", err)
 	}
 
 }
 
-func TestIncrementUsagePods(t *testing.T) {
+func TestIncrementUsagePodResources(t *testing.T) {
-	namespace := "default"
+	type testCase struct {
-	client := testclient.NewSimpleFake(&api.PodList{
+		testName      string
-		Items: []api.Pod{
+		existing      *api.Pod
-			{
+		input         *api.Pod
-				ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
+		resourceName  api.ResourceName
-				Spec: api.PodSpec{
+		hard          resource.Quantity
-					Volumes:    []api.Volume{{Name: "vol"}},
+		expectedUsage resource.Quantity
-					Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}},
+		expectedError bool
-				},
+	}
-			},
+	testCases := []testCase{
+		{
+			testName:      "memory-allowed",
+			existing:      validPod("a", 1, getResourceRequirements(getResourceList("", "100Mi"), getResourceList("", ""))),
+			input:         validPod("b", 1, getResourceRequirements(getResourceList("", "100Mi"), getResourceList("", ""))),
+			resourceName:  api.ResourceMemory,
+			hard:          resource.MustParse("500Mi"),
+			expectedUsage: resource.MustParse("200Mi"),
+			expectedError: false,
 		},
-	})
+		{
+			testName:      "memory-not-allowed",
+			existing:      validPod("a", 1, getResourceRequirements(getResourceList("", "100Mi"), getResourceList("", ""))),
+			input:         validPod("b", 1, getResourceRequirements(getResourceList("", "450Mi"), getResourceList("", ""))),
+			resourceName:  api.ResourceMemory,
+			hard:          resource.MustParse("500Mi"),
+			expectedError: true,
+		},
+		{
+			testName:      "memory-no-request",
+			existing:      validPod("a", 1, getResourceRequirements(getResourceList("", "100Mi"), getResourceList("", ""))),
+			input:         validPod("b", 1, getResourceRequirements(getResourceList("", ""), getResourceList("", ""))),
+			resourceName:  api.ResourceMemory,
+			hard:          resource.MustParse("500Mi"),
+			expectedError: true,
+		},
+		{
+			testName:      "cpu-allowed",
+			existing:      validPod("a", 1, getResourceRequirements(getResourceList("1", ""), getResourceList("", ""))),
+			input:         validPod("b", 1, getResourceRequirements(getResourceList("1", ""), getResourceList("", ""))),
+			resourceName:  api.ResourceCPU,
+			hard:          resource.MustParse("2"),
+			expectedUsage: resource.MustParse("2"),
+			expectedError: false,
+		},
+		{
+			testName:      "cpu-not-allowed",
+			existing:      validPod("a", 1, getResourceRequirements(getResourceList("1", ""), getResourceList("", ""))),
+			input:         validPod("b", 1, getResourceRequirements(getResourceList("600m", ""), getResourceList("", ""))),
+			resourceName:  api.ResourceCPU,
+			hard:          resource.MustParse("1500m"),
+			expectedError: true,
+		},
+		{
+			testName:      "cpu-no-request",
+			existing:      validPod("a", 1, getResourceRequirements(getResourceList("1", ""), getResourceList("", ""))),
+			input:         validPod("b", 1, getResourceRequirements(getResourceList("", ""), getResourceList("", ""))),
+			resourceName:  api.ResourceCPU,
+			hard:          resource.MustParse("1500m"),
+			expectedError: true,
+		},
+	}
+	for _, item := range testCases {
+		podList := &api.PodList{Items: []api.Pod{*item.existing}}
+		client := testclient.NewSimpleFake(podList)
+		status := &api.ResourceQuotaStatus{
+			Hard: api.ResourceList{},
+			Used: api.ResourceList{},
+		}
+		used, err := resourcequotacontroller.PodRequests(item.existing, item.resourceName)
+		if err != nil {
+			t.Errorf("Test %s, unexpected error %v", item.testName, err)
+		}
+		status.Hard[item.resourceName] = item.hard
+		status.Used[item.resourceName] = *used
+
+		dirty, err := IncrementUsage(admission.NewAttributesRecord(item.input, "Pod", item.input.Namespace, item.input.Name, "pods", "", admission.Create, nil), status, client)
+		if err == nil && item.expectedError {
+			t.Errorf("Test %s, expected error", item.testName)
+		}
+		if err != nil && !item.expectedError {
+			t.Errorf("Test %s, unexpected error", err)
+		}
+		if !item.expectedError {
+			if !dirty {
+				t.Errorf("Test %s, expected the quota to be dirty", item.testName)
+			}
+			quantity := status.Used[item.resourceName]
+			if quantity.String() != item.expectedUsage.String() {
+				t.Errorf("Test %s, expected usage %s, actual usage %s", item.testName, item.expectedUsage.String(), quantity.String())
+			}
+		}
+	}
+}
+
+func TestIncrementUsagePods(t *testing.T) {
+	pod := validPod("123", 1, getResourceRequirements(getResourceList("100m", "1Gi"), getResourceList("", "")))
+	podList := &api.PodList{Items: []api.Pod{*pod}}
+	client := testclient.NewSimpleFake(podList)
 	status := &api.ResourceQuotaStatus{
 		Hard: api.ResourceList{},
 		Used: api.ResourceList{},
@@ -103,7 +205,7 @@ func TestIncrementUsagePods(t *testing.T) {
 	r := api.ResourcePods
 	status.Hard[r] = resource.MustParse("2")
 	status.Used[r] = resource.MustParse("1")
-	dirty, err := IncrementUsage(admission.NewAttributesRecord(&api.Pod{}, "Pod", namespace, "name", "pods", "", admission.Create, nil), status, client)
+	dirty, err := IncrementUsage(admission.NewAttributesRecord(&api.Pod{}, "Pod", pod.Namespace, "new-pod", "pods", "", admission.Create, nil), status, client)
 	if err != nil {
 		t.Errorf("Unexpected error: %v", err)
 	}
@@ -116,233 +218,10 @@ func TestIncrementUsagePods(t *testing.T) {
 	}
 }
 
-func TestIncrementUsageMemory(t *testing.T) {
-	namespace := "default"
-	client := testclient.NewSimpleFake(&api.PodList{
-		Items: []api.Pod{
-			{
-				ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
-				Spec: api.PodSpec{
-					Volumes:    []api.Volume{{Name: "vol"}},
-					Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}},
-				},
-			},
-		},
-	})
-	status := &api.ResourceQuotaStatus{
-		Hard: api.ResourceList{},
-		Used: api.ResourceList{},
-	}
-	r := api.ResourceMemory
-	status.Hard[r] = resource.MustParse("2Gi")
-	status.Used[r] = resource.MustParse("1Gi")
-
-	newPod := &api.Pod{
-		ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
-		Spec: api.PodSpec{
-			Volumes:    []api.Volume{{Name: "vol"}},
-			Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}},
-		}}
-	dirty, err := IncrementUsage(admission.NewAttributesRecord(newPod, "Pod", namespace, "name", "pods", "", admission.Create, nil), status, client)
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
-	if !dirty {
-		t.Errorf("Expected the status to get incremented, therefore should have been dirty")
-	}
-	expectedVal := resource.MustParse("2Gi")
-	quantity := status.Used[r]
-	if quantity.Value() != expectedVal.Value() {
-		t.Errorf("Expected %v was %v", expectedVal.Value(), quantity.Value())
-	}
-}
-
-func TestExceedUsageMemory(t *testing.T) {
-	namespace := "default"
-	client := testclient.NewSimpleFake(&api.PodList{
-		Items: []api.Pod{
-			{
-				ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
-				Spec: api.PodSpec{
-					Volumes:    []api.Volume{{Name: "vol"}},
-					Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}},
-				},
-			},
-		},
-	})
-	status := &api.ResourceQuotaStatus{
-		Hard: api.ResourceList{},
-		Used: api.ResourceList{},
-	}
-	r := api.ResourceMemory
-	status.Hard[r] = resource.MustParse("2Gi")
-	status.Used[r] = resource.MustParse("1Gi")
-
-	newPod := &api.Pod{
-		ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
-		Spec: api.PodSpec{
-			Volumes:    []api.Volume{{Name: "vol"}},
-			Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "3Gi")}},
-		}}
-	_, err := IncrementUsage(admission.NewAttributesRecord(newPod, "Pod", namespace, "name", "pods", "", admission.Create, nil), status, client)
-	if err == nil {
-		t.Errorf("Expected memory usage exceeded error")
-	}
-}
-
-func TestIncrementUsageCPU(t *testing.T) {
-	namespace := "default"
-	client := testclient.NewSimpleFake(&api.PodList{
-		Items: []api.Pod{
-			{
-				ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
-				Spec: api.PodSpec{
-					Volumes:    []api.Volume{{Name: "vol"}},
-					Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}},
-				},
-			},
-		},
-	})
-	status := &api.ResourceQuotaStatus{
-		Hard: api.ResourceList{},
-		Used: api.ResourceList{},
-	}
-	r := api.ResourceCPU
-	status.Hard[r] = resource.MustParse("200m")
-	status.Used[r] = resource.MustParse("100m")
-
-	newPod := &api.Pod{
-		ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
-		Spec: api.PodSpec{
-			Volumes:    []api.Volume{{Name: "vol"}},
-			Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}},
-		}}
-	dirty, err := IncrementUsage(admission.NewAttributesRecord(newPod, "Pod", namespace, "name", "pods", "", admission.Create, nil), status, client)
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
-	if !dirty {
-		t.Errorf("Expected the status to get incremented, therefore should have been dirty")
-	}
-	expectedVal := resource.MustParse("200m")
-	quantity := status.Used[r]
-	if quantity.Value() != expectedVal.Value() {
-		t.Errorf("Expected %v was %v", expectedVal.Value(), quantity.Value())
-	}
-}
-
-func TestUnboundedCPU(t *testing.T) {
-	namespace := "default"
-	client := testclient.NewSimpleFake(&api.PodList{
-		Items: []api.Pod{
-			{
-				ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
-				Spec: api.PodSpec{
-					Volumes:    []api.Volume{{Name: "vol"}},
-					Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}},
-				},
-			},
-		},
-	})
-	status := &api.ResourceQuotaStatus{
-		Hard: api.ResourceList{},
-		Used: api.ResourceList{},
-	}
-	r := api.ResourceCPU
-	status.Hard[r] = resource.MustParse("200m")
-	status.Used[r] = resource.MustParse("100m")
-
-	newPod := &api.Pod{
-		ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
-		Spec: api.PodSpec{
-			Volumes:    []api.Volume{{Name: "vol"}},
-			Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("0m", "1Gi")}},
-		}}
-	_, err := IncrementUsage(admission.NewAttributesRecord(newPod, "Pod", namespace, "name", "pods", "", admission.Create, nil), status, client)
-	if err == nil {
-		t.Errorf("Expected CPU unbounded usage error")
-	}
-}
-
-func TestUnboundedMemory(t *testing.T) {
-	namespace := "default"
-	client := testclient.NewSimpleFake(&api.PodList{
-		Items: []api.Pod{
-			{
-				ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
-				Spec: api.PodSpec{
-					Volumes:    []api.Volume{{Name: "vol"}},
-					Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}},
-				},
-			},
-		},
-	})
-	status := &api.ResourceQuotaStatus{
-		Hard: api.ResourceList{},
-		Used: api.ResourceList{},
-	}
-	r := api.ResourceMemory
-	status.Hard[r] = resource.MustParse("10Gi")
-	status.Used[r] = resource.MustParse("1Gi")
-
-	newPod := &api.Pod{
-		ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
-		Spec: api.PodSpec{
-			Volumes:    []api.Volume{{Name: "vol"}},
-			Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("250m", "0")}},
-		}}
-	_, err := IncrementUsage(admission.NewAttributesRecord(newPod, "Pod", namespace, "name", "pods", "", admission.Create, nil), status, client)
-	if err == nil {
-		t.Errorf("Expected memory unbounded usage error")
-	}
-}
-
-func TestExceedUsageCPU(t *testing.T) {
-	namespace := "default"
-	client := testclient.NewSimpleFake(&api.PodList{
-		Items: []api.Pod{
-			{
-				ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
-				Spec: api.PodSpec{
-					Volumes:    []api.Volume{{Name: "vol"}},
-					Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}},
-				},
-			},
-		},
-	})
-	status := &api.ResourceQuotaStatus{
-		Hard: api.ResourceList{},
-		Used: api.ResourceList{},
-	}
-	r := api.ResourceCPU
-	status.Hard[r] = resource.MustParse("200m")
-	status.Used[r] = resource.MustParse("100m")
-
-	newPod := &api.Pod{
-		ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
-		Spec: api.PodSpec{
-			Volumes:    []api.Volume{{Name: "vol"}},
-			Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("500m", "1Gi")}},
-		}}
-	_, err := IncrementUsage(admission.NewAttributesRecord(newPod, "Pod", namespace, newPod.Name, "pods", "", admission.Create, nil), status, client)
-	if err == nil {
-		t.Errorf("Expected CPU usage exceeded error")
-	}
-}
-
 func TestExceedUsagePods(t *testing.T) {
-	namespace := "default"
+	pod := validPod("123", 1, getResourceRequirements(getResourceList("100m", "1Gi"), getResourceList("", "")))
-	client := testclient.NewSimpleFake(&api.PodList{
+	podList := &api.PodList{Items: []api.Pod{*pod}}
-		Items: []api.Pod{
+	client := testclient.NewSimpleFake(podList)
-			{
-				ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
-				Spec: api.PodSpec{
-					Volumes:    []api.Volume{{Name: "vol"}},
-					Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}},
-				},
-			},
-		},
-	})
 	status := &api.ResourceQuotaStatus{
 		Hard: api.ResourceList{},
 		Used: api.ResourceList{},
@@ -350,7 +229,7 @@ func TestExceedUsagePods(t *testing.T) {
 	r := api.ResourcePods
 	status.Hard[r] = resource.MustParse("1")
 	status.Used[r] = resource.MustParse("1")
-	_, err := IncrementUsage(admission.NewAttributesRecord(&api.Pod{}, "Pod", namespace, "name", "pods", "", admission.Create, nil), status, client)
+	_, err := IncrementUsage(admission.NewAttributesRecord(&api.Pod{}, "Pod", pod.Namespace, "name", "pods", "", admission.Create, nil), status, client)
 	if err == nil {
 		t.Errorf("Expected error because this would exceed your quota")
 	}