Fix the messaging for resource limits (flyteorg#251)

Signed-off-by: Prafulla Mahindrakar <[email protected]>

pkg/manager/impl/validation/task_validator.go

@@ -4,16 +4,16 @@ package validation
 import (
 	"context"
 
-	"github.com/flyteorg/flyteadmin/pkg/repositories"
-
 	"github.com/flyteorg/flyteadmin/pkg/common"
 	"github.com/flyteorg/flyteadmin/pkg/errors"
 	"github.com/flyteorg/flyteadmin/pkg/manager/impl/shared"
+	"github.com/flyteorg/flyteadmin/pkg/repositories"
 	runtime "github.com/flyteorg/flyteadmin/pkg/runtime/interfaces"
 	runtimeInterfaces "github.com/flyteorg/flyteadmin/pkg/runtime/interfaces"
 	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
 	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core"
 	"github.com/flyteorg/flytestdlib/logger"
+
 	"google.golang.org/grpc/codes"
 	"k8s.io/apimachinery/pkg/api/resource"
 )
@@ -124,7 +124,9 @@ func addResourceEntryToMap(
 	quantity, err := resource.ParseQuantity(entry.Value)
 	if err != nil {
 		return errors.NewFlyteAdminErrorf(codes.InvalidArgument,
-			"Invalid quantity %s for resource: %v for task [%+v]", entry.Value, entry.Name, identifier)
+			"Parsing of %v request failed for value %v - reason  %v. "+
+				"Please follow K8s conventions for resources "+
+				"https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/", entry.Name, entry.Value, err)
 	}
 	(*resourceEntries)[entry.Name] = quantity
 	return nil
@@ -196,18 +198,23 @@ func validateTaskResources(
 			if ok && limitQuantity.Value() < defaultQuantity.Value() {
 				// Only assert the requested limit is greater than than the requested default when the limit is actually set
 				return errors.NewFlyteAdminErrorf(codes.InvalidArgument,
-					"Type %v for [%+v] cannot set default > limit", resourceName, identifier)
+					"Requested %v default [%v] is greater than the limit [%v]."+
+						" Please fix your configuration", resourceName, defaultQuantity.String(), limitQuantity.String())
 			}
 			platformLimit, platformLimitOk := platformTaskResourceLimits[resourceName]
 			if ok && platformLimitOk && limitQuantity.Value() > platformLimit.Value() {
 				// Also check that the requested limit is less than the platform task limit.
 				return errors.NewFlyteAdminErrorf(codes.InvalidArgument,
-					"Type %v for [%+v] cannot set limit > platform limit", resourceName, identifier)
+					"Requested %v limit [%v] is greater than current limit set in the platform configuration"+
+						" [%v]. Please contact Flyte Admins to change these limits or consult the configuration",
+					resourceName, limitQuantity.String(), platformLimit.String())
 			}
 			if platformLimitOk && defaultQuantity.Value() > platformTaskResourceLimits[resourceName].Value() {
 				// Also check that the requested limit is less than the platform task limit.
 				return errors.NewFlyteAdminErrorf(codes.InvalidArgument,
-					"Type %v for [%+v] cannot set default > platform limit", resourceName, identifier)
+					"Requested %v default [%v] is greater than  current limit set in the platform configuration"+
+						" [%v]. Please contact Flyte Admins to change these limits or consult the configuration",
+					resourceName, defaultQuantity.String(), platformTaskResourceLimits[resourceName].String())
 			}
 		case core.Resources_GPU:
 			limitQuantity, ok := requestedResourceLimits[resourceName]
@@ -219,7 +226,9 @@ func validateTaskResources(
 			platformLimit, platformLimitOk := platformTaskResourceLimits[resourceName]
 			if platformLimitOk && defaultQuantity.Value() > platformLimit.Value() {
 				return errors.NewFlyteAdminErrorf(codes.InvalidArgument,
-					"Type %v for [%+v] cannot set default > platform limit", resourceName, identifier)
+					"Requested %v default [%v] is greater than  current limit set in the platform configuration"+
+						" [%v]. Please contact Flyte Admins to change these limits or consult the configuration",
+					resourceName, defaultQuantity.String(), platformLimit.String())
 			}
 		}
 	}

pkg/manager/impl/validation/task_validator_test.go

@@ -210,7 +210,7 @@ func TestAddResourceEntryToMap(t *testing.T) {
 		Name:  core.Resources_MEMORY,
 		Value: "foo",
 	}, &resourceEntries)
-	assert.Contains(t, err.Error(), "Invalid quantity")
+	assert.Contains(t, err.Error(), "Parsing of MEMORY request failed")
 
 	quantity = resourceEntries[core.Resources_CPU]
 	val = quantity.Value()
@@ -289,6 +289,24 @@ func TestValidateTaskResources(t *testing.T) {
 		requestedTaskResourceDefaults, requestedTaskResourceLimits))
 }
 
+func TestValidateTaskResources_ParsingIssue(t *testing.T) {
+	err := validateTaskResources(&core.Identifier{
+		Name: "name",
+	}, runtimeInterfaces.TaskResourceSet{},
+		[]*core.Resources_ResourceEntry{
+			{
+				Name:  core.Resources_CPU,
+				Value: "200Q",
+			},
+		}, []*core.Resources_ResourceEntry{
+			{
+				Name:  core.Resources_CPU,
+				Value: "200Q",
+			},
+		})
+	assert.EqualError(t, err, "Parsing of CPU request failed for value 200Q - reason  quantities must match the regular expression '^([+-]?[0-9.]+)([eEinumkKMGTP]*[-+]?[0-9]*)$'. Please follow K8s conventions for resources https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/")
+}
+
 func TestValidateTaskResources_LimitLessThanRequested(t *testing.T) {
 	err := validateTaskResources(&core.Identifier{
 		Name: "name",
@@ -304,7 +322,7 @@ func TestValidateTaskResources_LimitLessThanRequested(t *testing.T) {
 				Value: "1Gi",
 			},
 		})
-	assert.EqualError(t, err, "Type CPU for [name:\"name\" ] cannot set default > limit")
+	assert.EqualError(t, err, "Requested CPU default [1536Mi] is greater than the limit [1Gi]. Please fix your configuration")
 }
 
 func TestValidateTaskResources_LimitGreaterThanConfig(t *testing.T) {
@@ -324,7 +342,7 @@ func TestValidateTaskResources_LimitGreaterThanConfig(t *testing.T) {
 				Value: "1.5Gi",
 			},
 		})
-	assert.EqualError(t, err, "Type CPU for [name:\"name\" ] cannot set limit > platform limit")
+	assert.EqualError(t, err, "Requested CPU limit [1536Mi] is greater than current limit set in the platform configuration [1Gi]. Please contact Flyte Admins to change these limits or consult the configuration")
 }
 
 func TestValidateTaskResources_DefaultGreaterThanConfig(t *testing.T) {
@@ -339,7 +357,7 @@ func TestValidateTaskResources_DefaultGreaterThanConfig(t *testing.T) {
 				Value: "1.5Gi",
 			},
 		}, []*core.Resources_ResourceEntry{})
-	assert.EqualError(t, err, "Type CPU for [name:\"name\" ] cannot set default > platform limit")
+	assert.EqualError(t, err, "Requested CPU default [1536Mi] is greater than  current limit set in the platform configuration [1Gi]. Please contact Flyte Admins to change these limits or consult the configuration")
 }
 
 func TestValidateTaskResources_GPULimitNotEqualToRequested(t *testing.T) {
@@ -378,7 +396,7 @@ func TestValidateTaskResources_GPULimitGreaterThanConfig(t *testing.T) {
 				Value: "2",
 			},
 		})
-	assert.EqualError(t, err, "Type GPU for [name:\"name\" ] cannot set default > platform limit")
+	assert.EqualError(t, err, "Requested GPU default [2] is greater than  current limit set in the platform configuration [1]. Please contact Flyte Admins to change these limits or consult the configuration")
 }
 
 func TestValidateTaskResources_GPUDefaultGreaterThanConfig(t *testing.T) {
@@ -393,7 +411,7 @@ func TestValidateTaskResources_GPUDefaultGreaterThanConfig(t *testing.T) {
 				Value: "2",
 			},
 		}, []*core.Resources_ResourceEntry{})
-	assert.EqualError(t, err, "Type GPU for [name:\"name\" ] cannot set default > platform limit")
+	assert.EqualError(t, err, "Requested GPU default [2] is greater than  current limit set in the platform configuration [1]. Please contact Flyte Admins to change these limits or consult the configuration")
 }
 
 func TestIsWholeNumber(t *testing.T) {