[kubeadm]fix DynamicKubeletConfig feature to beta by stewart-yu · Pull Request #65576 · kubernetes/kubernetes

stewart-yu · 2018-06-28T10:01:17Z

What this PR does / why we need it:
As PR get merged, DynamicKubeletConfig feature convert to beta in v1.11, and set to true in default.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #kubernetes/kubeadm#957

Special notes for your reviewer:

Release note:

NONE

neolit123 · 2018-06-28T20:46:14Z

@stewart-yu some unit tests are not happy.

@kubernetes/sig-cluster-lifecycle-pr-reviews

neolit123

added some notes about not handling feature-gate==false

neolit123 · 2018-06-28T20:50:01Z

cmd/kubeadm/app/phases/kubelet/flags.go

neolit123 · 2018-06-28T20:51:04Z

cmd/kubeadm/app/cmd/init.go

[1] what if the user has disabled the DKC via the feature gate?

neolit123 · 2018-06-28T20:51:20Z

cmd/kubeadm/app/cmd/join.go

timothysc · 2018-06-28T21:35:41Z

/hold

We haven't decided to default or not in 1.11. I think before we do we need to make certain we'd thought through the implications of enabling by default. You can easily brick a whole cluster on a single config change.

Please open an issue in the kubeadm repo and outline your motivation and we can discuss within the SIG.

/cc @luxas

stewart-yu · 2018-06-29T01:02:58Z

sorry, my fault. @neolit123 @timothysc
misunderstand about set true as default and GA, revert some change now.

stewart-yu · 2018-06-29T05:29:14Z

/retest

timothysc

Before we can approve this we need to answer a bunch of other questions.

What is the test plan for this?
How are we going to upgrade legacy?

/cc @rdodev

k8s-ci-robot · 2018-07-02T16:26:01Z

@timothysc: GitHub didn't allow me to request PR reviews from the following users: rdodev.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

Before we can approve this we need to answer a bunch of other questions.

What is the test plan for this?
How are we going to upgrade legacy?

/cc @rdodev

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.

fabriziopandini · 2018-07-03T06:33:57Z

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

Being dynamic kubelet beta and enabled by default, could you kindly confirm that it is still the case we should set DynamicKubeletConfig=true. I was expecting the logic here to be changed (set false when we don't want dynamic kubelet)....

fabriziopandini

I agree with @timothysc that we should discuss this further at SIG level. In the meantime please answer to the question ^^^

stewart-yu · 2018-07-03T09:09:55Z

Actually, i'm little knowledge about DynamicKubeletConfig in kubeadm how works
@fabriziopandini @timothysc thanks for your attention, i think may be need deeply investigate. If all done, will ping you back

timothysc · 2018-07-03T18:19:16Z

@stewart-yu - We discussed this on the sig call today, and have decided that we do not want to enable this by default, but update to beta in the feature gates.

stewart-yu · 2018-07-04T02:12:09Z

@timothysc thanks
PR updated

dixudx · 2018-07-04T02:19:49Z

cmd/kubeadm/app/features/features.go

 	HighAvailability:     {FeatureSpec: utilfeature.FeatureSpec{Default: false, PreRelease: utilfeature.Alpha}, HiddenInHelpText: true},
 	CoreDNS:              {FeatureSpec: utilfeature.FeatureSpec{Default: true, PreRelease: utilfeature.GA}},
-	DynamicKubeletConfig: {FeatureSpec: utilfeature.FeatureSpec{Default: false, PreRelease: utilfeature.Alpha}},
+	DynamicKubeletConfig: {FeatureSpec: utilfeature.FeatureSpec{Default: false, PreRelease: utilfeature.Beta}},


We normally enable beta features by default. Also this is already enabled in v1.11 kubelet. We should make it consistent. @timothysc

This is correct, we had long conversation on the sig

dixudx · 2018-07-04T02:20:13Z

cmd/kubeadm/app/phases/kubelet/dynamic.go

 	fmt.Printf("[kubelet] Enabling Dynamic Kubelet Config for Node %q; config sourced from ConfigMap %q in namespace %s\n",
 		nodeName, configMapName, metav1.NamespaceSystem)
-	fmt.Println("[kubelet] WARNING: The Dynamic Kubelet Config feature is alpha and off by default. It hasn't been well-tested yet at this stage, use with caution.")
+	fmt.Println("[kubelet] WARNING: The Dynamic Kubelet Config feature is beta, but off by default. It hasn't been well-tested yet at this stage, use with caution.")


is beta, and enabled by default.

timothysc

/lgtm
/approve

timothysc · 2018-07-13T20:32:19Z

cmd/kubeadm/app/features/features.go

 	HighAvailability:     {FeatureSpec: utilfeature.FeatureSpec{Default: false, PreRelease: utilfeature.Alpha}, HiddenInHelpText: true},
 	CoreDNS:              {FeatureSpec: utilfeature.FeatureSpec{Default: true, PreRelease: utilfeature.GA}},
-	DynamicKubeletConfig: {FeatureSpec: utilfeature.FeatureSpec{Default: false, PreRelease: utilfeature.Alpha}},
+	DynamicKubeletConfig: {FeatureSpec: utilfeature.FeatureSpec{Default: false, PreRelease: utilfeature.Beta}},


This is correct, we had long conversation on the sig

k8s-ci-robot · 2018-07-13T20:33:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stewart-yu, timothysc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~cmd/kubeadm/OWNERS~~ [timothysc]

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

stewart-yu · 2018-07-14T05:01:33Z

Thanks. @timothysc
Removing /hold label by myself
/hold cancel

k8s-github-robot · 2018-07-14T08:46:45Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-07-14T10:04:43Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from dixudx and fabriziopandini June 28, 2018 10:01

stewart-yu force-pushed the stewart-kubelet-read#02 branch 3 times, most recently from 7d96bfe to 0883365 Compare June 28, 2018 10:44

neolit123 reviewed Jun 28, 2018

View reviewed changes

k8s-ci-robot requested a review from luxas June 28, 2018 21:35

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 28, 2018

stewart-yu force-pushed the stewart-kubelet-read#02 branch from 0883365 to 1249a13 Compare June 29, 2018 00:59

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

stewart-yu force-pushed the stewart-kubelet-read#02 branch 3 times, most recently from bd86f0a to 2c42c14 Compare June 29, 2018 02:45

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 Jun 29, 2018

stewart-yu force-pushed the stewart-kubelet-read#02 branch 3 times, most recently from 8f146fd to b41f72a Compare June 29, 2018 03:46

luxas assigned timothysc and luxas Jun 29, 2018

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2018

stewart-yu force-pushed the stewart-kubelet-read#02 branch from b41f72a to c93b6eb Compare July 2, 2018 01:06

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2018

stewart-yu force-pushed the stewart-kubelet-read#02 branch from c93b6eb to 354512a Compare July 2, 2018 01:53

timothysc reviewed Jul 2, 2018

View reviewed changes

fabriziopandini reviewed Jul 3, 2018

View reviewed changes

fix DynamicKubeletConfig feature to beta

37fdd1d

stewart-yu force-pushed the stewart-kubelet-read#02 branch from 354512a to 37fdd1d Compare July 4, 2018 00:53

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 4, 2018

dixudx suggested changes Jul 4, 2018

View reviewed changes

timothysc approved these changes Jul 13, 2018

View reviewed changes

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

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

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 14, 2018

k8s-github-robot merged commit 49454a2 into kubernetes:master Jul 14, 2018

CecileRobertMichon mentioned this pull request Sep 18, 2020

Support dynamic kubelet configuration kubernetes-sigs/cluster-api-provider-azure#944

Closed

Conversation

stewart-yu commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neolit123 commented Jun 28, 2018

Uh oh!

neolit123 left a comment

Choose a reason for hiding this comment

Uh oh!

neolit123 Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

neolit123 Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neolit123 Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

timothysc commented Jun 28, 2018

Uh oh!

stewart-yu commented Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stewart-yu commented Jun 29, 2018

Uh oh!

timothysc left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jul 2, 2018

Uh oh!

fabriziopandini Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabriziopandini left a comment

Choose a reason for hiding this comment

Uh oh!

stewart-yu commented Jul 3, 2018

Uh oh!

timothysc commented Jul 3, 2018

Uh oh!

stewart-yu commented Jul 4, 2018

Uh oh!

dixudx Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timothysc Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

dixudx Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

timothysc left a comment

Choose a reason for hiding this comment

Uh oh!

timothysc Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jul 13, 2018

Uh oh!

stewart-yu commented Jul 14, 2018

Uh oh!

k8s-github-robot commented Jul 14, 2018

Uh oh!

k8s-github-robot commented Jul 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

stewart-yu commented Jun 28, 2018 •

edited

Loading

neolit123 Jun 28, 2018 •

edited

Loading

stewart-yu commented Jun 29, 2018 •

edited

Loading

fabriziopandini Jul 3, 2018 •

edited

Loading

dixudx Jul 4, 2018 •

edited

Loading