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

re-enable nodeipam in kube-controller-manager #63049

Merged
merged 1 commit into from May 11, 2018

Conversation

@andrewsykim
Member

andrewsykim commented Apr 24, 2018

What this PR does / why we need it:
Re-enables nodeipam controller for external clouds. Also does a small refactor so that we don't need to pass in allocateNodeCidr into the controller.

In v1.10 we made a change (9187b34#diff-f11913dc67d80d36b3d06a93f61c49cf in #57492) where nodeipam would be disabled for any cluster that sets --cloud-provider=external. The original intention behind this was that the nodeipam controller is cloud specific for some clouds (only GCE at the moment) so it should be moved to the CCM (cloud controller manager). After some discussions with wg-cloud-provider it makes sense to re-enable nodeipam controller in KCM and have GCE CCM enable its own cloud-specific IPAM controller as part of Initialize(). This would allow for GCE to run nodeipam in both KCM (by setting --cloud-provider=gce and --allocate-node-cidr) and in the CCM (once implemented in Initialize()) without disabling nodeipam in the KCM for all external clouds and avoids having to implement nodeipam in CCM.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Re-enable nodeipam controller for external clouds. 
@liggitt

This comment has been minimized.

Member

liggitt commented Apr 24, 2018

/sig network
/sig node
/unassign

@andrewsykim

This comment has been minimized.

Member

andrewsykim commented Apr 24, 2018

/area cloudprovider

@andrewsykim

This comment has been minimized.

Member

andrewsykim commented Apr 24, 2018

glog.Warningf("Unsuccessful parsing of cluster CIDR %v: %v", ctx.ComponentConfig.KubeCloudShared.ClusterCIDR, err)
}
if !ctx.ComponentConfig.KubeCloudShared.AllocateNodeCIDRs {

This comment has been minimized.

@cheftako

cheftako Apr 25, 2018

Member

I think I'm missing something. This seems to be disabling the node ipam controller on the KCM if "--allocate-node-cidrs" isn't set (https://github.com/kubernetes/kubernetes/blob/master/cmd/controller-manager/app/options/kubecloudshared.go#L57). GCE and local both explicitly set this flag to true. I also know kubeadm sets it to true based on "Networking.PodSubnet" not being null. (Not sure for which cloud providers that will be true). My understanding is that our goal (eventual) is to have node ipam running on the KCM for all the providers except GCE. For GCE running the CCM, node ipam should be disabled on the KCM.

I generally like this clean up but I don't think it belongs with re-enabling nodeipam in the KCM. (I would like to be able to revert one without the other) Also I don't think it meets my concerns about being able to properly control whether nodeipam is running in the KCM or CCM. (That can be handled later and is not necessarily blocking)

This comment has been minimized.

@andrewsykim

andrewsykim Apr 26, 2018

Member

My understanding is that our goal (eventual) is to have node ipam running on the KCM for all the providers except GCE. For GCE running the CCM, node ipam should be disabled on the KCM.

Yup, this change would allow us to do that. Set --allocate-node-cidr if you want nodeipam on KCM (right now this is broken if you set --cloud-provider=external on KCM), if you are running GCE setup with CCM, make sure you set --cloud-provider=external and unset --allocate-node-cidr on KCM.

This comment has been minimized.

@andrewsykim

andrewsykim Apr 26, 2018

Member

I generally like this clean up but I don't think it belongs with re-enabling nodeipam in the KCM. (I would like to be able to revert one without the other) Also I don't think it meets my concerns about being able to properly control whether nodeipam is running in the KCM or CCM. (That can be handled later and is not necessarily blocking)

If you prefer, I can simplify the PR by only including this change. But functionally it would be the same thing since nodeipam will do nothing if --allocate-node-cidr is not set and from GCE you would still have to unset --allocate-node-cidr. The nice thing about this change is that we would explicity log that nodeipam controller was skipped.

This comment has been minimized.

@cheftako

cheftako May 7, 2018

Member

I guess I'm fine for now. However I think we need to get away from conflating meaning on configuration flags. I believe at some point will want to be able to have allocate-node-cidrs set to true and disable this controller on the KCM. Especially if we move forward on component config (kubernetes/enhancements#115).

@andrewsykim

This comment has been minimized.

Member

andrewsykim commented Apr 26, 2018

cc @gmarek @bowei PTAL

@cheftako

I guess I'm fine for now. However I think we need to get away from conflating meaning on configuration flags. I believe at some point will want to be able to have allocate-node-cidrs set to true and disable this controller on the KCM. Especially if we move forward on component config (kubernetes/enhancements#115).

@andrewsykim

This comment has been minimized.

Member

andrewsykim commented May 7, 2018

That makes sense, and as much as I want to push for that here, the state of v1.10 as is, is not acceptable for external clouds hence why I think it's important we patch this first. @gmarek or @bowei PTAL

@gmarek

This comment has been minimized.

Member

gmarek commented May 11, 2018

/lgtm

@mikedanese

This comment has been minimized.

Member

mikedanese commented May 11, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented May 11, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, gmarek, mikedanese

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented May 11, 2018

Automatic merge from submit-queue (batch tested with PRs 63049, 59731). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 5a54555 into kubernetes:master May 11, 2018

16 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation andrewsykim authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@andrewsykim

This comment has been minimized.

Member

andrewsykim commented May 11, 2018

This PR needs to be cherry picked into v1.10, PTAL #63725

k8s-merge-robot added a commit that referenced this pull request May 17, 2018

Merge pull request #63725 from andrewsykim/automated-cherry-pick-of-#…
…63049-origin-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #63049: renable nodeipam in kube-controller-manager

**What this PR does / why we need it**:
Re-enables nodeipam controller for external clouds. Also does a small refactor so that we don't need to pass in `allocateNodeCidr` into the controller. 

Cherry pick of #63049 which fixes a critical bug in v1.10 that blocks external clouds from running nodeipam controller. 

**Release note**:
```release-note
Re-enable nodeipam controller for external clouds. 
```

zioproto added a commit to zioproto/k8s-on-openstack that referenced this pull request May 25, 2018

Update to version 1.10.3
We also re-enable nodeipam in kube-controller-manager

kubernetes/kubernetes#63049

zioproto added a commit to zioproto/k8s-on-openstack that referenced this pull request Jul 5, 2018

Update to version 1.10.3
We also re-enable nodeipam in kube-controller-manager

kubernetes/kubernetes#63049

ctrlaltdel added a commit to infraly/k8s-on-openstack that referenced this pull request Jul 13, 2018

Update to version 1.10.3
We also re-enable nodeipam in kube-controller-manager

kubernetes/kubernetes#63049
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment