Config Controller: ignore changes that are not relevant #1791
Conversation
fedepaol
requested review from
rata,
russellb,
gclawes and
oribon
as code owners
fedepaol
force-pushed
the
ignorechanges
branch
2 times, most recently
from
69e2c38
to
d1532d9
Compare
| @@ -133,7 +139,12 @@ func (r *ConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr | |||
| } | |||
|
|
|||
| level.Debug(r.Logger).Log("controller", "ConfigReconciler", "rendered config", dumpConfig(cfg)) | |||
| if r.oldConfig != nil && reflect.DeepEqual(r.oldConfig, cfg) { | |||
| level.Error(r.Logger).Log("controller", "ConfigReconciler", "event", "configuration did not change, ignoring") | |||
There was a problem hiding this comment.
Should we have level.Info here instead?
There was a problem hiding this comment.
this could even be debug imo, unless you expect this to not show so often?
| 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") | ||
| } |
There was a problem hiding this comment.
Why do we need the second reconciler call with the bgppeer in this test?
There was a problem hiding this comment.
I wanted to ensure that if the configuration is changed, the handler is 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"))}}) | ||
|
|
There was a problem hiding this comment.
nit: extra newline between the err = .... and the if err != nil
There was a problem hiding this comment.
the line above is on multiple lines, having it attached seems a bit cramped.
|
Left a couple of comments, regarding the node selector tests, I don't understand what you mean by |
it didn't "broke them" but the handler wasn't being called anymore because the configuration wasn't changing |
Thanks for the clarification. |
|
Hi. Is there any estimated date for when this improvement will hit master and be released in a new version? |
|
|
||
| r.oldConfig = cfg |
There was a problem hiding this comment.
maybe I'm missing something, but isn't setting this before the Handler problematic in the sense that if the Handler fails with a retry error the config would be ignored in the next reconcile (because of the new DeepEqual conditional)?
There was a problem hiding this comment.
no the consideration is totally fair. Let me change it so we set the current only when the current is processed successfuly
| @@ -133,7 +139,12 @@ func (r *ConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr | |||
| } | |||
|
|
|||
| level.Debug(r.Logger).Log("controller", "ConfigReconciler", "rendered config", dumpConfig(cfg)) | |||
| if r.oldConfig != nil && reflect.DeepEqual(r.oldConfig, cfg) { | |||
| level.Error(r.Logger).Log("controller", "ConfigReconciler", "event", "configuration did not change, ignoring") | |||
There was a problem hiding this comment.
this could even be debug imo, unless you expect this to not show so often?
| @@ -44,9 +45,14 @@ type ConfigReconciler struct { | |||
| ValidateConfig config.Validate | |||
| ForceReload func() | |||
| BGPType string | |||
| oldConfig *config.Config | |||
There was a problem hiding this comment.
nit: isn't this more currentConfig?
fedepaol
force-pushed
the
ignorechanges
branch
from
d1532d9
to
45e640a
Compare
@pupseba no promises, but aiming at merging by end of this week and doing a new release by end of next week |
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. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
fedepaol
force-pushed
the
ignorechanges
branch
from
45e640a
to
4d7df7d
Compare
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 #1770