Add warning if client/server version difference exceeds the supported skew #98250

brianpursley · 2021-01-21T03:08:47Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR writes a warning message to stderr when you run kubectl version if the version skew between the client and server is more than the supported skew (+/- 1 minor version).

Example warning message:

WARNING: version difference between client (1.21+) and server (1.19) exceeds the supported minor version skew of +/-1

Which issue(s) this PR fixes:

kubernetes/kubectl#685

Special notes for your reviewer:
The approach of only warning when the user runs kubectl version was discussed at the 1/13/2021 SIG-CLI meeting.

Long-term we may want to introduce a more intrusive warning to alert users of an unsupported skew, but that is beyond the scope of this PR.

Does this PR introduce a user-facing change?:

kubectl version changed to write a warning message to stderr if the client and server version difference exceeds the supported version skew of +/-1 minor version.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

brianpursley · 2021-01-22T20:06:13Z

/assign @brendandburns

dougsland · 2021-01-23T15:51:05Z

/cc

rikatz · 2021-01-24T14:17:26Z

/cc

staging/src/k8s.io/kubectl/pkg/cmd/version/skew_warning.go

rikatz

Left some comments just as a second opinion :)

staging/src/k8s.io/kubectl/pkg/cmd/version/skew_warning.go

dougsland

I have few comments. However, I must say: LGTM.

staging/src/k8s.io/kubectl/pkg/cmd/version/skew_warning.go

dougsland · 2021-01-30T01:36:56Z

staging/src/k8s.io/kubectl/pkg/cmd/version/skew_warning.go

+	if supported, err := isWithinSupportedVersionSkew(clientVersion, serverVersion); err != nil {
+		return err
+	} else if !supported {
+		fmt.Fprintf(w, "WARNING: version difference between client (%s.%s) and server (%s.%s) exceeds the supported minor version skew of +/-%d\n",


Maybe it's only me but I think +/- in a statement is a little confusing. However, I don't have a better suggestion, sorry.

WARNING: version difference between client (1.21+) and server (1.18) exceeds the supported minor version skew of +/-1

Another question users might have: "Right, so now I have the warning of version skew, what should I do ?"
Maybe add: "It is recommended to keep upgrading client and server with the same version." (or similar/improved msg)

Yeah, I don’t really know what else to say here. It’s really not horrible to use an out of skew client, but if you are running into problems, you should be aware that it could be an unsupported skew.

A main reason for adding this warning is to make people aware of the unsupported skew before they open an issue in the kubectl repo.

but you’re right, it is not super actionable.

Hey @briandorsey I believe the warning you created is super valid but it's difficult to avoid users to open issues with unsupported skew IMO.

Probably we might have such scenarios with users out there:

didn't understand the message - what's skew +/- 1?

simple ignore it as they don't know what to do next

not sure "how to fix it"

not sure "how to make the cluster in a minimum good shape" to open the issue.

From my side, the code looks great but I would request more people to review the message to have more ideas. Probably a good PR for request ideas in the SIG-CLI meeting.

staging/src/k8s.io/kubectl/pkg/cmd/version/skew_warning.go

brianpursley · 2021-02-13T21:01:50Z

/test pull-kubernetes-verify

brianpursley · 2021-02-14T16:17:48Z

Thanks to a comment @soltysh left in another PR, I became aware of the existence of ParseSemantic in api machinery. I updated this PR to use this to get the major and minor versions instead of reinventing the wheel, so to speak.

brianpursley · 2021-02-15T01:52:10Z

@dougsland See my comment above, I was able to re-use an existing function to get major/minor version, so the code has changed. Can you take another look? Thanks!

soltysh · 2021-03-03T20:34:45Z

/assign

seans3 · 2021-03-03T22:34:58Z

/milestone v1.21
/priority important-soon

seans3 · 2021-03-03T22:35:15Z

/triage accepted

staging/src/k8s.io/kubectl/pkg/cmd/version/skew_warning_test.go

seans3

This looks good to me. I actually think the warning message is about as good as it can be. I've added the v1.21 milestone, since we hope to get this in by code freeze. I can LGTM once Maciej is happy (and the BUILD file is removed).

… skew

soltysh

/lgtm
/approve

soltysh · 2021-03-04T13:49:57Z

staging/src/k8s.io/kubectl/pkg/cmd/version/skew_warning.go

+	"io"
+	"k8s.io/apimachinery/pkg/util/version"
+	apimachineryversion "k8s.io/apimachinery/pkg/version"
+	"math"


Nit: but try to keep imports split into:

built-in

3rd party

k8s

k8s-ci-robot · 2021-03-04T13:50:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianpursley, seans3, soltysh

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:

~~staging/src/k8s.io/kubectl/OWNERS~~ [seans3,soltysh]

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

soltysh · 2021-03-04T13:50:47Z

/retest

k8s-ci-robot requested review from pwittrock and shiywang January 21, 2021 03:09

k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 21, 2021

brianpursley force-pushed the kubectl-685 branch 2 times, most recently from 549b63c to 7976208 Compare January 21, 2021 13:30

k8s-ci-robot assigned brendandburns Jan 22, 2021

k8s-ci-robot requested a review from dougsland January 23, 2021 15:51

k8s-ci-robot requested a review from rikatz January 24, 2021 14:17

rikatz reviewed Jan 24, 2021

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/version/skew_warning.go Outdated Show resolved Hide resolved

rikatz reviewed Jan 24, 2021

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/version/skew_warning.go Show resolved Hide resolved

staging/src/k8s.io/kubectl/pkg/cmd/version/skew_warning.go Outdated Show resolved Hide resolved

brianpursley force-pushed the kubectl-685 branch 2 times, most recently from 8982e74 to 3d1a258 Compare January 25, 2021 00:49

dougsland reviewed Jan 30, 2021

View reviewed changes

brianpursley force-pushed the kubectl-685 branch 2 times, most recently from 82ff0e2 to 7b3d7aa Compare February 13, 2021 19:05

k8s-ci-robot assigned soltysh Mar 3, 2021

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Mar 3, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 3, 2021

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Mar 3, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 3, 2021

seans3 reviewed Mar 3, 2021

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/version/skew_warning_test.go Outdated Show resolved Hide resolved

seans3 approved these changes Mar 3, 2021

View reviewed changes

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 3, 2021

Add warning if client/server version difference exceeds the supported…

0c28cad

… skew

brianpursley force-pushed the kubectl-685 branch from 7b3d7aa to 0c28cad Compare March 4, 2021 01:57

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

soltysh approved these changes Mar 4, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2021

k8s-ci-robot merged commit d2d9b0e into kubernetes:master Mar 4, 2021

eddiezane mentioned this pull request May 12, 2021

kubectl should have a version skew warning kubernetes/kubectl#685

Closed

brianpursley deleted the kubectl-685 branch February 2, 2023 01:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning if client/server version difference exceeds the supported skew #98250

Add warning if client/server version difference exceeds the supported skew #98250

brianpursley commented Jan 21, 2021

brianpursley commented Jan 22, 2021

dougsland commented Jan 23, 2021

rikatz commented Jan 24, 2021

rikatz left a comment

dougsland left a comment

dougsland Jan 30, 2021

brianpursley Feb 14, 2021

dougsland Feb 16, 2021

brianpursley commented Feb 13, 2021

brianpursley commented Feb 14, 2021

brianpursley commented Feb 15, 2021

soltysh commented Mar 3, 2021

seans3 commented Mar 3, 2021

seans3 commented Mar 3, 2021

seans3 left a comment •

edited

soltysh left a comment

soltysh Mar 4, 2021

k8s-ci-robot commented Mar 4, 2021

soltysh commented Mar 4, 2021

Add warning if client/server version difference exceeds the supported skew #98250

Add warning if client/server version difference exceeds the supported skew #98250

Conversation

brianpursley commented Jan 21, 2021

brianpursley commented Jan 22, 2021

dougsland commented Jan 23, 2021

rikatz commented Jan 24, 2021

rikatz left a comment

Choose a reason for hiding this comment

dougsland left a comment

Choose a reason for hiding this comment

dougsland Jan 30, 2021

Choose a reason for hiding this comment

brianpursley Feb 14, 2021

Choose a reason for hiding this comment

dougsland Feb 16, 2021

Choose a reason for hiding this comment

brianpursley commented Feb 13, 2021

brianpursley commented Feb 14, 2021

brianpursley commented Feb 15, 2021

soltysh commented Mar 3, 2021

seans3 commented Mar 3, 2021

seans3 commented Mar 3, 2021

seans3 left a comment • edited

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

soltysh Mar 4, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 4, 2021

soltysh commented Mar 4, 2021

seans3 left a comment •

edited