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

…array (#1768)

* 🎨 Revive the functional test harness and add a regression test for #1699.

Test is currently failing, as expected.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>

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

DeepDerivative considers it fine if the found HPA has extra elements in the array as long as the ones that are there in the computed HPA match the found. In our case, that causes updating the HPA to be skipped if you remove the last trigger (or any number as long as they are at the end). Checking the lengths first is quick and should cover all of these wonky cases.

Fixes #1699.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>

* 📝 Update CHANGELOG.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>

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())
 })