ComputeClass overprovisioning request floor ignores minimum (#2381) (#2396)

Author: Oscar Ward

pkg/computeclasses/computeclasses.go

@@ -13,7 +13,6 @@ import (
 	internaladminv1 "github.com/acorn-io/runtime/pkg/apis/internal.admin.acorn.io/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -121,11 +120,7 @@ func Validate(cc apiv1.ComputeClass, memory resource.Quantity, memDefault *int64
 	return nil
 }
 
-func CalculateCPU(cc internaladminv1.ProjectComputeClassInstance, memDefault *int64, memory resource.Quantity) (resource.Quantity, error) {
-	if err := ValidateProjectComputeClass(cc, memory, memDefault); err != nil {
-		return resource.Quantity{}, err
-	}
-
+func CalculateCPU(cc internaladminv1.ProjectComputeClassInstance, memory resource.Quantity) (resource.Quantity, error) {
 	// The CPU scaler calculates the CPUs per Gi of memory so get the memory in a ratio of Gi
 	memoryInGi := memory.AsApproximateFloat64() / gi
 	// Since we're putting this in to mili-cpu's, multiply memoryInGi by the scaler and by 1000
@@ -134,18 +129,6 @@ func CalculateCPU(cc internaladminv1.ProjectComputeClassInstance, memDefault *in
 	return *resource.NewMilliQuantity(int64(math.Ceil(value)), resource.DecimalSI), nil
 }
 
-func ValidateProjectComputeClass(cc internaladminv1.ProjectComputeClassInstance, memory resource.Quantity, memDefault *int64) error {
-	return Validate(apiv1.ComputeClass{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: cc.Name,
-		},
-		Memory:           cc.Memory,
-		Description:      cc.Description,
-		Default:          cc.Default,
-		SupportedRegions: cc.SupportedRegions,
-	}, memory, memDefault)
-}
-
 func GetComputeClassNameForWorkload(workload string, container internalv1.Container, computeClasses internalv1.ComputeClassMap) string {
 	var cc string
 	if specific, ok := computeClasses[workload]; ok {

pkg/controller/scheduling/computeclass_test.go

@@ -61,6 +61,10 @@ func TestGenericResourcesComputeClass(t *testing.T) {
 	tester.DefaultTest(t, scheme.Scheme, "testdata/computeclass/generic-resources", Calculate)
 }
 
+func TestRequestScalerValues(t *testing.T) {
+	tester.DefaultTest(t, scheme.Scheme, "testdata/computeclass/request-scaler-values", Calculate)
+}
+
 func TestRequestScaler(t *testing.T) {
 	tester.DefaultTest(t, scheme.Scheme, "testdata/computeclass/request-scaler", Calculate)
 }

pkg/controller/scheduling/scheduling.go

@@ -197,17 +197,6 @@ func ResourceRequirements(req router.Request, app *v1.AppInstance, containerName
 	if computeClass != nil && computeClass.Memory.RequestScaler != 0 {
 		// The following line should hold up without loss of precision up to 4 petabytes
 		memoryRequest.Set(int64(memoryLimit.AsApproximateFloat64() * computeClass.Memory.RequestScaler))
-
-		// Never allocate less than the defined minimum of the compute class
-		if computeClass.Memory.Min != "" && computeClass.Memory.Min != "0" {
-			minValue, err := resource.ParseQuantity(computeClass.Memory.Min)
-			if err != nil {
-				return nil, err
-			}
-			if minValue.Cmp(memoryRequest) == 1 {
-				memoryRequest = minValue
-			}
-		}
 	}
 
 	if memoryLimit.Value() != 0 {
@@ -216,7 +205,7 @@ func ResourceRequirements(req router.Request, app *v1.AppInstance, containerName
 	}
 
 	if computeClass != nil {
-		cpuQuantity, err := computeclasses.CalculateCPU(*computeClass, memDefault, memoryRequest)
+		cpuQuantity, err := computeclasses.CalculateCPU(*computeClass, memoryRequest)
 		if err != nil {
 			return nil, err
 		}

pkg/controller/scheduling/testdata/computeclass/request-scaler-floor/existing.yaml

@@ -9,7 +9,7 @@ memory:
   min: 1Mi # 1Mi
   max: 2Mi # 2Mi
   default: 2Mi # 2Mi
-  requestScaler: .1
+  requestScaler: .125
 affinity:
   nodeAffinity:
     requiredDuringSchedulingIgnoredDuringExecution:

pkg/controller/scheduling/testdata/computeclass/request-scaler-floor/expected.golden

@@ -59,7 +59,7 @@ status:
           memory: 2Mi
         requests:
           cpu: 1m
-          memory: 1Mi
+          memory: 256Ki
     oneimage:
       affinity:
         nodeAffinity:
@@ -75,7 +75,7 @@ status:
           memory: 2Mi
         requests:
           cpu: 1m
-          memory: 1Mi
+          memory: 256Ki
       tolerations:
       - key: taints.acorn.io/workload
         operator: Exists

pkg/controller/scheduling/testdata/computeclass/request-scaler-values/existing.yaml

@@ -0,0 +1,22 @@
+---
+kind: ClusterComputeClassInstance
+apiVersion: internal.admin.acorn.io/v1
+metadata:
+  name: sample-compute-class
+description: Simple description for a simple ComputeClass
+cpuScaler: 0.25
+memory:
+  min: 1Mi # 1Mi
+  max: 2Mi # 2Mi
+  default: 2Mi # 2Mi
+  values: [1Mi, 2Mi]
+  requestScaler: .25
+affinity:
+  nodeAffinity:
+    requiredDuringSchedulingIgnoredDuringExecution:
+      nodeSelectorTerms:
+      - matchExpressions:
+        - key: foo
+          operator: In
+          values:
+          - bar

pkg/controller/scheduling/testdata/computeclass/request-scaler-values/expected.golden

@@ -0,0 +1,88 @@
+`apiVersion: internal.acorn.io/v1
+kind: AppInstance
+metadata:
+  creationTimestamp: null
+  name: app-name
+  namespace: app-namespace
+  uid: 1234567890abcdef
+spec:
+  computeClass:
+    oneimage: sample-compute-class
+  image: test
+status:
+  appImage:
+    buildContext: {}
+    id: test
+    imageData: {}
+    vcs: {}
+  appSpec:
+    containers:
+      oneimage:
+        build:
+          context: .
+          dockerfile: Dockerfile
+        image: image-name
+        metrics: {}
+        ports:
+        - port: 80
+          protocol: http
+          targetPort: 81
+        probes: null
+        sidecars:
+          left:
+            image: foo
+            metrics: {}
+            ports:
+            - port: 90
+              protocol: tcp
+              targetPort: 91
+            probes: null
+  appStatus: {}
+  columns: {}
+  conditions:
+    reason: Success
+    status: "True"
+    success: true
+    type: scheduling
+  defaults:
+    memory:
+      "": 0
+      left: 2097152
+      oneimage: 2097152
+  namespace: app-created-namespace
+  observedGeneration: 1
+  resolvedOfferings: {}
+  scheduling:
+    left:
+      requirements:
+        limits:
+          memory: 2Mi
+        requests:
+          cpu: 1m
+          memory: 512Ki
+    oneimage:
+      affinity:
+        nodeAffinity:
+          requiredDuringSchedulingIgnoredDuringExecution:
+            nodeSelectorTerms:
+            - matchExpressions:
+              - key: foo
+                operator: In
+                values:
+                - bar
+      requirements:
+        limits:
+          memory: 2Mi
+        requests:
+          cpu: 1m
+          memory: 512Ki
+      tolerations:
+      - key: taints.acorn.io/workload
+        operator: Exists
+  staged:
+    appImage:
+      buildContext: {}
+      imageData: {}
+      vcs: {}
+  summary: {}
+`

pkg/controller/scheduling/testdata/computeclass/request-scaler-values/input.yaml

@@ -0,0 +1,39 @@
+kind: AppInstance
+apiVersion: internal.acorn.io/v1
+metadata:
+  name: app-name
+  namespace: app-namespace
+  uid: 1234567890abcdef
+spec:
+  image: test
+  computeClass:
+    oneimage: sample-compute-class
+status:
+  observedGeneration: 1
+  defaults:
+    memory:
+      "": 0
+      left: 2097152 # 2Mi
+      oneimage: 2097152 # 2Mi
+  namespace: app-created-namespace
+  appImage:
+    id: test
+    defaults:
+  appSpec:
+    containers:
+      oneimage:
+        sidecars:
+          left:
+            image: "foo"
+            ports:
+              - port: 90
+                targetPort: 91
+                protocol: tcp
+        ports:
+        - port: 80
+          targetPort: 81
+          protocol: http
+        image: "image-name"
+        build:
+          dockerfile: "Dockerfile"
+          context: "."