Improve LeaderElector

[twz123]

Is your feature request related to a problem? Please describe.

K0s's current implementation of the LeaderElector needs some thoughts and design decisions:

Interface/Contract

The interface in use is as follows:

k0s/pkg/component/controller/leaderelector.go

Lines 29 to 34 in 62bef64

	// LeaderElector is the common leader elector component to manage each controller leader status
	type LeaderElector interface {
	IsLeader() bool
	AddAcquiredLeaseCallback(fn func())
	AddLostLeaseCallback(fn func())
	}

1. How and when are callbacks invoked?

   1. Are they called in sequence, or concurrently, i.e. should those callbacks spawn their own goroutine if required, or are they already in a goroutine?
   2. Will the acquire callbacks be invoked concurrently to the lost callbacks? Is it the responsibility of the LeaderElector or the responsibility of the callback implementors to ensure that nothing is overlapping?
   3. Acquire and lost callbacks are added separately, not as pairs. This seems odd.

2. When it is allowed to add function callbacks?
   The interface looks like it's legit to add new callbacks at any time. This is not on par with the implementations. More on that below.

3. How to remove previously added callbacks?
   Other components register themselves with the LeaderElector, but they also do have their own lifecycle. What if they get stopped? There's currently no way to unregister from the LeaderElector again. So their callback would be invoked even after they have been stopped. It's not a big deal in the way that k0s is currently implemented, since the only situation in which things are stopped is on shutdown, which inherently means that everything is stopped. Still, this seems like a design flaw, since components registering themselves with the LeaderElector simply cannot be stopped correctly at the moment.

Implementations

There are currently two implementations of the LeaderElector interface, one dummy implementation that pretends that it's always leading, used for the --single controller, and the real thing, which is backed by client-go's leader elector, wrapped by k0s's "LeasePool". Both of them are Components, so they have a lifecycle. Besides the general points about the interface itself, I see two implementation-specific problems:

4. The dummy leader elector is too simple.
   It doesn't implement any lifecycle at all. The only lifecycle-aware thing that it does is that it's calling the acquire callbacks in its run method. It's neither doing similar things for its IsLeader method, which is unconditionally returning true, no matter if the component has just been created, it is started, or if it is stopped already, nor is it calling any lost callbacks when it's stopped.

5. Adding new callbacks to already running leader electors won't work as expected.
   The lease pool backed implementation will at least call them with the next matching event, i.e. if it's already leading, an acquire callback will only be called the next time the lease has been won, not directly after adding it. While this will be eventually be catching up in theory, in practice this means that it's not working, since the lease will be held for a long time, possibly as long as the whole controller process's lifetime. The dummy leader elector will not do anything with those newly added callbacks at all.

Describe the solution you would like

1. I tend to require that all callbacks be called in their own goroutine by all LeaderElectors. We can document that on the interface, the implementation is simple. The benefits are: Users of the LeaderElector don't have to spawn their own goroutine, just to make sure that they aren't blocking the LeaderElector, and the LeaderElector doesn't need to rely on the callbacks that they actually don't block it.

2. and 5.: With the current implementations, it's only possible to add callbacks before the leader electors have been started. I see two ways to improve on that:

   1. Implement callback addition correctly, no matter in which state the leader elector component is. This would be more versatile and allow for things like nested components, which are started and stopped depending on the cluster configuration.
   2. Error out or panic when trying to add callbacks when the leader electors are already running. This is not very elegant, doesn't really fit well to the LeaderElector contract and still requires components to be started "in the right order", but at least it catches problems early and makes them visible, instead of just continuing in a broken state.

3. Combine the acquire and lost callbacks into a single callback that gets a context which is cancelled when the lease gets lost, and return a "Registration" object, i.e. like so:

   type LeaderElector interface {
   	AddAcquiredLeaseCallback(callback func(context.Context)) Registration
   	// ...
   }
   
   type Registration interface {
   	Cancel()
   }

   This has several advantages. It natively combines the acquire/lost callback pairs into a single callback function, making it easier for both the LeaderElector implementation and callback implementors. The LeaderElector has only one callback type that needs to be managed, instead of two, and there's no need to group callbacks into pairs. The implementor has a ready-to-be-used context at its disposal, and doesn't need to setup something on its own, which it then cancels when the lost callback is invoked. Even straight forward implementations can just block on <-ctx.Done() to wait for the lease to be lost. This is actually the technique that's also used inside the vanilla client-go leader elector implementation, but it gets covered up by LeasePool. Having a single callback function makes it easier to define expectations around concurrent calls of the callbacks. It seems easy enough for the LeaderElector implementation to be able to ensure that each callback is never invoked concurrently, i.e. the next invocation only happens after the previous one returned. The Cancel method can be used to unregister the previously added callback, and allows for a clean way to implement component stopping.

4. Replace the dummy leader elector with the real implementation, except that the internal LeasePool will be a "fake" one. That way, both implementation behave the same from a component/lifecycle point of view, i.e. callback registrationm dispatching and so on.

Describe alternatives you've considered

No response

Additional context

No response

[mikhail-sakhnov]

Do we have any bugs that would be fixed or features that are blocked by lack of implementation?
I am trying to understand, how deep should we go here. Since we do not use those callbacks anywhere apart from stack applier, I see only additional complexity from redesigning leader elector significantly.

[twz123]

I ran into this during the node-local load balancer experiments, where some newly added components didn't receive any callbacks because of this. I can probably work-around this by adding some quirks, but it was definitely unexpected behavior that required some debugging.

This will probably become a problem with dynamic configuration, when components might be started/stopped independently from a k0s restart.

[mikhail-sakhnov]

I ran into this during the node-local load balancer experiments, where some newly added components didn't receive any callbacks because of this. I can probably work-around this by adding some quirks, but it was definitely unexpected behavior that required some debugging.

yeah, that's fair, better to fix it rather than implementing quirks in-place...

This will probably become a problem with dynamic configuration, when components might be started/stopped independently from a k0s restart.

You mean it will become a problem because we can't register new callbacks after start, right?

So I see here two issues described:

• we want to allow adding callbacks after leader elector had started
• we want dummy elector act the same way as real elector from the callback management perspective

I am slightly against two suggestions, but it is generally minor

• it is usually possible to make async function from sync (by just adding go call()) so I'd keep goroutine management out of leader-elector
• I think it might be easier, if instead of returning interface Registration we return function (the same, as for example, context package does in std lib)

[twz123]

This will probably become a problem with dynamic configuration, when components might be started/stopped independently from a k0s restart.

You mean it will become a problem because we can't register new callbacks after start, right?

We can't register new callbacks after start, and we cannot unregister from the callbacks on stop, meaning that stopped components may still receive unwanted callbacks.

I am slightly against two suggestions, but it is generally minor

• it is usually possible to make async function from sync (by just adding go call()) so I'd keep goroutine management out of leader-elector

This is concerning point 1, right? I'm fine with that, too. I proposed that so the LeaderElector would be more be more resilient because we can rule out that some misbehaving LeaderElector client blocks inside the callback, hence blocking the whole LeaderElector.

• I think it might be easier, if instead of returning interface Registration we return function (the same, as for example, context package does in std lib)

Something like this?

type UnregisterFunc func()

[jnummelin]

Not going into too deep details (I trust you guys :) ), but I think what we essentially want (from random components perspective) is sort of pub-sub pattern where there's one actual leader election going on and it provides the state changes as events, right? IF so, maybe we could utilize channels (one channel per "registration") for the events?

[twz123]

Not going into too deep details (I trust you guys :) ), but I think what we essentially want (from random components perspective) is sort of pub-sub pattern where there's one actual leader election going on and it provides the state changes as events, right? IF so, maybe we could utilize channels (one channel per "registration") for the events?

A <-chan bool could work. Whichever boolean is received last via that channel represents the current state of the leader status. Closing of the channel could indicate that the LeaderElector has been stopped.

The interface could look like this (not quite confident with the naming yet):

type UnregisterFunc func()
type LeaderElector interface {
	NewLeaderChan() (<-chan bool, UnregisterFunc)
}

Looks a bit awkward at first glance, but technically I think it could work like that, too. A problem could be the buffer size of the channel, when events aren't consumed properly, which would eventually block the LeaderElector, quite similar as in the direct callback case.

[mikhail-sakhnov]

I have some prejudice against using channels for signaling unless it is strictly required, may be there is a way to do signaling based on context?

Another option is to have blocking function on elector, something like that (the snippet is mostly pseudocode)

wdyt, folks?

type UnregisterFunc func(context.Context)
type CallbackFunc func(context.Context)

type LeaderElector interface {
	AddComponent(callback CallbackFunc) UnregisterFunc 
	Wait(context.Context)
}

elector := NewElector(...)

unregs := make([]UnregisterFunc, 0)

// adding some callbacks
unregs = append(unregs, elector.AddComponent(...))


elector.Wait(ctx) // blocks until it lost the leadership

// delete everything, as example how to delete
for _, unregC := range unregs {
   unregC(cbCtx)
}

[twz123]

Not sure I follow. How would you use it from inside of a component? The current design works in a way that a component registers itself with a LeaderElector, typically during the startup phase.

[makhov]

Well, I see an issue only with dynamic configuration, where we need to add and remove callbacks on the fly. So the easiest way will be to extend the existing interface by adding a function to remove callback and start running callback on addition in case the leaderElector is running.

 // LeaderElector is the common leader elector component to manage each controller leader status 
 type LeaderElector interface { 
 	IsLeader() bool 
 	AddAcquiredLeaseCallback(name string, fn func()) 
 	AddLostLeaseCallback(name string, fn func()) 
        RemoveLeaseCallback(name string)
 } 

func (l *LeasePoolLeaderElector) AddAcquiredLeaseCallback(name string, fn func()) {
	l.acquiredLeaseCallbacks[name] = fn
        if l.isRunning() and l.IsLeader() {
            go fn()
        }
}