Refactored fallback logic to only patch status when the fallback is e…

controllers/keda/scaledobject_controller.go

@@ -48,6 +48,7 @@ import (
 	"github.com/kedacore/keda/v2/pkg/common/message"
 	"github.com/kedacore/keda/v2/pkg/eventemitter"
 	"github.com/kedacore/keda/v2/pkg/eventreason"
+	"github.com/kedacore/keda/v2/pkg/fallback"
 	"github.com/kedacore/keda/v2/pkg/metricscollector"
 	"github.com/kedacore/keda/v2/pkg/scaling"
 	kedastatus "github.com/kedacore/keda/v2/pkg/status"
@@ -206,6 +207,10 @@ func (r *ScaledObjectReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		conditions.SetReadyCondition(metav1.ConditionTrue, kedav1alpha1.ScaledObjectConditionReadySuccessReason, msg)
 	}
 
+	if scaledObject.Spec.Fallback == nil || !fallback.HasValidFallback(scaledObject) {
+		conditions.SetFallbackCondition(metav1.ConditionFalse, "NoFallbackFound", "No fallbacks are active on this scaled object")
+	}
+
 	if err := kedastatus.SetStatusConditions(ctx, r.Client, reqLogger, scaledObject, &conditions); err != nil {
 		r.EventEmitter.Emit(scaledObject, req.NamespacedName, corev1.EventTypeWarning, eventingv1alpha1.ScaledObjectFailedType, eventreason.ScaledObjectUpdateFailed, err.Error())
 		return ctrl.Result{}, err

pkg/fallback/fallback.go

@@ -69,7 +69,7 @@ func GetMetricsWithFallback(ctx context.Context, client runtimeclient.Client, me
 	switch {
 	case !isFallbackEnabled(scaledObject, metricSpec):
 		return nil, false, suppressedError
-	case !validateFallback(scaledObject):
+	case !HasValidFallback(scaledObject):
 		log.Info("Failed to validate ScaledObject Spec. Please check that parameters are positive integers", "scaledObject.Namespace", scaledObject.Namespace, "scaledObject.Name", scaledObject.Name)
 		return nil, false, suppressedError
 	case *healthStatus.NumberOfFailures > scaledObject.Spec.Fallback.FailureThreshold:
@@ -79,11 +79,7 @@ func GetMetricsWithFallback(ctx context.Context, client runtimeclient.Client, me
 	}
 }
 
-func fallbackExistsInScaledObject(scaledObject *kedav1alpha1.ScaledObject, metricSpec v2.MetricSpec) bool {
-	if !isFallbackEnabled(scaledObject, metricSpec) || !validateFallback(scaledObject) {
-		return false
-	}
-
+func fallbackExistsInScaledObject(scaledObject *kedav1alpha1.ScaledObject) bool {
 	for _, element := range scaledObject.Status.Health {
 		if element.Status == kedav1alpha1.HealthStatusFailing && *element.NumberOfFailures > scaledObject.Spec.Fallback.FailureThreshold {
 			return true
@@ -93,7 +89,7 @@ func fallbackExistsInScaledObject(scaledObject *kedav1alpha1.ScaledObject, metri
 	return false
 }
 
-func validateFallback(scaledObject *kedav1alpha1.ScaledObject) bool {
+func HasValidFallback(scaledObject *kedav1alpha1.ScaledObject) bool {
 	return scaledObject.Spec.Fallback.FailureThreshold >= 0 &&
 		scaledObject.Spec.Fallback.Replicas >= 0
 }
@@ -115,7 +111,11 @@ func doFallback(scaledObject *kedav1alpha1.ScaledObject, metricSpec v2.MetricSpe
 func updateStatus(ctx context.Context, client runtimeclient.Client, scaledObject *kedav1alpha1.ScaledObject, status *kedav1alpha1.ScaledObjectStatus, metricSpec v2.MetricSpec) {
 	patch := runtimeclient.MergeFrom(scaledObject.DeepCopy())
 
-	if fallbackExistsInScaledObject(scaledObject, metricSpec) {
+	if !isFallbackEnabled(scaledObject, metricSpec) || !HasValidFallback(scaledObject) {
+		return
+	}
+
+	if fallbackExistsInScaledObject(scaledObject) {
 		status.Conditions.SetFallbackCondition(metav1.ConditionTrue, "FallbackExists", "At least one trigger is falling back on this scaled object")
 	} else {
 		status.Conditions.SetFallbackCondition(metav1.ConditionFalse, "NoFallbackFound", "No fallbacks are active on this scaled object")

pkg/fallback/fallback_test.go

@@ -67,7 +67,7 @@ var _ = Describe("fallback", func() {
 		primeGetMetrics(scaler, expectedMetricValue)
 		so := buildScaledObject(nil, nil)
 		metricSpec := createMetricSpec(3)
-		expectStatusPatch(ctrl, client)
+		//expectStatusPatch(ctrl, client)
 
 		metrics, _, err := scaler.GetMetricsAndActivity(context.Background(), metricName)
 		metrics, _, err = GetMetricsWithFallback(context.Background(), client, metrics, err, metricName, so, metricSpec)
@@ -114,7 +114,7 @@ var _ = Describe("fallback", func() {
 
 		so := buildScaledObject(nil, nil)
 		metricSpec := createMetricSpec(3)
-		expectStatusPatch(ctrl, client)
+		//expectStatusPatch(ctrl, client)
 
 		metrics, _, err := scaler.GetMetricsAndActivity(context.Background(), metricName)
 		_, _, err = GetMetricsWithFallback(context.Background(), client, metrics, err, metricName, so, metricSpec)

pkg/scaling/scale_handler_test.go

@@ -62,7 +62,7 @@ func TestGetScaledObjectMetrics_DirectCall(t *testing.T) {
 	recorder := record.NewFakeRecorder(1)
 	mockClient := mock_client.NewMockClient(ctrl)
 	mockExecutor := mock_executor.NewMockScaleExecutor(ctrl)
-	mockStatusWriter := mock_client.NewMockStatusWriter(ctrl)
+	//mockStatusWriter := mock_client.NewMockStatusWriter(ctrl)
 
 	metricsSpecs := []v2.MetricSpec{createMetricSpec(10, metricName)}
 	metricValue := scalers.GenerateMetricInMili(metricName, float64(10))
@@ -130,8 +130,8 @@ func TestGetScaledObjectMetrics_DirectCall(t *testing.T) {
 	mockExecutor.EXPECT().RequestScale(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
 	sh.checkScalers(context.TODO(), &scaledObject, &sync.RWMutex{})
 
-	mockClient.EXPECT().Status().Return(mockStatusWriter)
-	mockStatusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
+	//mockClient.EXPECT().Status().Return(mockStatusWriter)
+	//mockStatusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
 	scaler.EXPECT().GetMetricSpecForScaling(gomock.Any()).Return(metricsSpecs)
 	// hitting directly GetMetricsAndActivity()
 	scaler.EXPECT().GetMetricsAndActivity(gomock.Any(), gomock.Any()).Return([]external_metrics.ExternalMetricValue{metricValue}, true, nil)
@@ -153,7 +153,7 @@ func TestGetScaledObjectMetrics_FromCache(t *testing.T) {
 	recorder := record.NewFakeRecorder(1)
 	mockClient := mock_client.NewMockClient(ctrl)
 	mockExecutor := mock_executor.NewMockScaleExecutor(ctrl)
-	mockStatusWriter := mock_client.NewMockStatusWriter(ctrl)
+	//mockStatusWriter := mock_client.NewMockStatusWriter(ctrl)
 
 	metricsSpecs := []v2.MetricSpec{createMetricSpec(10, metricName)}
 	metricValue := scalers.GenerateMetricInMili(metricName, float64(10))
@@ -221,8 +221,8 @@ func TestGetScaledObjectMetrics_FromCache(t *testing.T) {
 	mockExecutor.EXPECT().RequestScale(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
 	sh.checkScalers(context.TODO(), &scaledObject, &sync.RWMutex{})
 
-	mockClient.EXPECT().Status().Return(mockStatusWriter)
-	mockStatusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
+	//mockClient.EXPECT().Status().Return(mockStatusWriter)
+	//mockStatusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
 	scaler.EXPECT().GetMetricSpecForScaling(gomock.Any()).Return(metricsSpecs)
 	// hitting cache here instead of calling GetMetricsAndActivity()
 	metrics, err := sh.GetScaledObjectMetrics(context.TODO(), scaledObjectName, scaledObjectNamespace, metricName)
@@ -259,7 +259,7 @@ func TestGetScaledObjectMetrics_InParallel(t *testing.T) {
 	recorder := record.NewFakeRecorder(1)
 	mockClient := mock_client.NewMockClient(ctrl)
 	mockExecutor := mock_executor.NewMockScaleExecutor(ctrl)
-	mockStatusWriter := mock_client.NewMockStatusWriter(ctrl)
+	//mockStatusWriter := mock_client.NewMockStatusWriter(ctrl)
 
 	scalerCollection := []*mock_scalers.MockScaler{}
 
@@ -353,8 +353,8 @@ func TestGetScaledObjectMetrics_InParallel(t *testing.T) {
 		return true
 	}, 1*time.Second, 400*time.Millisecond, "timeout exceeded: scalers not processed in parallel during `checkScalers`")
 
-	mockClient.EXPECT().Status().Times(len(metricNames)).Return(mockStatusWriter)
-	mockStatusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Times(len(metricNames)).Return(nil)
+	//mockClient.EXPECT().Status().Times(len(metricNames)).Return(mockStatusWriter)
+	//mockStatusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Times(len(metricNames)).Return(nil)
 	for i := 0; i < len(metricNames); i++ {
 		i := i
 		scalerCollection[i].EXPECT().GetMetricSpecForScaling(gomock.Any()).Return(metricsSpecFn(i))
@@ -907,7 +907,7 @@ func TestScalingModifiersFormula(t *testing.T) {
 	recorder := record.NewFakeRecorder(1)
 	mockClient := mock_client.NewMockClient(ctrl)
 	mockExecutor := mock_executor.NewMockScaleExecutor(ctrl)
-	mockStatusWriter := mock_client.NewMockStatusWriter(ctrl)
+	//mockStatusWriter := mock_client.NewMockStatusWriter(ctrl)
 
 	metricsSpecs1 := []v2.MetricSpec{createMetricSpec(2, metricName1)}
 	metricsSpecs2 := []v2.MetricSpec{createMetricSpec(5, metricName2)}
@@ -998,8 +998,8 @@ func TestScalingModifiersFormula(t *testing.T) {
 	mockExecutor.EXPECT().RequestScale(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
 	sh.checkScalers(context.TODO(), &scaledObject, &sync.RWMutex{})
 
-	mockClient.EXPECT().Status().Return(mockStatusWriter).Times(2)
-	mockStatusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(2)
+	//mockClient.EXPECT().Status().Return(mockStatusWriter).Times(2)
+	//mockStatusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(2)
 	scaler1.EXPECT().GetMetricSpecForScaling(gomock.Any()).Return(metricsSpecs1)
 	scaler2.EXPECT().GetMetricSpecForScaling(gomock.Any()).Return(metricsSpecs2)
 	scaler1.EXPECT().GetMetricsAndActivity(gomock.Any(), gomock.Any()).Return([]external_metrics.ExternalMetricValue{metricValue1, metricValue2}, true, nil)