Dual-Stack Integration with Kubeadm #79033

Arvinderpal · 2019-06-14T14:17:21Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
The PR brings necessary dual-stack related changes to --pod-network-cidr handling in kubeadm.
See kubernetes/kubeadm#1612 for the complete list of kubeadm changes.

Implements:

--pod-network-cidr support a comma separated list of pod CIDRs. This flag is passed to the kube-controller-manager and kube-proxy.

For building a test dual-stack cluster using kubeadm, within the kubeadm config, we must enable this feature. For example:

---
kind: ClusterConfiguration
featureGates:
  IPv6DualStack=true

For a vagrant based test cluster, please see the dual-stack branch of this project (https://github.com/Nordix/k8s-ipv6/tree/dual-stack) or DM on slack.

Does this PR introduce a user-facing change?:

kubeadm: use the --pod-network-cidr flag to init or use the podSubnet field in the kubeadm config to pass a comma separated list of pod CIDRs.

k8s-ci-robot · 2019-06-14T14:17:28Z

Hi @Arvinderpal. 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.

Arvinderpal · 2019-06-14T14:18:27Z

/assign @yastij
/assign @aojea

k8s-ci-robot · 2019-06-14T14:18:28Z

@Arvinderpal: GitHub didn't allow me to assign the following users: aojea.

Note that only kubernetes members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @yastij
/assign @aojea

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.

Arvinderpal · 2019-06-14T14:20:07Z

/sig cluster-lifecycle

yastij · 2019-06-14T14:23:23Z

/priority important-soon

aojea · 2019-06-14T14:40:04Z

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

 	if len(c.PodSubnet) != 0 {
-		allErrs = append(allErrs, ValidateIPNetFromString(c.PodSubnet, constants.MinimumAddressesInServiceSubnet, field.NewPath("podSubnet"))...)
+		allErrs = append(allErrs, ValidatePodSubnetsFromString(c.PodSubnet, field.NewPath("podSubnet"))...)


this is only going to be a comma separated list if the feature is enable. right?
if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.IPv6DualStack) , otherwise it has to work as always

I've looked through the feature-gate code in kubeadm, and from what I can tell feature-gates only apply to k8s components like api, kubelet, etc. For example, here is my kubeadm init config file. I'm enabling dual-stack in individual components.

There is also a top level featureGate at the ClusterConfig level; however, this throws an error.

kind: ClusterConfiguration ... featureGates: IPv6DualStack: true

Error:
featureGates: Invalid value: map[string]bool{"IPv6DualStack":true}: IPv6DualStack is not a valid feature name

Perhaps the kubeadm folks can shed some light here. @neolit123 @yastij do you know?

IMO, the following works whether the config.ClusterCIDR is a single IP or a list of IPs, so it should be safe to use:
cidrs := strings.Split(config.ClusterCIDR, ",")

so on slack i said "maybe we can avoid feature gates for this", but we probably shouldn't given we are toggling an alpha feature for a number of components that can cause problems that we cannot predict:
https://github.com/kubernetes/kubernetes/blob/master/pkg/features/kube_features.go#L538

kubeadm maintains it's own way of doing feature gates (for reasons).
please, have a look at:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/features/features.go#L41
the unit tests should provide an example of how to use this?

kubeadm feature gates are supported in the ClusterConfiguration object (as long as they are registered), they are also supported on the CLI side but we need to double check if all init phases have the --feature-gates flag propagated.

we also need to organize the tasks better:

first PR that we merge should add the kubeadm gate (should have a release note about the gate addition)

second PR (this one) should modify the validation to support ",". examine if validation changes are needed based on the gate.

third PR should add e2e tests under "kubeadm-e2e"

more comments on the implementation:

as mentioned above validators should/might behave differently, but in my thinking they should not:

passing a comma separated list from the config or CLI should be tolerated by the new validators

a comma separated list is backwards compatible and a super-set of field that has a single value, therefore the new validators should can a superset of the of previous ones.

this lets us get away with supporting "," separated lists in the old configs - e.g. v1beta2

if the kubeadm gate is enabled all components such as kubelet, control-plane, need the respected k8s feature gate to be passed to them.

we need to maintain the kubeadm gate until the feature is GA. once it's GA we have to deprecate the feature gate.

if we decide to deprecate the feature gate we need to follow the deprecation policy of the "state" it's currently at e.g. longer for GA.

@Arvinderpal please ping me on the PR that adds the kubeadm feature gate.
thanks!

so on slack i said "maybe we can avoid feature gates for this", but we probably shouldn't given we are toggling an alpha feature for a number of components that can cause problems that we cannot predict:

Ok. I will look at adding feature-gate in kubeadm as well.

we also need to organize the tasks better:

first PR that we merge should add the kubeadm gate (should have a release note about the gate addition)

second PR (this one) should modify the validation to support ",". examine if validation changes are needed based on the gate.

third PR should add e2e tests under "kubeadm-e2e"

Ok. I have updated the tracking ticket and will use that to track current and future tasks. kubernetes/kubeadm#1612

@Arvinderpal please ping me on the PR that adds the kubeadm feature gate.

Will do! Thanks.

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

cmd/kubeadm/app/phases/controlplane/manifests.go

cmd/kubeadm/app/preflight/checks.go

pkg/proxy/apis/config/validation/validation.go

cmd/kubeadm/app/phases/controlplane/manifests.go

neolit123

@Arvinderpal please remove the WIP when you think it's OK.
leaving the LGTM to someone else.
thanks.

/approve
/ok-to-test

yastij · 2019-08-08T13:58:04Z

pkg/proxy/apis/config/validation/validation.go

@@ -28,6 +28,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	componentbaseconfig "k8s.io/component-base/config"
 	apivalidation "k8s.io/kubernetes/pkg/apis/core/validation"
+	kubefeatures "k8s.io/kubernetes/pkg/features"


this might complicate things for kubernetes/kubeadm#1600, I'd suggest dropping this import.

cc @neolit123 @rosti

but this import is in pkg/proxy....validation and not cmd/kubeadm.
kubeadm does not import pkg/features (last time i've checked). why do you see this as a problem?

my bad, I thought I was reading on cmd/kubeadm

@Arvinderpal - can you run hack/update-bazel.sh to reflect the introduced imports on the BUILD files ?

neolit123 · 2019-08-08T18:50:48Z

looks like there is are some failing unit tests related to the kube-proxy validation. nothing major:
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/79033/pull-kubernetes-bazel-test/1159491098037456903

separated pod CIDRs. Dual-stack feature must be enabled for the validation to be done.

CIDRs. This is a necesary change for dual-stack.

Arvinderpal · 2019-08-09T23:44:50Z

Before I even dig too far into this, where are the tests?

At least a set of e2e's to verify behavior even if the are [Feature:] blocked.

@timothysc I believe I have addressed the change that you requested -- see e2e test PR. If all looks well, then please remove the change request. :)

neolit123 · 2019-08-10T00:00:47Z

@Arvinderpal this looks good on the kubeadm side, but we need an approved on the kube-proxy side:
https://github.com/kubernetes/kubernetes/blob/master/pkg/proxy/apis/config/OWNERS

looks like @thockin is the only approver in the file.
/assign @thockin
PTAL at the pkg/proxy/apis/config changes, thanks!

thockin · 2019-08-14T23:51:23Z

Thanks!

/approve

k8s-ci-robot · 2019-08-14T23:52:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Arvinderpal, neolit123, 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/kubeadm/OWNERS~~ [neolit123,thockin]
~~pkg/proxy/apis/config/OWNERS~~ [thockin]

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

aojea · 2019-08-15T21:58:11Z

/lgtm

ping @timothysc :)
#79033 (comment)

fejta-bot · 2019-08-16T00:43:05Z

/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 · 2019-08-16T04:12:56Z

/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 · 2019-08-16T10:09:56Z

/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.

neolit123 · 2019-08-24T12:41:05Z

@Arvinderpal
could you please fix the typo in release note:
kubeamd: -> kubeadm
thanks!

there is a "release-notes" tool that picks these and autogenerates a change log.
someone tried to fix it in a PR #81783

Arvinderpal · 2019-08-25T19:11:10Z

@neolit123 Done.
I assume the change is only needed in the above description and nowhere else.

neolit123 · 2019-08-25T19:18:46Z

@Arvinderpal yes, AFAIK the tool operates on existing merged PRs, so having it only fixed here should be fine.

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 14, 2019

k8s-ci-robot requested a review from dcbw June 14, 2019 14:18

k8s-ci-robot assigned yastij Jun 14, 2019

k8s-ci-robot requested a review from dchen1107 June 14, 2019 14:18

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 14, 2019

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 Jun 14, 2019