fix: Fallback calculation returns correct value (#4263)

* fix: Fallback calculation returns correct value Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * remove unrelated change Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * reduce stabilization window Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * set scaling value after restarting the server Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * set scaling value after restarting the server Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> --------- Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
kedacore · Feb 22, 2023 · e6f019e · e6f019e
1 parent 8e7169d
commit e6f019e
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -75,6 +75,7 @@ Here is an overview of all new **experimental** features:
 
 ### Fixes
 
+- **General**: Fix regression in fallback mechanism ([#4249](https://github.com/kedacore/keda/issues/4249))
 - **General**: Prevent a panic that might occur while refreshing a scaler cache ([#4092](https://github.com/kedacore/keda/issues/4092))
 - **Azure Service Bus Scaler:** Use correct auth flows with pod identity ([#4026](https://github.com/kedacore/keda/issues/4026)|[#4123](https://github.com/kedacore/keda/issues/4123))
 - **Cassandra Scaler**: Checking whether the port information is entered in the ClusterIPAddres is done correctly. ([#4110](https://github.com/kedacore/keda/issues/4110))

diff --git a/pkg/fallback/fallback.go b/pkg/fallback/fallback.go
@@ -100,10 +100,10 @@ func validateFallback(scaledObject *kedav1alpha1.ScaledObject) bool {
 
 func doFallback(scaledObject *kedav1alpha1.ScaledObject, metricSpec v2.MetricSpec, metricName string, suppressedError error) []external_metrics.ExternalMetricValue {
 	replicas := int64(scaledObject.Spec.Fallback.Replicas)
-	normalisationValue, _ := metricSpec.External.Target.AverageValue.AsInt64()
+	normalisationValue := metricSpec.External.Target.AverageValue.AsApproximateFloat64()
 	metric := external_metrics.ExternalMetricValue{
 		MetricName: metricName,
-		Value:      *resource.NewQuantity(normalisationValue*replicas, resource.DecimalSI),
+		Value:      *resource.NewMilliQuantity(int64(normalisationValue*1000)*replicas, resource.DecimalSI),
 		Timestamp:  metav1.Now(),
 	}
 	fallbackMetrics := []external_metrics.ExternalMetricValue{metric}

diff --git a/pkg/fallback/fallback_test.go b/pkg/fallback/fallback_test.go
@@ -63,7 +63,7 @@ var _ = Describe("fallback", func() {
 
 	It("should return the expected metric when fallback is disabled", func() {
 
-		expectedMetricValue := int64(5)
+		expectedMetricValue := float64(5)
 		primeGetMetrics(scaler, expectedMetricValue)
 		so := buildScaledObject(nil, nil)
 		metricSpec := createMetricSpec(3)
@@ -73,12 +73,12 @@ var _ = Describe("fallback", func() {
 		metrics, err = GetMetricsWithFallback(context.Background(), client, metrics, err, metricName, so, metricSpec)
 
 		Expect(err).ToNot(HaveOccurred())
-		value, _ := metrics[0].Value.AsInt64()
+		value := metrics[0].Value.AsApproximateFloat64()
 		Expect(value).Should(Equal(expectedMetricValue))
 	})
 
 	It("should reset the health status when scaler metrics are available", func() {
-		expectedMetricValue := int64(6)
+		expectedMetricValue := float64(6)
 		startingNumberOfFailures := int32(5)
 		primeGetMetrics(scaler, expectedMetricValue)
 
@@ -104,7 +104,7 @@ var _ = Describe("fallback", func() {
 		metrics, err = GetMetricsWithFallback(context.Background(), client, metrics, err, metricName, so, metricSpec)
 
 		Expect(err).ToNot(HaveOccurred())
-		value, _ := metrics[0].Value.AsInt64()
+		value := metrics[0].Value.AsApproximateFloat64()
 		Expect(value).Should(Equal(expectedMetricValue))
 		Expect(so.Status.Health[metricName]).To(haveFailureAndStatus(0, kedav1alpha1.HealthStatusHappy))
 	})
@@ -156,7 +156,7 @@ var _ = Describe("fallback", func() {
 	It("should return a normalised metric when number of failures are beyond threshold", func() {
 		scaler.EXPECT().GetMetricsAndActivity(gomock.Any(), gomock.Eq(metricName)).Return(nil, false, errors.New("Some error"))
 		startingNumberOfFailures := int32(3)
-		expectedMetricValue := int64(100)
+		expectedMetricValue := float64(100)
 
 		so := buildScaledObject(
 			&kedav1alpha1.Fallback{
@@ -179,7 +179,7 @@ var _ = Describe("fallback", func() {
 		metrics, err = GetMetricsWithFallback(context.Background(), client, metrics, err, metricName, so, metricSpec)
 
 		Expect(err).ToNot(HaveOccurred())
-		value, _ := metrics[0].Value.AsInt64()
+		value := metrics[0].Value.AsApproximateFloat64()
 		Expect(value).Should(Equal(expectedMetricValue))
 		Expect(so.Status.Health[metricName]).To(haveFailureAndStatus(4, kedav1alpha1.HealthStatusFailing))
 	})
@@ -209,7 +209,7 @@ var _ = Describe("fallback", func() {
 	It("should ignore error if we fail to update kubernetes status", func() {
 		scaler.EXPECT().GetMetricsAndActivity(gomock.Any(), gomock.Eq(metricName)).Return(nil, false, errors.New("Some error"))
 		startingNumberOfFailures := int32(3)
-		expectedMetricValue := int64(100)
+		expectedMetricValue := float64(100)
 
 		so := buildScaledObject(
 			&kedav1alpha1.Fallback{
@@ -235,7 +235,7 @@ var _ = Describe("fallback", func() {
 		metrics, err = GetMetricsWithFallback(context.Background(), client, metrics, err, metricName, so, metricSpec)
 
 		Expect(err).ToNot(HaveOccurred())
-		value, _ := metrics[0].Value.AsInt64()
+		value := metrics[0].Value.AsApproximateFloat64()
 		Expect(value).Should(Equal(expectedMetricValue))
 		Expect(so.Status.Health[metricName]).To(haveFailureAndStatus(4, kedav1alpha1.HealthStatusFailing))
 	})
@@ -411,10 +411,10 @@ func buildScaledObject(fallbackConfig *kedav1alpha1.Fallback, status *kedav1alph
 	return scaledObject
 }
 
-func primeGetMetrics(scaler *mock_scalers.MockScaler, value int64) {
+func primeGetMetrics(scaler *mock_scalers.MockScaler, value float64) {
 	expectedMetric := external_metrics.ExternalMetricValue{
 		MetricName: metricName,
-		Value:      *resource.NewQuantity(value, resource.DecimalSI),
+		Value:      *resource.NewQuantity(int64(value), resource.DecimalSI),
 		Timestamp:  metav1.Now(),
 	}
 

diff --git a/tests/internals/fallback/fallback_test.go b/tests/internals/fallback/fallback_test.go
@@ -175,7 +175,13 @@ spec:
   fallback:
     failureThreshold: 3
     replicas: {{.DefaultFallback}}
+  advanced:
+    horizontalPodAutoscalerConfig:
+      behavior:
+        scaleDown:
+          stabilizationWindowSeconds: 1
   cooldownPeriod: 1
+  pollingInterval: 5
   triggers:
   - type: metrics-api
     metadata:
@@ -194,6 +200,7 @@ metadata:
   name: update-ms-value
   namespace: {{.Namespace}}
 spec:
+  ttlSecondsAfterFinished: 30
   template:
     spec:
       containers:
@@ -252,12 +259,18 @@ func testFallback(t *testing.T, kc *kubernetes.Clientset, data templateData) {
 	KubectlApplyWithTemplate(t, data, "fallbackMSDeploymentTemplate", fallbackMSDeploymentTemplate)
 	assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, namespace, defaultFallback, 60, 3),
 		"replica count should be %d after 3 minutes", defaultFallback)
+	// We need to ensure that the fallback value is stable to cover this regression
+	// https://github.com/kedacore/keda/issues/4249
+	AssertReplicaCountNotChangeDuringTimePeriod(t, kc, deploymentName, namespace, defaultFallback, 180)
 }
 
 // restore MS to scale back from fallback replicas
 func testRestoreAfterFallback(t *testing.T, kc *kubernetes.Clientset, data templateData) {
 	t.Log("--- testing after fallback ---")
 	KubectlApplyWithTemplate(t, data, "metricsServerDeploymentTemplate", metricsServerDeploymentTemplate)
+	data.MetricValue = 50
+	KubectlApplyWithTemplate(t, data, "updateMetricsTemplate", updateMetricsTemplate)
+
 	assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, namespace, maxReplicas, 60, 3),
 		"replica count should be %d after 3 minutes", maxReplicas)
 }