From b86dfcf60a13aefd3fc6ae201ddce67f3f18b435 Mon Sep 17 00:00:00 2001
From: Rudi Chiarito <rudi@clarifai.com>
Date: Sun, 10 Jul 2016 16:09:16 -0400
Subject: [PATCH] Fix GPU resource validation

Because of defaulting, requests are always set if limits are. Thus, the check can never succeed. Instead, make sure that the two values are equal.

Also, remove a few other error messages and remove unnecessary Sprintf calls.
---
 pkg/api/validation/validation.go      | 14 +++++++-------
 pkg/api/validation/validation_test.go | 25 +++++++++++++++++++++----
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go
index 3854004d8f0..589c5b97df1 100644
--- a/pkg/api/validation/validation.go
+++ b/pkg/api/validation/validation.go
@@ -1197,11 +1197,11 @@ func validateContainerResourceDivisor(rName string, divisor resource.Quantity, f
 	switch rName {
 	case "limits.cpu", "requests.cpu":
 		if !validContainerResourceDivisorForCPU.Has(divisor.String()) {
-			allErrs = append(allErrs, field.Invalid(fldPath.Child("divisor"), rName, fmt.Sprintf("only divisor's values 1m and 1 are supported with the cpu resource")))
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("divisor"), rName, "only divisor's values 1m and 1 are supported with the cpu resource"))
 		}
 	case "limits.memory", "requests.memory":
 		if !validContainerResourceDivisorForMemory.Has(divisor.String()) {
-			allErrs = append(allErrs, field.Invalid(fldPath.Child("divisor"), rName, fmt.Sprintf("only divisor's values 1, 1k, 1M, 1G, 1T, 1P, 1E, 1Ki, 1Mi, 1Gi, 1Ti, 1Pi, 1Ei are supported with the memory resource")))
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("divisor"), rName, "only divisor's values 1, 1k, 1M, 1G, 1T, 1P, 1E, 1Ki, 1Mi, 1Gi, 1Ti, 1Pi, 1Ei are supported with the memory resource"))
 		}
 	}
 	return allErrs
@@ -2780,11 +2780,11 @@ func ValidateResourceRequirements(requirements *api.ResourceRequirements, fldPat
 		// Check that request <= limit.
 		requestQuantity, exists := requirements.Requests[resourceName]
 		if exists {
-			// For GPUs, require that no request be set.
-			if resourceName == api.ResourceNvidiaGPU {
-				allErrs = append(allErrs, field.Invalid(reqPath, requestQuantity.String(), "cannot be set"))
+			// For GPUs, not only requests can't exceed limits, they also can't be lower, i.e. must be equal.
+			if resourceName == api.ResourceNvidiaGPU && quantity.Cmp(requestQuantity) != 0 {
+				allErrs = append(allErrs, field.Invalid(reqPath, requestQuantity.String(), fmt.Sprintf("must be equal to %s limit", api.ResourceNvidiaGPU)))
 			} else if quantity.Cmp(requestQuantity) < 0 {
-				allErrs = append(allErrs, field.Invalid(fldPath, quantity.String(), "must be greater than or equal to request"))
+				allErrs = append(allErrs, field.Invalid(limPath, quantity.String(), fmt.Sprintf("must be greater than or equal to %s request", resourceName)))
 			}
 		}
 	}
@@ -2957,7 +2957,7 @@ func validateFinalizerName(stringValue string, fldPath *field.Path) field.ErrorL
 
 	if len(strings.Split(stringValue, "/")) == 1 {
 		if !api.IsStandardFinalizerName(stringValue) {
-			return append(allErrs, field.Invalid(fldPath, stringValue, fmt.Sprintf("name is neither a standard finalizer name nor is it fully qualified")))
+			return append(allErrs, field.Invalid(fldPath, stringValue, "name is neither a standard finalizer name nor is it fully qualified"))
 		}
 	}
 
diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go
index 3fe0547367a..aa0ea148296 100644
--- a/pkg/api/validation/validation_test.go
+++ b/pkg/api/validation/validation_test.go
@@ -1551,7 +1551,24 @@ func TestValidateContainers(t *testing.T) {
 			ImagePullPolicy: "IfNotPresent",
 		},
 		{
-			Name:  "resources-test-with-gpu",
+			Name:  "resources-test-with-gpu-with-request",
+			Image: "image",
+			Resources: api.ResourceRequirements{
+				Requests: api.ResourceList{
+					api.ResourceName(api.ResourceCPU):       resource.MustParse("10"),
+					api.ResourceName(api.ResourceMemory):    resource.MustParse("10G"),
+					api.ResourceName(api.ResourceNvidiaGPU): resource.MustParse("1"),
+				},
+				Limits: api.ResourceList{
+					api.ResourceName(api.ResourceCPU):       resource.MustParse("10"),
+					api.ResourceName(api.ResourceMemory):    resource.MustParse("10G"),
+					api.ResourceName(api.ResourceNvidiaGPU): resource.MustParse("1"),
+				},
+			},
+			ImagePullPolicy: "IfNotPresent",
+		},
+		{
+			Name:  "resources-test-with-gpu-without-request",
 			Image: "image",
 			Resources: api.ResourceRequirements{
 				Requests: api.ResourceList{
@@ -1789,15 +1806,15 @@ func TestValidateContainers(t *testing.T) {
 				ImagePullPolicy: "IfNotPresent",
 			},
 		},
-		"Resource can only have GPU limit": {
+		"Resource GPU limit must match request": {
 			{
-				Name:  "resources-request-limit-edge",
+				Name:  "gpu-resource-request-limit",
 				Image: "image",
 				Resources: api.ResourceRequirements{
 					Requests: api.ResourceList{
 						api.ResourceName(api.ResourceCPU):       resource.MustParse("10"),
 						api.ResourceName(api.ResourceMemory):    resource.MustParse("10G"),
-						api.ResourceName(api.ResourceNvidiaGPU): resource.MustParse("1"),
+						api.ResourceName(api.ResourceNvidiaGPU): resource.MustParse("0"),
 					},
 					Limits: api.ResourceList{
 						api.ResourceName(api.ResourceCPU):       resource.MustParse("10"),