diff --git a/CHANGELOG.md b/CHANGELOG.md index 67ab2d6da1f..0998e78f962 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -99,6 +99,7 @@ New deprecation(s): - **General**: Use default metrics provider from sigs.k8s.io/custom-metrics-apiserver ([#4473](https://github.com/kedacore/keda/pull/4473)) - **General**: Refactor several functions for Status & Conditions handling into pkg util functions ([#2906](https://github.com/kedacore/keda/pull/2906)) - **General**: Bump `kubernetes-sigs/controller-runtime` to v0.15.0 and code alignment ([#4582](https://github.com/kedacore/keda/pull/4582)) +- **General**: Stop logging errors for paused ScaledObject (with "autoscaling.keda.sh/paused-replicas" annotation) by skipping reconciliation loop for the object (stop the scale loop and delete the HPA) ([#4253](https://github.com/kedacore/keda/pull/4253)) ## v2.10.1 diff --git a/controllers/keda/hpa.go b/controllers/keda/hpa.go index febeb0964b4..0b96382e8d7 100644 --- a/controllers/keda/hpa.go +++ b/controllers/keda/hpa.go @@ -183,13 +183,21 @@ func (r *ScaledObjectReconciler) updateHPAIfNeeded(ctx context.Context, logger l // deleteAndCreateHpa delete old HPA and create new one func (r *ScaledObjectReconciler) renameHPA(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject, foundHpa *autoscalingv2.HorizontalPodAutoscaler, gvkr *kedav1alpha1.GroupVersionKindResource) error { - logger.Info("Deleting old HPA", "HPA.Namespace", scaledObject.Namespace, "HPA.Name", foundHpa.Name) + if err := r.deleteHPA(ctx, logger, scaledObject, foundHpa); err != nil { + return err + } + return r.createAndDeployNewHPA(ctx, logger, scaledObject, gvkr) +} + +// deleteHpa delete existing HPA +func (r *ScaledObjectReconciler) deleteHPA(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject, foundHpa *autoscalingv2.HorizontalPodAutoscaler) error { + logger.Info("Deleting existing HPA", "HPA.Namespace", scaledObject.Namespace, "HPA.Name", foundHpa.Name) if err := r.Client.Delete(ctx, foundHpa); err != nil { logger.Error(err, "Failed to delete old HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name) return err } - return r.createAndDeployNewHPA(ctx, logger, scaledObject, gvkr) + return nil } // getScaledObjectMetricSpecs returns MetricSpec for HPA, generater from Triggers defitinion in ScaledObject diff --git a/controllers/keda/scaledobject_controller.go b/controllers/keda/scaledobject_controller.go index 869c51c3a88..9c2f034bf02 100644 --- a/controllers/keda/scaledobject_controller.go +++ b/controllers/keda/scaledobject_controller.go @@ -198,6 +198,20 @@ func (r *ScaledObjectReconciler) Reconcile(ctx context.Context, req ctrl.Request // reconcileScaledObject implements reconciler logic for ScaledObject func (r *ScaledObjectReconciler) reconcileScaledObject(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject) (string, error) { + // Check the presence of "autoscaling.keda.sh/paused-replicas" annotation on the scaledObject (since the presence of this annotation will pause + // autoscaling no matter what number of replicas is provided), and if so, stop the scale loop and delete the HPA on the scaled object. + _, paused := scaledObject.GetAnnotations()[kedacontrollerutil.PausedReplicasAnnotation] + if paused { + logger.Info("ScaledObject is paused, so skipping the request.") + if err := r.stopScaleLoop(ctx, logger, scaledObject); err != nil { + return "Failed to stop the scale loop for paused ScaledObject", err + } + if deleted, err := r.ensureHPAForScaledObjectIsDeleted(ctx, logger, scaledObject); !deleted { + return "Failed to delete HPA for paused ScaledObject", err + } + return kedav1alpha1.ScaledObjectConditionReadySuccessMessage, nil + } + // Check scale target Name is specified if scaledObject.Spec.ScaleTargetRef.Name == "" { err := fmt.Errorf("ScaledObject.spec.scaleTargetRef.name is missing") @@ -438,6 +452,31 @@ func (r *ScaledObjectReconciler) ensureHPAForScaledObjectExists(ctx context.Cont return false, nil } +// ensureHPAForScaledObjectIsDeleted ensures that in cluster any HPA for specified ScaledObject is deleted, returns true if no HPA exists +func (r *ScaledObjectReconciler) ensureHPAForScaledObjectIsDeleted(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject) (bool, error) { + var hpaName string + if scaledObject.Status.HpaName != "" { + hpaName = scaledObject.Status.HpaName + } else { + hpaName = getHPAName(scaledObject) + } + foundHpa := &autoscalingv2.HorizontalPodAutoscaler{} + // Check if HPA for this ScaledObject already exists + err := r.Client.Get(ctx, types.NamespacedName{Name: hpaName, Namespace: scaledObject.Namespace}, foundHpa) + if err != nil && errors.IsNotFound(err) { + return true, nil + } else if err != nil { + logger.Error(err, "Failed to get HPA from cluster") + return false, err + } + + if err := r.deleteHPA(ctx, logger, scaledObject, foundHpa); err != nil { + logger.Error(err, "Failed to delete HPA from cluster") + return false, err + } + return true, nil +} + func isHpaRenamed(scaledObject *kedav1alpha1.ScaledObject, foundHpa *autoscalingv2.HorizontalPodAutoscaler) bool { // if HPA name defined in SO -> check if equals to the found HPA if scaledObject.Spec.Advanced != nil && scaledObject.Spec.Advanced.HorizontalPodAutoscalerConfig != nil && scaledObject.Spec.Advanced.HorizontalPodAutoscalerConfig.Name != "" { diff --git a/controllers/keda/scaledobject_controller_test.go b/controllers/keda/scaledobject_controller_test.go index 9d9dfa278ac..71edb8dccd7 100644 --- a/controllers/keda/scaledobject_controller_test.go +++ b/controllers/keda/scaledobject_controller_test.go @@ -35,6 +35,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" kedav1alpha1 "github.com/kedacore/keda/v2/apis/keda/v1alpha1" + kedacontrollerutil "github.com/kedacore/keda/v2/controllers/keda/util" "github.com/kedacore/keda/v2/pkg/mock/mock_client" "github.com/kedacore/keda/v2/pkg/mock/mock_scaling" "github.com/kedacore/keda/v2/pkg/scalers" @@ -839,7 +840,69 @@ var _ = Describe("ScaledObjectController", func() { return -1 } return len(hpa.Spec.Metrics) - }, 5*time.Second).Should(Equal(2)) + }, 1*time.Minute).Should(Equal(2)) + }) + + // Fix issue 4253 + It("deletes hpa when scaledobject has pause annotation", func() { + // Create the scaling target. + deploymentName := "to-be-paused-name" + soName := "so-" + deploymentName + err := k8sClient.Create(context.Background(), generateDeployment(deploymentName)) + Expect(err).ToNot(HaveOccurred()) + + // Create the ScaledObject without specifying name. + so := &kedav1alpha1.ScaledObject{ + ObjectMeta: metav1.ObjectMeta{ + Name: soName, + Namespace: "default", + Annotations: map[string]string{ + kedacontrollerutil.PausedReplicasAnnotation: "0", + }, + }, + Spec: kedav1alpha1.ScaledObjectSpec{ + ScaleTargetRef: &kedav1alpha1.ScaleTarget{ + Name: deploymentName, + }, + Advanced: &kedav1alpha1.AdvancedConfig{ + HorizontalPodAutoscalerConfig: &kedav1alpha1.HorizontalPodAutoscalerConfig{}, + }, + Triggers: []kedav1alpha1.ScaleTriggers{ + { + Type: "cron", + Metadata: map[string]string{ + "timezone": "UTC", + "start": "0 * * * *", + "end": "1 * * * *", + "desiredReplicas": "1", + }, + }, + }, + }, + } + err = k8sClient.Create(context.Background(), so) + Expect(err).ToNot(HaveOccurred()) + + // wait so's ready condition Ready + Eventually(func() metav1.ConditionStatus { + err := k8sClient.Get(context.Background(), types.NamespacedName{Name: soName, Namespace: "default"}, so) + if err != nil { + return metav1.ConditionUnknown + } + return so.Status.Conditions.GetReadyCondition().Status + }).Should(Equal(metav1.ConditionTrue)) + + // validate annotation is set correctly + err = k8sClient.Get(context.Background(), types.NamespacedName{Name: soName, Namespace: "default"}, so) + Expect(err).ToNot(HaveOccurred()) + _, paused := so.GetAnnotations()[kedacontrollerutil.PausedReplicasAnnotation] + Expect(paused).To(Equal(true)) + + // And validate that hpa is deleted. + hpa := &autoscalingv2.HorizontalPodAutoscaler{} + Eventually(func() error { + return k8sClient.Get(context.Background(), types.NamespacedName{Name: fmt.Sprintf("keda-hpa-%s", soName), Namespace: "default"}, hpa) + }).Should(HaveOccurred()) }) }) diff --git a/tests/internals/pause_scaling/pause_scaling_test.go b/tests/internals/pause_scaling/pause_scaling_test.go index cfa8d5ed288..131535a579e 100644 --- a/tests/internals/pause_scaling/pause_scaling_test.go +++ b/tests/internals/pause_scaling/pause_scaling_test.go @@ -26,6 +26,9 @@ var ( scaledObjectName = fmt.Sprintf("%s-so", testName) maxReplicaCount = 1 minReplicaCount = 0 + testScaleOutWaitMin = 3 + testPauseAtNWaitMin = 3 + testScaleInWaitMin = 3 ) type templateData struct { @@ -175,8 +178,8 @@ func testScaleOut(t *testing.T, kc *kubernetes.Clientset, data templateData) { t.Log("--- testing scale out ---") KubectlApplyWithTemplate(t, data, "scaledObjectTemplate", scaledObjectTemplate) - assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, maxReplicaCount, 60, 1), - "replica count should be 1 after 1 minute") + assert.Truef(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, maxReplicaCount, 60, testScaleOutWaitMin), + "replica count should be 1 after %d minute(s)", testScaleOutWaitMin) } func testPauseAtN(t *testing.T, kc *kubernetes.Clientset, data templateData, n int) { @@ -186,14 +189,14 @@ func testPauseAtN(t *testing.T, kc *kubernetes.Clientset, data templateData, n i KubernetesScaleDeployment(t, kc, monitoredDeploymentName, 0, testNamespace) - assert.Truef(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, n, 60, 1), - "replica count should be %d after 1 minute", n) + assert.Truef(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, n, 60, testPauseAtNWaitMin), + "replica count should be %d after %d minute(s)", n, testPauseAtNWaitMin) } func testScaleIn(t *testing.T, kc *kubernetes.Clientset, data templateData) { t.Log("--- testing scale in ---") KubectlApplyWithTemplate(t, data, "scaledObjectTemplate", scaledObjectTemplate) - assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, minReplicaCount, 60, 2), - "replica count should be 0 after 2 minutes") + assert.Truef(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, minReplicaCount, 60, testScaleInWaitMin), + "replica count should be 0 after %d minutes", testScaleInWaitMin) }