Make controller preserve internal state after reboot

[liornoy]

This PR changes the behavior of the service reconciler
to fix the following bug:

• There is an LB service with a specific IP (annotated) assigned to it.
• Also there are other LB services in the cluster on "pending".
• MetalLB's controller resets and when it goes back up again,
  it loops over the services, sees first the "pending" LB service,
  and assigns it the IP that was assigned to the annotated service.

Here we make the reconciler ignore the services, up until the first
reprocessAll event, where we handle only the services with IP assigned
to them already.

fixes #1984

[fedepaol]

I haven't checked the tests yet but a few comments on the implementation. The implementation works, but it's a bit hacky and could be made more explicit by:

• letting the pool handler understand if it's the first time we receive a configuration (

  metallb/controller/main.go

  Line 115 in 1a49c30

  func (c *controller) SetPools(l log.Logger, pools *config.Pools) controllers.SyncState {

  )
• adding a new SyncState value (reprocessAssignedFirst)
• propagating the information (reloadAssignedFirst) through the channel adding a field to the reloadEvent

  metallb/internal/k8s/controllers/service_controller_reload.go

  Line 34 in 5385d5c

  type reloadEvent struct {

• handling the flag accordingly in the reprocessAll function

This has the advantage that the flow is clear(er), and the controller implementation is in charge of the mechanism

[liornoy]

I haven't checked the tests yet but a few comments on the implementation. The implementation works, but it's a bit hacky and could be made more explicit by:

• letting the pool handler understand if it's the first time we receive a configuration (

  metallb/controller/main.go

  Line 115 in 1a49c30

  func (c *controller) SetPools(l log.Logger, pools *config.Pools) controllers.SyncState {

  
  )
• adding a new SyncState value (reprocessAssignedFirst)
• propagating the information (reloadAssignedFirst) through the channel adding a field to the reloadEvent

  metallb/internal/k8s/controllers/service_controller_reload.go

  Line 34 in 5385d5c

  type reloadEvent struct {

• handling the flag accordingly in the reprocessAll function

This has the advantage that the flow is clear(er), and the controller implementation is in charge of the mechanism

@fedepaol without using some kind of manual boolean "isFirstReload" flag, how would you make the pool handler understand if it's the first time we receive a configuration?

I mean, the first time we run SetPools the pools aren't empty.
A different approach is inspecting the c.ips but still, having no allocated ips is a valid state even not the first time.

[fedepaol]

This has the advantage that the flow is clear(er), and the controller implementation is in charge of the mechanism

@fedepaol without using some kind of manual boolean "isFirstReload" flag, how would you make the pool handler understand if it's the first time we receive a configuration?

Having the flag is legit I guess, something like "firstConfiguration" to be checked and reset together when we get the first non nil set of pools.

[liornoy]

This has the advantage that the flow is clear(er), and the controller implementation is in charge of the mechanism

@fedepaol without using some kind of manual boolean "isFirstReload" flag, how would you make the pool handler understand if it's the first time we receive a configuration?

Having the flag is legit I guess, something like "firstConfiguration" to be checked and reset together when we get the first non nil set of pools.

Ok, thanks for the reply. I was just making sure that this flag wasn't the hacky element of the implementation.

[fedepaol]

This has the advantage that the flow is clear(er), and the controller implementation is in charge of the mechanism

@fedepaol without using some kind of manual boolean "isFirstReload" flag, how would you make the pool handler understand if it's the first time we receive a configuration?

Having the flag is legit I guess, something like "firstConfiguration" to be checked and reset together when we get the first non nil set of pools.

Ok, thanks for the reply. I was just making sure that this flag wasn't the hacky element of the implementation.

Nope, what I think is hacky is doing a sequence of actions from the wrapping site (the controller) which has no control on how the items are handled

[fedepaol]

As discussed offline, another possible solution is to always loop over the services with an IP first, and those without the IP afterwards. This will remove a lot of the complexity of understanding when to do what, but it has the downside of having this ad hoc logic in the wrapping controller instead of inside the business logic.
Thoughts?
Also, @oribon wdyt?

[liornoy]

I'll expand on this and say that we drift from the approach of signaling the "firstReload" from within the ReloadEvent struct because it is not normally reachable from the reconcile function and forcing it there will add unnecessary complexity.

[oribon]

few nits, will go over the e2es later

[liornoy]

@oribon the ci failures here are being worked on:
actions/runner-images#8080 (comment)

[fedepaol]

lgtm, thanks for fixing this!