Implement leaderelector.Interface in terms of the new leader elector client

[twz123]

Description

Leaderelector.LeasePool and K0sControllersLeaseCounter now use the new leader elector client internally. Deprecate all old interface methods in favor of CurrentStatus, which reports the current leader status along with a channel that is closed when the status changes. Callers should then call CurrentStatus again to get the new status.

This way, the leader elector doesn't have to keep track of callback methods, callers can't block each other, and there is no need to "deregister" when callers are no longer interested in leader status changes.

The old API is fully emulated via the new API, and can be easily removed once it's no longer being called.

As a PoC, convert the applier manager to the new API as the first component.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

How Has This Been Tested?

• Manual test
• Auto test added

Checklist:

• My code follows the style guidelines of this project
• My commit messages are signed-off
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• I have checked my code and corrected any misspellings

[jnummelin]

I don't seem to get this 🤔

Why are we running both acquired and lost callback at the same round? Is the ctx done when we lose the leadership? If so, maybe consider naming it somehow to reflect that, currently it looks too much like a "normal" context and thus hard to figure out the intention.

[twz123]

Is the ctx done when we lose the leadership?

Yes. This is the doc string on the RunLeaderTasks function:

// Runs the provided tasks function when the lead is taken. It continuously
// monitors the leader election status using statusFunc. When the lead is taken,
// the tasks function is called with a context that is cancelled either when the
// lead has been lost or ctx is done. After the tasks function returns, the
// process is repeated until ctx is done.

That function tries to encapsulate a common pattern: "Do something as long as you're holding the lease". When the context is done, this means that you should stop, no matter why. Could be because the lease got lost, could be because the parent context got cancelled for whichever reason (timeout, application shutdown, ...).

If so, maybe consider naming it somehow to reflect that, currently it looks too much like a "normal" context and thus hard to figure out the intention.

This indeed is is a normal context, there's nothing special about it. RunLeaderTasks just "adds" another cancellation cause to it (loosing the lease). Pretty much the same way as signal.NotifyContext can be used to "add" SIGINT/SIGTERM and friends as cancellation causes to a context.

That being said, do you have any suggestions on the naming, or how to structure it differently? We could add more explanatory comments, of course, and improve the RunLeaderTasks docstring. On the other hand, this exact function is sort of a compatibility layer anyways, as the plan is to completely replace the callbacks in the first place. Maybe that's why it's looking a bit weird. The new way would be to use RunLeaderTasks directly in the components that need it. The applier manager, for example, does this now:

leaderelection.RunLeaderTasks(ctx, m.LeaderElector.CurrentStatus, func(ctx context.Context) {
	wait.UntilWithContext(ctx, m.runWatchers, time.Minute)
})

This is kinda succinct and doesn't look that awkward, I think?

[jnummelin]

That being said, do you have any suggestions on the naming, or how to structure it differently?

I think the structure is quite ok. Just bit hard, at least for me, to grasp the context usage. Since we're essentially using the context to tell that we've lost the lease, maybe something like leaderContext would make it bit more obvious.

[github-actions]

The PR is marked as stale since no activity has been recorded in 30 days