Allow multiple node cidr masks in kube-controller-manager #79993

aramase · 2019-07-10T17:17:01Z

What type of PR is this?

/kind feature

What this PR does / why we need it:
Currently the default node cidr mask size is /24 for ipv4 and ipv6. Now that ipv6 dual stack feature has been merged, the user should have an option to configure a different mask size for each ip family. If the user doesn't define the mask sizes explicitly, then the default mask size will be /24 for ipv4 and /64 for ipv6.

Related PRs:
#73977
#79386

Changes implemented -

Add new flags --node-cidr-mask-size-ipv4 and --node-cidr-mask-size-ipv6 which can be used for dual-stack clusters.
Set the default mask size based on CIDR family - /24 for IPv4 and /64 for IPv6

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kube-controller-manager
--node-cidr-mask-size-ipv4 int32     Default: 24. Mask size for IPv4 node-cidr in dual-stack cluster.
--node-cidr-mask-size-ipv6 int32     Default: 64. Mask size for IPv6 node-cidr in dual-stack cluster.

These 2 flags can be used only for dual-stack clusters. For non dual-stack clusters, continue to use
--node-cidr-mask-size flag to configure the mask size.

The default node cidr mask size for IPv6 was 24 which is now changed to 64.

k8s-ci-robot · 2019-07-10T17:17:09Z

Hi @aramase. 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/test-infra repository.

aojea · 2019-07-10T20:41:05Z

/cc

neolit123 · 2019-07-11T01:36:22Z

--node-cidr-mask-size string Default: 24 for ipv4 and 98 for ipv6
Mask sizes for node cidr in cluster.

^ probably best to add kube-controller-manager in the release note.

roycaihw · 2019-07-11T20:13:51Z

/remove-sig api-machinery

roycaihw · 2019-07-11T20:14:26Z

/sig cloud-provider

BenTheElder · 2019-07-23T18:21:10Z

/ok-to-test

aramase · 2019-11-07T01:04:14Z

/retest

aramase · 2019-11-07T06:46:50Z

/retest

aramase · 2019-11-07T17:26:30Z

@thockin @bowei PTAL when you get a chance.

lachie83 · 2019-11-14T22:42:11Z

/milestone v1.17

thockin

I am missing documentation on compat and precedence. Will approve, but please follow up ASAP.

Thanks!

/lgtm
/approve

thockin · 2019-11-15T01:39:25Z

cmd/kube-controller-manager/app/options/options_test.go

@@ -279,7 +281,9 @@ func TestAddFlags(t *testing.T) {
 		},
 		NodeIPAMController: &NodeIPAMControllerOptions{
 			&nodeipamconfig.NodeIPAMControllerConfiguration{
-				NodeCIDRMaskSize: 48,
+				NodeCIDRMaskSize:     48,


We should spec and test what happens when these disagree

thockin · 2019-11-15T01:40:03Z

pkg/controller/nodeipam/config/types.go

@@ -22,6 +22,10 @@ type NodeIPAMControllerConfiguration struct {
 	ServiceCIDR string
 	// secondaryServiceCIDR is CIDR Range for Services in cluster. This is used in dual stack clusters. SecondaryServiceCIDR must be of different IP family than ServiceCIDR
 	SecondaryServiceCIDR string
-	// NodeCIDRMaskSize is the mask size for node cidr in cluster.
+	// NodeCIDRMaskSize is the mask size for node cidr in single-stack cluster.


Document how these all interact - are they compatible, mutually exclusive, or does the older form take precedence?

@thockin I've documented that they are mutually exclusive in the PR description. But will add those as comments in the follow up.

k8s-ci-robot · 2019-11-15T01:43:19Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~api/OWNERS~~ [thockin]
~~cmd/kube-controller-manager/OWNERS~~ [thockin]
~~pkg/controller/nodeipam/OWNERS~~ [thockin]
~~staging/src/k8s.io/kube-controller-manager/config/OWNERS~~ [thockin]
~~test/OWNERS~~ [thockin]

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

aramase · 2019-11-15T02:51:07Z

/retest

update tests add comment amend var name update comment add check for empty slice fix tests fix mask size in test review feedback add ipv4 and ipv6 flag for mask sizes add to violation exception list remove import alias run update-openapi-spec review feedback run update-bazel review feedback review feedback

aramase · 2019-11-15T04:06:44Z

@thockin Had to rebase the PR as new unit tests were added in the master to range_allocator_test.go. Can you PTAL and add lgtm again?

aramase · 2019-11-15T04:22:53Z

/test pull-kubernetes-node-e2e-containerd

aramase · 2019-11-15T05:28:41Z

/test pull-kubernetes-node-e2e-containerd

k8s-ci-robot · 2019-11-15T05:42:15Z

@aramase: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-node-e2e-containerd	`796faba`	link	`/test pull-kubernetes-node-e2e-containerd`

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.

thockin

I don't see the extra docs I want, but I approved before, so still willing to take a folowup

/lgtm

tedyu · 2019-11-15T12:16:28Z

cmd/kube-controller-manager/app/core.go

+// getNodeCIDRMaskSizes is a helper function that helps the generate the node cidr mask
+// sizes slice based on the cluster cidr slice
+func getNodeCIDRMaskSizes(clusterCIDRs []*net.IPNet, maskSizeIPv4, maskSizeIPv6 int) []int {
+	nodeMaskCIDRs := []int{}


nit: this can be presized to the length of clusterCIDRs

sftim · 2019-11-27T13:13:19Z

I don't see the extra docs I want, but I approved before, so still willing to take a folowup

@thockin / @aramase Can I help with those extra docs? Is that tracked anywhere?

aramase · 2019-11-27T17:07:26Z

Thank you for checking @sftim. I've added the comments detailing the changes in a follow up PR which has been merged. The docs PR has already been merged.

k8s-ci-robot requested review from bowei and davidopp July 10, 2019 17:18

aramase changed the title ~~[WIP] Allow multiple node cidr masks in controller-manager~~ [WIP] Allow multiple node cidr masks in kube-controller-manager Jul 10, 2019

k8s-ci-robot requested a review from aojea July 10, 2019 20:41

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 11, 2019

k8s-ci-robot added the sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. label Jul 11, 2019

aramase force-pushed the controller-manager-multiple-cidr branch from f4b0837 to 41b4ee1 Compare July 18, 2019 00:25

lachie83 mentioned this pull request Jul 19, 2019

Add IPv4/IPv6 dual-stack support kubernetes/enhancements#563

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 23, 2019

aramase force-pushed the controller-manager-multiple-cidr branch from 59221d8 to 5d66338 Compare July 23, 2019 19:07

k8s-ci-robot added this to the v1.17 milestone Nov 14, 2019

thockin reviewed Nov 15, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2019

aramase force-pushed the controller-manager-multiple-cidr branch from 5076264 to 796faba Compare November 15, 2019 04:04

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 15, 2019

thockin reviewed Nov 15, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2019

k8s-ci-robot merged commit ded6ee9 into kubernetes:master Nov 15, 2019

aramase deleted the controller-manager-multiple-cidr branch November 15, 2019 07:23

tedyu reviewed Nov 15, 2019

View reviewed changes

aramase mentioned this pull request Nov 15, 2019

[kube controller manager] add comments for compatibility #85367

Merged

This was referenced Nov 21, 2019

Fill in default node cidr mask size when dual stack is not enabled #85494

Merged

Add support for new dual-stack flags for kubernetes-controller-manager in kubeadm #85609

Merged

aojea mentioned this pull request Feb 13, 2020

KEP: remove IPv6 allocation limitations kubernetes/enhancements#1534

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple node cidr masks in kube-controller-manager #79993

Allow multiple node cidr masks in kube-controller-manager #79993

aramase commented Jul 10, 2019 •

edited

Loading

k8s-ci-robot commented Jul 10, 2019

aojea commented Jul 10, 2019

neolit123 commented Jul 11, 2019

roycaihw commented Jul 11, 2019

roycaihw commented Jul 11, 2019

BenTheElder commented Jul 23, 2019

aramase commented Nov 7, 2019

aramase commented Nov 7, 2019

aramase commented Nov 7, 2019

lachie83 commented Nov 14, 2019

thockin left a comment

thockin Nov 15, 2019

thockin Nov 15, 2019

aramase Nov 15, 2019

k8s-ci-robot commented Nov 15, 2019

aramase commented Nov 15, 2019

aramase commented Nov 15, 2019

aramase commented Nov 15, 2019

aramase commented Nov 15, 2019

k8s-ci-robot commented Nov 15, 2019

thockin left a comment

tedyu Nov 15, 2019

sftim commented Nov 27, 2019 •

edited

Loading

aramase commented Nov 27, 2019 •

edited

Loading

Allow multiple node cidr masks in kube-controller-manager #79993

Allow multiple node cidr masks in kube-controller-manager #79993

Conversation

aramase commented Jul 10, 2019 • edited Loading

k8s-ci-robot commented Jul 10, 2019

aojea commented Jul 10, 2019

neolit123 commented Jul 11, 2019

roycaihw commented Jul 11, 2019

roycaihw commented Jul 11, 2019

BenTheElder commented Jul 23, 2019

aramase commented Nov 7, 2019

aramase commented Nov 7, 2019

aramase commented Nov 7, 2019

lachie83 commented Nov 14, 2019

thockin left a comment

Choose a reason for hiding this comment

thockin Nov 15, 2019

Choose a reason for hiding this comment

thockin Nov 15, 2019

Choose a reason for hiding this comment

aramase Nov 15, 2019

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 15, 2019

aramase commented Nov 15, 2019

aramase commented Nov 15, 2019

aramase commented Nov 15, 2019

aramase commented Nov 15, 2019

k8s-ci-robot commented Nov 15, 2019

thockin left a comment

Choose a reason for hiding this comment

tedyu Nov 15, 2019

Choose a reason for hiding this comment

sftim commented Nov 27, 2019 • edited Loading

aramase commented Nov 27, 2019 • edited Loading

aramase commented Jul 10, 2019 •

edited

Loading

sftim commented Nov 27, 2019 •

edited

Loading

aramase commented Nov 27, 2019 •

edited

Loading