Make kubelet base configuration configmap with version #60287

xiangpengzhao · 2018-02-23T09:26:51Z

What this PR does / why we need it:
kubernetes/kubeadm#571 (comment)

I've just upgraded the kubelet (which is associated with the v1.9 configmap) to v1.10, I now need to repoint it to the v1.10 config.

UPDATE:
This PR can't fix the comment above. To fix it, one possible approach is as suggested in the same comment: maybe we can just document to use kubeadm phase kubelet config dynamic enable-for-node and make an alias for it as well. The alias have been added in #57224.

This PR only makes the name of configmap version-sensitive instead of hard-coding the name with "1.9".

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

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

Release note:

NONE

xiangpengzhao · 2018-02-23T09:40:12Z

Oh no, this is incorrect in some cases.

luxas

A couple of improvement suggestions. Thanks @xiangpengzhao for the PR!

luxas · 2018-02-23T11:06:29Z

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

@@ -55,9 +58,17 @@ func CreateBaseKubeletConfiguration(cfg *kubeadmapi.MasterConfiguration, client
 		return err
 	}

+	var majorMinorVersion string
+	version := strings.Split(strings.TrimPrefix(cfg.KubernetesVersion, "v"), ".")


I'd use pkg/util/version.ParseSemantic here and then use fmt.Sprintf("%s.%s", v.Major(), v.Minor())

luxas · 2018-02-23T11:07:31Z

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

+	if err != nil {
+		majorMinorVersion = "1.9"
+	} else {
+		majorMinorVersion = string(kubeletVersion.Major()) + "." + string(kubeletVersion.Minor())


nit: fmt.Sprintf("%s.%s", kubeletVersion.Major(), kubeletVersion.Minor())

luxas · 2018-02-23T11:08:10Z

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

+	var majorMinorVersion string
+	kubeletVersion, err := preflight.GetKubeletVersion(utilsexec.New())
+	if err != nil {
+		majorMinorVersion = "1.9"


just fallback on kubeadm's own version here instead in a generic sense or something?
you can use pkg/version for that

luxas · 2018-02-23T11:08:24Z

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

+	if len(version) == 3 {
+		majorMinorVersion = version[len(version)-1]
+	} else {
+		majorMinorVersion = "1.9"


get this value dynamically?

timothysc

/approve

k8s-ci-robot · 2018-02-23T21:42:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~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

xiangpengzhao · 2018-02-26T08:04:16Z

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

@@ -55,9 +56,25 @@ func CreateBaseKubeletConfiguration(cfg *kubeadmapi.MasterConfiguration, client
 		return err
 	}

+	var configmapName string
+	k8sVer, k8sErr := version.ParseSemantic(cfg.KubernetesVersion)


The idea in this PR is:
For master and init:
If kubernetes version is configured and can be parsed, use this version as the post-fix of configmap name, e.g., "-1.9"; else use kubeadm version.
For node and join:
If kubelet version of the node can be gotten, use this version when getting configmap, else use kubeadm version.

But this will cause issues in below cases:
Suppose kubeadm version is 1.10.x and kubernetes version is 1.9.x and kubelet version is 1.10.x:

master parses kubernetes version successfully and writes configmap version as 1.9;

node gets kubelet version as 1.10 and tries to get 1.10 configmap, then fails.

Suppose kubeadm version is 1.10.x and kubernetes version is 1.9.x and kubelet version is 1.9.x:

master parses kubernetes version failed but gets kubeadm version successfully and writes configmap version as 1.10;

node gets kubelet version as 1.9 and tries to get 1.9 configmap, then fails.

Suppose kubeadm version is 1.10.x and kubernetes version is 1.10.x and kubelet version is 1.9.x:

master parses kubernetes version or gets kubeadm version successfully and writes configmap version as 1.10;

node gets kubelet version as 1.9 and tries to get 1.9 configmap, then fails.

@luxas @timothysc any suggestions? cc @kubernetes/sig-cluster-lifecycle-pr-reviews

/hold

I hope kubernetes version (master) at 1.9.x with a kubelet at 1.10.x is impossible in all deployment scenarios. Masters should always be upgraded before clients.

I don't see why should the kubeadm version or the master version should ever be used to determine the config the kubelet gets. The kubelet version is what matters.

I hope kubernetes version (master) at 1.9.x with a kubelet at 1.10.x is impossible in all deployment scenarios. Masters should always be upgraded before clients.

agreed.

I don't see why should the kubeadm version or the master version should ever be used to determine the config the kubelet gets. The kubelet version is what matters.

(base) kubelet config is embedded in MasterConfiguration of kubeadm, so I think the kubeadm version matters in this case. The kubelet version is actually what matters though.

Are we just talking about the kubelet on the master here? I thought we were talking about all kubelets (masters and nodes).

No, we are talking about all kubelets as you thought :)
Now I'm thinking of whether we need to enable the DKC feature in kubeadm. If so, what benifits does this bring us? Currently what kubeadm does wrt DKC is writing/uploading the base kubelet configuration and making the nodes consume it. But since --init-config-dir has been removed and --config is supported, seems like the base kubelet configuration is no longer needed. Maybe we want to remove the DKC support in kubeadm? @mtaufen @luxas WDYT?

Shouldn't kubeadm install the config file for --config (which is the base kubelet configuration)?

It should. (https://kubernetes.io/docs/reference/setup-tools/kubeadm/kubeadm-init/#kubelet-drop-in)

But I'm not sure:

if kubeadm still should configure anything for kubelet in MasterConfiguration.KubeletConfiguration and create the base ConfigMap.

if kubeadm should create an base ConfigMap via the config file for --config;

I think kubelet itself will create the ConfigMap from its --config file so that kubeadm don't need to do anything for DKC. (I don't follow the very details of the latest DKC/--config mechanism. Feel free to correct me if I'm wrong)

The Kubelet does not create ConfigMaps nor does it configure Node.Spec.ConfigSource (the latter is blocked by the noderestriction admission controller - it's effectively a privilege escalation).

It may be better for Kubeadm to focus on --config and hold off trying to manage DKC for now - at least until DKC is beta. There is coordination involved with DKC that may be more appropriate to an always-on controller - like what is being designed in the Machines API.

It may be better for Kubeadm to focus on --config

@mtaufen do you mean migrating /etc/systemd/system/kubelet.service.d/10-kubeadm.conf to use --config, or anything else?

and hold off trying to manage DKC for now - at least until DKC is beta.

agreed.

xiangpengzhao · 2018-02-26T08:06:50Z

@luxas inline comments addressed. However, there are some cases should be addressed: #60287 (comment)

aoxn · 2018-02-26T11:31:16Z

@xiangpengzhao As each kubelet may have it own custmized config, shall we consider preparing a kubelet config map for each node instead of global kubelet config.

xiangpengzhao · 2018-02-26T12:06:55Z

As each kubelet may have it own custmized config, shall we consider preparing a kubelet config map for each node instead of global kubelet config.

@aoxn all of the kubelets in a kubeadm cluster nodes have the same config so far, but it sounds reasonable to customize kubelet config after install the cluster.

aoxn · 2018-02-26T12:23:19Z

Will this feature be proposed ASAP? @xiangpengzhao

xiangpengzhao · 2018-02-27T02:35:13Z

Will this feature be proposed ASAP?

I'd like to get the idea from @mtaufen . What is done by kubeadm is to provide the initial base config for kubelet. You can still customize your kubelet config with the DKC feature of kubelet I think.

mtaufen · 2018-02-27T22:43:46Z

If kubeadm is installing a new kubelet binary, and needs to tweak the base config in unison for some reason, it should also replace the base config at the same time as the binary.

Part of the kubeletconfig API contract is that binary upgrades are backwards-compatible with your old config settings.

Operationally, there are obviously cases where you need to version-pin dynamic config (bugs), but this functionality should probably be delegated to a higher-level controller. If kubeadm wants to manage these dynamic assignments, then ok. Make sure you can handle setting the correct config on new nodes that appear in autoscaling scenarios.

xiangpengzhao · 2018-04-13T08:44:50Z

@luxas @timothysc do we want to migrate the kubelet deb package to use --config file now?

timothysc · 2018-04-13T17:47:46Z

@xiangpengzhao - soon...
@mtaufen - what's the state?

mtaufen · 2018-04-13T18:19:59Z

--config is beta in 1.10

…

On Fri, Apr 13, 2018, 10:49 AM Timothy St. Clair ***@***.***> wrote: @xiangpengzhao <https://github.com/xiangpengzhao> - soon... @mtaufen <https://github.com/mtaufen> - what's the state? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#60287 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3JwcdAHC4tT8Z59aBzfb69a55LTyqEks5toOVAgaJpZM4SQmOu> .

xiangpengzhao · 2018-04-16T03:13:45Z

@timothysc is it OK to migrate deb while --config is beta or should we wait for it being GA?

k8s-github-robot · 2018-04-19T08:15:16Z

[MILESTONENOTIFIER] Milestone Removed From Pull Request

@timothysc @xiangpengzhao @kubernetes/sig-cluster-lifecycle-misc

Important: This pull request was missing the status/approved-for-milestone label for more than 7 days.

Help

timothysc · 2018-04-25T21:33:43Z

@xiangpengzhao - we should incorporate, no need to wait on GA b/c backward compat guarantees.

k8s-ci-robot · 2018-05-10T22:55:29Z

@xiangpengzhao: PR needs rebase.

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.

timothysc · 2018-05-14T19:07:20Z

/assign @luxas

luxas · 2018-05-21T05:14:12Z

I'm implementing a bit similar but other approach to fix this in #63887. Thanks for the initial work here to make us understand better what to do :)!

xiangpengzhao · 2018-07-16T06:13:03Z

Thanks @luxas !
BTW, sorry for having been absent from the community for the last several months for some reasons :)

k8s-ci-robot requested a review from luxas February 23, 2018 09:26

luxas reviewed Feb 23, 2018

View reviewed changes

timothysc reviewed Feb 23, 2018

View reviewed changes

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

timothysc self-assigned this Feb 24, 2018

xiangpengzhao force-pushed the kubeadm-kubelet-upgrade-configmap branch from 99c9364 to 775db4c Compare February 26, 2018 07:31

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 Feb 26, 2018

xiangpengzhao commented Feb 26, 2018

View reviewed changes

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

xiangpengzhao added 2 commits February 26, 2018 16:24

Make kubelet base configuration configmap with version

b046ab9

Add auto generated BUILD files.

e07fd5b

xiangpengzhao force-pushed the kubeadm-kubelet-upgrade-configmap branch from 775db4c to e07fd5b Compare February 26, 2018 08:26

timothysc added this to the v1.11 milestone Mar 9, 2018

timothysc added the area/kubeadm label Mar 9, 2018

k8s-github-robot added milestone/incomplete-labels milestone/removed and removed milestone/incomplete-labels labels Mar 20, 2018

k8s-github-robot added milestone/incomplete-labels and removed milestone/removed labels Apr 11, 2018

timothysc added milestone/removed do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. and removed milestone/incomplete-labels do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 11, 2018

k8s-github-robot added milestone/incomplete-labels and removed milestone/removed labels Apr 11, 2018

timothysc added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/feature Categorizes issue or PR as related to a new feature. labels Apr 11, 2018

k8s-github-robot added milestone/needs-approval and removed milestone/incomplete-labels labels Apr 11, 2018

k8s-github-robot added the milestone/removed label Apr 19, 2018

k8s-github-robot removed the milestone/needs-approval label Apr 19, 2018

k8s-github-robot removed this from the v1.11 milestone Apr 19, 2018

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

timothysc removed the do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. label May 11, 2018

k8s-ci-robot assigned luxas May 14, 2018

luxas closed this May 21, 2018

xiangpengzhao deleted the kubeadm-kubelet-upgrade-configmap branch July 16, 2018 06:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make kubelet base configuration configmap with version #60287

Make kubelet base configuration configmap with version #60287

xiangpengzhao commented Feb 23, 2018 •

edited

Loading

xiangpengzhao commented Feb 23, 2018

luxas left a comment

luxas Feb 23, 2018

luxas Feb 23, 2018

luxas Feb 23, 2018

luxas Feb 23, 2018

timothysc left a comment

k8s-ci-robot commented Feb 23, 2018

xiangpengzhao Feb 26, 2018

mtaufen Feb 27, 2018

xiangpengzhao Mar 20, 2018

mtaufen Mar 20, 2018

xiangpengzhao Mar 21, 2018

mtaufen Mar 21, 2018

xiangpengzhao Mar 22, 2018

mtaufen Mar 22, 2018

xiangpengzhao Apr 3, 2018

xiangpengzhao commented Feb 26, 2018

aoxn commented Feb 26, 2018

xiangpengzhao commented Feb 26, 2018

aoxn commented Feb 26, 2018

xiangpengzhao commented Feb 27, 2018

mtaufen commented Feb 27, 2018

xiangpengzhao commented Apr 13, 2018

timothysc commented Apr 13, 2018

mtaufen commented Apr 13, 2018 via email

xiangpengzhao commented Apr 16, 2018

k8s-github-robot commented Apr 19, 2018

timothysc commented Apr 25, 2018

k8s-ci-robot commented May 10, 2018

timothysc commented May 14, 2018

luxas commented May 21, 2018

xiangpengzhao commented Jul 16, 2018

Make kubelet base configuration configmap with version #60287

Make kubelet base configuration configmap with version #60287

Conversation

xiangpengzhao commented Feb 23, 2018 • edited Loading

xiangpengzhao commented Feb 23, 2018

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangpengzhao commented Feb 26, 2018

aoxn commented Feb 26, 2018

xiangpengzhao commented Feb 26, 2018

aoxn commented Feb 26, 2018

xiangpengzhao commented Feb 27, 2018

mtaufen commented Feb 27, 2018

xiangpengzhao commented Apr 13, 2018

timothysc commented Apr 13, 2018

mtaufen commented Apr 13, 2018 via email

xiangpengzhao commented Apr 16, 2018

k8s-github-robot commented Apr 19, 2018

timothysc commented Apr 25, 2018

k8s-ci-robot commented May 10, 2018

timothysc commented May 14, 2018

luxas commented May 21, 2018

xiangpengzhao commented Jul 16, 2018

xiangpengzhao commented Feb 23, 2018 •

edited

Loading