kubeadm: Extended KubeletVersionCheck #54868

kad · 2017-10-31T17:26:42Z

What this PR does / why we need it:
KubeletVersionCheck now able to detect if kubelet version
is higher than control plane. As this might lead to malfunctional
cluster setups, kubeadm will give warning.

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#496

Special notes for your reviewer:
/sig cluster-lifecycle
/area kubeadm

Release note:

- kubeadm will produce error if kubelet too new for control plane

kad · 2017-10-31T20:15:31Z

/test pull-kubernetes-bazel-test

kad · 2017-11-03T13:01:44Z

@krousey @dmmcquay any opinion on PR ?

luxas

Thanks for this PR, great!
I left some comments

luxas · 2017-11-08T16:54:38Z

cmd/kubeadm/app/preflight/checks.go

+		if err != nil {
+			return nil, []error{fmt.Errorf("couldn't parse kubernetes version %q: %v", kubever.KubernetesVersion, err)}
+		}
+		firstUnsupportedKubeletVersion := versionutil.MustParseSemantic(fmt.Sprintf("%d.%d.%s", k8sVersion.Major(), k8sVersion.Minor()+1, "0-0"))


This can be simpler I think... Why not just do if kubeletVersion.Minor() > k8sVersion.Minor() => error?

While developing next minor version, it is fine to have kubelet a bit newer than control plane.
e.g. testing kubelet 1.9.0-alpha.3+gitXYZ on top of 1.8.x stable cluster. Just be a bit more handy for developers.
Release 1.9.0 will complain, but development builds before 1.9.0 release will be ok.

luxas · 2017-11-08T16:56:04Z

cmd/kubeadm/app/preflight/checks.go

+		}
+		firstUnsupportedKubeletVersion := versionutil.MustParseSemantic(fmt.Sprintf("%d.%d.%s", k8sVersion.Major(), k8sVersion.Minor()+1, "0-0"))
+		if kubeletVersion.AtLeast(firstUnsupportedKubeletVersion) {
+			return []error{fmt.Errorf("kubernetes version is lower than kubelet version. This may lead to malfunctional cluster setup. Kubernetes version: %s. Kubelet version: %s", k8sVersion, kubeletVersion)}, nil


suggest: The kubelet version is higher than the control plane version. This is not a supported version skew and may lead to a malfunctional cluster. Kubelet version: %s. Control plane version: %s.

Also I think this should be an error

I'd suggest to keep it as warning. We don't break every single release interfaces between kubelet and control plane. It might be some cases where it will work incorrectly, but generally it should work. But I can change that to error, not big deal, as soon we will have custom controls on pre-flight checks :)

As it never is guaranteed to be a supported state, I want it to be an error. Please fix as above

luxas · 2017-11-08T16:58:07Z

cmd/kubeadm/app/preflight/checks_test.go

+	}
+
+	cases := []T{
+		{"v1.7.3", "v1.7.8", true, false},          // Too old kubelet


this confused me first: please say // too old kubelet generally, v1.7 kubelets not supported anymore

That is intentional, to check error path with kubeadmconstants.MinimumKubeletVersion

yeah, but I'd appreciate if you changed the comment to be clearer. That was my intention

luxas · 2017-11-08T16:58:38Z

cmd/kubeadm/app/preflight/checks_test.go

+		{"v1.7.3", "v1.7.8", true, false},          // Too old kubelet
+		{"v1.9.0", "v1.9.5", false, false},         // kubelet within same major.minor as control plane
+		{"v1.9.0", "v1.10.1", false, false},        // kubelet is lower than control plane, but newer than minimally supported
+		{"v1.10.0-alpha.1", "v1.9.1", false, true}, // kubelet is newer than control plane


I would expect this to fail

Also add a proper v1.10.3 and v1.9.4 test that should fail

Using development builds of kubelet on top of stable control plane would be good to allow. See other comment.

luxas · 2017-11-08T17:00:02Z

cmd/kubeadm/app/preflight/checks_test.go

+	defer os.RemoveAll(dir)
+
+	// We don't want to call real kubelet or something else in $PATH
+	oldPATH := os.Getenv("PATH")


Can you please re-engineer this to use k8s.io/utils/exec for this instead so you can fake out the exec calls in unit tests instead of doing this?

I haven't noticed that lib. Will check it. From quick look, it might require change also in implementation of GetKubeletVersion(). Do you want to have it as part of this PR or maybe let's do re-factoring to use utils/exec as separate PR ?

Follow up is fine by me

luxas

After the last comments are addressed, I'll LGTM

luxas · 2017-11-08T19:49:35Z

cmd/kubeadm/app/preflight/checks_test.go

+	}
+
+	cases := []T{
+		{"v1.7.3", "v1.7.8", true, false},          // Too old kubelet


yeah, but I'd appreciate if you changed the comment to be clearer. That was my intention

luxas · 2017-11-08T19:52:06Z

cmd/kubeadm/app/preflight/checks.go

+		}
+		firstUnsupportedKubeletVersion := versionutil.MustParseSemantic(fmt.Sprintf("%d.%d.%s", k8sVersion.Major(), k8sVersion.Minor()+1, "0-0"))
+		if kubeletVersion.AtLeast(firstUnsupportedKubeletVersion) {
+			return []error{fmt.Errorf("kubernetes version is lower than kubelet version. This may lead to malfunctional cluster setup. Kubernetes version: %s. Kubelet version: %s", k8sVersion, kubeletVersion)}, nil


As it never is guaranteed to be a supported state, I want it to be an error. Please fix as above

luxas · 2017-11-08T19:52:22Z

cmd/kubeadm/app/preflight/checks_test.go

+		{"v1.7.3", "v1.7.8", true, false},          // Too old kubelet
+		{"v1.9.0", "v1.9.5", false, false},         // kubelet within same major.minor as control plane
+		{"v1.9.0", "v1.10.1", false, false},        // kubelet is lower than control plane, but newer than minimally supported
+		{"v1.10.0-alpha.1", "v1.9.1", false, true}, // kubelet is newer than control plane


luxas · 2017-11-08T19:52:33Z

cmd/kubeadm/app/preflight/checks_test.go

+	defer os.RemoveAll(dir)
+
+	// We don't want to call real kubelet or something else in $PATH
+	oldPATH := os.Getenv("PATH")


Follow up is fine by me

kad · 2017-11-08T20:38:30Z

updated with making it as an error and addressing other comments.

luxas

/lgtm

Thanks @kad!

KubeletVersionCheck now able to detect if kubelet version is higher than control plane. As this might lead to malfunctional cluster setups, kubeadm will give warning. Fixes: kubernetes/kubeadm#496

kad · 2017-11-08T21:37:20Z

Sorry @luxas needed to push once again with fix for golint issue :(

luxas

/lgtm

kad · 2017-11-09T07:18:53Z

/retest

luxas · 2017-11-09T10:02:17Z

/lgtm

k8s-github-robot · 2017-11-09T10:02:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kad, luxas

Associated issue: 496

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

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [luxas]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-09T10:58:29Z

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

k8s-github-robot · 2017-11-09T12:12:48Z

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

k8s-github-robot assigned krousey and dmmcquay Oct 31, 2017

luxas reviewed Nov 8, 2017

View reviewed changes

kad force-pushed the kubeadm-issue-496 branch from 5dfa888 to 18f486c Compare November 8, 2017 20:35

luxas approved these changes Nov 8, 2017

View reviewed changes

k8s-ci-robot assigned luxas Nov 8, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2017

kubeadm: Extended KubeletVersionCheck

de272d0

KubeletVersionCheck now able to detect if kubelet version is higher than control plane. As this might lead to malfunctional cluster setups, kubeadm will give warning. Fixes: kubernetes/kubeadm#496

kad force-pushed the kubeadm-issue-496 branch from 18f486c to de272d0 Compare November 8, 2017 21:36

luxas reviewed Nov 8, 2017

View reviewed changes

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2017

k8s-github-robot merged commit 065e450 into kubernetes:master Nov 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Extended KubeletVersionCheck #54868

kubeadm: Extended KubeletVersionCheck #54868

kad commented Oct 31, 2017 •

edited

Loading

kad commented Oct 31, 2017

kad commented Nov 3, 2017

luxas left a comment

luxas Nov 8, 2017

kad Nov 8, 2017

luxas Nov 8, 2017

luxas Nov 8, 2017

kad Nov 8, 2017

luxas Nov 8, 2017

luxas Nov 8, 2017

kad Nov 8, 2017

luxas Nov 8, 2017

luxas Nov 8, 2017

luxas Nov 8, 2017

kad Nov 8, 2017

luxas Nov 8, 2017

luxas Nov 8, 2017

kad Nov 8, 2017

luxas Nov 8, 2017

luxas left a comment

luxas Nov 8, 2017

luxas Nov 8, 2017

luxas Nov 8, 2017

luxas Nov 8, 2017

kad commented Nov 8, 2017

luxas left a comment

kad commented Nov 8, 2017

luxas left a comment

kad commented Nov 9, 2017

luxas commented Nov 9, 2017

k8s-github-robot commented Nov 9, 2017

k8s-github-robot commented Nov 9, 2017

k8s-github-robot commented Nov 9, 2017

kubeadm: Extended KubeletVersionCheck #54868

kubeadm: Extended KubeletVersionCheck #54868

Conversation

kad commented Oct 31, 2017 • edited Loading

kad commented Oct 31, 2017

kad commented Nov 3, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

kad commented Nov 8, 2017

luxas left a comment

Choose a reason for hiding this comment

kad commented Nov 8, 2017

luxas left a comment

Choose a reason for hiding this comment

kad commented Nov 9, 2017

luxas commented Nov 9, 2017

k8s-github-robot commented Nov 9, 2017

k8s-github-robot commented Nov 9, 2017

k8s-github-robot commented Nov 9, 2017

kad commented Oct 31, 2017 •

edited

Loading