Merge pull request #25487 from derekwaynecarr/verify_resources

pkg/quota/evaluator/core/pods.go

@@ -23,12 +23,14 @@ import (
 	"k8s.io/kubernetes/pkg/admission"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/resource"
+	"k8s.io/kubernetes/pkg/api/validation"
 	clientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
 	"k8s.io/kubernetes/pkg/kubelet/qos/util"
 	"k8s.io/kubernetes/pkg/quota"
 	"k8s.io/kubernetes/pkg/quota/generic"
 	"k8s.io/kubernetes/pkg/runtime"
 	"k8s.io/kubernetes/pkg/util/sets"
+	"k8s.io/kubernetes/pkg/util/validation/field"
 )
 
 // NewPodEvaluator returns an evaluator that can evaluate pods
@@ -64,12 +66,26 @@ func NewPodEvaluator(kubeClient clientset.Interface) quota.Evaluator {
 }
 
 // PodConstraintsFunc verifies that all required resources are present on the pod
+// In addition, it validates that the resources are valid (i.e. requests < limits)
 func PodConstraintsFunc(required []api.ResourceName, object runtime.Object) error {
 	pod, ok := object.(*api.Pod)
 	if !ok {
 		return fmt.Errorf("Unexpected input object %v", object)
 	}
 
+	// Pod level resources are often set during admission control
+	// As a consequence, we want to verify that resources are valid prior
+	// to ever charging quota prematurely in case they are not.
+	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, idxPath.Child("resources"))...)
+	}
+	if len(allErrs) > 0 {
+		return allErrs.ToAggregate()
+	}
+
 	// TODO: fix this when we have pod level cgroups
 	// since we do not yet pod level requests/limits, we need to ensure each
 	// container makes an explict request or limit for a quota tracked resource

plugin/pkg/admission/resourcequota/admission_test.go

@@ -314,11 +314,18 @@ func TestAdmitEnforceQuotaConstraints(t *testing.T) {
 		evaluator: evaluator,
 	}
 	indexer.Add(resourceQuota)
+	// verify all values are specified as required on the quota
 	newPod := validPod("not-allowed-pod", 1, getResourceRequirements(getResourceList("100m", "2Gi"), getResourceList("200m", "")))
 	err := handler.Admit(admission.NewAttributesRecord(newPod, api.Kind("Pod").WithVersion("version"), newPod.Namespace, newPod.Name, api.Resource("pods").WithVersion("version"), "", admission.Create, nil))
 	if err == nil {
 		t.Errorf("Expected an error because the pod does not specify a memory limit")
 	}
+	// verify the requests and limits are actually valid (in this case, we fail because the limits < requests)
+	newPod = validPod("not-allowed-pod", 1, getResourceRequirements(getResourceList("200m", "2Gi"), getResourceList("100m", "1Gi")))
+	err = handler.Admit(admission.NewAttributesRecord(newPod, api.Kind("Pod").WithVersion("version"), newPod.Namespace, newPod.Name, api.Resource("pods").WithVersion("version"), "", admission.Create, nil))
+	if err == nil {
+		t.Errorf("Expected an error because the pod does not specify a memory limit")
+	}
 }
 
 // TestAdmitPodInNamespaceWithoutQuota ensures that if a namespace has no quota, that a pod can get in