🐛 Fix incorrect handling when removing from the end of the triggers array

CHANGELOG.md

@@ -30,6 +30,7 @@
 - Apache Kafka Scaler: Add `allowIdleConsumers` to the list of trigger parameters ([#1684](https://github.com/kedacore/keda/pull/1684))
 - Fixed goroutine leaks in usage of timers ([#1704](https://github.com/kedacore/keda/pull/1704) | [#1739](https://github.com/kedacore/keda/pull/1739))
 - Setting timeouts in the HTTP client used by the IBM MQ scaler ([#1758](https://github.com/kedacore/keda/pull/1758))
+- Fix cleanup of removed triggers ([#1768](https://github.com/kedacore/keda/pull/1768))
 
 ### Breaking Changes
 

controllers/hpa.go

@@ -111,7 +111,8 @@ func (r *ScaledObjectReconciler) updateHPAIfNeeded(logger logr.Logger, scaledObj
 		return err
 	}
 
-	if !equality.Semantic.DeepDerivative(hpa.Spec, foundHpa.Spec) {
+	// DeepDerivative ignores extra entries in arrays which makes removing the last trigger not update things, so trigger and update any time the metrics count is different.
+	if len(hpa.Spec.Metrics) != len(foundHpa.Spec.Metrics) || !equality.Semantic.DeepDerivative(hpa.Spec, foundHpa.Spec) {
 		logger.V(1).Info("Found difference in the HPA spec accordint to ScaledObject", "currentHPA", foundHpa.Spec, "newHPA", hpa.Spec)
 		if r.Client.Update(context.TODO(), hpa) != nil {
 			foundHpa.Spec = hpa.Spec

controllers/scaledobject_controller_test.go

@@ -1,16 +1,23 @@
 package controllers
 
 import (
+	"context"
 	"fmt"
 
 	"github.com/golang/mock/gomock"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	appsv1 "k8s.io/api/apps/v1"
+	autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+
 	kedav1alpha1 "github.com/kedacore/keda/v2/api/v1alpha1"
 	"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"
-	. "github.com/onsi/ginkgo"
-	. "github.com/onsi/gomega"
-	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 )
 
 type GinkgoTestReporter struct{}
@@ -176,4 +183,100 @@ var _ = Describe("ScaledObjectController", func() {
 			})
 		})
 	})
+
+	Describe("functional tests", func() {
+		var deployment *appsv1.Deployment
+
+		BeforeEach(func() {
+			deployment = &appsv1.Deployment{
+				ObjectMeta: metav1.ObjectMeta{Name: "myapp", Namespace: "default"},
+				Spec: appsv1.DeploymentSpec{
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"app": "myapp",
+						},
+					},
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"app": "myapp",
+							},
+						},
+						Spec: corev1.PodSpec{
+							Containers: []corev1.Container{
+								{
+									Name:  "app",
+									Image: "app",
+								},
+							},
+						},
+					},
+				},
+			}
+		})
+
+		It("cleans up a deleted trigger from the HPA", func() {
+			// Create the scaling target.
+			err := k8sClient.Create(context.Background(), deployment)
+			Expect(err).ToNot(HaveOccurred())
+
+			// Create the ScaledObject with two triggers.
+			so := &kedav1alpha1.ScaledObject{
+				ObjectMeta: metav1.ObjectMeta{Name: "clean-up-test", Namespace: "default"},
+				Spec: kedav1alpha1.ScaledObjectSpec{
+					ScaleTargetRef: &kedav1alpha1.ScaleTarget{
+						Name: "myapp",
+					},
+					Triggers: []kedav1alpha1.ScaleTriggers{
+						{
+							Type: "cron",
+							Metadata: map[string]string{
+								"timezone":        "UTC",
+								"start":           "0 * * * *",
+								"end":             "1 * * * *",
+								"desiredReplicas": "1",
+							},
+						},
+						{
+							Type: "cron",
+							Metadata: map[string]string{
+								"timezone":        "UTC",
+								"start":           "2 * * * *",
+								"end":             "3 * * * *",
+								"desiredReplicas": "2",
+							},
+						},
+					},
+				},
+			}
+			err = k8sClient.Create(context.Background(), so)
+			Expect(err).ToNot(HaveOccurred())
+
+			// Get and confirm the HPA.
+			hpa := &autoscalingv2beta2.HorizontalPodAutoscaler{}
+			Eventually(func() error {
+				return k8sClient.Get(context.Background(), types.NamespacedName{Name: "keda-hpa-clean-up-test", Namespace: "default"}, hpa)
+			}).ShouldNot(HaveOccurred())
+			Expect(hpa.Spec.Metrics).To(HaveLen(2))
+			Expect(hpa.Spec.Metrics[0].External.Metric.Name).To(Equal("cron-UTC-0xxxx-1xxxx"))
+			Expect(hpa.Spec.Metrics[1].External.Metric.Name).To(Equal("cron-UTC-2xxxx-3xxxx"))
+
+			// Remove the second trigger.
+			Eventually(func() error {
+				err = k8sClient.Get(context.Background(), types.NamespacedName{Name: "clean-up-test", Namespace: "default"}, so)
+				Expect(err).ToNot(HaveOccurred())
+				so.Spec.Triggers = so.Spec.Triggers[:1]
+				return k8sClient.Update(context.Background(), so)
+			}).ShouldNot(HaveOccurred())
+
+			// Wait until the HPA is updated.
+			Eventually(func() int {
+				err = k8sClient.Get(context.Background(), types.NamespacedName{Name: "keda-hpa-clean-up-test", Namespace: "default"}, hpa)
+				Expect(err).ToNot(HaveOccurred())
+				return len(hpa.Spec.Metrics)
+			}).Should(Equal(1))
+			// And it should only be the first one left.
+			Expect(hpa.Spec.Metrics[0].External.Metric.Name).To(Equal("cron-UTC-0xxxx-1xxxx"))
+		})
+	})
 })

controllers/suite_test.go

@@ -23,13 +23,16 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 
-	// "k8s.io/client-go/kubernetes/scheme"
-
+	"k8s.io/client-go/kubernetes/scheme"
+	"k8s.io/client-go/rest"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	"sigs.k8s.io/controller-runtime/pkg/envtest/printer"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
-	// kedav1alpha1 "github.com/kedacore/keda/v2/api/v1alpha1"
+
+	kedav1alpha1 "github.com/kedacore/keda/v2/api/v1alpha1"
 	// +kubebuilder:scaffold:imports
 )
 
@@ -38,7 +41,9 @@ import (
 // These tests use Ginkgo (BDD-style Go testing framework). Refer to
 // http://onsi.github.io/ginkgo/ to learn more about Ginkgo.
 
+var cfg *rest.Config
 var testEnv *envtest.Environment
+var k8sClient client.Client
 
 func TestAPIs(t *testing.T) {
 	RegisterFailHandler(Fail)
@@ -56,31 +61,43 @@ var _ = BeforeSuite(func(done Done) {
 		CRDDirectoryPaths: []string{filepath.Join("..", "config", "crd", "bases")},
 	}
 
-	// var err error
-	// cfg, err = testEnv.Start()
-	// Expect(err).ToNot(HaveOccurred())
-	// Expect(cfg).ToNot(BeNil())
+	var err error
+	cfg, err = testEnv.Start()
+	Expect(err).ToNot(HaveOccurred())
+	Expect(cfg).ToNot(BeNil())
 
-	// err = kedav1alpha1.AddToScheme(scheme.Scheme)
-	// Expect(err).NotTo(HaveOccurred())
+	err = kedav1alpha1.AddToScheme(scheme.Scheme)
+	Expect(err).NotTo(HaveOccurred())
 
-	// err = kedav1alpha1.AddToScheme(scheme.Scheme)
-	// Expect(err).NotTo(HaveOccurred())
+	// +kubebuilder:scaffold:scheme
 
-	// err = kedav1alpha1.AddToScheme(scheme.Scheme)
-	// Expect(err).NotTo(HaveOccurred())
+	k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
+		Scheme: scheme.Scheme,
+	})
+	Expect(err).ToNot(HaveOccurred())
 
-	// +kubebuilder:scaffold:scheme
+	err = (&ScaledObjectReconciler{
+		Client:   k8sManager.GetClient(),
+		Scheme:   k8sManager.GetScheme(),
+		Recorder: k8sManager.GetEventRecorderFor("keda-operator"),
+		Log:      ctrl.Log.WithName("controllers").WithName("ScaledObject"),
+	}).SetupWithManager(k8sManager)
+	Expect(err).ToNot(HaveOccurred())
+
+	k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
+	Expect(err).ToNot(HaveOccurred())
+	Expect(k8sClient).ToNot(BeNil())
 
-	// k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
-	// Expect(err).ToNot(HaveOccurred())
-	// Expect(k8sClient).ToNot(BeNil())
+	go func() {
+		err = k8sManager.Start(ctrl.SetupSignalHandler())
+		Expect(err).ToNot(HaveOccurred())
+	}()
 
 	close(done)
 }, 60)
 
 var _ = AfterSuite(func() {
-	// By("tearing down the test environment")
-	// err := testEnv.Stop()
-	// Expect(err).ToNot(HaveOccurred())
+	By("tearing down the test environment")
+	err := testEnv.Stop()
+	Expect(err).ToNot(HaveOccurred())
 })