Revise leader election logic for endpoints controller

[mateiidavid]

Revise leader election logic for endpoints controller

Our leader election logic can result in updates being missed under certain
conditions. Leases expire after their duration is up, even if their current
holder has been terminated. During this dead time, any changes in the
system will be observed by other controllers, but will not be written to
the API Server.

For example, during a rollout, a controller that has come up will not be
able to acquire the lease for a maximum time of 30 seconds (lease
duration). Within this time frame, any changes to the system (e.g. modified
workloads, services, deleted endpointslices) will be observed but not acted
on by the newly created controller. Once the controller gets into a bad
state, it can only recover after 10 minutes (via service resyncs) or if any
resources are modified.

To address this, we change our leader election mechanism. Instead of
pushing leader election to the edge (i.e. when performing writes) we only
allow events to be observed when a controller is leading (i.e. by
registering callbacks). When a controller stops leading, all of its
callbacks will be de-registered.

NOTE:

• controllers will have a grace period during which they can renew their
  lease. Their callbacks will be de-registered only if this fails. We will
  not register and de-register callbacks that often for a single
  controller.
• we do not lose out on any state. Other informers will continue to run
  (e.g. destination readers). When callbacks are registered, we pass all of
  the cached objects through them. In other words, we do not issue API
  requests on registration, we process the state of the cluster as observed
  from the cache.
• we make another change that's slightly orthogonal. Before we shutdown,
  we ensure to drain the queue. This should not be a race since we will
  first block until the queue is drained, then signal to the leader elector
  loop that we are done. This gives us some confidence that all events have
  been processed as soon as they were observed.

Signed-off-by: Matei David [email protected]

[zaharidichev]

There is going to be a problem here. Imagine the following sequence of events:

1. Controller acquire lease.
2. Creates endpoints for a service and external workload
3. Drops lease removes callback
4. Meanwhile endpoints membership is modified by another controller
5. Our controllers aquires the lease
6. We get an ADD for the service, external workload,etc
7. We try to do a reconciliation, but the state of the endpoints tracker is stale because we never saw the DELETE
8. We try to requeue but our state never recovers and we drop the update.
9. We end up in an inconsistent state.

The problem here is that you are keeping state in the endpointsslice tracker that might change under you feet. If you look at the upstream implementation, you will notice that we start with a new endpoitnsslices tracker each time we aquire the lease.

We need to do that here as well. Keeping this state around will get us into trouble.

[mateiidavid]

Great catch. The endpoint tracker does add a bit of complexity but the reasoning above is super compelling and likely to happen.

To fix, we wipe the tracker before callback registration happens. I've also added a test that exercises the scenario you described. I can get the test to fail without the aforementioned change.

[zaharidichev]

LGTM, with a minor comment to avoid races

[zaharidichev]

Obtain the lock here. Since addHandlers && removeHandlers are called from the leader callbacks, we should assume there is no strict synchronization here.

[mateiidavid]

ehh completely forgot to do it 🤦🏻 thx.

[zaharidichev]

Good!

[zaharidichev]

Very happy you have that here!

[alpeb]

Makes total sense!