diff --git a/.golangci.yml b/.golangci.yml index 5d95f0349e4..b75b1cca1a4 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -54,6 +54,9 @@ issues: - path: scale_handler.go linters: - gocyclo + - path: scale_scaledobjects.go + linters: + - gocyclo # Exclude for clustertriggerauthentication_controller and triggerauthentication_controller, reason: # controllers/clustertriggerauthentication_controller.go:1: 1-59 lines are duplicate of `controllers/triggerauthentication_controller.go:1-58` (dupl) - path: triggerauthentication_controller.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f70a0d4d95..52f9a2ac057 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,8 +33,8 @@ ### Improvements -- **General:** Fix generation of metric names if any of ScaledObject's triggers is unavailable ([#2592](https://github.com/kedacore/keda/issues/2592)) -- **General:** Fix failing tests based on the scale to zero bug ([#2603](https://github.com/kedacore/keda/issues/2603)) +- **General**: Fix generation of metric names if any of ScaledObject's triggers is unavailable ([#2592](https://github.com/kedacore/keda/issues/2592)) +- **General**: Fix logging in KEDA operator and properly set `ScaledObject.Status` in case there is a problem in a ScaledObject's trigger ([#2603](https://github.com/kedacore/keda/issues/2603)) ### Breaking Changes @@ -42,7 +42,7 @@ ### Other -- TODO ([#XXX](https://github.com/kedacore/keda/pull/XXX)) +- **General:** Fix failing tests based on the scale to zero bug ([#2603](https://github.com/kedacore/keda/issues/2603)) ## v2.6.0 diff --git a/apis/keda/v1alpha1/condition_types.go b/apis/keda/v1alpha1/condition_types.go index e5f7000e6ab..979d83e12e8 100644 --- a/apis/keda/v1alpha1/condition_types.go +++ b/apis/keda/v1alpha1/condition_types.go @@ -34,6 +34,13 @@ const ( ConditionFallback ConditionType = "Fallback" ) +const ( + // ScaledObjectConditionReadySucccesReason defines the default Reason for correct ScaledObject + ScaledObjectConditionReadySucccesReason = "ScaledObjectReady" + // ScaledObjectConditionReadySuccessMessage defines the default Message for correct ScaledObject + ScaledObjectConditionReadySuccessMessage = "ScaledObject is defined correctly and is ready for scaling" +) + // Condition to store the condition state type Condition struct { // Type of condition diff --git a/controllers/keda/scaledobject_controller.go b/controllers/keda/scaledobject_controller.go index fb28d3e1ea2..6b444f58729 100644 --- a/controllers/keda/scaledobject_controller.go +++ b/controllers/keda/scaledobject_controller.go @@ -190,7 +190,7 @@ func (r *ScaledObjectReconciler) Reconcile(ctx context.Context, req ctrl.Request r.Recorder.Event(scaledObject, corev1.EventTypeNormal, eventreason.ScaledObjectReady, "ScaledObject is ready for scaling") } reqLogger.V(1).Info(msg) - conditions.SetReadyCondition(metav1.ConditionTrue, "ScaledObjectReady", msg) + conditions.SetReadyCondition(metav1.ConditionTrue, kedav1alpha1.ScaledObjectConditionReadySucccesReason, msg) } if err := kedacontrollerutil.SetStatusConditions(ctx, r.Client, reqLogger, scaledObject, &conditions); err != nil { @@ -248,7 +248,7 @@ func (r *ScaledObjectReconciler) reconcileScaledObject(ctx context.Context, logg } logger.Info("Initializing Scaling logic according to ScaledObject Specification") } - return "ScaledObject is defined correctly and is ready for scaling", nil + return kedav1alpha1.ScaledObjectConditionReadySuccessMessage, nil } // ensureScaledObjectLabel ensures that scaledobject.keda.sh/name= label exist in the ScaledObject diff --git a/pkg/scaling/cache/scalers_cache.go b/pkg/scaling/cache/scalers_cache.go index 163b797421f..e7dc2c7fbe4 100644 --- a/pkg/scaling/cache/scalers_cache.go +++ b/pkg/scaling/cache/scalers_cache.go @@ -82,6 +82,7 @@ func (c *ScalersCache) GetMetricsForScaler(ctx context.Context, id int, metricNa func (c *ScalersCache) IsScaledObjectActive(ctx context.Context, scaledObject *kedav1alpha1.ScaledObject) (bool, bool, []external_metrics.ExternalMetricValue) { isActive := false isError := false + // Let's collect status of all scalers, no matter if any scaler raises error or is active for i, s := range c.Scalers { isTriggerActive, err := s.Scaler.IsActive(ctx) if err != nil { @@ -92,19 +93,21 @@ func (c *ScalersCache) IsScaledObjectActive(ctx context.Context, scaledObject *k } } + logger := c.Logger.WithValues("scaledobject.Name", scaledObject.Name, "scaledObject.Namespace", scaledObject.Namespace, + "scaleTarget.Name", scaledObject.Spec.ScaleTargetRef.Name) + if err != nil { - c.Logger.V(1).Info("Error getting scale decision", "Error", err) isError = true + logger.Error(err, "Error getting scale decision") c.Recorder.Event(scaledObject, corev1.EventTypeWarning, eventreason.KEDAScalerFailed, err.Error()) } else if isTriggerActive { isActive = true if externalMetricsSpec := s.Scaler.GetMetricSpecForScaling(ctx)[0].External; externalMetricsSpec != nil { - c.Logger.V(1).Info("Scaler for scaledObject is active", "Metrics Name", externalMetricsSpec.Metric.Name) + logger.V(1).Info("Scaler for scaledObject is active", "Metrics Name", externalMetricsSpec.Metric.Name) } if resourceMetricsSpec := s.Scaler.GetMetricSpecForScaling(ctx)[0].Resource; resourceMetricsSpec != nil { - c.Logger.V(1).Info("Scaler for scaledObject is active", "Metrics Name", resourceMetricsSpec.Name) + logger.V(1).Info("Scaler for scaledObject is active", "Metrics Name", resourceMetricsSpec.Name) } - break } } diff --git a/pkg/scaling/executor/scale_executor.go b/pkg/scaling/executor/scale_executor.go index 71b2d20e73f..78ca082d840 100644 --- a/pkg/scaling/executor/scale_executor.go +++ b/pkg/scaling/executor/scale_executor.go @@ -110,6 +110,13 @@ func (e *scaleExecutor) setCondition(ctx context.Context, logger logr.Logger, ob return err } +func (e *scaleExecutor) setReadyCondition(ctx context.Context, logger logr.Logger, object interface{}, status metav1.ConditionStatus, reason string, message string) error { + active := func(conditions kedav1alpha1.Conditions, status metav1.ConditionStatus, reason string, message string) { + conditions.SetReadyCondition(status, reason, message) + } + return e.setCondition(ctx, logger, object, status, reason, message, active) +} + func (e *scaleExecutor) setActiveCondition(ctx context.Context, logger logr.Logger, object interface{}, status metav1.ConditionStatus, reason string, message string) error { active := func(conditions kedav1alpha1.Conditions, status metav1.ConditionStatus, reason string, message string) { conditions.SetActiveCondition(status, reason, message) diff --git a/pkg/scaling/executor/scale_scaledobjects.go b/pkg/scaling/executor/scale_scaledobjects.go index 5a083b39ff1..aa517e6162e 100644 --- a/pkg/scaling/executor/scale_scaledobjects.go +++ b/pkg/scaling/executor/scale_scaledobjects.go @@ -75,6 +75,16 @@ func (e *scaleExecutor) RequestScale(ctx context.Context, scaledObject *kedav1al minReplicas = *scaledObject.Spec.MinReplicaCount } + // if the ScaledObject's triggers aren't in the error state, + // but ScaledObject.Status.ReadyCondition is set not set to 'true' -> set it back to 'true' + readyCondition := scaledObject.Status.Conditions.GetReadyCondition() + if !isError && !readyCondition.IsTrue() { + if err := e.setReadyCondition(ctx, logger, scaledObject, metav1.ConditionFalse, + kedav1alpha1.ScaledObjectConditionReadySucccesReason, kedav1alpha1.ScaledObjectConditionReadySuccessMessage); err != nil { + logger.Error(err, "error setting ready condition") + } + } + if isActive { switch { case scaledObject.Spec.IdleReplicaCount != nil && currentReplicas < minReplicas, @@ -89,6 +99,17 @@ func (e *scaleExecutor) RequestScale(ctx context.Context, scaledObject *kedav1al // Scale the ScaleTarget up e.scaleFromZeroOrIdle(ctx, logger, scaledObject, currentScale) + case isError: + // some triggers are active, but some responded with error + + // Set ScaledObject.Status.ReadyCondition to Unknown + msg := "Some triggers defined in ScaledObject are not working correctly" + logger.V(1).Info(msg) + if !readyCondition.IsUnknown() { + if err := e.setReadyCondition(ctx, logger, scaledObject, metav1.ConditionUnknown, "PartialTriggerError", msg); err != nil { + logger.Error(err, "error setting ready condition") + } + } default: // triggers are active, but we didn't need to scale (replica count > 0) @@ -109,6 +130,19 @@ func (e *scaleExecutor) RequestScale(ctx context.Context, scaledObject *kedav1al // Scale to the fallback replicas count e.doFallbackScaling(ctx, scaledObject, currentScale, logger, currentReplicas) + case isError && scaledObject.Spec.Fallback == nil: + // there are no active triggers, but a scaler responded with an error + // AND + // there is not a fallback replicas count defined + + // Set ScaledObject.Status.ReadyCondition to false + msg := "Triggers defined in ScaledObject are not working correctly" + logger.V(1).Info(msg) + if !readyCondition.IsFalse() { + if err := e.setReadyCondition(ctx, logger, scaledObject, metav1.ConditionFalse, "TriggerError", msg); err != nil { + logger.Error(err, "error setting ready condition") + } + } case scaledObject.Spec.IdleReplicaCount != nil && currentReplicas > *scaledObject.Spec.IdleReplicaCount, // there are no active triggers, Idle Replicas mode is enabled // AND diff --git a/pkg/scaling/executor/scale_scaledobjects_test.go b/pkg/scaling/executor/scale_scaledobjects_test.go index 52326292b7d..5ba8082cff9 100644 --- a/pkg/scaling/executor/scale_scaledobjects_test.go +++ b/pkg/scaling/executor/scale_scaledobjects_test.go @@ -145,8 +145,8 @@ func TestScaleToMinReplicasWhenNotActive(t *testing.T) { mockScaleInterface.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(scale, nil) mockScaleInterface.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Eq(scale), gomock.Any()) - client.EXPECT().Status().Return(statusWriter) - statusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()) + client.EXPECT().Status().Return(statusWriter).Times(2) + statusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Times(2) scaleExecutor.RequestScale(context.TODO(), &scaledObject, false, false) @@ -206,8 +206,8 @@ func TestScaleToMinReplicasFromLowerInitialReplicaCount(t *testing.T) { mockScaleInterface.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(scale, nil) mockScaleInterface.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Eq(scale), gomock.Any()) - client.EXPECT().Status().Return(statusWriter) - statusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()) + client.EXPECT().Status().Return(statusWriter).Times(2) + statusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Times(2) scaleExecutor.RequestScale(context.TODO(), &scaledObject, false, false) @@ -265,8 +265,8 @@ func TestScaleFromMinReplicasWhenActive(t *testing.T) { mockScaleInterface.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(scale, nil) mockScaleInterface.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Eq(scale), gomock.Any()) - client.EXPECT().Status().Times(2).Return(statusWriter) - statusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Times(2) + client.EXPECT().Status().Times(2).Return(statusWriter).Times(3) + statusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Times(3) scaleExecutor.RequestScale(context.TODO(), &scaledObject, true, false) @@ -328,8 +328,8 @@ func TestScaleToIdleReplicasWhenNotActive(t *testing.T) { mockScaleInterface.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(scale, nil) mockScaleInterface.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Eq(scale), gomock.Any()) - client.EXPECT().Status().Return(statusWriter) - statusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()) + client.EXPECT().Status().Return(statusWriter).Times(2) + statusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Times(2) scaleExecutor.RequestScale(context.TODO(), &scaledObject, false, false) @@ -389,8 +389,8 @@ func TestScaleFromIdleToMinReplicasWhenActive(t *testing.T) { mockScaleInterface.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(scale, nil) mockScaleInterface.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Eq(scale), gomock.Any()) - client.EXPECT().Status().Times(2).Return(statusWriter) - statusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Times(2) + client.EXPECT().Status().Times(2).Return(statusWriter).Times(3) + statusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Times(3) scaleExecutor.RequestScale(context.TODO(), &scaledObject, true, false) diff --git a/pkg/scaling/scale_handler_test.go b/pkg/scaling/scale_handler_test.go index 4aee0d3f607..e2399774a52 100644 --- a/pkg/scaling/scale_handler_test.go +++ b/pkg/scaling/scale_handler_test.go @@ -53,6 +53,11 @@ func TestCheckScaledObjectScalersWithError(t *testing.T) { Name: "test", Namespace: "test", }, + Spec: kedav1alpha1.ScaledObjectSpec{ + ScaleTargetRef: &kedav1alpha1.ScaleTarget{ + Name: "test", + }, + }, } cache := cache.ScalersCache{ @@ -71,34 +76,49 @@ func TestCheckScaledObjectScalersWithError(t *testing.T) { assert.Equal(t, true, isError) } -func TestCheckScaledObjectFindFirstActiveIgnoringOthers(t *testing.T) { +func TestCheckScaledObjectFindFirstActiveNotIgnoreOthers(t *testing.T) { ctrl := gomock.NewController(t) recorder := record.NewFakeRecorder(1) - activeScaler := mock_scalers.NewMockScaler(ctrl) - failingScaler := mock_scalers.NewMockScaler(ctrl) + + metricsSpecs := []v2beta2.MetricSpec{createMetricSpec(1)} + + activeFactory := func() (scalers.Scaler, error) { + scaler := mock_scalers.NewMockScaler(ctrl) + scaler.EXPECT().IsActive(gomock.Any()).Return(true, nil) + scaler.EXPECT().GetMetricSpecForScaling(gomock.Any()).Times(2).Return(metricsSpecs) + scaler.EXPECT().Close(gomock.Any()) + return scaler, nil + } + activeScaler, err := activeFactory() + assert.Nil(t, err) + + failingFactory := func() (scalers.Scaler, error) { + scaler := mock_scalers.NewMockScaler(ctrl) + scaler.EXPECT().IsActive(gomock.Any()).Return(false, errors.New("some error")) + scaler.EXPECT().Close(gomock.Any()) + return scaler, nil + } + failingScaler, err := failingFactory() + assert.Nil(t, err) + scaledObject := &kedav1alpha1.ScaledObject{ ObjectMeta: metav1.ObjectMeta{ Name: "test", Namespace: "test", }, + Spec: kedav1alpha1.ScaledObjectSpec{ + ScaleTargetRef: &kedav1alpha1.ScaleTarget{ + Name: "test", + }, + }, } - metricsSpecs := []v2beta2.MetricSpec{createMetricSpec(1)} - - activeScaler.EXPECT().IsActive(gomock.Any()).Return(true, nil) - activeScaler.EXPECT().GetMetricSpecForScaling(gomock.Any()).Times(2).Return(metricsSpecs) - activeScaler.EXPECT().Close(gomock.Any()) - failingScaler.EXPECT().Close(gomock.Any()) - - factory := func() (scalers.Scaler, error) { - return mock_scalers.NewMockScaler(ctrl), nil - } scalers := []cache.ScalerBuilder{{ Scaler: activeScaler, - Factory: factory, + Factory: activeFactory, }, { Scaler: failingScaler, - Factory: factory, + Factory: failingFactory, }} scalersCache := cache.ScalersCache{ @@ -111,7 +131,7 @@ func TestCheckScaledObjectFindFirstActiveIgnoringOthers(t *testing.T) { scalersCache.Close(context.Background()) assert.Equal(t, true, isActive) - assert.Equal(t, false, isError) + assert.Equal(t, true, isError) } func createMetricSpec(averageValue int) v2beta2.MetricSpec {