Trigger reprocess all event only when pool changes

[mlguerrero12]

This prevents the pool controller to trigger the reprocess all event when there are no changes in the pools.

[liornoy]

Maybe we should check the other way around, if is DeepEqual then return SyncStateSuccess,
with a log similar to what we have in the SetBalancer:
level.Debug(l).Log("event", "noChange", "msg", "service converged, no change")
else continue to execute the c.ips.SetPools and return Reprocess.
This is just to avoid running the c.ips.SetPools on the same/unchanged pools

[mlguerrero12]

I had a reason to always run c.ips.SetPools but I can't remember what it was.

Will change it as you suggest. Thank you.

[mlguerrero12]

done

[liornoy]

it's just a suggestion, but maybe don't negate the DeepEqual if statement,
so we won't have to branch if inside an if and make it easier to follow.
i.e. if the pools are equal we throw a log and return success, else
continue like before.

[mlguerrero12]

I prefer the last return to be a success

[liornoy]

you can also say that the Reprocess state is the happy path, as it was previously,
anyways it is unclear to me in what scenario would we reconcile and call the pool controller handler when the pool didn't change

[fedepaol]

it is unclear to me in what scenario would we reconcile and call the pool controller handler when the pool didn't change

This is a valid point too. The only scenario I can think of is when we do that is because a community entry changes but is not part of a legacy addresspool, which is a pretty cornery case.

[mlguerrero12]

yeah, mainly changes on community and namespaces not related to existing pools

[liornoy]

but the community resource is translated into a BGPadv (both legacy and non) and that's part of the pool, no?

[mlguerrero12]

community affects the legacy pools. BGPadv are part of the pools indeed but in the pools controller they are always empty

[fedepaol]

In the configController we have this part on the higher level controller. I'd rather do the same here

metallb/internal/k8s/controllers/config_controller.go

Line 156 in 5385d5c

if r.currentConfig != nil && reflect.DeepEqual(r.currentConfig, cfg) {

[mlguerrero12]

yeah, that will require more changes. I would need to add a new parameter in the PoolReconciler structure to keep track of the pools. Perhaps in another PR.

[fedepaol]

I might be missing something, but why comparing the rendered configs is not enough? At the end of the day it will contain only the pools.

[mlguerrero12]

the config controller has a filed to store the current config, the pool controller does not

[fedepaol]

Right, but it's about copying the config controller logic.
If we want this kind of optimization, we should be consistent.

[mlguerrero12]

I agree with the consistency. But perhaps the config controller should be the one that needs to be changed. I don't know. I'll study it soon. The only thing I don't like is to have (and maintain) many variables with pool. We have pools in the controller structure, allocator structure and now we are thinking about adding a new one for the pool controller structure.

[fedepaol]

I am suggesting not to keep only the pool, but do exactly the same as we do in the config controller, which is keeping an old version of the config and compare the rendered one (which yes, it will contain the pools).

I don't think it's a problem from readibility point of view, the config controller seems straightforward. We maintain a current config, check if changed and discard if it did not.

[mlguerrero12]

I changed it as you suggested

[fedepaol]

lgtm, thanks!