Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kube-controller-manager: Add configure-cloud-routes option #25614

Conversation

justinsb
Copy link
Member

This allows kube-controller-manager to allocate CIDRs to nodes (with
allocate-node-cidrs=true), but will not try to configure them on the
cloud provider, even if the cloud provider supports Routes.

The default is configure-cloud-routes=true, and it will only try to
configure routes if allocate-node-cidrs is also configured, so the
default behaviour is unchanged.

This is useful because on AWS the cloud provider configures routes by
setting up VPC routing table entries, but there is a limit of 50
entries. So setting configure-cloud-routes on AWS would allow us to
continue to allocate node CIDRs as today, but replace the VPC
route-table mechanism with something not limited to 50 nodes.

We can't just turn off the cloud-provider entirely because it also
controls other things - node discovery, load balancer creation etc.

Fix #25602

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/old-docs size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 14, 2016
@justinsb justinsb added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 14, 2016
@bprashanth
Copy link
Contributor

./federation/apis/federation/types.generated.go up to date.
FAILED   ./hack/../hack/verify-codecgen.sh  447s

@justinsb justinsb force-pushed the feature/flag-configure-cloud-routes branch 2 times, most recently from 58a4162 to 007f40a Compare May 16, 2016 13:19
@justinsb
Copy link
Member Author

Finally passed the integration tests - thanks for the heads-up @bprashanth :-)

Going to leave the e2e flake until I hear whether the PR is something we'll consider - not least because I suspect even if we accept the idea, we'll likely change the name.

@justinsb
Copy link
Member Author

@thockin any thoughts? Could support the general disaggregation of KCM; perhaps in that light it should be "enable-route-controller" (default true)?

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2016
@thockin
Copy link
Member

thockin commented May 20, 2016

LGTM

@mikedanese @cjcullen @Q-Lee FYI

if you rebase and get green before Monday, you're in.

@thockin
Copy link
Member

thockin commented May 20, 2016

also permission to self-lgtm when rebased and green

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2016
@mikedanese
Copy link
Member

LGTM

@cjcullen
Copy link
Member

Looks fine to me.

This allows kube-controller-manager to allocate CIDRs to nodes (with
allocate-node-cidrs=true), but will not try to configure them on the
cloud provider, even if the cloud provider supports Routes.

The default is configure-cloud-routes=true, and it will only try to
configure routes if allocate-node-cidrs is also configured, so the
default behaviour is unchanged.

This is useful because on AWS the cloud provider configures routes by
setting up VPC routing table entries, but there is a limit of 50
entries.  So setting configure-cloud-routes on AWS would allow us to
continue to allocate node CIDRs as today, but replace the VPC
route-table mechanism with something not limited to 50 nodes.

We can't just turn off the cloud-provider entirely because it also
controls other things - node discovery, load balancer creation etc.

Fix kubernetes#25602
@justinsb justinsb force-pushed the feature/flag-configure-cloud-routes branch from 007f40a to 6c66764 Compare May 27, 2016 14:11
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 27, 2016
@justinsb justinsb added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2016
@justinsb
Copy link
Member Author

Re-adding lgtm after rebase.

@justinsb justinsb added this to the v1.3 milestone May 27, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented May 28, 2016

GCE e2e build/test passed for commit 6c66764.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit a522257 into kubernetes:master May 28, 2016
@colemickens
Copy link
Contributor

Is there any reason to not allow the case where:

  • I want to allocate Pod CIDRs myself, but,
  • I want cloudprovider to write the route table myself?

It's a use-case that we ran into today, it's mostly to unblock some testing of some half-baked functionality, but if I squint I could imagine someone wanting that configuration in a real cluster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants