From 5a549088a7fe159b57736791ebd151f8ec77a391 Mon Sep 17 00:00:00 2001 From: ycabrer <43866176+ycabrer@users.noreply.github.com> Date: Mon, 1 Mar 2021 09:16:53 -0700 Subject: [PATCH] Fix memory leak by checking triggers uniqueness properly (#1640) Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com> --- CHANGELOG.md | 1 + controllers/hpa.go | 8 ++- controllers/scaledobject_controller.go | 33 ---------- controllers/scaledobject_controller_test.go | 73 ++++++++++++++++++--- 4 files changed, 72 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d3f2f1bcf5..a40c4f4c5c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ - Fix a memory leak in kafka client and close push scalers ([#1565](https://github.com/kedacore/keda/issues/1565)) - Add 'Metadata' header to AAD podIdentity request ([#1566](https://github.com/kedacore/keda/issues/1566)) - KEDA should make sure generate correct labels for HPA ([#1630](https://github.com/kedacore/keda/issues/1630)) +- Fix memory leak by checking triggers uniqueness properly ([#1640](https://github.com/kedacore/keda/pull/1640)) ### Breaking Changes diff --git a/controllers/hpa.go b/controllers/hpa.go index 85d32dc0b50..40c9a50741a 100644 --- a/controllers/hpa.go +++ b/controllers/hpa.go @@ -156,11 +156,17 @@ func (r *ScaledObjectReconciler) getScaledObjectMetricSpecs(logger logr.Logger, if metricSpec.Resource != nil { resourceMetricNames = append(resourceMetricNames, string(metricSpec.Resource.Name)) } + if metricSpec.External != nil { + externalMetricName := metricSpec.External.Metric.Name + if kedacontrollerutil.Contains(externalMetricNames, externalMetricName) { + return nil, fmt.Errorf("metricName %s defined multiple times in ScaledObject %s, please refer the documentation how to define metricName manually", externalMetricName, scaledObject.Name) + } + // add the scaledObjectName label. This is how the MetricsAdapter will know which scaledobject a metric is for when the HPA queries it. metricSpec.External.Metric.Selector = &metav1.LabelSelector{MatchLabels: make(map[string]string)} metricSpec.External.Metric.Selector.MatchLabels["scaledObjectName"] = scaledObject.Name - externalMetricNames = append(externalMetricNames, metricSpec.External.Metric.Name) + externalMetricNames = append(externalMetricNames, externalMetricName) } } scaledObjectMetricSpecs = append(scaledObjectMetricSpecs, metricSpecs...) diff --git a/controllers/scaledobject_controller.go b/controllers/scaledobject_controller.go index e9321f5f131..5efa1ea60cd 100644 --- a/controllers/scaledobject_controller.go +++ b/controllers/scaledobject_controller.go @@ -201,11 +201,6 @@ func (r *ScaledObjectReconciler) reconcileScaledObject(logger logr.Logger, scale return "ScaledObject doesn't have correct scaleTargetRef specification", err } - err = r.validateMetricNameUniqueness(logger, scaledObject) - if err != nil { - return "Error checking metric name uniqueness", err - } - // Create a new HPA or update existing one according to ScaledObject newHPACreated, err := r.ensureHPAForScaledObjectExists(logger, scaledObject, &gvkr) if err != nil { @@ -251,34 +246,6 @@ func (r *ScaledObjectReconciler) ensureScaledObjectLabel(logger logr.Logger, sca return r.Client.Update(context.TODO(), scaledObject) } -func (r *ScaledObjectReconciler) validateMetricNameUniqueness(logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject) error { - scalers, err := r.scaleHandler.GetScalers(scaledObject) - if err != nil { - logger.Error(err, "Unable to fetch scalers in metric name uniqueness check") - return err - } - - observedMetricNames := make(map[string]struct{}) - for _, scaler := range scalers { - for _, metric := range scaler.GetMetricSpecForScaling() { - // Only validate external metricNames - if metric.External == nil { - continue - } - - metricName := metric.External.Metric.Name - if _, ok := observedMetricNames[metricName]; ok { - return fmt.Errorf("metricName %s defined multiple times in ScaledObject %s, please refer the documentation how to define metircName manually", metricName, scaledObject.Name) - } - - observedMetricNames[metricName] = struct{}{} - } - } - - logger.V(1).Info("All metric names are unique in ScaledObject", "value", scaledObject.Name) - return nil -} - // checkTargetResourceIsScalable checks if resource targeted for scaling exists and exposes /scale subresource func (r *ScaledObjectReconciler) checkTargetResourceIsScalable(logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject) (kedav1alpha1.GroupVersionKindResource, error) { gvkr, err := kedautil.ParseGVKR(r.restMapper, scaledObject.Spec.ScaleTargetRef.APIVersion, scaledObject.Spec.ScaleTargetRef.Kind) diff --git a/controllers/scaledobject_controller_test.go b/controllers/scaledobject_controller_test.go index 513c4c8c878..0c33209564f 100644 --- a/controllers/scaledobject_controller_test.go +++ b/controllers/scaledobject_controller_test.go @@ -5,6 +5,7 @@ import ( "github.com/golang/mock/gomock" 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" @@ -31,6 +32,8 @@ var _ = Describe("ScaledObjectController", func() { var ( metricNameTestReconciler ScaledObjectReconciler mockScaleHandler *mock_scaling.MockScaleHandler + mockClient *mock_client.MockClient + mockStatusWriter *mock_client.MockStatusWriter ) var triggerMeta []map[string]string = []map[string]string{ @@ -39,18 +42,25 @@ var _ = Describe("ScaledObjectController", func() { } BeforeEach(func() { - mockScaleHandler = mock_scaling.NewMockScaleHandler(gomock.NewController(GinkgoTestReporter{})) + ctrl := gomock.NewController(GinkgoTestReporter{}) + mockScaleHandler = mock_scaling.NewMockScaleHandler(ctrl) + mockClient = mock_client.NewMockClient(ctrl) + mockStatusWriter = mock_client.NewMockStatusWriter(ctrl) metricNameTestReconciler = ScaledObjectReconciler{ scaleHandler: mockScaleHandler, + Client: mockClient, } }) Context("With Unique Values", func() { - var uniqueNamedScaledObjectTrigger = &kedav1alpha1.ScaledObject{} + var uniquelyNamedScaledObject = &kedav1alpha1.ScaledObject{} It("should pass metric name validation", func() { + // Generate test data testScalers := make([]scalers.Scaler, 0) + expectedExternalMetricNames := make([]string, 0) + for i, tm := range triggerMeta { config := &scalers.ScalerConfig{ Name: fmt.Sprintf("test.%d", i), @@ -66,14 +76,33 @@ var _ = Describe("ScaledObjectController", func() { } testScalers = append(testScalers, s) + for _, metricSpec := range s.GetMetricSpecForScaling() { + if metricSpec.External != nil { + expectedExternalMetricNames = append(expectedExternalMetricNames, metricSpec.External.Metric.Name) + } + } } - mockScaleHandler.EXPECT().GetScalers(uniqueNamedScaledObjectTrigger).Return(testScalers, nil) + // Set up expectations + mockScaleHandler.EXPECT().GetScalers(uniquelyNamedScaledObject).Return(testScalers, nil) + mockClient.EXPECT().Status().Return(mockStatusWriter) + mockStatusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()) + + // Call function to be tested + metricSpecs, err := metricNameTestReconciler.getScaledObjectMetricSpecs(testLogger, uniquelyNamedScaledObject) - Ω(metricNameTestReconciler.validateMetricNameUniqueness(testLogger, uniqueNamedScaledObjectTrigger)).Should(BeNil()) + // Test that the status was updated with metric names + Ω(uniquelyNamedScaledObject.Status.ExternalMetricNames).Should(Equal(expectedExternalMetricNames)) + + // Test returned values + Ω(len(metricSpecs)).Should(Equal(len(testScalers))) + Ω(err).Should(BeNil()) }) It("should pass metric name validation with single value", func() { + // Generate test data + expectedExternalMetricNames := make([]string, 0) + config := &scalers.ScalerConfig{ Name: "test", Namespace: "test", @@ -86,17 +115,34 @@ var _ = Describe("ScaledObjectController", func() { if err != nil { Fail(err.Error()) } + for _, metricSpec := range s.GetMetricSpecForScaling() { + if metricSpec.External != nil { + expectedExternalMetricNames = append(expectedExternalMetricNames, metricSpec.External.Metric.Name) + } + } + + // Set up expectations + mockScaleHandler.EXPECT().GetScalers(uniquelyNamedScaledObject).Return([]scalers.Scaler{s}, nil) + mockClient.EXPECT().Status().Return(mockStatusWriter) + mockStatusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()) + + // Call function to be tested + metricSpecs, err := metricNameTestReconciler.getScaledObjectMetricSpecs(testLogger, uniquelyNamedScaledObject) - mockScaleHandler.EXPECT().GetScalers(uniqueNamedScaledObjectTrigger).Return([]scalers.Scaler{s}, nil) + // Test that the status was updated + Ω(uniquelyNamedScaledObject.Status.ExternalMetricNames).Should(Equal(expectedExternalMetricNames)) - Ω(metricNameTestReconciler.validateMetricNameUniqueness(testLogger, uniqueNamedScaledObjectTrigger)).Should(BeNil()) + // Test returned values + Ω(len(metricSpecs)).Should(Equal(1)) + Ω(err).Should(BeNil()) }) }) Context("With Duplicate Values", func() { - var duplicateNamedScaledObjectTrigger = &kedav1alpha1.ScaledObject{} + var duplicateNamedScaledObject = &kedav1alpha1.ScaledObject{} It("should pass metric name validation", func() { + // Generate test data testScalers := make([]scalers.Scaler, 0) for i := 0; i < 4; i++ { config := &scalers.ScalerConfig{ @@ -115,9 +161,18 @@ var _ = Describe("ScaledObjectController", func() { testScalers = append(testScalers, s) } - mockScaleHandler.EXPECT().GetScalers(duplicateNamedScaledObjectTrigger).Return(testScalers, nil) + // Set up expectations + mockScaleHandler.EXPECT().GetScalers(duplicateNamedScaledObject).Return(testScalers, nil) + + // Call function tobe tested + metricSpecs, err := metricNameTestReconciler.getScaledObjectMetricSpecs(testLogger, duplicateNamedScaledObject) + + // Test that the status was not updated + Ω(duplicateNamedScaledObject.Status.ExternalMetricNames).Should(BeNil()) - Ω(metricNameTestReconciler.validateMetricNameUniqueness(testLogger, duplicateNamedScaledObjectTrigger)).ShouldNot(BeNil()) + // Test returned values + Ω(metricSpecs).Should(BeNil()) + Ω(err).ShouldNot(BeNil()) }) }) })