kubeadm: refactor validateStableVersion() #72059

rojkov · 2018-12-14T13:05:05Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Currently the function cmd/kubeadm/app/util.validateStableVersion() doesn't validate remote versions in the special case when the client version is empty. This makes the code more difficult to reason about, because the function may successfully return a string which isn't a valid version. And this conflicts with name of the function.

Move handling the special case outside of the function to the place where its meaning is more obvious.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2018-12-14T13:05:12Z

Hi @rojkov. 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.

neolit123

@rojkov

doesn't validate remote versions in the special case when the client version is empty

the client version should never be empty.
if that's the case then we can consider this a build environment problem.
i'd argue that we pretty much need to os.Exit(1) if pkgversion.Get().String() fails to obtain an actual version.

it means that you could be building kubectl, the kubelet and control plane with the same environment and that not a great idea.
we had bug reports where we could not figure out what is happening and eventually found out that the user was building k8s components without the version info that the linker passes from GIT.

make all WHAT=./cmd/kubeadm
should populate the version info for you and you don't need an internet connection for that.
this will will feed a valid client version into kubeadm.

/priority backlog
/assign kad

neolit123 · 2018-12-14T14:39:35Z

@kubernetes/sig-cluster-lifecycle-pr-reviews

bart0sh · 2018-12-14T15:08:35Z

/ok-to-test

bart0sh · 2018-12-14T15:40:56Z

@neolit123 can you elaborate a bit more on this? Existing code already checks if version is empty. This PR just moves it to another place where it is more understandable. Am I missing something here? Are you suggesting to add os.Exit(1) call on top of the proposed change?

neolit123 · 2018-12-14T18:41:27Z

@bart0sh
my point is that the fallback from client version to remote version should never happen.
yes, we have a check for that but this is a weird edge case.

@rojkov
please explain your problem again - pkgversion.Get().String() should not return an empty version.
if that's the case, this is a build system issue as explained here:
#72059 (review)
and should be resolved in the build system.

rojkov · 2018-12-17T10:27:25Z

@neolit123 Thanks for review!

This PR doesn't solve any problem except clarifying the code. Just wanted to keep the PR as simple and clear as possible.

At least now it became more obvious that the empty string check is a weird edge case with missing explanation why it's needed. To illustrate why exactly this check is needed I submitted one more commit removing the check. Here's the result
https://gubernator.k8s.io/build/kubernetes-jenkins/pr-logs/pull/72059/pull-kubernetes-bazel-test/70250/

As you can see 7 bazel tests in CI start to fail and I can't say the reason is obvious. I've added one more commit which brings the fallback back and adds a TODO comment and an explicit error check.

Then I'd argue that it's a build system problem. By definition a unit test is supposed to test only its corresponding unit. If it depends on other units like build systems it's not unit test any more or it's a manifestation of an architectural issue. As clientVersion I'd prefer using a constant that Ed is going to introduce.

rosti · 2018-12-17T10:33:03Z

@rojkov @bart0sh @neolit123 the string returned by pkgversion.Get().String() is a GitVersion. I haven't checked the build system code, but it's entirely possible that in certain cases this won't get filled in. For example, when building from a release source tarball or in case the actual build environment does not have git.

bart0sh · 2018-12-17T10:48:23Z

@neolit123, @rosti guys, I'd propose to look at this PR as a simple refactoring. First commit is quite obvious. Second commit improves readability. That's it. Let's not go to build system issues just yet, ok? Do you agree that as a simple refactoring PR it does make sense and worth merging?

rosti · 2018-12-17T14:00:41Z

Yes, we may have gone a bit stray from the original intent here. Sorry about that.
As long as this version is more readable than the original, which it is, then I am fine with it.
@rojkov could you squash your commits?

neolit123

As you can see 7 bazel tests in CI start to fail and I can't say the reason is obvious. I've added one more commit which brings the fallback back and adds a TODO comment and an explicit error check.

i forgot about this.
it happens because when doing unit tests on individual packages, there is nothing to populate the version.

neolit123 · 2018-12-17T13:50:11Z

cmd/kubeadm/app/util/version.go

@@ -95,6 +94,13 @@ func KubernetesReleaseVersion(version string) (string, error) {
 			klog.Infof("falling back to the local client version: %s", clientVersion)
 			return KubernetesReleaseVersion(clientVersion)
 		}
+
+		if clientVersionErr != nil {
+			// TODO: theoretically clinetVersion is always accessible, but only if build system is tweaked correctly


i'd move the comment above:
pkgversion.Get().String()
and make it say:

pkgversion.Get().String() should always return a correct version added by the golang linker and the build system. The version can still be missing when doing unit tests on individual packages.

neolit123 · 2018-12-17T13:52:02Z

cmd/kubeadm/app/util/version.go

+
+		if clientVersionErr != nil {
+			// TODO: theoretically clinetVersion is always accessible, but only if build system is tweaked correctly
+			klog.Warningf("could not obtain client version; using remote version: %s", body)


at this point kubeadmVersion() can fail because of not being able to parse a remoteVersion too.
this case makes this error message invalid.

Very true! I guess if both versions are unavailable we can do nothing, but os.Exit(1) with some explanation. I'll squash these 3 commits and submit one more PR.

@rojkov FYI, @bart0sh is currently working on a PR (?) that may add a constant that refers to the actual current version of the client (even if not obtained from a build system).

this could be our final fallback.
let's decide if we want to do that.

Yeap, I'll discuss with Ed what would be the best option.

neolit123 · 2018-12-17T14:09:31Z

@bart0sh

Do you agree that as a simple refactoring PR it does make sense and worth merging?

we can agree on that, sure.
just FYI this is not the only place where we are using pkgversion.Get().String(), upgrades are another area.

so if your production binary is lacking a GIT version and this is air gapped scenario, upgrades might be broken too.

Currently the function `cmd/kubeadm/app/util.validateStableVersion()` doesn't validate remote versions in the special case when the client version is empty. This makes the code more difficult to reason about, because the function may successfully return a string which isn't a valid version. Move handling the special case outside of the function to the place where its meaning is more obvious.

rojkov · 2018-12-17T18:01:51Z

/test pull-kubernetes-e2e-kops-aws

cmd/kubeadm/app/util/version.go

fabriziopandini

@rojkov thanks! for this pr!
I'm ok with the proposed changes.
Ping me when all the feedback addressed for lgtm/approval

fabriziopandini · 2018-12-20T09:56:54Z

/approve
/lgtm

k8s-ci-robot · 2018-12-20T09:59:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, rojkov

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~~ [fabriziopandini]

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

kubeadm: print stack trace in case of unexpected error

4b6d91c

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 14, 2018

k8s-ci-robot requested review from fabriziopandini and yagonobre December 14, 2018 13:05

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 14, 2018

neolit123 reviewed Dec 14, 2018

View reviewed changes

k8s-ci-robot assigned kad Dec 14, 2018

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 14, 2018

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 14, 2018

neolit123 reviewed Dec 17, 2018

View reviewed changes

rojkov force-pushed the kubeadm-check-remote-version-v2 branch from 04aaad8 to b9c2139 Compare December 17, 2018 15:06

dixudx suggested changes Dec 18, 2018

View reviewed changes

cmd/kubeadm/app/util/version.go Show resolved Hide resolved

rojkov mentioned this pull request Dec 19, 2018

Fix hermetic tests in kubeadm #72028

Closed

fabriziopandini reviewed Dec 20, 2018

View reviewed changes

k8s-ci-robot assigned fabriziopandini Dec 20, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 20, 2018

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

k8s-ci-robot merged commit 4a21a77 into kubernetes:master Dec 20, 2018

rojkov deleted the kubeadm-check-remote-version-v2 branch February 18, 2019 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: refactor validateStableVersion() #72059

kubeadm: refactor validateStableVersion() #72059

rojkov commented Dec 14, 2018

k8s-ci-robot commented Dec 14, 2018

neolit123 left a comment

neolit123 commented Dec 14, 2018

bart0sh commented Dec 14, 2018

bart0sh commented Dec 14, 2018

neolit123 commented Dec 14, 2018

rojkov commented Dec 17, 2018

rosti commented Dec 17, 2018

bart0sh commented Dec 17, 2018

rosti commented Dec 17, 2018

neolit123 left a comment

neolit123 Dec 17, 2018

neolit123 Dec 17, 2018

rojkov Dec 17, 2018

neolit123 Dec 17, 2018

rojkov Dec 17, 2018

neolit123 commented Dec 17, 2018 •

edited

rojkov commented Dec 17, 2018

fabriziopandini left a comment

fabriziopandini commented Dec 20, 2018

k8s-ci-robot commented Dec 20, 2018

kubeadm: refactor validateStableVersion() #72059

kubeadm: refactor validateStableVersion() #72059

Conversation

rojkov commented Dec 14, 2018

k8s-ci-robot commented Dec 14, 2018

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Dec 14, 2018

bart0sh commented Dec 14, 2018

bart0sh commented Dec 14, 2018

neolit123 commented Dec 14, 2018

rojkov commented Dec 17, 2018

rosti commented Dec 17, 2018

bart0sh commented Dec 17, 2018

rosti commented Dec 17, 2018

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 Dec 17, 2018

Choose a reason for hiding this comment

neolit123 Dec 17, 2018

Choose a reason for hiding this comment

rojkov Dec 17, 2018

Choose a reason for hiding this comment

neolit123 Dec 17, 2018

Choose a reason for hiding this comment

rojkov Dec 17, 2018

Choose a reason for hiding this comment

neolit123 commented Dec 17, 2018 • edited

rojkov commented Dec 17, 2018

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini commented Dec 20, 2018

k8s-ci-robot commented Dec 20, 2018

neolit123 commented Dec 17, 2018 •

edited