WIP: feat(ccm): watch-based route controller reconciliation #131220
Conversation
|
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Wed Apr 9 13:32:32 UTC 2025. |
|
Welcome @lukasmetzner! |
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
|
Hi @lukasmetzner. Thanks for your PR. I'm waiting for a kubernetes 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. |
|
/ok-to-test I do not think we need a KEP for this change, commented in the linked issue |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lukasmetzner 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 |
|
/retest |
| // It is only necessary to reconcile the routes for any events that have the potential to impact them: | ||
| // - Node is added | ||
| // - Node is removed | ||
| // - Node .Status.Addresses is changed | ||
| // TODO: is this a reasonable assumption or are there other values that can impact the Routes? | ||
| // - Node .Spec.PodCIDRs is changed |
There was a problem hiding this comment.
why do we need to filter, just reconcile the node as we did before
There was a problem hiding this comment.
The current situation is a reconcile every 10s and no event-based reconciliation.
Our goal is to reduce the rate of requests to cloudprovider.Routes.ListRoutes and subsequently to the cloud provider API. As node updates can happen for a number of reasons and are reasonably frequent, running the reconcile without filter, causes even more requests to the cloud provider.
| // We should still regularly run the reconcile, even if no events come in. Usually controllers use the | ||
| // `syncPeriod` for this, but for the route controller the default period is very low (5s) and not useful for | ||
| // watch-based reconciliation. | ||
| // Because of our event filters the usual resync period from the informer does not trigger reconciles. | ||
| // TODO: What time should we use? |
There was a problem hiding this comment.
if we do not filter on the event handlers, is not possible to use the resync period then? doing watch based and periodic at the same time is racy and we are worse than before
There was a problem hiding this comment.
if we do not filter on the event handlers, is not possible to use the resync period then?
This is possible, but for the reasons mentioned in #131220 (comment) we would like to introduce filtering.
doing watch based and periodic at the same time is racy and we are worse than before
To mitigate this issue we use the workqueue with a single key (rc.workqueue.Add("routes")). This makes sure, that only a single reconcile runs at the same time.
| err := func(key string) error { | ||
| defer rc.workqueue.Done(key) | ||
|
|
||
| // Run the route reconciliation | ||
| if err := rc.reconcileNodeRoutes(ctx); err != nil { |
There was a problem hiding this comment.
the point is to reconcile just the node that has changed, not all the nodes
There was a problem hiding this comment.
In our understanding the route controller is responsible for two main topics.
- If nodes are added, deleted or updated the corresponding routes need to created, deleted or updated.
- Regular reconciliation to cleanup any leftover routes, that were not processed as events, because the CCM was not running for example. In addition, this avoids exceeding limits on the cloud provider side.
1. Event-based reconciliation of single nodes
Although this is possible, we think that this might cause more requests to the cloud provider than the current implementation.
The method ListRoutes from the cloudprovider.Routes interface is the only option we have to get information from the cloud provider about existing routes. As this will always include all routes it makes sense to utilize this information directly for a full reconciliation. Otherwise, each node event will cause a full fetch of all routes from the cloud provider, which in larger clusters is worse than the current full fetch every 10s (default). In this PR we use the workqueue with a single key (rc.workqueue.Add("routes")) to deduplicate multiple node events and do a single reconcile for all nodes.
2. Regular cleanup
Events can't be used here because we need to clean up routes for nodes that have left the cluster without emitting an event (e.g. ccm outage). To do that, we need the full list of routes and nodes.
Conclusion
As we anyway need to do the cleanup regularly, and we might cause a larger overhead with respect to API requests to the cloud provider, we propose to do a full reconcile in the event handler.
Introduces a new watch based route controller for the cloud-controller-manager. This prevents the exhaustion of API rate limits through an overagressive routes controller. This is further described in kubernetes#60646. The corresponding feature gate is called "CloudControllerManagerWatchBasedRoutesReconciliation".
8f86a33 to
c1bb6c8
Compare
What type of PR is this?
/kind feature
What this PR does / why we need it:
The route controller in the cloud-controller-manager currently operates with a static reconciliation interval. Depending on the scale of the cluster, this can quickly lead to exceeding the rate limits set by cloud providers.
This PR introduces a new watch-based route controller that triggers reconciliation only when a node is added, deleted, or when there are updates to the
Status.AddressesorPodCIDRfields.Additionally, a new feature gate,
CloudControllerManagerWatchBasedRoutesReconciliation, is introduced, which is disabled by default.Which issue(s) this PR fixes:
Fixes #60646
Special notes for your reviewer:
KEP 5237 is currently in progress, but is looking good so far.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: