Repository: metallb/metallb
Pull request #1791 (2023-01-20 16:06:00)

Config Controller: ignore changes that are not relevant
The configuration controller is triggered by multiple events, including all the secrets in the metallb's namespace. This cause a cascade effect of reprocessing all the services which is not necessary.

Here we compare the last processed configuration with the one we just produced (which takes into account of non used secrets and non used namespaces, for example), limiting configuration reload events to those that really are changing the metallb's internal configuration.

On top of that, in order to keep the node label test effective, the implementation of the reconciliation function is deferred to a local function that can be overridden by the tests.

Fixes https://github.com/metallb/metallb/issues/1770

liornoy: Left a couple of comments, regarding the node selector tests, I don't understand what you mean by `to keep the node label test effective`. Are those changes required because the changes of the reconciler broke them?
fedepaol: > Left a couple of comments, regarding the node selector tests, I don't understand what you mean by `to keep the node label test effective`. Are those changes required because the changes of the reconciler broke them?

it didn't "broke them" but the handler wasn't being called anymore because the configuration wasn't changing
liornoy: > > Left a couple of comments, regarding the node selector tests, I don't understand what you mean by `to keep the node label test effective`. Are those changes required because the changes of the reconciler broke them?
> 
> it didn't "broke them" but the handler wasn't being called anymore because the configuration wasn't changing

Thanks for the clarification. 
pupseba: Hi. Is there any estimated date for when this improvement will hit master and be released in a new version?
fedepaol: > Hi. Is there any estimated date for when this improvement will hit master and be released in a new version?

@pupseba no promises, but aiming at merging by end of this week and doing a new release by end of next week

Patch:
diff --git a/internal/k8s/controllers/config_controller.go b/internal/k8s/controllers/config_controller.go
index d783f93e..8a9702fe 100644
--- a/internal/k8s/controllers/config_controller.go
+++ b/internal/k8s/controllers/config_controller.go
@@ -18,6 +18,7 @@ package controllers
 
 import (
 	"context"
+	"reflect"
 
 	"github.com/go-kit/log"
 	"github.com/go-kit/log/level"
@@ -44,9 +45,14 @@ type ConfigReconciler struct {
 	ValidateConfig config.Validate
 	ForceReload    func()
 	BGPType        string
+	currentConfig  *config.Config
 }
 
 func (r *ConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+	return requestHandler(r, ctx, req)
+}
+
+var requestHandler = func(r *ConfigReconciler, ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	level.Info(r.Logger).Log("controller", "ConfigReconciler", "start reconcile", req.NamespacedName.String())
 	defer level.Info(r.Logger).Log("controller", "ConfigReconciler", "end reconcile", req.NamespacedName.String())
 	updates.Inc()
@@ -133,12 +139,22 @@ func (r *ConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	}
 
 	level.Debug(r.Logger).Log("controller", "ConfigReconciler", "rendered config", dumpConfig(cfg))
+	if r.currentConfig != nil && reflect.DeepEqual(r.currentConfig, cfg) {
+		level.Debug(r.Logger).Log("controller", "ConfigReconciler", "event", "configuration did not change, ignoring")
+		return ctrl.Result{}, nil
+	}
+
+	r.currentConfig = cfg
 
 	res := r.Handler(r.Logger, cfg)
 	switch res {
 	case SyncStateError:
 		configStale.Set(1)
 		updateErrors.Inc()
+		// if the configuration load failed, we reset the current config because this is gonna lead to a retry
+		// of the reconciliaton loop. If we don't reset, the retry will find the config identical and will exit,
+		// which is not what we want here.
+		r.currentConfig = nil
 		level.Error(r.Logger).Log("controller", "ConfigReconciler", "metallb CRs and Secrets", dumpClusterResources(&resources), "event", "reload failed, retry")
 		return ctrl.Result{}, retryError
 	case SyncStateReprocessAll:


Test patch:
diff --git a/internal/k8s/controllers/config_controller_test.go b/internal/k8s/controllers/config_controller_test.go
index e22b0b53..0b48cce9 100644
--- a/internal/k8s/controllers/config_controller_test.go
+++ b/internal/k8s/controllers/config_controller_test.go
@@ -36,6 +36,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	k8sscheme "k8s.io/client-go/kubernetes/scheme"
+	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -144,6 +145,80 @@ func TestConfigController(t *testing.T) {
 	}
 }
 
+func TestSecretShouldntTrigger(t *testing.T) {
+	initObjects := objectsFromResources(configControllerValidResources)
+	fakeClient, err := newFakeClient(initObjects)
+	if err != nil {
+		t.Fatalf("test failed to create fake client: %v", err)
+	}
+
+	handlerCalled := false
+	mockHandler := func(l log.Logger, cfg *config.Config) SyncState {
+		handlerCalled = true
+		return SyncStateSuccess
+	}
+
+	r := &ConfigReconciler{
+		Client:         fakeClient,
+		Logger:         log.NewNopLogger(),
+		Scheme:         scheme,
+		Namespace:      testNamespace,
+		ValidateConfig: config.DontValidate,
+		Handler:        mockHandler,
+		ForceReload:    func() {},
+	}
+	req := reconcile.Request{
+		NamespacedName: types.NamespacedName{
+			Namespace: testNamespace,
+		},
+	}
+
+	_, err = r.Reconcile(context.TODO(), req)
+	if err != nil {
+		t.Fatalf("reconcile failed: %v", err)
+	}
+	if !handlerCalled {
+		t.Fatalf("handler not called")
+	}
+	handlerCalled = false
+	err = fakeClient.Create(context.TODO(), &v1beta2.BGPPeer{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "peer2",
+			Namespace: testNamespace,
+		},
+		Spec: v1beta2.BGPPeerSpec{
+			MyASN:      42,
+			ASN:        142,
+			Address:    "1.2.3.4",
+			BFDProfile: "default",
+		},
+	})
+	if err != nil {
+		t.Fatalf("create failed on peer2: %v", err)
+	}
+	_, err = r.Reconcile(context.TODO(), req)
+	if err != nil {
+		t.Fatalf("reconcile failed: %v", err)
+	}
+	if !handlerCalled {
+		t.Fatalf("handler not called")
+	}
+
+	handlerCalled = false
+	err = fakeClient.Create(context.TODO(), &corev1.Secret{Type: corev1.SecretTypeBasicAuth, ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: testNamespace},
+		Data: map[string][]byte{"password": []byte([]byte("nopass"))}})
+	if err != nil {
+		t.Fatalf("create failed on secret foo: %v", err)
+	}
+	_, err = r.Reconcile(context.TODO(), req)
+	if err != nil {
+		t.Fatalf("reconcile failed: %v", err)
+	}
+	if handlerCalled {
+		t.Fatalf("handler called")
+	}
+}
+
 func TestNodeEvent(t *testing.T) {
 	g := NewGomegaWithT(t)
 	testEnv := &envtest.Environment{
@@ -166,26 +241,22 @@ func TestNodeEvent(t *testing.T) {
 
 	var configUpdate int
 	var mutex sync.Mutex
-	mockHandler := func(l log.Logger, cfg *config.Config) SyncState {
+	oldRequestHandler := requestHandler
+	defer func() { requestHandler = oldRequestHandler }()
+
+	requestHandler = func(r *ConfigReconciler, ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 		mutex.Lock()
 		defer mutex.Unlock()
 		configUpdate++
-		return SyncStateSuccess
-	}
-	var forceReload int
-	mockForceReload := func() {
-		mutex.Lock()
-		defer mutex.Unlock()
-		forceReload++
+		return ctrl.Result{}, nil
 	}
+
 	r := &ConfigReconciler{
 		Client:         m.GetClient(),
 		Logger:         log.NewNopLogger(),
 		Scheme:         scheme,
 		Namespace:      testNamespace,
 		ValidateConfig: config.DontValidate,
-		Handler:        mockHandler,
-		ForceReload:    mockForceReload,
 	}
 	err = r.SetupWithManager(m)
 	g.Expect(err).To(BeNil())
@@ -221,11 +292,6 @@ func TestNodeEvent(t *testing.T) {
 		defer mutex.Unlock()
 		return configUpdate
 	}, 5*time.Second, 200*time.Millisecond).Should(Equal(initialConfigUpdateCount + 1))
-	g.Eventually(func() int {
-		mutex.Lock()
-		defer mutex.Unlock()
-		return forceReload
-	}, 5*time.Second, 200*time.Millisecond).Should(Equal(0))
 
 	// test update node event with no changes into node label.
 	g.Eventually(func() error {
@@ -247,11 +313,6 @@ func TestNodeEvent(t *testing.T) {
 		defer mutex.Unlock()
 		return configUpdate
 	}, 5*time.Second, 200*time.Millisecond).Should(Equal(initialConfigUpdateCount + 1))
-	g.Eventually(func() int {
-		mutex.Lock()
-		defer mutex.Unlock()
-		return forceReload
-	}, 5*time.Second, 200*time.Millisecond).Should(Equal(0))
 
 	// test update node event with changes into node label.
 	g.Eventually(func() error {
@@ -273,11 +334,7 @@ func TestNodeEvent(t *testing.T) {
 		defer mutex.Unlock()
 		return configUpdate
 	}, 5*time.Second, 200*time.Millisecond).Should(Equal(initialConfigUpdateCount + 2))
-	g.Eventually(func() int {
-		mutex.Lock()
-		defer mutex.Unlock()
-		return forceReload
-	}, 5*time.Second, 200*time.Millisecond).Should(Equal(0))
+
 }
 
 var (