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

Propagate kubeadm dual-stack feature-gate to all k8s components #80531

Merged
merged 1 commit into from
Jul 31, 2019

Conversation

Arvinderpal
Copy link
Contributor

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR propagates the kubeadm feature-gate, if set in ClusterConfiguration, to the all individual kubernetes components on the control-plane node.

kind: ClusterConfiguration
featureGates:
  IPv6DualStack: true

For worker nodes, users will have to specify the dual-stack feature-gate to kubelet using the nodeRegistration.kubeletExtraArgs option as part of their join config. Alternatively, they can use KUBELET_EXTRA_ARGS as well.

Which issue(s) this PR fixes:

This addresses one of the TODO items in the dual-stack kubeadm tracker ticket: kubernetes/kubeadm#1612
It's a follow up to the initial kubeadm dual-stack feature-gate PR: #80145

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

In order to enable dual-stack support within kubeadm and kubernetes components, as part of the init config file, the user should set feature-gate IPv6DualStack=true in the ClusterConfiguration. Additionally, for each worker node, the user should set the feature-gate for kubelet using either nodeRegistration.kubeletExtraArgs or  KUBELET_EXTRA_ARGS.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 24, 2019
@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 24, 2019
@Arvinderpal
Copy link
Contributor Author

@neolit123 @yastij @fabriziopandini @aojea
As requested, this PR propagates the dual-stack feature-gate flag to all kubernetes components.
I made this a separate PR from the comma separated list support for --cluster-cidr. I did not want to mix these two together.

@neolit123
Copy link
Member

/assign
/priority important-longterm
/ok-to-test

@k8s-ci-robot k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 24, 2019
@Arvinderpal Arvinderpal force-pushed the kubeadm-ds-FG-propagate branch 2 times, most recently from 0556136 to 86be728 Compare July 24, 2019 22:33
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

thanks @Arvinderpal
added minor comments, but overall this is looking good.

cmd/kubeadm/app/util/config/common.go Outdated Show resolved Hide resolved
@@ -102,6 +105,11 @@ func SetNodeRegistrationDynamicDefaults(cfg *kubeadmapi.NodeRegistrationOptions,
klog.V(1).Infof("detected and using CRI socket: %s", cfg.CRISocket)
}

if isDualStack {
Copy link
Member

Choose a reason for hiding this comment

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

can be:
if features.Enabled(cfg.ClusterConfiguration.FeatureGates, features.IPv6DualStack) {
to avoid passing the isDualStack arg
[1]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That won't work since we pass in cfg *kubeadmapi.NodeRegistrationOptions and not ClusterConfiguration

Copy link
Member

Choose a reason for hiding this comment

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

ah i see, so one solution is to make SetNodeRegistrationDynamicDefaults accept ClusterConfiguration (it think it used to be like that at some point) 🤔

@rosti WDYT?
/cc @rosti

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the place to do the feature gate patching for the kubelet. The correct place is buildKubeletArgMap:

func buildKubeletArgMap(opts kubeletFlagsOpts) map[string]string {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rosti I see a problem with how BuildArgumentListFromMap works today -- it does not handle comma separated fields like "feature-gates", since it does a simple overwrite of default values with user specified values. For example, if we set IPv6DualStack=true in base, then that will be overwritten if the user has specified some additional feature-gates in KubeletExtraArgs.

We have a couple of options:

  1. Make BuildArgumentListFromMap capable of "merging" fields like feature-gates rather than simple key/value overwrite. We would do something like this:
for k, v := range overrideArguments {
		if k == "feature-gates" && len(argsMap[k]) > 0{
			argsMap[k] += "," + v
		}else{
			argsMap[k] = v
		}
	}
  1. Modify top-level funcs like WriteKubeletDynamicEnvFile so that they perform the merge on feature-gates. This would require updating the argList list returned by BuildArgumentListFromMap. This is not ideal, IMO.

We could also just leave the code in the higher-level funcs as it is in the current PR.
I'll wait for you feedback before proceeding. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep things simple for now and just allow the users to overwrite the whole of the feature-gates flags if they wish.
Either way, this is going to be documented as part of the feature gate documentation.
Another possibility is to modify BuildArgumentListFromMap to warn on overwrites.

Certainly, we should not do this in the dynamic defaulting code.

@fabriziopandini @neolit123 WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

i think that @rosti is correct here that dynamic defaults is not the best place.

Let's keep things simple for now and just allow the users to overwrite the whole of the feature-gates flags if they wish.

makes sense to me.

passing the dual stack feature gate to the kubelet will be implicit (if the kubeadm FG is on). so if the user provides their own feature-gates for the kubelet extraArgs, this should overwrite the implicit gate but should also show a warning.

Either way, this is going to be documented as part of the feature gate documentation.

we might add a sentence about overrides, yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

cmd/kubeadm/app/util/config/initconfiguration.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/config/initconfiguration.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/config/initconfiguration.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/config/common.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/config/common_test.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/config/common_test.go Outdated Show resolved Hide resolved
@neolit123
Copy link
Member

/retest

Copy link
Contributor Author

@Arvinderpal Arvinderpal left a comment

Choose a reason for hiding this comment

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

@neolit123 Thanks for the feedback. Please see my comments.

@@ -102,6 +105,11 @@ func SetNodeRegistrationDynamicDefaults(cfg *kubeadmapi.NodeRegistrationOptions,
klog.V(1).Infof("detected and using CRI socket: %s", cfg.CRISocket)
}

if isDualStack {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That won't work since we pass in cfg *kubeadmapi.NodeRegistrationOptions and not ClusterConfiguration

cmd/kubeadm/app/util/config/initconfiguration.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/config/initconfiguration.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/config/initconfiguration.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/config/common.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/config/common_test.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/config/common_test.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot requested a review from rosti July 25, 2019 02:08
@aojea
Copy link
Member

aojea commented Jul 25, 2019

/test pull-kubernetes-integration

Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

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

Thanks @Arvinderpal !
However, we need the feature gate propagation to be done at a more lower code level, than in the config dynamic defaulting code.

@@ -102,6 +105,11 @@ func SetNodeRegistrationDynamicDefaults(cfg *kubeadmapi.NodeRegistrationOptions,
klog.V(1).Infof("detected and using CRI socket: %s", cfg.CRISocket)
}

if isDualStack {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the place to do the feature gate patching for the kubelet. The correct place is buildKubeletArgMap:

func buildKubeletArgMap(opts kubeletFlagsOpts) map[string]string {

cmd/kubeadm/app/util/config/initconfiguration.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 29, 2019
@Arvinderpal
Copy link
Contributor Author

@rosti @neolit123 Thank you for the feedback. I have made the changes requested.
It actually makes things a lot simpler now :)

Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

thanks for the update
/lgtm
approve on @rosti

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2019
@neolit123
Copy link
Member

looks like some build tests are failing.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2019
Copy link
Member

@yastij yastij left a comment

Choose a reason for hiding this comment

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

just some nits, also +1 on the previous feedback @rosti

enabled, present := cfg.FeatureGates[features.IPv6DualStack]
if present && enabled {
defaultArguments["feature-gates"] = features.IPv6DualStack + "=true"
} else if present && !enabled {
Copy link
Member

Choose a reason for hiding this comment

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

why not just else ? (the remaining case is !present)

Copy link
Contributor

Choose a reason for hiding this comment

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

[1] Ideally, we would like to cover the present==true case. So something like this should work:

if present {
        defaultArguments["feature-gates"] = fmt.Sprintf("%s=%t", features.IPv6DualStack, enabled)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

cmd/kubeadm/app/phases/controlplane/manifests.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/phases/kubelet/flags.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

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

Thanks @Arvinderpal !
Leaving final lgtm to @yastij or @neolit123
/approve

enabled, present := cfg.FeatureGates[features.IPv6DualStack]
if present && enabled {
defaultArguments["feature-gates"] = features.IPv6DualStack + "=true"
} else if present && !enabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

[1] Ideally, we would like to cover the present==true case. So something like this should work:

if present {
        defaultArguments["feature-gates"] = fmt.Sprintf("%s=%t", features.IPv6DualStack, enabled)
}

cmd/kubeadm/app/phases/controlplane/manifests.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/phases/controlplane/manifests.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/phases/kubelet/flags.go Outdated Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Arvinderpal, rosti

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2019
progagates the kubeadm FG to the individual k8scomponents
on the control-plane node.

* Note: Users who want to join worker nodes to the cluster
will have to specify the dual-stack FG to kubelet using the
nodeRegistration.kubeletExtraArgs option as part of their
join config. Alternatively, they can use KUBELET_EXTRA_ARGS.

kubeadm FG: kubernetes/kubeadm#1612
@yastij
Copy link
Member

yastij commented Jul 30, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2019
@Arvinderpal
Copy link
Contributor Author

/retest

@fejta-bot
Copy link

/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
Copy link

/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
Copy link

/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 5bfa366 into kubernetes:master Jul 31, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Jul 31, 2019
@k8s-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce 585ef37 link /test pull-kubernetes-e2e-gce

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.

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. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants