📖 Add a design for supporting warm replicas. #3121
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @godwinpang! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: godwinpang The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @godwinpang. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
| behavior when manager is not in leader mode. This interface should be as follows: | ||
| ```go | ||
| type WarmupRunnable interface { | ||
| NeedWarmup() bool |
There was a problem hiding this comment.
Why add a boolean marker on top of the actual interface?
| } | ||
|
|
||
| // GetWarmupRunnable implements WarmupRunnable | ||
| func (c *Controller[request]) GetWarmupRunnable() Runnable { |
There was a problem hiding this comment.
Why return a runnable rather than just have a synchronous Warmup()? Runnable is expected to keep running, this is expected to terminate. There is also a timeout for this in the controller which we should re-use here
|
|
||
| ## Concerns/Questions | ||
| 1. Controllers opted into this feature will break the workqueue.depth metric as the controller will | ||
| have a pre filled queue before it starts processing items. |
There was a problem hiding this comment.
As discussed offline, one way to avoid this is to use a metrics wrapper that supresses them until the leader election is won. But not sure if its worth bothering
There was a problem hiding this comment.
Does this actually break the metric? Sounds like the metric will just show the reality
It might break alerts that assume the queue length should be pretty low, but that's an issue of the alerts.
There was a problem hiding this comment.
It might break alerts that assume the queue length should be pretty low, but that's an issue of the alerts.
Not sure I quite agree with that. The alerts work today, if we change the behavior here, we break them. To my knowledge there also isn't a metric that indicates if a given replica is the leader, so I don't even see a good way to unbreak them
There was a problem hiding this comment.
Yeah, but the current definition of the metric is that it should show the length of the queue
There was a problem hiding this comment.
To my knowledge there also isn't a metric that indicates if a given replica is the leader
That could be solved, I hope :)
There was a problem hiding this comment.
Yeah, but the current definition of the metric is that it should show the length of the queue
Be that as it may, the reality is that there are a lot of ppl that use controller-runtime so Hyrums Law applies - That we always have an empty workqueue when not leader is observable behavior and changing that will break ppl
There was a problem hiding this comment.
Then we maybe shouldn't store the items in the queue at this time because that's observable behavior as well (not only through the metric) and not just make it look like the queue is empty through the metric
There was a problem hiding this comment.
Then we maybe shouldn't store the items in the queue at this time because that's observable behavior as well (not only through the metric) and not just make it look like the queue is empty through the metric
How would that be obseveable except for through the metric if we don't start the controller?
There was a problem hiding this comment.
- logs via logState
- other workqueue metrics: adds_total, queue_duration_seconds
- Although I guess we can also fake these. What would happen when the controller starts? I assume we would set the length metric immediately to it's correct value. Similar for adds_total and probably also queue_duration_seconds
I also think folks have programmatic access to the queue (at least by instantiating the queue in controller.Options.NewQueue)
So we don't know what kind of things folks are doing with the queue, e.g. accessing queue length or taking items out of the queue even if the leader election controllers are not running.
| ## Concerns/Questions | ||
| 1. Controllers opted into this feature will break the workqueue.depth metric as the controller will | ||
| have a pre filled queue before it starts processing items. | ||
| 2. Ideally, non-leader runnables should block readyz and healthz checks until they are in sync. I am |
There was a problem hiding this comment.
This will break conversion webhooks. I don't know if there is a good way to figure out if the binary contains a conversion webhook, but if in doubt we have to retain the current behavior
| 2. Ideally, non-leader runnables should block readyz and healthz checks until they are in sync. I am | ||
| not sure what the best way of implementing this is, because we would have to add a healthz check | ||
| that blocks on WaitForSync for all the sources started as part of the non-leader runnables. | ||
| 3. An alternative way of implementing the above is to moving the source starting / management code |
There was a problem hiding this comment.
That is kind-of already the case, the source uses the cache which is a runnable to get the actual informer and the cache is shared and started before anything else except conversion webhooks (as conversion webhooks might be needed to start it). The problem is just that the cache does not actually cache anything for resources for which no informer was requested and that in turn only happens after the source is started which happens post controller start and thus post leader election
|
|
||
| ## Motivation | ||
| Controllers reconcile all objects during startup / leader election failover to account for changes | ||
| in the reconciliation logic. For certain sources, the time to serve the initial list can be |
There was a problem hiding this comment.
Just curious. Is it literally the list call that takes minutes or the subsequent reconciliation?
Does this change when the new ListWatch feature is used?
There was a problem hiding this comment.
This is purely about the time it takes to start reconciling after a replica went down because of a rollout or an unforseen event. Right now, that means we first acquire the leader lease, then sync all caches, then start reconciling. The goal of this doc is to do the second step before we even try to aquire the leader lease, as that will take the time it takes to sync the caches out of the transition time.
Agree the description could be a bit clearer.
There was a problem hiding this comment.
Yeah that's fine. I was just curious about the list call :) (aka the cache sync)
| downtime as even after leader election, the controller has to wait for the initial list to be served | ||
| before it can start reconciling. | ||
|
|
||
| ## Proposal |
There was a problem hiding this comment.
What if there's a lot of churn and the controller doesn't become leader maybe not at all or maybe after a few days?
The queue length will increase while there is nothing that takes items out of the queue.
I know the queue doesn't require significant memory to store an item but is there something we should be concerned about if the queue has eg millions of items (let's say we watch pods and we don't become leader for a month)
There was a problem hiding this comment.
Worst case it at some point gets oom killed, restarts, does everything again, I don't think this is likely to become an actual issue
There was a problem hiding this comment.
Yeah definitely not a big problem. Maybe just good to know
| func (cm *controllerManager) Start(ctx context.Context) (err error) { | ||
| // ... | ||
|
|
||
| // Start the warmup runnables |
There was a problem hiding this comment.
At which exact place in the current Start func would this go? (what do we do directly before/after)
There was a problem hiding this comment.
Initially I thought before we start the caches but that would mean that we can not respect the startup timeout in the controller so I guess it has to come after
|
|
||
| ## Concerns/Questions | ||
| 1. Controllers opted into this feature will break the workqueue.depth metric as the controller will | ||
| have a pre filled queue before it starts processing items. |
There was a problem hiding this comment.
Does this actually break the metric? Sounds like the metric will just show the reality
It might break alerts that assume the queue length should be pretty low, but that's an issue of the alerts.
This change describes the motivation and implementation details for supporting warm replicas in controller-runtime. I have floated this idea offline with @alvaroaleman to address some really slow sources that we work with that take 10s of minutes to serve the initial list.There is no open issue discussing it. Let me know if that is preferred and I can open one.
Previously discussed in #2005 and #2600