Browse Source

fix: disable the priority queue which misbehaves at scale (#6083)

Gergely Bräutigam 2 months ago
parent
commit
710c846d7d
3 changed files with 23 additions and 37 deletions
  1. 2 9
      cmd/controller/certcontroller.go
  2. 7 28
      cmd/controller/root.go
  3. 14 0
      pkg/controllers/common/common.go

+ 2 - 9
cmd/controller/certcontroller.go

@@ -32,7 +32,6 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	"sigs.k8s.io/controller-runtime/pkg/metrics/server"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -118,10 +117,7 @@ var certcontrollerCmd = &cobra.Command{
 				SecretNamespace: secretNamespace,
 				Resources:       crdNames,
 			})
-		if err := crdctrl.SetupWithManager(mgr, controller.Options{
-			MaxConcurrentReconciles: concurrent,
-			RateLimiter:             ctrlcommon.BuildRateLimiter(),
-		}); err != nil {
+		if err := crdctrl.SetupWithManager(mgr, ctrlcommon.BuildControllerOptions(concurrent)); err != nil {
 			setupLog.Error(err, errCreateController, "controller", "CustomResourceDefinition")
 			os.Exit(1)
 		}
@@ -135,10 +131,7 @@ var certcontrollerCmd = &cobra.Command{
 				SecretNamespace: secretNamespace,
 				RequeueInterval: crdRequeueInterval,
 			})
-		if err := whc.SetupWithManager(mgr, controller.Options{
-			MaxConcurrentReconciles: concurrent,
-			RateLimiter:             ctrlcommon.BuildRateLimiter(),
-		}); err != nil {
+		if err := whc.SetupWithManager(mgr, ctrlcommon.BuildControllerOptions(concurrent)); err != nil {
 			setupLog.Error(err, errCreateController, "controller", "WebhookConfig")
 			os.Exit(1)
 		}

+ 7 - 28
cmd/controller/root.go

@@ -30,7 +30,6 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
 	"sigs.k8s.io/controller-runtime/pkg/metrics/server"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -207,10 +206,7 @@ var rootCmd = &cobra.Command{
 				ControllerClass:   controllerClass,
 				RequeueInterval:   storeRequeueInterval,
 				PushSecretEnabled: enablePushSecretReconciler,
-			}).SetupWithManager(mgr, controller.Options{
-				MaxConcurrentReconciles: concurrent,
-				RateLimiter:             ctrlcommon.BuildRateLimiter(),
-			}); err != nil {
+			}).SetupWithManager(mgr, ctrlcommon.BuildControllerOptions(concurrent)); err != nil {
 				setupLog.Error(err, errCreateController, "controller", "SecretStore")
 				os.Exit(1)
 			}
@@ -225,10 +221,7 @@ var rootCmd = &cobra.Command{
 				ControllerClass:   controllerClass,
 				RequeueInterval:   storeRequeueInterval,
 				PushSecretEnabled: enablePushSecretReconciler,
-			}).SetupWithManager(mgr, controller.Options{
-				MaxConcurrentReconciles: concurrent,
-				RateLimiter:             ctrlcommon.BuildRateLimiter(),
-			}); err != nil {
+			}).SetupWithManager(mgr, ctrlcommon.BuildControllerOptions(concurrent)); err != nil {
 				setupLog.Error(err, errCreateController, "controller", "ClusterSecretStore")
 				os.Exit(1)
 			}
@@ -238,10 +231,7 @@ var rootCmd = &cobra.Command{
 			Log:        ctrl.Log.WithName("controllers").WithName("GeneratorState"),
 			Scheme:     mgr.GetScheme(),
 			RestConfig: mgr.GetConfig(),
-		}).SetupWithManager(mgr, controller.Options{
-			MaxConcurrentReconciles: concurrent,
-			RateLimiter:             ctrlcommon.BuildRateLimiter(),
-		}); err != nil {
+		}).SetupWithManager(mgr, ctrlcommon.BuildControllerOptions(concurrent)); err != nil {
 			setupLog.Error(err, errCreateController, "controller", "GeneratorState")
 			os.Exit(1)
 		}
@@ -257,10 +247,7 @@ var rootCmd = &cobra.Command{
 			EnableFloodGate:           enableFloodGate,
 			EnableGeneratorState:      enableGeneratorState,
 			AllowGenericTargets:       allowGenericTargets,
-		}).SetupWithManager(cmd.Context(), mgr, controller.Options{
-			MaxConcurrentReconciles: concurrent,
-			RateLimiter:             ctrlcommon.BuildRateLimiter(),
-		}); err != nil {
+		}).SetupWithManager(cmd.Context(), mgr, ctrlcommon.BuildControllerOptions(concurrent)); err != nil {
 			setupLog.Error(err, errCreateController, "controller", "ExternalSecret")
 			os.Exit(1)
 		}
@@ -273,10 +260,7 @@ var rootCmd = &cobra.Command{
 				ControllerClass: controllerClass,
 				RestConfig:      mgr.GetConfig(),
 				RequeueInterval: time.Hour,
-			}).SetupWithManager(cmd.Context(), mgr, controller.Options{
-				MaxConcurrentReconciles: concurrent,
-				RateLimiter:             ctrlcommon.BuildRateLimiter(),
-			}); err != nil {
+			}).SetupWithManager(cmd.Context(), mgr, ctrlcommon.BuildControllerOptions(concurrent)); err != nil {
 				setupLog.Error(err, errCreateController, "controller", "PushSecret")
 				os.Exit(1)
 			}
@@ -289,10 +273,7 @@ var rootCmd = &cobra.Command{
 				Log:             ctrl.Log.WithName("controllers").WithName("ClusterExternalSecret"),
 				Scheme:          mgr.GetScheme(),
 				RequeueInterval: time.Hour,
-			}).SetupWithManager(mgr, controller.Options{
-				MaxConcurrentReconciles: concurrent,
-				RateLimiter:             ctrlcommon.BuildRateLimiter(),
-			}); err != nil {
+			}).SetupWithManager(mgr, ctrlcommon.BuildControllerOptions(concurrent)); err != nil {
 				setupLog.Error(err, errCreateController, "controller", "ClusterExternalSecret")
 				os.Exit(1)
 			}
@@ -307,9 +288,7 @@ var rootCmd = &cobra.Command{
 				Scheme:          mgr.GetScheme(),
 				RequeueInterval: time.Hour,
 				Recorder:        mgr.GetEventRecorderFor("external-secrets-controller"),
-			}).SetupWithManager(mgr, controller.Options{
-				MaxConcurrentReconciles: concurrent,
-			}); err != nil {
+			}).SetupWithManager(mgr, ctrlcommon.BuildControllerOptions(concurrent)); err != nil {
 				setupLog.Error(err, errCreateController, "controller", "ClusterPushSecret")
 				os.Exit(1)
 			}

+ 14 - 0
pkg/controllers/common/common.go

@@ -26,9 +26,11 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/selection"
 	"k8s.io/client-go/util/workqueue"
+	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
@@ -95,6 +97,18 @@ func BuildManagedSecretClient(mgr ctrl.Manager, namespace string) (client.Client
 	return secretClient, nil
 }
 
+// BuildControllerOptions creates controller options with the given concurrency
+// and our standard rate limiter. The priority queue introduced in
+// controller-runtime v0.23.0 is explicitly disabled because it has known
+// issues at scale (see https://github.com/external-secrets/external-secrets/issues/6053).
+func BuildControllerOptions(concurrent int) controller.Options {
+	return controller.Options{
+		MaxConcurrentReconciles: concurrent,
+		RateLimiter:             BuildRateLimiter(),
+		UsePriorityQueue:        ptr.To(false),
+	}
+}
+
 // BuildRateLimiter creates a new rate limiter for our controllers.
 // NOTE: we dont use `DefaultTypedControllerRateLimiter` because it retries very aggressively, starting at 5ms!
 func BuildRateLimiter() workqueue.TypedRateLimiter[reconcile.Request] {