From 7430aa0bad241a0d7e64bb0c6c9d3a90ffa4b10a Mon Sep 17 00:00:00 2001 From: rickbrouwer Date: Wed, 15 Jan 2025 17:34:58 +0100 Subject: [PATCH] Update after feedback Signed-off-by: rickbrouwer --- apis/keda/v1alpha1/scaledobject_types.go | 14 +++++- config/crd/bases/keda.sh_scaledobjects.yaml | 4 +- pkg/fallback/fallback.go | 10 ++-- pkg/fallback/fallback_test.go | 47 +++---------------- pkg/scaling/executor/scale_scaledobjects.go | 7 +-- .../executor/scale_scaledobjects_test.go | 30 ++++++------ 6 files changed, 47 insertions(+), 65 deletions(-) diff --git a/apis/keda/v1alpha1/scaledobject_types.go b/apis/keda/v1alpha1/scaledobject_types.go index ad3d3e45c64..066cdfbcc53 100644 --- a/apis/keda/v1alpha1/scaledobject_types.go +++ b/apis/keda/v1alpha1/scaledobject_types.go @@ -56,6 +56,8 @@ const ScaledObjectTransferHpaOwnershipAnnotation = "scaledobject.keda.sh/transfe const ValidationsHpaOwnershipAnnotation = "validations.keda.sh/hpa-ownership" const PausedReplicasAnnotation = "autoscaling.keda.sh/paused-replicas" const PausedAnnotation = "autoscaling.keda.sh/paused" +const FallbackBehaviorStatic = "static" +const FallbackBehaviorUseCurrentReplicasAsMin = "useCurrentReplicasAsMinimum" // HealthStatus is the status for a ScaledObject's health type HealthStatus struct { @@ -110,7 +112,7 @@ type Fallback struct { FailureThreshold int32 `json:"failureThreshold"` Replicas int32 `json:"replicas"` // +optional - UseCurrentReplicasAsMinimum *bool `json:"useCurrentReplicasAsMinimum,omitempty"` + Behavior string `json:"behavior,omitempty"` } // AdvancedConfig specifies advance scaling options @@ -288,6 +290,16 @@ func CheckFallbackValid(scaledObject *ScaledObject) error { scaledObject.Spec.Fallback.FailureThreshold, scaledObject.Spec.Fallback.Replicas) } + // Check if behavior is valid when specified + if scaledObject.Spec.Fallback.Behavior != "" && + scaledObject.Spec.Fallback.Behavior != FallbackBehaviorStatic && + scaledObject.Spec.Fallback.Behavior != FallbackBehaviorUseCurrentReplicasAsMin { + return fmt.Errorf("Behaviour=%s must be either '%s' or '%s'", + scaledObject.Spec.Fallback.Behavior, + FallbackBehaviorStatic, + FallbackBehaviorUseCurrentReplicasAsMin) + } + for _, trigger := range scaledObject.Spec.Triggers { if trigger.Type == cpuString || trigger.Type == memoryString { return fmt.Errorf("type is %s , but fallback it is not supported by the CPU & memory scalers", trigger.Type) diff --git a/config/crd/bases/keda.sh_scaledobjects.yaml b/config/crd/bases/keda.sh_scaledobjects.yaml index 9d9c71cd566..df2f7e202f7 100644 --- a/config/crd/bases/keda.sh_scaledobjects.yaml +++ b/config/crd/bases/keda.sh_scaledobjects.yaml @@ -231,8 +231,8 @@ spec: replicas: format: int32 type: integer - useCurrentReplicasAsMinimum: - type: boolean + behavior: + type: string required: - failureThreshold - replicas diff --git a/pkg/fallback/fallback.go b/pkg/fallback/fallback.go index 7c03a250c30..f7efc2f8458 100644 --- a/pkg/fallback/fallback.go +++ b/pkg/fallback/fallback.go @@ -111,10 +111,10 @@ func HasValidFallback(scaledObject *kedav1alpha1.ScaledObject) bool { func doFallback(scaledObject *kedav1alpha1.ScaledObject, metricSpec v2.MetricSpec, metricName string, currentReplicas int32, suppressedError error) []external_metrics.ExternalMetricValue { replicas := int64(scaledObject.Spec.Fallback.Replicas) + fallbackBehavior := scaledObject.Spec.Fallback.Behavior - // Check if we should use current replicas as minimum - if scaledObject.Spec.Fallback.UseCurrentReplicasAsMinimum != nil && - *scaledObject.Spec.Fallback.UseCurrentReplicasAsMinimum { + // Check behavior 'UseCurrentReplicasAsMin' + if fallbackBehavior == kedav1alpha1.FallbackBehaviorUseCurrentReplicasAsMin { currentReplicasCount := int64(currentReplicas) if currentReplicasCount > replicas { replicas = currentReplicasCount @@ -141,7 +141,9 @@ func doFallback(scaledObject *kedav1alpha1.ScaledObject, metricSpec v2.MetricSpe "scaledObject.Namespace", scaledObject.Namespace, "scaledObject.Name", scaledObject.Name, "suppressedError", suppressedError, - "fallback.replicas", replicas) + "fallback.behavior", fallbackBehavior, + "fallback.replicas", replicas, + "workload.currentReplicas", currentReplicas) return fallbackMetrics } diff --git a/pkg/fallback/fallback_test.go b/pkg/fallback/fallback_test.go index 2982c0468e0..78687356bc7 100644 --- a/pkg/fallback/fallback_test.go +++ b/pkg/fallback/fallback_test.go @@ -378,13 +378,13 @@ var _ = Describe("fallback", func() { It("should use fallback replicas when current replicas is lower", func() { scaler.EXPECT().GetMetricsAndActivity(gomock.Any(), gomock.Eq(metricName)).Return(nil, false, errors.New("some error")) startingNumberOfFailures := int32(3) - useCurrentAsMin := true + behavior := "useCurrentReplicasAsMinimum" so := buildScaledObject( &kedav1alpha1.Fallback{ - FailureThreshold: int32(3), - Replicas: int32(10), - UseCurrentReplicasAsMinimum: &useCurrentAsMin, + FailureThreshold: int32(3), + Replicas: int32(10), + Behavior: behavior, }, &kedav1alpha1.ScaledObjectStatus{ Health: map[string]kedav1alpha1.HealthStatus{ @@ -409,49 +409,16 @@ var _ = Describe("fallback", func() { Expect(value).Should(Equal(expectedValue)) }) - It("should ignore current replicas when UseCurrentReplicasAsMinimum is false", func() { - scaler.EXPECT().GetMetricsAndActivity(gomock.Any(), gomock.Eq(metricName)).Return(nil, false, errors.New("some error")) - startingNumberOfFailures := int32(3) - useCurrentAsMin := false - - so := buildScaledObject( - &kedav1alpha1.Fallback{ - FailureThreshold: int32(3), - Replicas: int32(10), - UseCurrentReplicasAsMinimum: &useCurrentAsMin, - }, - &kedav1alpha1.ScaledObjectStatus{ - Health: map[string]kedav1alpha1.HealthStatus{ - metricName: { - NumberOfFailures: &startingNumberOfFailures, - Status: kedav1alpha1.HealthStatusHappy, - }, - }, - }, - ) - metricSpec := createMetricSpec(10) - expectStatusPatch(ctrl, client) - - mockScaleAndDeployment(ctrl, client, scaleClient, 15) - - metrics, _, err := scaler.GetMetricsAndActivity(context.Background(), metricName) - metrics, _, err = GetMetricsWithFallback(context.Background(), client, scaleClient, metrics, err, metricName, so, metricSpec) - - Expect(err).ToNot(HaveOccurred()) - value := metrics[0].Value.AsApproximateFloat64() - expectedValue := float64(100) // 10 replicas * 10 target value, ignoring current 15 - Expect(value).Should(Equal(expectedValue)) - }) - - It("should handle nil UseCurrentReplicasAsMinimum the same as false", func() { + It("should ignore current replicas when behavior is 'static'", func() { scaler.EXPECT().GetMetricsAndActivity(gomock.Any(), gomock.Eq(metricName)).Return(nil, false, errors.New("some error")) startingNumberOfFailures := int32(3) + behavior := "static" so := buildScaledObject( &kedav1alpha1.Fallback{ FailureThreshold: int32(3), Replicas: int32(10), - // UseCurrentReplicasAsMinimum is nil + Behavior: behavior, }, &kedav1alpha1.ScaledObjectStatus{ Health: map[string]kedav1alpha1.HealthStatus{ diff --git a/pkg/scaling/executor/scale_scaledobjects.go b/pkg/scaling/executor/scale_scaledobjects.go index bafb307528a..d5d6680476c 100644 --- a/pkg/scaling/executor/scale_scaledobjects.go +++ b/pkg/scaling/executor/scale_scaledobjects.go @@ -207,15 +207,16 @@ func (e *scaleExecutor) RequestScale(ctx context.Context, scaledObject *kedav1al func (e *scaleExecutor) doFallbackScaling(ctx context.Context, scaledObject *kedav1alpha1.ScaledObject, currentScale *autoscalingv1.Scale, logger logr.Logger, currentReplicas int32) { targetReplicas := scaledObject.Spec.Fallback.Replicas + fallbackBehavior := scaledObject.Spec.Fallback.Behavior - // Check if we should use current replicas as minimum - if scaledObject.Spec.Fallback.UseCurrentReplicasAsMinimum != nil && *scaledObject.Spec.Fallback.UseCurrentReplicasAsMinimum && currentReplicas > targetReplicas { + // Check behavior 'UseCurrentReplicasAsMin' + if fallbackBehavior == kedav1alpha1.FallbackBehaviorUseCurrentReplicasAsMin && currentReplicas > targetReplicas { targetReplicas = currentReplicas } _, err := e.updateScaleOnScaleTarget(ctx, scaledObject, currentScale, targetReplicas) if err == nil { - logger.Info("Successfully set ScaleTarget replicas count to calculated fallback replicas", "Original Replicas Count", currentReplicas, "New Replicas Count", targetReplicas) + logger.Info("Successfully set ScaleTarget replicas count to calculated fallback replicas", "Original Replicas Count", currentReplicas, "New Replicas Count", targetReplicas, "Behavior", fallbackBehavior) } if e := e.setFallbackCondition(ctx, logger, scaledObject, metav1.ConditionTrue, "FallbackExists", "At least one trigger is falling back on this scaled object"); e != nil { logger.Error(e, "Error setting fallback condition") diff --git a/pkg/scaling/executor/scale_scaledobjects_test.go b/pkg/scaling/executor/scale_scaledobjects_test.go index 96b07da4f1c..b58fa168571 100644 --- a/pkg/scaling/executor/scale_scaledobjects_test.go +++ b/pkg/scaling/executor/scale_scaledobjects_test.go @@ -523,7 +523,7 @@ func TestEventWitTriggerInfo(t *testing.T) { assert.Equal(t, "Normal KEDAScaleTargetActivated Scaled namespace/name from 2 to 5, triggered by testTrigger", eventstring) } -// UseCurrentReplicasAsMinimum is true and current replicas is higher than fallback replicas +// Behavior is UseCurrentReplicasAsMinimum and current replicas is higher than fallback replicas func TestScaleToFallbackWithCurrentReplicasAsMinimum(t *testing.T) { ctrl := gomock.NewController(t) client := mock_client.NewMockClient(ctrl) @@ -534,7 +534,7 @@ func TestScaleToFallbackWithCurrentReplicasAsMinimum(t *testing.T) { scaleExecutor := NewScaleExecutor(client, mockScaleClient, nil, recorder) - useCurrentAsMin := true + behavior := "useCurrentReplicasAsMinimum" scaledObject := v1alpha1.ScaledObject{ ObjectMeta: v1.ObjectMeta{ Name: "name", @@ -545,9 +545,9 @@ func TestScaleToFallbackWithCurrentReplicasAsMinimum(t *testing.T) { Name: "name", }, Fallback: &v1alpha1.Fallback{ - FailureThreshold: 3, - Replicas: 5, - UseCurrentReplicasAsMinimum: &useCurrentAsMin, + FailureThreshold: 3, + Replicas: 5, + Behavior: behavior, }, }, Status: v1alpha1.ScaledObjectStatus{ @@ -590,7 +590,7 @@ func TestScaleToFallbackWithCurrentReplicasAsMinimum(t *testing.T) { assert.Equal(t, true, condition.IsTrue()) } -// UseCurrentReplicasAsMinimum is true and current replicas is lower than fallback replicas +// Behavior is UseCurrentReplicasAsMinimum and is true and current replicas is lower than fallback replicas func TestScaleToFallbackIgnoringLowerCurrentReplicas(t *testing.T) { ctrl := gomock.NewController(t) client := mock_client.NewMockClient(ctrl) @@ -601,7 +601,7 @@ func TestScaleToFallbackIgnoringLowerCurrentReplicas(t *testing.T) { scaleExecutor := NewScaleExecutor(client, mockScaleClient, nil, recorder) - useCurrentAsMin := true + behavior := "useCurrentReplicasAsMinimum" scaledObject := v1alpha1.ScaledObject{ ObjectMeta: v1.ObjectMeta{ Name: "name", @@ -612,9 +612,9 @@ func TestScaleToFallbackIgnoringLowerCurrentReplicas(t *testing.T) { Name: "name", }, Fallback: &v1alpha1.Fallback{ - FailureThreshold: 3, - Replicas: 5, - UseCurrentReplicasAsMinimum: &useCurrentAsMin, + FailureThreshold: 3, + Replicas: 5, + Behavior: behavior, }, }, Status: v1alpha1.ScaledObjectStatus{ @@ -657,7 +657,7 @@ func TestScaleToFallbackIgnoringLowerCurrentReplicas(t *testing.T) { assert.Equal(t, true, condition.IsTrue()) } -// UseCurrentReplicasAsMinimum is false and current replicas is higher than fallback replicas +// Behavior is Static and current replicas is higher than fallback replicas func TestScaleToFallbackWithoutCurrentReplicasAsMinimum(t *testing.T) { ctrl := gomock.NewController(t) client := mock_client.NewMockClient(ctrl) @@ -668,7 +668,7 @@ func TestScaleToFallbackWithoutCurrentReplicasAsMinimum(t *testing.T) { scaleExecutor := NewScaleExecutor(client, mockScaleClient, nil, recorder) - useCurrentAsMin := false + behavior := "static" scaledObject := v1alpha1.ScaledObject{ ObjectMeta: v1.ObjectMeta{ Name: "name", @@ -679,9 +679,9 @@ func TestScaleToFallbackWithoutCurrentReplicasAsMinimum(t *testing.T) { Name: "name", }, Fallback: &v1alpha1.Fallback{ - FailureThreshold: 3, - Replicas: 5, - UseCurrentReplicasAsMinimum: &useCurrentAsMin, + FailureThreshold: 3, + Replicas: 5, + Behavior: behavior, }, }, Status: v1alpha1.ScaledObjectStatus{