Added hpa update predicate to reconcile a scaledobject only if hpa sp…

…ec,label or annotation is changed (kedacore#5282) Signed-off-by: deefreak <deepakgts2@gmail.com> Signed-off-by: anton.lysina <alysina@gmail.com>
toniiiik · Jan 15, 2024 · 26bfcf1 · 26bfcf1
1 parent 7c6876b
commit 26bfcf1
Show file tree

Hide file tree

Showing 4 changed files with 290 additions and 1 deletion.
diff --git a/controllers/keda/hpa.go b/controllers/keda/hpa.go
@@ -175,6 +175,17 @@ func (r *ScaledObjectReconciler) updateHPAIfNeeded(ctx context.Context, logger l
 		logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)
 	}
 
+	if (hpa.ObjectMeta.Annotations == nil && foundHpa.ObjectMeta.Annotations != nil) ||
+		!equality.Semantic.DeepDerivative(hpa.ObjectMeta.Annotations, foundHpa.ObjectMeta.Annotations) {
+		logger.V(1).Info("Found difference in the HPA annotations according to ScaledObject", "currentHPA", foundHpa.ObjectMeta.Annotations, "newHPA", hpa.ObjectMeta.Annotations)
+		if err = r.Client.Update(ctx, hpa); err != nil {
+			foundHpa.ObjectMeta.Annotations = hpa.ObjectMeta.Annotations
+			logger.Error(err, "Failed to update HPA", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)
+			return err
+		}
+		logger.Info("Updated HPA according to ScaledObject", "HPA.Namespace", foundHpa.Namespace, "HPA.Name", foundHpa.Name)
+	}
+
 	return nil
 }
 

diff --git a/controllers/keda/scaledobject_controller.go b/controllers/keda/scaledobject_controller.go
@@ -132,7 +132,14 @@ func (r *ScaledObjectReconciler) SetupWithManager(mgr ctrl.Manager, options cont
 				predicate.GenerationChangedPredicate{},
 			),
 		)).
-		Owns(&autoscalingv2.HorizontalPodAutoscaler{}).
+		// Trigger a reconcile only when the HPA spec,label or annotation changes.
+		// Ignore updates to HPA status
+		Owns(&autoscalingv2.HorizontalPodAutoscaler{}, builder.WithPredicates(
+			predicate.Or(
+				predicate.LabelChangedPredicate{},
+				predicate.AnnotationChangedPredicate{},
+				kedacontrollerutil.HPASpecChangedPredicate{},
+			))).
 		Complete(r)
 }
 

diff --git a/controllers/keda/scaledobject_controller_test.go b/controllers/keda/scaledobject_controller_test.go
@@ -1028,6 +1028,264 @@ var _ = Describe("ScaledObjectController", func() {
 			return k8sClient.Get(context.Background(), types.NamespacedName{Name: fmt.Sprintf("keda-hpa-%s", soName), Namespace: "default"}, hpa)
 		}).Should(HaveOccurred())
 	})
+
+	// Fix issue 5281
+	It("reconciles scaledobject when hpa spec is changed", func() {
+		var (
+			deploymentName        = "hpa-spec-change"
+			soName                = "so-" + deploymentName
+			min             int32 = 1
+			max             int32 = 5
+			newMin          int32 = 2
+			newMax          int32 = 6
+			pollingInterVal int32 = 1
+		)
+
+		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",
+			},
+			Spec: kedav1alpha1.ScaledObjectSpec{
+				ScaleTargetRef: &kedav1alpha1.ScaleTarget{
+					Name: deploymentName,
+				},
+				MinReplicaCount: &min,
+				MaxReplicaCount: &max,
+				PollingInterval: &pollingInterVal,
+				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())
+
+		// And validate that hpa is created.
+		hpa := &autoscalingv2.HorizontalPodAutoscaler{}
+		Eventually(func() error {
+			return k8sClient.Get(context.Background(), types.NamespacedName{Name: fmt.Sprintf("keda-hpa-%s", soName), Namespace: "default"}, hpa)
+		}).ShouldNot(HaveOccurred())
+
+		// Change hpa spec and update
+		hpa.Spec.MinReplicas = &newMin
+		hpa.Spec.MaxReplicas = newMax
+		err = k8sClient.Update(context.Background(), hpa)
+		Expect(err).ToNot(HaveOccurred())
+
+		// scaledobject should be reconciled and hpa spec should match with scaledobject spec
+		Eventually(func() bool {
+			err = k8sClient.Get(context.Background(), types.NamespacedName{Name: fmt.Sprintf("keda-hpa-%s", soName), Namespace: "default"}, hpa)
+			if err != nil {
+				return false
+			}
+			return *hpa.Spec.MinReplicas == min && hpa.Spec.MaxReplicas == max
+		}).Should(BeTrue())
+	})
+
+	It("reconciles scaledobject when hpa label is changed", func() {
+		var (
+			deploymentName = "hpa-label-change"
+			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",
+			},
+			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())
+
+		// And validate that hpa is created.
+		hpa := &autoscalingv2.HorizontalPodAutoscaler{}
+		Eventually(func() error {
+			return k8sClient.Get(context.Background(), types.NamespacedName{Name: fmt.Sprintf("keda-hpa-%s", soName), Namespace: "default"}, hpa)
+		}).ShouldNot(HaveOccurred())
+
+		// Add a new label to the hpa and update
+		hpa.ObjectMeta.Labels = map[string]string{"new-label": "new-label-value"}
+		err = k8sClient.Update(context.Background(), hpa)
+		Expect(err).ToNot(HaveOccurred())
+
+		// scaledobject should be reconciled and hpa should not contain this manually added label
+		Eventually(func() bool {
+			err = k8sClient.Get(context.Background(), types.NamespacedName{Name: fmt.Sprintf("keda-hpa-%s", soName), Namespace: "default"}, hpa)
+			if err != nil {
+				return false
+			}
+			// Check if the label is not present
+			if _, ok := hpa.ObjectMeta.Labels["new-label"]; !ok {
+				return true
+			}
+			return false
+		}).Should(BeTrue())
+	})
+
+	It("reconciles scaledobject when hpa annotation is changed", func() {
+		var (
+			deploymentName = "hpa-annotation-change"
+			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",
+			},
+			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())
+
+		// And validate that hpa is created.
+		hpa := &autoscalingv2.HorizontalPodAutoscaler{}
+		Eventually(func() error {
+			return k8sClient.Get(context.Background(), types.NamespacedName{Name: fmt.Sprintf("keda-hpa-%s", soName), Namespace: "default"}, hpa)
+		}).ShouldNot(HaveOccurred())
+
+		// Add a new annotation to the hpa and update
+		hpa.ObjectMeta.Annotations = map[string]string{"new-annotation": "new-annotation-value"}
+		err = k8sClient.Update(context.Background(), hpa)
+		Expect(err).ToNot(HaveOccurred())
+
+		// scaledobject should be reconciled and hpa should not contain this manually added annotation
+		Eventually(func() bool {
+			err = k8sClient.Get(context.Background(), types.NamespacedName{Name: fmt.Sprintf("keda-hpa-%s", soName), Namespace: "default"}, hpa)
+			if err != nil {
+				return false
+			}
+			// Check if the annotation is not present
+			if _, ok := hpa.ObjectMeta.Annotations["new-annotation"]; !ok {
+				return true
+			}
+			return false
+		}, 5*time.Second).Should(BeTrue())
+	})
+
+	// Fix issue 5281
+	It("reconciles scaledobject and creates hpa when child hpa is deleted", func() {
+		var (
+			deploymentName        = "hpa-deleted"
+			soName                = "so-" + deploymentName
+			min             int32 = 1
+			max             int32 = 5
+			pollingInterVal int32 = 1
+		)
+
+		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",
+			},
+			Spec: kedav1alpha1.ScaledObjectSpec{
+				ScaleTargetRef: &kedav1alpha1.ScaleTarget{
+					Name: deploymentName,
+				},
+				MinReplicaCount: &min,
+				MaxReplicaCount: &max,
+				PollingInterval: &pollingInterVal,
+				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())
+
+		// And validate that hpa is created.
+		hpa := &autoscalingv2.HorizontalPodAutoscaler{}
+		Eventually(func() error {
+			return k8sClient.Get(context.Background(), types.NamespacedName{Name: fmt.Sprintf("keda-hpa-%s", soName), Namespace: "default"}, hpa)
+		}).ShouldNot(HaveOccurred())
+
+		// Delete the child hpa
+		err = k8sClient.Delete(context.Background(), hpa)
+		Expect(err).ToNot(HaveOccurred())
+
+		// scaledobject should be reconciled and again the corresponding hpa should be created
+		Eventually(func() error {
+			return k8sClient.Get(context.Background(), types.NamespacedName{Name: fmt.Sprintf("keda-hpa-%s", soName), Namespace: "default"}, hpa)
+
+		}).Should(BeNil())
+	})
+
 })
 
 func generateDeployment(name string) *appsv1.Deployment {

diff --git a/controllers/keda/util/predicate.go b/controllers/keda/util/predicate.go
@@ -1,6 +1,8 @@
 package util
 
 import (
+	autoscalingv2 "k8s.io/api/autoscaling/v2"
+	"k8s.io/apimachinery/pkg/api/equality"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
@@ -89,3 +91,14 @@ func (PausedPredicate) Update(e event.UpdateEvent) bool {
 
 	return newPausedValue != oldPausedValue
 }
+
+type HPASpecChangedPredicate struct {
+	predicate.Funcs
+}
+
+func (HPASpecChangedPredicate) Update(e event.UpdateEvent) bool {
+	newObj := e.ObjectNew.(*autoscalingv2.HorizontalPodAutoscaler)
+	oldObj := e.ObjectOld.(*autoscalingv2.HorizontalPodAutoscaler)
+
+	return len(newObj.Spec.Metrics) != len(oldObj.Spec.Metrics) || !equality.Semantic.DeepDerivative(newObj.Spec, oldObj.Spec)
+}