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

Dual-Stack Integration with Kubeadm #79033

Merged
merged 2 commits into from Aug 16, 2019

Conversation

@Arvinderpal
Copy link
Contributor

commented Jun 14, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

/assign @yastij
/assign @aojea

@k8s-ci-robot k8s-ci-robot requested a review from dcbw Jun 14, 2019

@k8s-ci-robot k8s-ci-robot requested a review from dchen1107 Jun 14, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

/sig cluster-lifecycle

@yastij

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

/priority important-soon

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 comment has been minimized.

Copy link
@aojea

aojea Jun 14, 2019

Contributor

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

This comment has been minimized.

Copy link
@Arvinderpal

Arvinderpal Jun 20, 2019

Author Contributor

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, ",")

This comment has been minimized.

Copy link
@neolit123

neolit123 Jul 10, 2019

Member

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!

This comment has been minimized.

Copy link
@Arvinderpal

Arvinderpal Jul 13, 2019

Author Contributor

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.

@neolit123
Copy link
Member

left a comment

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

/approve
/ok-to-test

@@ -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 comment has been minimized.

Copy link
@yastij

yastij Aug 8, 2019

Member

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

cc @neolit123 @rosti

This comment has been minimized.

Copy link
@neolit123

neolit123 Aug 8, 2019

Member

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?

This comment has been minimized.

Copy link
@yastij

yastij Aug 8, 2019

Member

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

This comment has been minimized.

Copy link
@yastij

yastij Aug 8, 2019

Member

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

This comment has been minimized.

Copy link
@Arvinderpal

Arvinderpal Aug 8, 2019

Author Contributor

Done!

@Arvinderpal Arvinderpal force-pushed the Nordix:kubeadm-ds-pod-network-cidr branch from 0caa164 to 3a8eb00 Aug 8, 2019

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

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

Arvinderpal added 2 commits Jun 24, 2019
Update kubeproxy config validation to support list of comma
separated pod CIDRs. Dual-stack feature must be enabled for the
validation to be done.
kubeadm --pod-network-cidr supports a comma separated list of pod
CIDRs. This is a necesary change for dual-stack.

@Arvinderpal Arvinderpal force-pushed the Nordix:kubeadm-ds-pod-network-cidr branch from 3a8eb00 to 3ac7ae6 Aug 9, 2019

@Arvinderpal Arvinderpal changed the title [WIP] Dual-Stack Integration with Kubeadm Dual-Stack Integration with Kubeadm Aug 9, 2019

@Arvinderpal

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

@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

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Thanks!

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

[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:

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

@aojea

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

@fejta-bot

This comment has been minimized.

Copy link

commented Aug 16, 2019

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

2 similar comments
@fejta-bot

This comment has been minimized.

Copy link

commented Aug 16, 2019

/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

This comment has been minimized.

Copy link

commented Aug 16, 2019

/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 k8s-ci-robot merged commit e6d4273 into kubernetes:master Aug 16, 2019

23 checks passed

cla/linuxfoundation Arvinderpal authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 16, 2019

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

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

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.