RFC: Per-reconciler leader election (and leader-aware reconcilers)

[mattmoor]

/area API
/kind cleanup

I've been thinking a bit about leader election, and the way we run things today we basically allocate N backup pods that wait to become the leader and effectively do nothing until they become the leader, they are essentially there as scheduling reservations.

How we achieve this is by wrapping the entire controller bootstrap process in a giant resource lock, which kicks things off when it acquires the lock and terminates the process if it should lose it. This means that our Reconcilers get no benefit from horizontal scaling because no load is spread, and in fact they pay a penalty of having to also run the leader election logic as well.

Given 10 reconcilers living in a single controller, by leader electing at the pod level, leader failovers result in a standby pod spinning up 10 reconcilers worth of state before they can become functional again. Given that we build none of that state in advance, it doesn't seem like a particularly hot failover.

---

The tl;dr of what I've been considering is this:

1. Have per controller.Impl / Reconciler leaders
2. Run all of the informers and controllers, eliding* reconciliation when not the leader.
3. When we become the leader, global resync.

* - We may want to run leaderelection-aware reconcilers even when not the leader, e.g. if the resources program routing information for an admission control webhook.

The cons of this approach are:

• Higher resource utilization for non-leader pods because they have the full informer cache.
• A multiple on the number of leader locks we would need, and a corresponding increase in API Server load.

The pros of this approach are:

• Leaders can be spread across replicas, so instead of having 10 on the single leader, if I run 3 replicas, I might only run 3-4 reconcilers per Pod. So our controllers start to see benefits from horizontal scaling.
• Hot failovers, which best case cost O(nop global resync). This is amplified by the above because generally fewer things should be failing over each time a pod fails.

---

I think it'd be interesting to try this with our webhooks, which don't currently perform leader election, and in some cases (e.g. Bindings), need the informer cache to program the webhook. If we decide to move ahead with this approach, then I think we should probably integrate with the // +genreconciler work, which I believe should have all of the information to do all(?) of the heavy lifting for (now) the vast majority of our reconcilers.

I'd love to try this out, but my schedule is chaotic and I don't want to be a bottleneck if others have time and are interested.

cc @pmorie @markusthoemmes @dprotaso @vaikas @vagababov for thoughts (LMK if I am missing any pros/cons)

[lionelvillard]

Here some random thoughts:

• how long does it take for a pod with 10 controllers to be functional? If it less than 2 or 3 seconds, then plain k8s pod restart might be faster than Knative HA feature. Do we know?
• for worker node upgrade scenario where graceful pod termination is possible, the active pod can wait for the passive pod to become active before terminating. In that case, there is no need for hot failover. BTW, I don't think our controllers support this scenario, do they?

[markusthoemmes]

@lionelvillard they do via leader election. There's more to it apart from just hot failover though. For instance, it "reserves" (as-in: runs) a second pod to make sure capacity constraints don't cause the pod to not be able to come up again after a crash.

[mattmoor]

how long does it take for a pod with 10 controllers to be functional?

This is what we have today (disabled by default), and part of my concern is that this is strictly more expensive than a global resync.

then plain k8s pod restart might be faster

This cannot possibly be true because when the Pod restarts it needs to start the same 10 controllers.

for worker node upgrade scenario where graceful pod termination is possible, the active pod can wait for the passive pod to become active before terminating

This is orthogonal, but plausible. I think we could achieve this by directing the leaderelection code to have the leader stop renewing its lease once it gets the interrupt. It will keep reconciling until it becomes the leader, and once it isn't the leader (in this proposal, anywhere) it can terminate.

[lionelvillard]

By faster I meant that k8s pod restart kicks in before Knative leaders have time to react. I did some experiments with the MT ping source adapter, reconciling 1000 resources (fairly small I admit), and most (all?) of time the passive pod never becomes active after the active pod is gracefully killed.

Should I open an issue for the worker node update scenario?

[vagababov]

In general, this makes sense.
Though the way losing lease now leads to a restart makes it a bit awkward (do we still restart? but the other controllers did not lose their lease? etc).
Also we can probably move all this to codegen to generate regular and failover aware controllers.

[mattmoor]

Losing the lease would just revert to the same behavior as before we acquired it.

[pmorie]

One principle I think we should adopt: I am game to evaluate this possibility, but I think we should agree not to write our own LE utilities.

I am not personally aware of anyone who has used the upstream LE code from client-go in this manner. Does anyone have examples? I have only seen (that I can readibly remember) the pattern I recreated in the sharedmain pkg where the controller exits once the leader election lock is lost and is restarted in order to try becoming the leader again. If anyone can point to examples of a different pattern in the wild, I would be very interested to see them.

In my view, since I have not seen examples of others doing this, it seems like we would be using the upstream LE code indiosyncratically (ie, we are the only ones with this use case). Potential risks:

• We may hit corner cases or scenarios not covered by test or current field usage
• We may have trouble driving fixes into upstream for (1) if our use in Knative is perceived as 'not as designed'

I'm really not clear what we think the run-time tradeoff here is:

• Leaders can be spread across replicas, so instead of having 10 on the single leader, if I run 3 replicas, I might only run 3-4 reconcilers per Pod. So our controllers start to see benefits from horizontal scaling.

I'm not sure exactly what benefits would be here - can you elaborate? Are we planning the number of leader elected reconcilers per replica? Leaving it to probability? Do we expect performance benefits of some kind? Do we still have to request resources necessary to run all loops simultaneously?

• Hot failovers, which best case cost O(nop global resync). This is amplified by the above because generally fewer things should be failing over each time a pod fails.

No question that the current behavior is a cold start, I think we can look at failover speed separately. I previously had wondered to myself how much work it would be to prime informer caches outside the leader election lock, and run the callback only once the leader election lock is obtained. We will need to ensure that is possible in order to implement your idea anyway.

[pmorie]

Also, would we be trying to solve this problem if we had not adopted the pattern of running the stateful reconciler and stateless webhook for the same resource in the same process? I think the idea is still valid to explore if we had not made that choice, but the fact that we made the choice appears to be what is supplying our need to do this.

I think it's worth thinking about the cost associated with engineering this idea vs return (how much resources saved, what specific performance gained with likely scale factors) vs separating webhook and reconciler and running reconciler HA.

[mattmoor]

Leaving it to probability?

I figured we'd start here, but we can always get smarter.

would we be trying to solve this problem if we had not adopted the pattern of running the stateful reconciler and stateless webhook for the same resource in the same process

Very possibly. The way I see leaderelection as we have it, we gain effectively nothing by horizontally scaling the controllers other than prescheduling pods. The goal of the proposed mechanism is to reduce the scope of failovers to a subset of reconcilers running on failed nodes, and to enable us to "warm up" a lot more of our controller infrastructure for faster failovers.

I also don't view what our webhooks are doing today as "stateful" per se, the informer cache is just that: a cache. We cache to avoid the latency / load of hitting the API server on every request, and (aside from potentially stale caching) which replica you hit has no relevance.

The places our reconcilers write to the API Server today (async from the request path) is likely contentious, but of the ones we run the only state that meaningfully distinguishes between the multiple threads we run vs. horizontally scaled pods is the workqueue logic to avoid simultaneous key processing (and shared caches). I think for the cases we're talking about I worry about conflicting updates (which we should try and avoid thru leader election), but not competing controllers that spin loop fighting each other in perpetuity.

Contrast this with the KPA reconciler, where multiple replicas may build up different views of the Metric data, and fight over how large the Deployment should be. 😬I get this facet of why horizontally scaling the controllers is hard.

I think it's worth thinking about the cost associated with engineering this idea vs return

I guess what's not sitting well with me is that the "return" on horizontally scaling our controllers is still SPOFs with pretty cold failovers. I agree with Lionel's sentiment that in clusters with capacity, this might even slow down failovers, while simultaneously increasing the work the SPOF does (very slightly) by enabling leader election.

vs. separating webhook and reconciler and running reconciler HA

This needs more thought than just moving imports and calls because we combine the (idempotent singleton) reconcilers with the admission control logic in a few ways. One of these is TLS certs, another is the mutating webhooks (which become nontrivial with bindings).

---

I'll try to sketch something a little more concrete. I get that this is crazy talk. I'll do my best to make it seem less crazy 😁

[mattmoor]

I have a POC of this working with Serving's webhook (currently just hacked into vendor). I've tested it with LE-disabled, LE-enabled unscaled, LE-enabled scaled to 10, and LE-enabled scaled to 10 with the reconcilers forced across multiple pods (see below).

Note that the webhook-8584d54c6-f8jzv prefix is the Pod name:

kubectl get leases -nknative-serving
NAME                          HOLDER                                                         AGE
webhook.configmapwebhook      webhook-8584d54c6-f8jzv_59d3844d-6a54-4751-aad8-595e1d117b3c   15m
webhook.conversionwebhook     webhook-8584d54c6-hltzh_e868e32e-2274-45e7-9145-5057a8acc807   15m
webhook.defaultingwebhook     webhook-8584d54c6-9h8zb_3aa39eaf-3fa6-4db4-8ef7-cde31fbc2eb5   15m
webhook.validationwebhook     webhook-8584d54c6-whsz8_19b4e145-75fb-4c66-88ec-2cb6d9619dc0   15m
webhook.webhookcertificates   webhook-8584d54c6-cqj5x_9a999a43-9a5e-4f47-8142-b4fa612b3dfe   15m

If I kill just that one pod, only that leader changes:

kubectl get leases -nknative-serving
NAME                          HOLDER                                                         AGE
webhook.configmapwebhook      webhook-8584d54c6-jczjg_91013423-e113-40b8-9e1b-70caf77175e3   24m
webhook.conversionwebhook     webhook-8584d54c6-hltzh_e868e32e-2274-45e7-9145-5057a8acc807   24m
webhook.defaultingwebhook     webhook-8584d54c6-9h8zb_3aa39eaf-3fa6-4db4-8ef7-cde31fbc2eb5   24m
webhook.validationwebhook     webhook-8584d54c6-whsz8_19b4e145-75fb-4c66-88ec-2cb6d9619dc0   24m
webhook.webhookcertificates   webhook-8584d54c6-cqj5x_9a999a43-9a5e-4f47-8142-b4fa612b3dfe   24m

Looking at the logs for the new leader, it correctly global resyncs when it wins the election:

{"level":"info","ts":"2020-04-25T16:26:02.595Z","logger":"webhook","caller":"leaderelection/config.go:230","msg":"\"webhook-8584d54c6-jczjg_91013423-e113-40b8-9e1b-70caf77175e3\" has started leading \"webhook.configmapwebhook\"","commit":"6ec7f43"}
{"level":"info","ts":"2020-04-25T16:26:02.600Z","logger":"webhook","caller":"configmaps/configmaps.go:162","msg":"Webhook is valid","commit":"6ec7f43","knative.dev/traceid":"37f9e8d8-2455-48e6-9412-67965dbdf262","knative.dev/key":"config.webhook.serving.knative.dev"}
{"level":"info","ts":"2020-04-25T16:26:02.600Z","logger":"webhook","caller":"controller/controller.go:464","msg":"Reconcile succeeded. Time taken: 5.092617ms.","commit":"6ec7f43","knative.dev/traceid":"37f9e8d8-2455-48e6-9412-67965dbdf262","knative.dev/key":"config.webhook.serving.knative.dev"}

Our webhooks are singleton reconcilers (so they are a somewhat contrived case), but with the primed informer state we're caught back up in 5ms. It'll be interesting to see this for proper global resyncs 😈

I've also set things up in a way that we could actually set/unset the client fields based on whether we are leader, which would act as a nice defense-in-depth against accidental mutations from non-leaders.

Right now this is clocking in at about ~300 LoC to change our 5 webhook reconcilers, including a bunch of random refactoring I'm likely going to try to break off independently since it's general goodness. None of these reconcilers are using // +genreconciler yet either, and I think that unless folks want to opt-in we can make this require no boilerplate code via our codegen.

---

I'll put together a doc of some kind to organize thoughts here, and a more complete PoC if folks want to take it for a spin.

[mattmoor]

Ok, I've written up a sketch of what this would look like here.

I'll also toss it on the agenda for API WG Weds to discuss more f2f.