📖 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
|
|
||
| ## 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.
- 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.
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
That makes sense to me. I guess reading between the lines, the suggestion here is that non-leader runnables should The specific behavior I want to solve for is to have the queue ready to go asap after leader election, so would we implement something like "populate queue from cache on winning leader election"? I am not too familiar with the details here so would appreciate some concrete nudges in the right direction
There was a problem hiding this comment.
That makes sense to me. I guess reading between the lines, the suggestion here is that non-leader runnables should The specific behavior I want to solve for is to have the queue ready to go asap after leader election, so would we implement something like "populate queue from cache on winning leader election"?
I don't know how that would be possible, we would need to implement a dummy queue to buffer.
Lets punt on this problem until we make this the default behavior, we don't have to solve it right away. For as long as its opt-in and there is a metric indicating if a given replica is a leader, it won't break anyone by default and people can adjust their alerts accordingly.
There was a problem hiding this comment.
Thought more about this. Probably the initial suggestion is fine. We should just make sure that the metrics popping up once the controller becomes leader make sense
| ## 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
There was a problem hiding this comment.
Can you explain why this breaks conversion webhooks? Taking a step back, why does controller-runtime offer a way to run conversion webhooks in the first place?
There was a problem hiding this comment.
Can you explain why this breaks conversion webhooks?
Because they need to be up to sync the cache, so blocking readyz until the cache is ready creates a deadlock.
Taking a step back, why does controller-runtime offer a way to run conversion webhooks in the first place?
Because they are part of some controllers.
There was a problem hiding this comment.
got it, we can just put the burden on the user to register readyz as they want then
There was a problem hiding this comment.
Potentially we can provide a util func like we did with mgr.GetWebhookServer().StartedChecker() to make it easier (can be done in a follow-up PR)
But agree we can't add this per default as it can lead to deadlocks for controllers that serve conversion webhooks.
| 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
designs/warmreplicas.md
Outdated
|
|
||
| ## 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
There was a problem hiding this comment.
Doesn't the queue have deduplication built in? I thought that you couldn't re-add a key to the queue if the key already existed within the queue.
In which case, when you perform the initial list, it adds every item to the queue, and then every object created after would be added to the queue, but the queue length would always be equal to the number of those objects in the cluster
We should double check if this is actually how they work
There was a problem hiding this comment.
Doesn't the queue have deduplication built in? I thought that you couldn't re-add a key to the queue if the key already existed within the queue.
Yes there is de-duplication. We are basically talking about a map :)
queue length would always be equal to the number of those objects in the cluster
I don't think that deleted objects are removed from our queue. So if you have churn (i.e. any objects are deleted) the queue length won't be equal to the number of objects in the cluster.
But the queue only stores namespace + name. So it doesn't require a lot of memory per object.
|
|
||
| ## 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.
|
Took another look. Definitely fine for now. I would suggest let's review/merge #3192 and then we can finalize this PR afterwards |
There was a problem hiding this comment.
Do you have a POC or any implementation that would show what the difference is for an end user? Sources generally are informers right? So, as someone implementing a controller, how would I start the informers in this case vs what I would have normally done?
A worked example of the usage would probably be good to include in the design doc
| 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.
Doesn't the queue have deduplication built in? I thought that you couldn't re-add a key to the queue if the key already existed within the queue.
In which case, when you perform the initial list, it adds every item to the queue, and then every object created after would be added to the queue, but the queue length would always be equal to the number of those objects in the cluster
We should double check if this is actually how they work
| // when the manager is not the leader. | ||
| // Defaults to false, which means that the controller will wait for leader election to start | ||
| // before starting sources. | ||
| ShouldWarmupWithoutLeadership *bool |
There was a problem hiding this comment.
We usually like to use *bool so we can differentiate between default value and whatever a user configured.
This is also necessary as we have two levels of configuration ("manager" and per controller) and we have to be able to figure out if the per-controller config overrides the manager config
|
Has the impact to API servers been assessed for this proposal? IIUC, the slow to start sources are likely informers that are having to page through large volumes of resources, right? So there's already a higher than average load on the API server for these types of resources, and this EP is basically going to double the API server load? Am I following this correctly? |
Its an opt-in feature and in the vast majority of cases, the kube-apiserver is managed. The problem this solves is that failover is currently slow due to the fact that the watches are only started after leader election is won. |
|
Maybe additional apiserver load is worth mentioning in the godoc of the field that enables the feature?
The implementation can be found here: #3192 You only have to set I would recommend reviewing the PR as we are figuring out some details there at the moment (e.g. I raised the question if WarmupRunnable should be exported, xref: #3192 (comment), this would drastically reduce the API surface of this feature) |
Opt-in for the operator author, or opt-in for the end user who deploys that operator top their cluster? I suspect we might want to recommend that folks expose an option to end users to be able to turn this on/off on a cluster by cluster basis I guess that would be exposing the |
That is up to the operator author to decide. I personally don't think the duplicates watches are an issue for the apiserver, if that can make it fall over, its too small anyways. But generally speaking, making a call as to what to do or not to do by default and if an end-user option should be exposed or not is up to the controller author, I don't believe controller-runtime is in a good position to make a recommendation there. |
|
Usually we don't go as far as recommending command line flags in controller-runtime. That's maybe more kubebuilder territory. |
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