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

Add support for IP aliases for pod IPs (GCP alpha feature) #42147

Merged
merged 6 commits into from
Apr 12, 2017

Conversation

bowei
Copy link
Member

@bowei bowei commented Feb 27, 2017

Adds support for allocation of pod IPs via IP aliases.

Adds KUBE_GCE_ENABLE_IP_ALIASES flag to the cluster up scripts (kube-{up,down}.sh).

KUBE_GCE_ENABLE_IP_ALIASES=true will enable allocation of PodCIDR ips
using the ip alias mechanism rather than using routes. This feature is currently
only available on GCE.

Usage

$ CLUSTER_IP_RANGE=10.100.0.0/16 KUBE_GCE_ENABLE_IP_ALIASES=true bash -x cluster/kube-up.sh

Adds CloudAllocator to the node CIDR allocator (kubernetes-controller manager).

If CIDRAllocatorType is set to CloudCIDRAllocator, then allocation
of CIDR allocation instead is done by the external cloud provider and
the node controller is only responsible for reflecting the allocation
into the node spec.

  • Splits off the rangeAllocator from the cidr_allocator.go file.
  • Adds cloudCIDRAllocator, which is used when the cloud provider allocates
    the CIDR ranges externally. (GCE support only)
  • Updates RBAC permission for node controller to include PATCH

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 27, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@bowei
Copy link
Member Author

bowei commented Feb 27, 2017

@thockin @freehan

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 27, 2017
glog.V(3).Infof("Node %v has PodCIDR %v", node.Name, podCIDR)
return nil
}
glog.Errorf("PodCIDR cannot be reassigned, node %v spec has %v, but cloud provider has assigned %v",
Copy link
Contributor

@xiangpengzhao xiangpengzhao Feb 27, 2017

Choose a reason for hiding this comment

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

Should here return error? According to the error info, it seems yes.

But according to the logic below, the PodCIDR should update to CIDR assigned by cloud provider. So IMO here may need to be modified to something like:

	glog.V(3).Infof("PodCIDR will be reassigned, node %v spec has %v, but cloud provider has assigned %v",
		node.Name, node.Spec.PodCIDR, podCIDR)

Copy link
Member Author

Choose a reason for hiding this comment

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

We do not support reassignment of PodCIDRs once they have been assigned.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we should return error here, otherwise PodCIDR will be reassigned to cidr[0] in line #L110, right? Hopefully I understand correctly:)

Copy link
Member Author

Choose a reason for hiding this comment

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

The range_allocator.go code logs an error, but continues with setting the node spec. This basically copies the current behavior, I'll add a comment to be more clear. This is somewhat of a unhandled state for the system, as the underlying layers will ignore the CIDR update.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation!

@bowei bowei force-pushed the ip-alias-2 branch 8 times, most recently from cc18de2 to 3d90228 Compare February 28, 2017 07:49
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2017
@bowei
Copy link
Member Author

bowei commented Mar 1, 2017

@k8s-bot kubemark e2e test this

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 2, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2017
@bowei bowei force-pushed the ip-alias-2 branch 2 times, most recently from 2a3520e to bc60716 Compare March 21, 2017 20:37
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2017
@roberthbailey
Copy link
Contributor

I only looked at the shell code. By the way, why is this change in 7 commits?

@bowei
Copy link
Member Author

bowei commented Apr 10, 2017

@roberthbailey -- commits are grouped in a way to make it easy to review / rebase

@bowei
Copy link
Member Author

bowei commented Apr 11, 2017

@k8s-bot test this

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 11, 2017

@bowei: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins GKE smoke e2e cbff59eeb157c2615d3aa00f41e622a176f25adb link @k8s-bot cvm gke e2e test this
Jenkins GCI GKE smoke e2e cbff59eeb157c2615d3aa00f41e622a176f25adb link @k8s-bot gci gke e2e test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

@roberthbailey roberthbailey self-assigned this Apr 11, 2017
@bowei bowei force-pushed the ip-alias-2 branch 2 times, most recently from fa09323 to 96298b8 Compare April 11, 2017 19:30
@bowei bowei changed the title IP alias 2 Add support for IP aliases for pod IPs (this is a GCP alpha feature) Apr 11, 2017
@bowei bowei changed the title Add support for IP aliases for pod IPs (this is a GCP alpha feature) Add support for IP aliases for pod IPs (GCP alpha feature) Apr 11, 2017
KUBE_GCE_ENABLE_IP_ALIASES=true will enable allocation of PodCIDR ips
using the ip alias mechanism rather than using routes.

NODE_IP_RANGE will control the node instance IP cidr
KUBE_GCE_IP_ALIAS_SIZE controls the size of each podCIDR
IP_ALIAS_SUBNETWORK controls the name of the subnet created for the cluster
If CIDRAllocatorType is set to `CloudCIDRAllocator`, then allocation
of CIDR allocation instead is done by the external cloud provider and
the node controller is only responsible for reflecting the allocation
into the node spec.

- Splits off the rangeAllocator from the cidr_allocator.go file.
- Adds cloudCIDRAllocator, which is used when the cloud provider allocates
  the CIDR ranges externally. (GCE support only)
- Updates RBAC permission for node controller to include PATCH
I also sorted the file, it was almost sorted with a few exceptions.
@bowei
Copy link
Member Author

bowei commented Apr 11, 2017

@thockin -- there will be follow on commits mostly likely as I get the e2e testing running through in ip alias mode.

@thockin
Copy link
Member

thockin commented Apr 11, 2017

/lgtm since @roberthbailey is ooo

@thockin
Copy link
Member

thockin commented Apr 11, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, thockin

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit ceccd30 into kubernetes:master Apr 12, 2017
@bowei bowei deleted the ip-alias-2 branch April 12, 2017 05:10
@enisoc enisoc mentioned this pull request Apr 20, 2017
k8s-github-robot pushed a commit that referenced this pull request Apr 21, 2017
Automatic merge from submit-queue

Clean up CHANGELOG.md

There was extra stuff inside the `release-note` block for #42147 that messed up the formatting for CHANGELOG.md. I edited the PR to fix it, but the CHANGELOG.md had already been pushed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants