Warn if incompatible kubectl version is in use #5596

nanikjava · 2019-10-11T12:46:59Z

Modification is done inside start.go where additional checking on the
version returned via the kubectl CLI is checked.

Running 'kubectl version --output=json' will return both client
and server information.

Fixes: kubernetes#3329 Modification is done inside start.go where additional checking on the version returned via the kubectl CLI is checked. Running 'kubectl version --output=json' will return both client and server information.

minikube-bot · 2019-10-11T12:47:03Z

Can one of the admins verify this patch?

k8s-ci-robot · 2019-10-11T12:47:06Z

Hi @nanikjava. 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 · 2019-10-11T12:47:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nanikjava
To complete the pull request process, please assign medyagh
You can assign the PR to them by writing /assign @medyagh in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

codecov-io · 2019-10-11T12:54:27Z

Codecov Report

Merging #5596 into master will increase coverage by 0.25%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #5596      +/-   ##
==========================================
+ Coverage   36.89%   37.15%   +0.25%     
==========================================
  Files         102      103       +1     
  Lines        7369     7545     +176     
==========================================
+ Hits         2719     2803      +84     
- Misses       4297     4369      +72     
- Partials      353      373      +20

Impacted Files	Coverage Δ
cmd/minikube/cmd/start.go	`22% <0%> (-0.64%)`	⬇️
pkg/minikube/config/profile.go	`71.59% <0%> (-1.67%)`	⬇️
pkg/minikube/assets/addons.go	`36.66% <0%> (ø)`	⬆️
pkg/minikube/cluster/machine.go	`50% <0%> (ø)`
pkg/minikube/cluster/cluster.go	`38.05% <0%> (+0.58%)`	⬆️
cmd/minikube/cmd/delete.go	`30% <0%> (+30%)`	⬆️

tstromberg · 2019-10-17T17:22:29Z

cmd/minikube/cmd/start.go

+			out.T(out.Tip, "For best results, install kubectl: https://kubernetes.io/docs/tasks/tools/install-kubectl/")
+		}
+	}
+
 	if kcs.KeepContext {


Move this connection output to the top of showKubectlConnectInfo.

This will allow both the version mismatch error to come at the end, which is more visible, and will allow you to exit early if kubectl does not exist, which will allow for more readable code as the version checking won't have to be indented.

tstromberg · 2019-10-17T17:25:38Z

cmd/minikube/cmd/start.go

+		out.T(out.Tip, "For best results, install kubectl: https://kubernetes.io/docs/tasks/tools/install-kubectl/")
+	} else {
+		glog.Infof("Kubectl found at the following location %s. Let's execute and get the result", path)
+		output, err = exec.Command(path, "version", "--output=json").Output()


This may show the wrong server version, as it queries the active context kubectl is configured for, which may not be minikube.

My suggestion is to use kubectl version --short --output=json, and add k8sVersion as an argument to the function.

tstromberg · 2019-10-17T17:26:20Z

cmd/minikube/cmd/start.go

+	if err != nil {
+		out.T(out.Tip, "For best results, install kubectl: https://kubernetes.io/docs/tasks/tools/install-kubectl/")
+	} else {
+		glog.Infof("Kubectl found at the following location %s. Let's execute and get the result", path)


While good for initial debugging, I think this log message can be removed.

tstromberg · 2019-10-17T17:27:20Z

cmd/minikube/cmd/start.go

+		output, err = exec.Command(path, "version", "--output=json").Output()
+
+		// ...once we get something back do some version checking
+		if err == nil {


I mention this later, but try to structure functions so that exceptional conditions are handled first, and exited early. So for istance:

if err != nil {
.. return ..
}
.. do the rest with one level less indentation

tstromberg · 2019-10-17T17:28:32Z

cmd/minikube/cmd/start.go

+			clientjsonErr := json.Unmarshal(output, &clientVersion)
+
+			// .......  and {server} json
+			serverjsonErr := json.Unmarshal(output, &serverVersion)


No need to unmarshal the same output twice: the server version is already in clientVersion.SVersion

tstromberg · 2019-10-17T17:28:58Z

cmd/minikube/cmd/start.go

+			serverjsonErr := json.Unmarshal(output, &serverVersion)
+
+			if (clientjsonErr != nil) || (serverjsonErr != nil) {
+				glog.Infof("There was an error processing the json output")


here is another opportunity to return early.

tstromberg · 2019-10-17T17:33:40Z

cmd/minikube/cmd/start.go

+				serverMinor, _ := strconv.Atoi(serverVersion.SVersion.Minor)
+				clientMinor, _ := strconv.Atoi(serverVersion.CVersion.Minor)
+
+				if clientMinor < serverMinor {


This is not quite correct yet. https://kubernetes.io/docs/setup/release/version-skew-policy/#kubectl dictates that you are allowed up to one version difference. One possibility is:

if math.Abs(float64(clientMinor - serverMinor)) > 1 {

tstromberg · 2019-10-17T17:36:54Z

cmd/minikube/cmd/start.go

+				clientMinor, _ := strconv.Atoi(serverVersion.CVersion.Minor)
+
+				if clientMinor < serverMinor {
+					out.T(out.Tip, "The version of kubectl {{.kubectl}} installed is incompatible with your Kubernetes {{.kubernetes}}  deployment. Please upgrade/downgrade as necessary, or use minikube kubectlto connect to your cluster",


It would be helpful to include the path. How about something like:

{{.path}} is version {{.version}}, and is incompatible with your specified Kubernetes version. You will need to update {{.path}} or use 'minikube kubectl' to connect with this cluster

* When nil is returned from function just return back * Restructure the output so that it will make more sense for the user

k8s-ci-robot · 2019-10-17T21:07:32Z

Keywords which can automatically close issues and at(@) mentions are not allowed in commit messages.

The list of commits with invalid commit messages:

9bf9041 Warn if incompatible kubectl version is in use

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.

tstromberg

Thank you!

Warn if incompatible kubectl version is in use

9bf9041

Fixes: kubernetes#3329 Modification is done inside start.go where additional checking on the version returned via the kubectl CLI is checked. Running 'kubectl version --output=json' will return both client and server information.

k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 11, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 11, 2019

nanikjava mentioned this pull request Oct 11, 2019

Warn if incompatible kubectl version is in use #3329

Closed

k8s-ci-robot requested review from medyagh and tstromberg October 11, 2019 12:47

tstromberg reviewed Oct 17, 2019

View reviewed changes

tstromberg suggested changes Oct 17, 2019

View reviewed changes

Restructure code based on PR comments.

789cc7a

* When nil is returned from function just return back * Restructure the output so that it will make more sense for the user

tstromberg approved these changes Oct 17, 2019

View reviewed changes

tstromberg merged commit a84b3a9 into kubernetes:master Oct 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn if incompatible kubectl version is in use #5596

Warn if incompatible kubectl version is in use #5596

nanikjava commented Oct 11, 2019

minikube-bot commented Oct 11, 2019

k8s-ci-robot commented Oct 11, 2019

k8s-ci-robot commented Oct 11, 2019

codecov-io commented Oct 11, 2019 •

edited

Loading

tstromberg Oct 17, 2019 •

edited

Loading

tstromberg Oct 17, 2019

tstromberg Oct 17, 2019

tstromberg Oct 17, 2019

tstromberg Oct 17, 2019

tstromberg Oct 17, 2019

tstromberg Oct 17, 2019

tstromberg Oct 17, 2019

k8s-ci-robot commented Oct 17, 2019

tstromberg left a comment

Warn if incompatible kubectl version is in use #5596

Warn if incompatible kubectl version is in use #5596

Conversation

nanikjava commented Oct 11, 2019

minikube-bot commented Oct 11, 2019

k8s-ci-robot commented Oct 11, 2019

k8s-ci-robot commented Oct 11, 2019

codecov-io commented Oct 11, 2019 • edited Loading

Codecov Report

tstromberg Oct 17, 2019 • edited Loading

Choose a reason for hiding this comment

tstromberg Oct 17, 2019

Choose a reason for hiding this comment

tstromberg Oct 17, 2019

Choose a reason for hiding this comment

tstromberg Oct 17, 2019

Choose a reason for hiding this comment

tstromberg Oct 17, 2019

Choose a reason for hiding this comment

tstromberg Oct 17, 2019

Choose a reason for hiding this comment

tstromberg Oct 17, 2019

Choose a reason for hiding this comment

tstromberg Oct 17, 2019

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 17, 2019

tstromberg left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 11, 2019 •

edited

Loading

tstromberg Oct 17, 2019 •

edited

Loading