Dual-stack: make nodeipam compatible with existing single-stack clusters when dual-stack feature gate become enabled by default #90439

SataQiu · 2020-04-24T08:24:02Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Dual-stack: make nodeipam compatible with existing single-stack clusters when dual-stack feature gate become enabled by default

Which issue(s) this PR fixes:

Fixes #90251

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

dual-stack: make nodeipam compatible with existing single-stack clusters when dual-stack feature gate become enabled by default

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

SataQiu · 2020-04-24T08:25:32Z

/cc @danwinship @aojea @aramase

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

aojea · 2020-04-24T08:47:00Z

Thanks for the PR but my personal opinion is to postpone all the dual-stack patches that are not addressing specific bugs to the resolution of the KEP, this is addressing a bug in the upgrade process but that's exactly one of the points that are being discussed
kubernetes/enhancements#1679 (comment)

SataQiu · 2020-04-24T08:47:07Z

/priority important-soon

danwinship

The code needs a little bit more reorganizing than this. It was wrong to have separate setNodeCIDRMaskSizes / setNodeCIDRMaskSizesDualStack behavior based on whether the feature gate is set. The distinction in behavior should be based solely on whether the configuration is single-stack or dual-stack.

So looking at serviceCIDR / secondaryServiceCIDR is right, except I think it would make more sense to look at clusterCIDRs instead. (In theory either both should be single stack or both should be dual stack and so it doesn't matter, but anyway, the node CIDRs are allocated out of the cluster CIDRs, so logically it makes more sense to be looking at that.)

For backward compatibility with both before the dual-stack code was added, and with after the dual-stack code was added but before now, the behavior needs to be:

if clusterCIDRs contains only an IPv4 CIDR then:
- you can pass --node-cidr-mask-size regardless of the feature gate setting, without getting any warnings, and that will set nodeCIDRMaskSizeIPv4
- you can pass --node-cidr-mask-size-ipv4 at least if the feature gate is enabled, and that will set nodeCIDRMaskSizeIPv4. (It would be fine to make it so that you are allowed to use --node-cidr-mask-size-ipv4 when the feature gate is not enabled too...)
- you get an error if you pass --node-cidr-mask-size-ipv6, because there's no IPv6 cluster CIDR
if clusterCIDRs contains only an IPv6 CIDR then exactly as above with "4" and "6" swapped
if clusterCIDRs contains both an IPv4 CIDR and an IPv6 CIDR, then you have to pass --node-cidr-mask-size-ipv4 and --node-cidr-mask-size-ipv6, and it's an error to pass --node-cidr-mask-size.

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

SataQiu · 2020-04-25T10:12:40Z

/test pull-kubernetes-e2e-kind-ipv6

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

SataQiu · 2020-05-29T02:56:09Z

/test pull-kubernetes-node-e2e
/test pull-kubernetes-e2e-kind-ipv6

SataQiu · 2020-06-01T02:47:00Z

Hi @aojea @danwinship Is it ok with you now?

danwinship

so actually at this point maybe it would make sense to merge setNodeCIDRMaskSizes and getNodeCIDRMaskSizes into a single function?

So when I said that I was thinking that you could just do a single loop over clusterCIDRs and pick the right flag to use to assign the nodeMaskCIDRs value for each one, and it would simplify the code. But I guess that doesn't really work, and the previous organization, where getNodeCIDRMaskSizes does the error checking and setNodeCIDRMaskSizes does the assignment is probably cleaner.

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

thockin · 2020-07-02T20:11:42Z

I'm going to approve, but I want @khenidak to decide if this is too big of a rebase pain for his mega changes that are coming.

/approve

k8s-ci-robot · 2020-07-02T20:12:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SataQiu, 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:

~~cmd/kube-controller-manager/OWNERS~~ [thockin]

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

danwinship · 2020-07-05T14:24:52Z

This doesn't touch Services at all; it's just about --allocate-node-cidrs. So there should be no conflicts.

…ers when dual-stack feature gate become enabled by default Signed-off-by: SataQiu <1527062125@qq.com>

SataQiu · 2020-07-10T08:47:25Z

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

@@ -156,13 +156,13 @@ func startNodeIpamController(ctx ControllerContext) (http.Handler, bool, error)
 	}

 	var nodeCIDRMaskSizeIPv4, nodeCIDRMaskSizeIPv6 int
-	if utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) {
+	if dualStack {


dualStack will be true when the --cluster-cidr is configured as dual-stack (at least one from each IPFamily) regardless of the IPv6DualStack feature gate is enabled or not. So even IPv6DualStack feature gate is enabled by default, if the user does not set --cluster-cidr as dual-stack, we will not enter this if branch, --node-cidr-mask-size is valid as before.

danwinship · 2020-07-10T15:37:48Z

oh, well, that's simple.

So this version means that if there were users who were using nodeipam, and who enabled IPv6DualStack but didn't have a dual-stack configuration, then things will break for them again. (They'll need to revert back from --node-cidr-mask-size-ipv4 to --node-cidr-mask-size.) Given that this only affects people who enabled an alpha feature gate and then didn't even use that feature, that seems like a reasonable break.

/hold cancel
/lgtm

fejta-bot · 2020-07-10T18:17:29Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

SataQiu · 2020-09-10T06:22:20Z

Hi @danwinship This PR has been waiting for a very long time, can we merge it now?

SataQiu · 2020-09-10T06:37:24Z

@dims Can you help set the milestone label for the PR?

aojea · 2020-09-10T09:00:06Z

@SataQiu did something happen to this PR? I can't see most of the changes 🤔 , is it my browser?

dims · 2020-09-10T11:01:40Z

/test pull-kubernetes-e2e-kind-ipv6
/milestone v1.20

danwinship · 2020-09-10T11:17:39Z

did something happen to this PR? I can't see most of the changes thinking , is it my browser?

no, it just turned out that the final fix was much simpler than the intermediate versions; the old (current git) code does "if the IPv6DualStack feature gate is disabled, you need to use the old CIDR flags, and if the feature gate is enabled, you need to use the new CIDR flags". Eventually @SataQiu figured out that all we needed to do was change that do "if the --cluster-cidr is single stack, you need to use the old CIDR flags, and if the --cluster-cidr is dual stack you need to use the new CIDR flags".

In theory we could also allow using --node-cidr-mask-size-ipv4 in single-stack IPv4 clusters and --node-cidr-mask-size-ipv6 in single-stack IPv6 clusters, but we don't need to, and this PR as is fixes the API break.

aojea · 2020-09-10T11:20:39Z

👍 thanks

fejta-bot · 2020-09-10T15:09:36Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-09-10T18:18:34Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-09-11T00:57:00Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from aojea, aramase and danwinship April 24, 2020 08:25

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 24, 2020

k8s-ci-robot requested review from davidopp and nikhiljindal April 24, 2020 08:26

aojea reviewed Apr 24, 2020

View reviewed changes

cmd/kube-controller-manager/app/core.go Outdated Show resolved Hide resolved

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 24, 2020

SataQiu force-pushed the dual-stack-node-cidr-20200424 branch from a1d9481 to 1ced4a1 Compare April 24, 2020 09:58

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 24, 2020

danwinship suggested changes Apr 24, 2020

View reviewed changes

cmd/kube-controller-manager/app/core.go Outdated Show resolved Hide resolved

cmd/kube-controller-manager/app/core.go Outdated Show resolved Hide resolved

SataQiu force-pushed the dual-stack-node-cidr-20200424 branch from 1ced4a1 to db71cd4 Compare April 25, 2020 07:21

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 25, 2020

aojea reviewed Apr 26, 2020

View reviewed changes

cmd/kube-controller-manager/app/core.go Outdated Show resolved Hide resolved

SataQiu force-pushed the dual-stack-node-cidr-20200424 branch from db71cd4 to e518935 Compare April 27, 2020 08:45

SataQiu requested a review from aojea June 1, 2020 02:47

danwinship reviewed Jun 30, 2020

View reviewed changes

cmd/kube-controller-manager/app/core.go Outdated Show resolved Hide resolved

cmd/kube-controller-manager/app/core.go Outdated Show resolved Hide resolved

thockin assigned khenidak Jul 2, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 2, 2020

dual-stack: make nodeipam compatible with existing single-stack clust…

ec1efc3

…ers when dual-stack feature gate become enabled by default Signed-off-by: SataQiu <1527062125@qq.com>

SataQiu force-pushed the dual-stack-node-cidr-20200424 branch from b50500b to ec1efc3 Compare July 10, 2020 08:34

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 10, 2020

SataQiu commented Jul 10, 2020

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2020

k8s-ci-robot assigned danwinship Jul 10, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2020

k8s-ci-robot added this to the v1.20 milestone Sep 10, 2020

k8s-ci-robot merged commit 56b9a69 into kubernetes:master Sep 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dual-stack: make nodeipam compatible with existing single-stack clusters when dual-stack feature gate become enabled by default #90439

Dual-stack: make nodeipam compatible with existing single-stack clusters when dual-stack feature gate become enabled by default #90439

SataQiu commented Apr 24, 2020 •

edited

SataQiu commented Apr 24, 2020

aojea commented Apr 24, 2020 •

edited

SataQiu commented Apr 24, 2020

danwinship left a comment

SataQiu commented Apr 25, 2020

SataQiu commented May 29, 2020

SataQiu commented Jun 1, 2020

danwinship left a comment

thockin commented Jul 2, 2020

k8s-ci-robot commented Jul 2, 2020

danwinship commented Jul 5, 2020

SataQiu Jul 10, 2020

danwinship commented Jul 10, 2020

fejta-bot commented Jul 10, 2020

SataQiu commented Sep 10, 2020

SataQiu commented Sep 10, 2020

aojea commented Sep 10, 2020

dims commented Sep 10, 2020

danwinship commented Sep 10, 2020 •

edited

aojea commented Sep 10, 2020

fejta-bot commented Sep 10, 2020

fejta-bot commented Sep 10, 2020

fejta-bot commented Sep 11, 2020

Dual-stack: make nodeipam compatible with existing single-stack clusters when dual-stack feature gate become enabled by default #90439

Dual-stack: make nodeipam compatible with existing single-stack clusters when dual-stack feature gate become enabled by default #90439

Conversation

SataQiu commented Apr 24, 2020 • edited

SataQiu commented Apr 24, 2020

aojea commented Apr 24, 2020 • edited

SataQiu commented Apr 24, 2020

danwinship left a comment

Choose a reason for hiding this comment

SataQiu commented Apr 25, 2020

SataQiu commented May 29, 2020

SataQiu commented Jun 1, 2020

danwinship left a comment

Choose a reason for hiding this comment

thockin commented Jul 2, 2020

k8s-ci-robot commented Jul 2, 2020

danwinship commented Jul 5, 2020

SataQiu Jul 10, 2020

Choose a reason for hiding this comment

danwinship commented Jul 10, 2020

fejta-bot commented Jul 10, 2020

SataQiu commented Sep 10, 2020

SataQiu commented Sep 10, 2020

aojea commented Sep 10, 2020

dims commented Sep 10, 2020

danwinship commented Sep 10, 2020 • edited

aojea commented Sep 10, 2020

fejta-bot commented Sep 10, 2020

fejta-bot commented Sep 10, 2020

fejta-bot commented Sep 11, 2020

SataQiu commented Apr 24, 2020 •

edited

aojea commented Apr 24, 2020 •

edited

danwinship commented Sep 10, 2020 •

edited