Scaling Modifiers: cast to float internally and add tests

apis/keda/v1alpha1/scaledobject_webhook.go

@@ -22,6 +22,7 @@ import (
 	"errors"
 	"fmt"
 	"strconv"
+	"strings"
 
 	"github.com/antonmedv/expr"
 	"github.com/antonmedv/expr/vm"
@@ -334,6 +335,10 @@ func ValidateAndCompileScalingModifiers(so *ScaledObject) (*vm.Program, error) {
 		return nil, fmt.Errorf("error ScalingModifiers.Formula is mandatory")
 	}
 
+	// cast return value of formula to float if necessary to avoid wrong value return
+	// type (ternary operator doesnt return float)
+	so.Spec.Advanced.ScalingModifiers.Formula = castToFloatIfNecessary(sm.Formula)
+
 	// validate formula if not empty
 	compiledFormula, err := validateScalingModifiersFormula(so)
 	if err != nil {
@@ -409,3 +414,13 @@ func validateScalingModifiersTarget(so *ScaledObject) error {
 
 	return nil
 }
+
+// castToFloatIfNecessary takes input formula and casts its return value to float
+// if necessary to avoid wrong return value type like ternary operator has and/or
+// to relief user of having to add it to the formula themselves.
+func castToFloatIfNecessary(formula string) string {
+	if strings.HasPrefix(formula, "float(") {
+		return formula
+	}
+	return "float(" + formula + ")"
+}

apis/keda/v1alpha1/scaledobject_webhook_test.go

@@ -437,6 +437,9 @@ var _ = It("shouldn't create so when stabilizationWindowSeconds exceeds 3600", f
 		Should(HaveOccurred())
 })
 
+// ============================ SCALING MODIFIERS ============================ \\
+// =========================================================================== \\
+
 var _ = It("should validate the so creation with ScalingModifiers.Formula", func() {
 	namespaceName := "scaling-modifiers-formula-good"
 	namespace := createNamespace(namespaceName)
@@ -590,13 +593,244 @@ var _ = It("should validate the so creation with ScalingModifiers when formula t
 	}).ShouldNot(HaveOccurred())
 })
 
+var _ = It("should validate the so creation with ScalingModifiers when formula casts to float already", func() {
+	namespaceName := "scaling-modifiers-cast-to-float-good"
+	namespace := createNamespace(namespaceName)
+	workload := createDeployment(namespaceName, true, true)
+
+	sm := ScalingModifiers{Target: "2", Formula: "float(workload_trig + 1)"}
+
+	triggers := []ScaleTriggers{
+		{
+			Type: "kubernetes-workload",
+			Name: "workload_trig",
+			Metadata: map[string]string{
+				"podSelector": "pod=workload-test",
+				"value":       "1",
+			},
+		},
+	}
+
+	so := createScaledObjectScalingModifiers(namespaceName, sm, triggers)
+
+	err := k8sClient.Create(context.Background(), namespace)
+	Expect(err).ToNot(HaveOccurred())
+	err = k8sClient.Create(context.Background(), workload)
+	Expect(err).ToNot(HaveOccurred())
+	Eventually(func() error {
+		return k8sClient.Create(context.Background(), so)
+	}).ShouldNot(HaveOccurred())
+})
+
+var _ = It("should validate the so creation with ScalingModifiers.Formula - casting float from ternary operator", func() {
+	namespaceName := "scaling-modifiers-formula-casting-float-good"
+	namespace := createNamespace(namespaceName)
+	workload := createDeployment(namespaceName, false, false)
+
+	sm := ScalingModifiers{Target: "2", Formula: "float(workload_trig < 5 ? cron_trig + workload_trig : 5)"}
+
+	triggers := []ScaleTriggers{
+		{
+			Type: "cron",
+			Name: "cron_trig",
+			Metadata: map[string]string{
+				"timezone":        "UTC",
+				"start":           "0 * * * *",
+				"end":             "1 * * * *",
+				"desiredReplicas": "1",
+			},
+		},
+		{
+			Type: "kubernetes-workload",
+			Name: "workload_trig",
+			Metadata: map[string]string{
+				"podSelector": "pod=workload-test",
+				"value":       "1",
+			},
+		},
+	}
+
+	so := createScaledObjectScalingModifiers(namespaceName, sm, triggers)
+
+	err := k8sClient.Create(context.Background(), namespace)
+	Expect(err).ToNot(HaveOccurred())
+	err = k8sClient.Create(context.Background(), workload)
+	Expect(err).ToNot(HaveOccurred())
+	Eventually(func() error {
+		return k8sClient.Create(context.Background(), so)
+	}).ShouldNot(HaveOccurred())
+})
+
+// this test checks that internally, the casting to float happened successfully
+// to override the return value of ternary operator '?' - because it tries to compile
+// in webhook validator or during hpa setup and wouldnt compile without float return
+// value
+var _ = It("should validate the so creation with ScalingModifiers.Formula - ternary operator without casting float", func() {
+	namespaceName := "scaling-modifiers-formula-ternary-no-casting-float-good"
+	namespace := createNamespace(namespaceName)
+	workload := createDeployment(namespaceName, false, false)
+
+	sm := ScalingModifiers{Target: "2", Formula: "workload_trig < 5 ? cron_trig + workload_trig : 5"}
+
+	triggers := []ScaleTriggers{
+		{
+			Type: "cron",
+			Name: "cron_trig",
+			Metadata: map[string]string{
+				"timezone":        "UTC",
+				"start":           "0 * * * *",
+				"end":             "1 * * * *",
+				"desiredReplicas": "1",
+			},
+		},
+		{
+			Type: "kubernetes-workload",
+			Name: "workload_trig",
+			Metadata: map[string]string{
+				"podSelector": "pod=workload-test",
+				"value":       "1",
+			},
+		},
+	}
+
+	so := createScaledObjectScalingModifiers(namespaceName, sm, triggers)
+
+	err := k8sClient.Create(context.Background(), namespace)
+	Expect(err).ToNot(HaveOccurred())
+	err = k8sClient.Create(context.Background(), workload)
+	Expect(err).ToNot(HaveOccurred())
+	Eventually(func() error {
+		return k8sClient.Create(context.Background(), so)
+	}).ShouldNot(HaveOccurred())
+})
+
+var _ = It("should validate the so creation with ScalingModifiers.Formula - count operator", func() {
+	namespaceName := "scaling-modifiers-formula-count-function-good"
+	namespace := createNamespace(namespaceName)
+	workload := createDeployment(namespaceName, false, false)
+
+	sm := ScalingModifiers{Target: "2", Formula: "count([trig_one,trig_two],{#>1}) > 1 ? 5 : 0"}
+
+	triggers := []ScaleTriggers{
+		{
+			Type: "cron",
+			Name: "trig_one",
+			Metadata: map[string]string{
+				"timezone":        "UTC",
+				"start":           "0 * * * *",
+				"end":             "1 * * * *",
+				"desiredReplicas": "1",
+			},
+		},
+		{
+			Type: "kubernetes-workload",
+			Name: "trig_two",
+			Metadata: map[string]string{
+				"podSelector": "pod=workload-test",
+				"value":       "1",
+			},
+		},
+	}
+
+	so := createScaledObjectScalingModifiers(namespaceName, sm, triggers)
+
+	err := k8sClient.Create(context.Background(), namespace)
+	Expect(err).ToNot(HaveOccurred())
+	err = k8sClient.Create(context.Background(), workload)
+	Expect(err).ToNot(HaveOccurred())
+	Eventually(func() error {
+		return k8sClient.Create(context.Background(), so)
+	}).ShouldNot(HaveOccurred())
+})
+
+var _ = It("should validate the so creation with ScalingModifiers.Formula - complex ternary", func() {
+	namespaceName := "scaling-modifiers-formula-complex-ternary-good"
+	namespace := createNamespace(namespaceName)
+	workload := createDeployment(namespaceName, false, false)
+
+	sm := ScalingModifiers{Target: "2", Formula: "float(trig_one < 2 ? trig_one+trig_two >= 2 ? 5 : 10 : 0)"}
+
+	triggers := []ScaleTriggers{
+		{
+			Type: "cron",
+			Name: "trig_one",
+			Metadata: map[string]string{
+				"timezone":        "UTC",
+				"start":           "0 * * * *",
+				"end":             "1 * * * *",
+				"desiredReplicas": "1",
+			},
+		},
+		{
+			Type: "kubernetes-workload",
+			Name: "trig_two",
+			Metadata: map[string]string{
+				"podSelector": "pod=workload-test",
+				"value":       "1",
+			},
+		},
+	}
+
+	so := createScaledObjectScalingModifiers(namespaceName, sm, triggers)
+
+	err := k8sClient.Create(context.Background(), namespace)
+	Expect(err).ToNot(HaveOccurred())
+	err = k8sClient.Create(context.Background(), workload)
+	Expect(err).ToNot(HaveOccurred())
+	Eventually(func() error {
+		return k8sClient.Create(context.Background(), so)
+	}).ShouldNot(HaveOccurred())
+})
+
+var _ = It("should validate the so creation with ScalingModifiers.Formula - double float cast", func() {
+	namespaceName := "scaling-modifiers-formula-double-float-cast-good"
+	namespace := createNamespace(namespaceName)
+	workload := createDeployment(namespaceName, false, false)
+
+	sm := ScalingModifiers{Target: "2", Formula: "float(float(trig_two < 5 ? trig_one + trig_two : 5))"}
+	triggers := []ScaleTriggers{
+		{
+			Type: "cron",
+			Name: "trig_one",
+			Metadata: map[string]string{
+				"timezone":        "UTC",
+				"start":           "0 * * * *",
+				"end":             "1 * * * *",
+				"desiredReplicas": "1",
+			},
+		},
+		{
+			Type: "kubernetes-workload",
+			Name: "trig_two",
+			Metadata: map[string]string{
+				"podSelector": "pod=workload-test",
+				"value":       "1",
+			},
+		},
+	}
+
+	so := createScaledObjectScalingModifiers(namespaceName, sm, triggers)
+
+	err := k8sClient.Create(context.Background(), namespace)
+	Expect(err).ToNot(HaveOccurred())
+	err = k8sClient.Create(context.Background(), workload)
+	Expect(err).ToNot(HaveOccurred())
+	Eventually(func() error {
+		return k8sClient.Create(context.Background(), so)
+	}).ShouldNot(HaveOccurred())
+})
+
 var _ = AfterSuite(func() {
 	cancel()
 	By("tearing down the test environment")
 	err := testEnv.Stop()
 	Expect(err).NotTo(HaveOccurred())
 })
 
+// -------------------------------------------------------------------------- //
+// ----------------------------- HELP FUNCTIONS ----------------------------- //
+// -------------------------------------------------------------------------- //
+
 func createNamespace(name string) *v1.Namespace {
 	return &v1.Namespace{
 		ObjectMeta: metav1.ObjectMeta{Name: name},

tests/internals/scaling_modifiers/scaling_modifiers_test.go

@@ -166,6 +166,48 @@ spec:
       podSelector: pod=workload-test
 `
 
+	soComplexFormula = `
+apiVersion: keda.sh/v1alpha1
+kind: ScaledObject
+metadata:
+  name: {{.ScaledObject}}
+  namespace: {{.TestNamespace}}
+  labels:
+    app: {{.DeploymentName}}
+spec:
+  scaleTargetRef:
+    name: {{.DeploymentName}}
+  advanced:
+    horizontalPodAutoscalerConfig:
+      behavior:
+        scaleDown:
+          stabilizationWindowSeconds: 5
+    scalingModifiers:
+      formula: "count([kw_trig,metrics_api],{#>1}) > 1 ? 5 : 0"
+      target: '2'
+      activationTarget: '2'
+  pollingInterval: 5
+  cooldownPeriod: 5
+  minReplicaCount: 0
+  maxReplicaCount: 10
+  fallback:
+    replicas: 5
+    failureThreshold: 3
+  triggers:
+  - type: metrics-api
+    name: metrics_api
+    metadata:
+      url: "{{.MetricsServerEndpoint}}"
+      valueLocation: 'value'
+      method: "query"
+    authenticationRef:
+      name: {{.TriggerAuthName}}
+  - type: kubernetes-workload
+    name: kw_trig
+    metadata:
+      podSelector: pod=workload-test
+`
+
 	workloadDeploymentTemplate = `
 apiVersion: apps/v1
 kind: Deployment
@@ -231,7 +273,7 @@ func TestScalingModifiers(t *testing.T) {
 
 	testFormula(t, kc, data)
 
-	templates = append(templates, Template{Name: "soFallbackTemplate", Config: soFallbackTemplate})
+	templates = append(templates, Template{Name: "soComplexFormula", Config: soComplexFormula})
 	DeleteKubernetesResources(t, namespace, data, templates)
 }
 
@@ -272,6 +314,24 @@ func testFormula(t *testing.T, kc *kubernetes.Clientset, data templateData) {
 	// 2+2=4; target = 2 -> 4/2 replicas should be 2
 	assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, namespace, 2, 12, 10),
 		"replica count should be %d after 2 minutes", 2)
+
+	data.MetricValue = 0
+	KubectlReplaceWithTemplate(t, data, "updateMetricsTemplate", updateMetricsTemplate)
+
+	// apply new SO
+	KubectlApplyWithTemplate(t, data, "soComplexFormula", soComplexFormula)
+
+	// formula has count() which needs atleast 2 metrics to have value over 1 to scale up
+	// now should be 0
+	assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, namespace, 0, 12, 10),
+		"replica count should be %d after 2 minutes", 0)
+
+	data.MetricValue = 2
+	KubectlReplaceWithTemplate(t, data, "updateMetricsTemplate", updateMetricsTemplate)
+
+	// 5//2 = 3
+	assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, namespace, 3, 12, 10),
+		"replica count should be %d after 2 minutes", 3)
 }
 
 func getTemplateData() (templateData, []Template) {