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

Add phase command for dynamic kubelet configuration in kubeadm. #57224

Merged
merged 5 commits into from Apr 17, 2018

Conversation

@xiangpengzhao
Member

xiangpengzhao commented Dec 15, 2017

What this PR does / why we need it:
As the title says.

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

Special notes for your reviewer:
/cc @luxas @fabriziopandini
@kubernetes/sig-cluster-lifecycle-pr-reviews

Release note:

Phase `kubeadm alpha phase kubelet` is added to support dynamic kubelet configuration in kubeadm.
var (
kubeletWriteInitConfigLongDesc = normalizer.LongDesc(`
Write init kubelet configuration to disk for dynamic kubelet configuration feature, functionally equivalent to what implemented by kubeadm init.

This comment has been minimized.

@fabriziopandini

fabriziopandini Dec 21, 2017

Contributor

kubeadm init executes both write-init-config and upload-dynamic-config, so I will remove "functionally equivalent to what implemented by kubeadm init" from the description of commands implementing only one of those actions.

As additional background on this comment, please note that in other phase command the sentency "functionally equivalent to what implemented by kubeadm init" is typically used only with the all subcommand that acts as a wrapper for all the available actions (e.g. kubeadm phase kubeconfig all).

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Dec 23, 2017

Member

Totally reasonable. Agreed. Thanks!

This comment has been minimized.

@xiangpengzhao
kubeletWriteInitConfigExample = normalizer.Examples(`
# Write init kubelet configuration to disk.
kubeadm alpha phase kubelet write-init-config

This comment has been minimized.

@fabriziopandini

fabriziopandini Dec 21, 2017

Contributor

What about adding --config to the examples?

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Dec 23, 2017

Member

Yeah, it'd be good to give user a more clear example. I was thinking of if I should use --config as the flag name for this command. It may cause confusing. --config is used to specify the kubeadm config. But in an example, users may think it as the config which is going to be written on the disk. @fabriziopandini any suggestions here?

This comment has been minimized.

@fabriziopandini

fabriziopandini Dec 24, 2017

Contributor

Good point.
IMO we should be consistent across command and always use --config to specify the kubeadm config, which is the case also for kubelet subcommands; what about:
a. always use configuration instead of config for kubelet phase subcommand names. e.g.

  • kubeadm alpha phase kubelet write-init-configuration
  • kubeadm alpha phase kubelet upload-dynamic-configuration

or

b. remove config from kubelet phase subcommand names

  • kubeadm alpha phase kubelet init
  • kubeadm alpha phase kubelet upload

and add the following sentence (or something similar) to long description of all commandsPlease note that the kubelet configuration can be passed to kubeadm as a value into the master configuration file.

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Dec 26, 2017

Member

I prefer the latter but I'm not sure if these commands are clear enough to users. Maybe we can give more tips in the description besides your suggested sentence. @luxas WDYT?

This comment has been minimized.

@errordeveloper

errordeveloper Jan 10, 2018

Member

I also prefer the second option, unless we can already think of other subcommands that may go in there. Also, it's alpha, so we can change later.

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Jan 15, 2018

Member

done with the second option.

`)
kubeletUploadDynamicConfigLongDesc = normalizer.LongDesc(`
Upload dynamic kubelet configuration to apiserver as ConfigMap, functionally equivalent to what implemented by kubeadm init.

This comment has been minimized.

@fabriziopandini

fabriziopandini Dec 21, 2017

Contributor

What about:
Uploads dynamic kubelet configuration as ConfigMap and links it to the current node as ConfigMapRef.

This comment has been minimized.

@xiangpengzhao

This comment has been minimized.

@xiangpengzhao
legacyscheme.Scheme.Default(cfg)
// This call returns the ready-to-use configuration based on the configuration file that might or might not exist and the default cfg populated by flags
internalcfg, err := configutil.ConfigFileAndDefaultsToInternalConfig(cfgPath, cfg)

This comment has been minimized.

@fabriziopandini

fabriziopandini Dec 21, 2017

Contributor

If I'm not mistaken, KubernetesVersion is not used by kubelet phase, so it is recommended to set this explicitly to some costant value in order to avoid the lookup of the version from the internet when executing ConfigFileAndDefaultsToInternalConfig. See e.g. here
The same applies below

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Dec 23, 2017

Member

Agreed. Will do.

This comment has been minimized.

@xiangpengzhao
Long: kubeletEnableDynamicConfigLongDesc,
Example: kubeletEnableDynamicConfigExample,
Run: func(cmd *cobra.Command, args []string) {
nodeName, err := getNodeName(cfgPath, cfg)

This comment has been minimized.

@fabriziopandini

fabriziopandini Dec 21, 2017

Contributor

I think that you can get the nodeName from the config file using configutil.ConfigFileAndDefaultsToInternalConfig, thus avoiding to replicate the initialisation logic for this field

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Dec 23, 2017

Member

Agreed. Will do.

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Jan 15, 2018

Member

I took another look at the code and found that configutil.ConfigFileAndDefaultsToInternalConfig only applies to MasterConfiguration whereas the cfg here is NodeConfiguration, so I can't use that func to get the nodeName :(

@fabriziopandini

First round of comments inline 😄

More general consideration: enable-dynamic-config implements a step of the kubeadm join sequence, which is not fully covered by kubeadm phases.

If we want to preserve this, we should consider if/how make clear to the users which phase to run on the master node and which phase to run on workers

@luxas luxas assigned kad and fabriziopandini and unassigned mikedanese Dec 22, 2017

@xiangpengzhao

This comment has been minimized.

Member

xiangpengzhao commented Dec 23, 2017

If we want to preserve this, we should consider if/how make clear to the users which phase to run on the master node and which phase to run on workers

Yeah we should document this :)

@kad

This comment has been minimized.

Member

kad commented Dec 27, 2017

Do we want to have some check that kubelet version is enough for that functionality to be executed ?

@xiangpengzhao

This comment has been minimized.

Member

xiangpengzhao commented Dec 27, 2017

Do we want to have some check that kubelet version is enough for that functionality to be executed ?

@kad in the current 1.10 cycle, the minimum version of kubelet which kubeadm supports is 1.9.0, and dynamic kubelet config feature is supported in 1.9.0, so I don't think we need to check the kubelet version. WDYT?

@kad

This comment has been minimized.

Member

kad commented Dec 27, 2017

@xiangpengzhao I think this check for minimum 1.9.0 kubelet is executed during init, but not sure if that check if performed in this phase command. Do I miss it ?

@xiangpengzhao

This comment has been minimized.

Member

xiangpengzhao commented Dec 27, 2017

@kad sounds reasonable to check it in phase command. 👍
From this point, I have another concern, should we should check the DKC feature gate in this phase command?

@xiangpengzhao xiangpengzhao changed the title from Add phase for dynamic kubelet configuration in kubeadm. to Add phase command for dynamic kubelet configuration in kubeadm. Dec 28, 2017

@kad

This comment has been minimized.

Member

kad commented Dec 28, 2017

@xiangpengzhao if we can easily do that from kubeadm, yes, that sounds reasonable to check that featuregate.

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jan 10, 2018

@xiangpengzhao please address comments in review :)

@xiangpengzhao

This comment has been minimized.

Member

xiangpengzhao commented Jan 15, 2018

Sorry for the delay. inline comments addressed. folks PTAL. Thanks!

@xiangpengzhao

This comment has been minimized.

Member

xiangpengzhao commented Jan 15, 2018

if we can easily do that from kubeadm, yes, that sounds reasonable to check that featuregate.

@kad I check the feature gate in the phase command and by doing so, I think we don't need to check kubelet version (supposing it's the right version if the feature gate is enabled). WDYT?

@kad

This comment has been minimized.

Member

kad commented Jan 15, 2018

@xiangpengzhao I think it is fine. Check the CI failures, squash implementation commits, rest looks good for me.

@xiangpengzhao

This comment has been minimized.

Member

xiangpengzhao commented Feb 24, 2018

Folks PTAL :)

@xiangpengzhao

This comment has been minimized.

Member

xiangpengzhao commented Feb 26, 2018

Added a commit e7f4290 (Add an alias update for subcommand enable) to fix kubernetes/kubeadm#571 (comment)

@@ -160,7 +160,8 @@ func NewCmdKubeletEnableDynamicConfig() *cobra.Command {
var cfgPath string
cmd := &cobra.Command{
Use: "enable",
Short: "Enables dynamic kubelet configuration on node",
Aliases: []string{"update"},
Short: "Enables of updates dynamic kubelet configuration on node",

This comment has been minimized.

@kad

kad Feb 26, 2018

Member

of -> or ?

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 27, 2018

Member

👍 👍 Thanks! Done :)

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 23, 2018

[MILESTONENOTIFIER] Milestone Removed From Pull Request

@fabriziopandini @jamiehannaford @kad @xiangpengzhao

Important: This pull request was missing labels required for the v1.11 milestone for more than 3 days:

kind: Must specify exactly one of kind/bug, kind/cleanup or kind/feature.

Help
@timothysc

/approve

federating the review to:
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews

@timothysc timothysc added the approved label Apr 13, 2018

@kargakis

This comment has been minimized.

Member

kargakis commented Apr 16, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 16, 2018

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: kad, kargakis, timothysc, xiangpengzhao

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

@fejta-bot

This comment has been minimized.

fejta-bot commented Apr 16, 2018

/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 comment for consistent failures.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 16, 2018

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

@xiangpengzhao

This comment has been minimized.

Member

xiangpengzhao commented Apr 17, 2018

/retest

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 17, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit 923f6c5 into kubernetes:master Apr 17, 2018

14 of 15 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation xiangpengzhao authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@xiangpengzhao xiangpengzhao deleted the xiangpengzhao:kubeadm-phase-kubelet branch Apr 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment