nil checkers a refactors, new fallback unit tests

kedacore · Jul 7, 2023 · a7151a6 · a7151a6
1 parent 1d6a251
commit a7151a6
Show file tree

Hide file tree

Showing 5 changed files with 567 additions and 103 deletions.
diff --git a/apis/keda/v1alpha1/condition_types.go b/apis/keda/v1alpha1/condition_types.go
@@ -228,6 +228,13 @@ func (c *Conditions) GetPausedCondition() Condition {
 	return c.getCondition(ConditionPaused)
 }
 
+func (c *Conditions) GetExternalFallbackCondition() Condition {
+	if *c == nil {
+		c = GetInitializedConditions()
+	}
+	return c.getCondition(ConditionExternalFallback)
+}
+
 func (c Conditions) getCondition(conditionType ConditionType) Condition {
 	for i := range c {
 		if c[i].Type == conditionType {

diff --git a/controllers/keda/hpa.go b/controllers/keda/hpa.go
@@ -250,9 +250,10 @@ func (r *ScaledObjectReconciler) getScaledObjectMetricSpecs(ctx context.Context,
 
 	updateHealthStatus(scaledObject, externalMetricNames, status)
 
-	// if ComplexScalingLogic struct is not nil, expect Formula or ExternalCalculation
+	// if ComplexScalingLogic struct is not empty, expect Formula or ExternalCalculation
 	// to be non-empty. If target is > 0.0 create a compositeScaler structure
-	if !reflect.DeepEqual(scaledObject.Spec.Advanced.ComplexScalingLogic, kedav1alpha1.ComplexScalingLogic{}) {
+	if scaledObject.Spec.Advanced != nil &&
+		!reflect.DeepEqual(scaledObject.Spec.Advanced.ComplexScalingLogic, kedav1alpha1.ComplexScalingLogic{}) {
 		validNumTarget, validMetricType, err := validateCompositeScalingLogic(scaledObject, scaledObjectMetricSpecs)
 		if err != nil {
 			logger.Error(err, "error validating compositeScalingLogic")

diff --git a/pkg/fallback/fallback.go b/pkg/fallback/fallback.go
@@ -49,7 +49,8 @@ func isFallbackEnabled(scaledObject *kedav1alpha1.ScaledObject, metricSpec v2.Me
 			return false
 		}
 	case externalCalculatorStr:
-		if scaledObject.Spec.Advanced.ComplexScalingLogic.Fallback == nil {
+		// check for nil pointer & only then check for fallback
+		if scaledObject.Spec.Advanced == nil || scaledObject.Spec.Advanced.ComplexScalingLogic.Fallback == nil {
 			return false
 		}
 	default:
@@ -59,9 +60,7 @@ func isFallbackEnabled(scaledObject *kedav1alpha1.ScaledObject, metricSpec v2.Me
 	return true
 }
 
-func GetMetricsWithFallbackExternalCalculator(ctx context.Context, client runtimeclient.Client, metrics *cl.MetricsList, suppressedError error, metricName string, scaledObject *kedav1alpha1.ScaledObject) (bool, error) {
-	// init empty list if it is nil because of an error earlier
-
+func GetMetricsWithFallbackExternalCalculator(ctx context.Context, client runtimeclient.Client, metrics []*cl.Metric, suppressedError error, metricName string, scaledObject *kedav1alpha1.ScaledObject) ([]*cl.Metric, bool, error) {
 	const determiner string = "externalcalculator"
 	status := scaledObject.Status.DeepCopy()
 
@@ -72,7 +71,7 @@ func GetMetricsWithFallbackExternalCalculator(ctx context.Context, client runtim
 	if healthStatus == nil {
 		// should never be nil
 		err := fmt.Errorf("internal error getting health status in GetMetricsWithFallbackExternalCalculator - wrong determiner")
-		return false, err
+		return []*cl.Metric{}, false, err
 	}
 
 	// if there is no error
@@ -84,7 +83,7 @@ func GetMetricsWithFallbackExternalCalculator(ctx context.Context, client runtim
 
 		updateStatus(ctx, client, scaledObject, status, v2.MetricSpec{})
 
-		return false, nil
+		return metrics, false, nil
 	}
 
 	healthStatus.Status = kedav1alpha1.HealthStatusFailing
@@ -94,16 +93,14 @@ func GetMetricsWithFallbackExternalCalculator(ctx context.Context, client runtim
 
 	switch {
 	case !isFallbackEnabled(scaledObject, v2.MetricSpec{}, determiner):
-		return false, suppressedError
+		return metrics, false, suppressedError
 	case !validateFallback(scaledObject, determiner):
 		log.Info("Failed to validate ScaledObject ComplexScalingLogic Fallback. Please check that parameters are positive integers", "scaledObject.Namespace", scaledObject.Namespace, "scaledObject.Name", scaledObject.Name)
-		return false, suppressedError
+		return metrics, false, suppressedError
 	case *healthStatus.NumberOfFailures > scaledObject.Spec.Advanced.ComplexScalingLogic.Fallback.FailureThreshold:
-		doExternalCalculationFallback(scaledObject, metrics, metricName, suppressedError)
-
-		return true, nil
+		return doExternalCalculationFallback(scaledObject, metrics, metricName, suppressedError), true, nil
 	default:
-		return false, suppressedError
+		return metrics, false, suppressedError
 	}
 }
 
@@ -200,21 +197,20 @@ func doFallback(scaledObject *kedav1alpha1.ScaledObject, metricSpec v2.MetricSpe
 	return fallbackMetrics
 }
 
-func doExternalCalculationFallback(scaledObject *kedav1alpha1.ScaledObject, metrics *cl.MetricsList, metricName string, suppressedError error) {
+func doExternalCalculationFallback(scaledObject *kedav1alpha1.ScaledObject, metrics []*cl.Metric, metricName string, suppressedError error) []*cl.Metric {
 	replicas := int64(scaledObject.Spec.Advanced.ComplexScalingLogic.Fallback.Replicas)
 	normalisationValue, err := strconv.ParseFloat(scaledObject.Spec.Advanced.ComplexScalingLogic.Target, 64)
 	if err != nil {
 		log.Error(err, "error converting string to float in ExternalCalculation fallback")
-		return
+		return metrics
 	}
 	metric := cl.Metric{
 		Name:  metricName,
 		Value: float32(normalisationValue * float64(replicas)),
 	}
 
-	metrics.MetricValues = []*cl.Metric{&metric}
-
 	log.Info(fmt.Sprintf("Suppressing error, externalCalculator falling back to %d fallback.replicas", scaledObject.Spec.Advanced.ComplexScalingLogic.Fallback.Replicas), "scaledObject.Namespace", scaledObject.Namespace, "scaledObject.Name", scaledObject.Name, "suppressedError", suppressedError)
+	return []*cl.Metric{&metric}
 }
 
 func updateStatus(ctx context.Context, client runtimeclient.Client, scaledObject *kedav1alpha1.ScaledObject, status *kedav1alpha1.ScaledObjectStatus, metricSpec v2.MetricSpec) {