Skip to content
This repository has been archived by the owner on Jul 30, 2021. It is now read-only.

pkg/asset/internal: Add --cloud-provider to bootstrap manifests #464

Merged
merged 1 commit into from
Apr 25, 2017
Merged

pkg/asset/internal: Add --cloud-provider to bootstrap manifests #464

merged 1 commit into from
Apr 25, 2017

Conversation

dghubble
Copy link
Contributor

@dghubble dghubble commented Apr 24, 2017

This was a solution to #463, not necessarily the right solution. I'm not familiar with the original reasons we set configure-cloud-routes=true to false (upstream default is true) or its impact on other providers.

  • Add --cloud-provider to bootstrap apiserver and controller-manager (missing)
    * Set --configure-cloud-routes true. Upstream description: "Should CIDRs allocated by allocate-node-cidrs be configured on the cloud provider. (default true)"

@ghost
Copy link

ghost commented Apr 24, 2017

Can one of the admins verify this patch?

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 24, 2017
@diegs
Copy link
Contributor

diegs commented Apr 24, 2017

rktbot run tests

Copy link
Contributor

@aaronlevy aaronlevy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add the --cloud-provider flags to the temp control-plane -- but we need to keep --configure-cloud-routes left to false. We assume a CNI plugin is going to handle the routing (not the cloud provider).

One reason for this is that using the AWS provider, for example, you are limited to 50 nodes because that's the max # of routes it can create (last I checked).

By assuming a CNI plugin to handle routes it makes the system more generic across providers.

@@ -439,7 +440,7 @@ spec:
- --allocate-node-cidrs=true
- --cloud-provider={{ .CloudProvider }}
- --cluster-cidr={{ .PodCIDR }}
- --configure-cloud-routes=false
- --configure-cloud-routes=true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We assume the use of flannel (or another CNI plugin) - so we do not want the cloud-provider to also be trying to configure routes.

* Add --cloud-provider to bootstrap apiserver and controller-manager
@dghubble
Copy link
Contributor Author

@aaronlevy I've scoped back this change just to set --cloud-provider on bootstrap manifests. It will no longer solve #463 however.

Copy link
Contributor

@diegs diegs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (and sorry for the oversight when making the bootstrap manifests)

@dghubble
Copy link
Contributor Author

cc @aaronlevy

@aaronlevy aaronlevy merged commit ffb3d37 into kubernetes-retired:master Apr 25, 2017
@dghubble dghubble deleted the bootstrap-cloud-provider branch April 25, 2017 18:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants