From f056fe5cc6c435dacb372713baaa4ba5464d0acc Mon Sep 17 00:00:00 2001 From: Noah Kantrowitz Date: Fri, 25 Dec 2020 21:41:22 -0800 Subject: [PATCH 1/2] Add a cache so not every reconcile has to do a GET to the /scale endpoint, even when the data isn't needed. Signed-off-by: Noah Kantrowitz --- CHANGELOG.md | 1 + controllers/scaledobject_controller.go | 51 +++++++++++++++++++------- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c8a3d1d1b8a..22e806e20e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ - Mask password in postgres scaler auto generated metricName. ([PR #1381](https://github.com/kedacore/keda/pull/1381)) - Bug fix for pending jobs in ScaledJob's accurateScalingStrategy . ([#1323](https://github.com/kedacore/keda/issues/1323)) - Fix memory leak because of unclosed scalers. ([#1413](https://github.com/kedacore/keda/issues/1413)) +- Reduce unnecessary /scale requests from ScaledObject controller ([#1453](https://github.com/kedacore/keda/pull/1453)) ### Breaking Changes diff --git a/controllers/scaledobject_controller.go b/controllers/scaledobject_controller.go index 3a249ac57df..1e8a25a81f1 100644 --- a/controllers/scaledobject_controller.go +++ b/controllers/scaledobject_controller.go @@ -7,6 +7,7 @@ import ( "time" "github.com/go-logr/logr" + autoscalingv1 "k8s.io/api/autoscaling/v1" autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" @@ -53,6 +54,16 @@ type ScaledObjectReconciler struct { globalHTTPTimeout time.Duration } +// A cache mapping "resource.group" to true or false if we know if this resource is scalable. +var isScalableCache map[string]bool + +func init() { + // Prefill the cache with some known values for core resources in case of future parallelism to avoid stampeding herd on startup. + isScalableCache = map[string]bool{ + "deploymen.apps": true, + } +} + // SetupWithManager initializes the ScaledObjectReconciler instance and starts a new controller managed by the passed Manager instance. func (r *ScaledObjectReconciler) SetupWithManager(mgr ctrl.Manager) error { // create Discovery clientset @@ -231,26 +242,38 @@ func (r *ScaledObjectReconciler) checkTargetResourceIsScalable(logger logr.Logge gvkString := gvkr.GVKString() logger.V(1).Info("Parsed Group, Version, Kind, Resource", "GVK", gvkString, "Resource", gvkr.Resource) - // let's try to detect /scale subresource - scale, errScale := (*r.scaleClient).Scales(scaledObject.Namespace).Get(context.TODO(), gvkr.GroupResource(), scaledObject.Spec.ScaleTargetRef.Name, metav1.GetOptions{}) - if errScale != nil { - // not able to get /scale subresource -> let's check if the resource even exist in the cluster - unstruct := &unstructured.Unstructured{} - unstruct.SetGroupVersionKind(gvkr.GroupVersionKind()) - if err := r.Client.Get(context.TODO(), client.ObjectKey{Namespace: scaledObject.Namespace, Name: scaledObject.Spec.ScaleTargetRef.Name}, unstruct); err != nil { - // resource doesn't exist - logger.Error(err, "Target resource doesn't exist", "resource", gvkString, "name", scaledObject.Spec.ScaleTargetRef.Name) - return gvkr, err + // do we need the scale to update the status later? + wantStatusUpdate := scaledObject.Status.ScaleTargetKind != gvkString || scaledObject.Status.OriginalReplicaCount == nil + + // check if we already know. + var scale *autoscalingv1.Scale + gr := gvkr.GroupResource() + isScalable := isScalableCache[gr.String()] + if !isScalable || wantStatusUpdate { + // not cached, let's try to detect /scale subresource + // also rechecks when we need to update the status. + var errScale error + scale, errScale = (*r.scaleClient).Scales(scaledObject.Namespace).Get(context.TODO(), gr, scaledObject.Spec.ScaleTargetRef.Name, metav1.GetOptions{}) + if errScale != nil { + // not able to get /scale subresource -> let's check if the resource even exist in the cluster + unstruct := &unstructured.Unstructured{} + unstruct.SetGroupVersionKind(gvkr.GroupVersionKind()) + if err := r.Client.Get(context.TODO(), client.ObjectKey{Namespace: scaledObject.Namespace, Name: scaledObject.Spec.ScaleTargetRef.Name}, unstruct); err != nil { + // resource doesn't exist + logger.Error(err, "Target resource doesn't exist", "resource", gvkString, "name", scaledObject.Spec.ScaleTargetRef.Name) + return gvkr, err + } + // resource exist but doesn't expose /scale subresource + logger.Error(errScale, "Target resource doesn't expose /scale subresource", "resource", gvkString, "name", scaledObject.Spec.ScaleTargetRef.Name) + return gvkr, errScale } - // resource exist but doesn't expose /scale subresource - logger.Error(errScale, "Target resource doesn't expose /scale subresource", "resource", gvkString, "name", scaledObject.Spec.ScaleTargetRef.Name) - return gvkr, errScale + isScalableCache[gr.String()] = true } // if it is not already present in ScaledObject Status: // - store discovered GVK and GVKR // - store original scaleTarget's replica count (before scaling with KEDA) - if scaledObject.Status.ScaleTargetKind != gvkString || scaledObject.Status.OriginalReplicaCount == nil { + if wantStatusUpdate { status := scaledObject.Status.DeepCopy() if scaledObject.Status.ScaleTargetKind != gvkString { status.ScaleTargetKind = gvkString From 521b03866a549f77158a9377aad3542a775fecd0 Mon Sep 17 00:00:00 2001 From: Noah Kantrowitz Date: Mon, 28 Dec 2020 12:03:36 -0800 Subject: [PATCH 2/2] Add statefulsets to the prefill list too. Signed-off-by: Noah Kantrowitz --- controllers/scaledobject_controller.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/controllers/scaledobject_controller.go b/controllers/scaledobject_controller.go index 1e8a25a81f1..182b04eed5c 100644 --- a/controllers/scaledobject_controller.go +++ b/controllers/scaledobject_controller.go @@ -60,7 +60,8 @@ var isScalableCache map[string]bool func init() { // Prefill the cache with some known values for core resources in case of future parallelism to avoid stampeding herd on startup. isScalableCache = map[string]bool{ - "deploymen.apps": true, + "deployments.apps": true, + "statefusets.apps": true, } }