KEP-5237: watch-based route controller reconciliation using informers

[lukasmetzner]

• One-line PR description: Introduce a feature gate to enable informer-based reconciliation in the routes' controller of cloud-controller-manager, reducing API calls and improving efficiency.

• Issue link: CCM: watch-based route controller reconciliation using informers #5237

• Other comments:

[k8s-ci-robot]

Welcome @lukasmetzner!

It looks like this is your first PR to kubernetes/enhancements 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/enhancements has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lukasmetzner
Once this PR has been reviewed and has the lgtm label, please assign cheftako for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• keps/sig-cloud-provider/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[lukasmetzner]

/cc @elmiko @JoelSpeed

[apricote]

/ok-to-test

[elmiko]

the core concepts make sense to me, i think we should clean up the language around the term "cloud provider". in some places we use it to mean the controllers (ie the ccm), and other places we use it to mean the infrastructure provider (eg aws, azure, gcp), and we also refer to the framework as well.

we also need to make some decisions about the open questions. i wonder if we should go over these questions at the next sig meeting?

[elmiko]

if we aren't going to capitalize "Cloud Provider" in other places, we shouldn't capitalize here.

alternatively, if we want to distinguish the "cloud provider" (eg aws, gcp, etc), then i think we should a less overloaded term, like "infrastructure provider".

[lukasmetzner]

I am fine settling for the term infrastructure provider here.

[elmiko]

which "cloud providers" are we talking about here: the cloud-controller-manager, the infrastructure provider, or the maintainers of other ccm projects?

[lukasmetzner]

I think both infrastructure providers and maintainers of other ccm projects are welcome to test this feature and provide feedback on the fields, which were choosen to trigger a reconcile.

[elmiko]

is this "cloud provider" referring to the cloud-controller-manager?

[lukasmetzner]

No, this is referring to the infrastructure provider. If an infrastructure provider has a limit on the max. amount of routes for a private network we can get rid of unused routes. This is at least the case for Hetzner.

Founds also a small typo here in from the cloud provider, which I am going to clean up.

[lukasmetzner]

@elmiko If possible, I’d love to work through the open questions here in the PR so we can keep things moving, and then have a discussion in the next SIG meeting. Otherwise, we might end up losing a two of weeks of time. What do you think?

Open Questions:

• What should be the default frequency for the periodic full reconciliation?
  • Input from @JoelSpeed: We should do it similar to other controllers and choose a random time between 12h and 24h.
  • As we use the same shared informers' factory as in other controllers, this should already be implemented.
• Are there other Node fields besides node.status.addresses and PodCIDRs that should trigger a route update?
• How should we set the interval for the periodic reconcile? Options:
  • Adjust --route-reconcile-period when feature gate enabled
  • Use --min-resync-period; currently defaults to 12h
  • Introduce a new flag
  • If we use the 12h-24h option we can probably reuse --min-resync-period.

[elmiko]

i'm fine to continue the discussions here.

Input from @JoelSpeed: We should do it similar to other controllers and choose a random time between 12h and 24h.

i think 12h sounds fine to me.

Are there other Node fields besides node.status.addresses and PodCIDRs that should trigger a route update?

i'll have to think about this a little more, i have a feeling that those are good to start with.

How should we set the interval for the periodic reconcile?

i like the idea of adjusting the default for the --route-reconcile-period, but i don't want users to get confused about this.

my only issue with using --min-resync-period is that it sounds much more general and we are just focusing on the route controller.