kubeadm: modify SetKubernetesVersion #69645

neolit123 · 2018-10-11T00:18:28Z

What this PR does / why we need it:

Without this patch SetKubernetesVersion does not set
a version in all locations where it's called, because
the passed config to it always has a default version
stable-1.

This always triggers the != "" check and the function
returns without setting a version.

Validate against DefaultKubernetesVersion instead.

This fixes all cases where fetching a version from the internet
is not needed at all - e.g. "kubeadm token create".

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

Special notes for your reviewer:
NONE

Release note:

kubeadm: fix a case where fetching a kubernetesVersion from the internet still happened even if some commands don't need it.

/assign @xiangpengzhao
/assign @fabriziopandini
/cc @timothysc
/kind bug

Peter, this is related to your patch about:

// KubernetesVersion is not used, but we set it explicitly to avoid the lookup
// of the version from the internet when executing ConfigFileAndDefaultsToInternalConfig

5cf9291

WDYT?
i don't see a problem with having the function like that in the current codebase - i.e it would override with the client version, but this will avoid fetching from the internet if labels are passed.

also, it's not a critical bug, but i'd vote for us to cherry pick this for 1.12.

neolit123 · 2018-10-11T01:08:46Z

found issue with the initial patch that i sent, applied modification and renamed the PR too.

neolit123 · 2018-10-11T01:22:43Z

/retest

xiangpengzhao

@neolit123 Agree with your idea 👍 EDIT: I misread the PR body. You're correct on the condition change.

xiangpengzhao

Just one concern, otherwise LGTM. Thanks for fixing this! @neolit123

xiangpengzhao · 2018-10-11T07:25:22Z

cmd/kubeadm/app/cmd/phases/util.go

Although cfg.KubernetesVersion is not empty by default, do we still want to check against the empty case here ? In case it changes to an empty default value for any reason in the future?

@xiangpengzhao

hm, so if the default changes to "" the branch will handle it and return.
but i'm wondering if we should handle "" with the current default not being "".

if cfg.KubernetesVersion != kubeadmapiv1beta1.DefaultKubernetesVersion && cfg.KubernetesVersion != "") { return nil }

^ i assume this is what we want?

So, my general hot-take is that we should test all the different scenarios of input to the function.

sounds good.
will update the PR tomorrow.

timothysc

This points to a gap in testing, could you please add a unit test for the different scenarios.

timothysc · 2018-10-11T16:25:18Z

cmd/kubeadm/app/cmd/phases/util.go

So, my general hot-take is that we should test all the different scenarios of input to the function.

neolit123 · 2018-10-12T13:03:50Z

updated:

added a unit test for SetKubernetesVersion
modified the function to use version.Get().String() directly too.

xiangpengzhao

Good refactoring 👍 Just one concern, otherwise LGTM.

xiangpengzhao · 2018-10-13T03:27:04Z

cmd/kubeadm/app/cmd/phases/util_test.go

Seems like the logic of SetKubernetesVersion is "empty version and default version is processed and any other version is skipped". @neolit123 WDYT?

sorry, was in a rush and got things mixed up. i will update the PR later today.

updated. :)

Without this patch SetKubernetesVersion does not set a version in all locations where it's called, because the passed config to it always has a default version stable-1. This always triggers the != "" check and the function returns without setting a version. Validate against DefaultKubernetesVersion and "" instead. This fixes all cases where fetching a version from the internet is not needed at all - e.g. "kubeadm token create". Also make SetKubernetesVersion default to version.Get().String() and add unit tests for the function.

neolit123 · 2018-10-13T14:37:58Z

/retest

timothysc

Thanks for the cleanup @neolit123

/lgtm
/approve

k8s-ci-robot · 2018-10-15T16:35:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, timothysc

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

The pull request process is described here

Details

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

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 11, 2018

k8s-ci-robot assigned fabriziopandini and xiangpengzhao Oct 11, 2018

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 11, 2018

k8s-ci-robot requested a review from timothysc October 11, 2018 00:18

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Oct 11, 2018

neolit123 mentioned this pull request Oct 11, 2018

kubeadm token create mix: --print-join-command and --config kubernetes/kubeadm#1166

Closed

neolit123 force-pushed the kubeadm-set-version-fix branch from 05970c2 to af105bb Compare October 11, 2018 01:07

neolit123 changed the title ~~kubeadm: make SetKubernetesVersion explicit~~ kubeadm: modify SetKubernetesVersion Oct 11, 2018

neolit123 force-pushed the kubeadm-set-version-fix branch from af105bb to d89ed39 Compare October 11, 2018 01:11

xiangpengzhao reviewed Oct 11, 2018

View reviewed changes

k8s-ci-robot assigned bart0sh Oct 11, 2018

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

neolit123 mentioned this pull request Oct 11, 2018

kubeadm: allow usage --config with --print-join-command #69624

Merged

timothysc suggested changes Oct 11, 2018

View reviewed changes

neolit123 force-pushed the kubeadm-set-version-fix branch from d89ed39 to 7c8ce3d Compare October 12, 2018 13:01

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 12, 2018

xiangpengzhao reviewed Oct 13, 2018

View reviewed changes

neolit123 force-pushed the kubeadm-set-version-fix branch from 7c8ce3d to fa9940c Compare October 13, 2018 14:17

timothysc approved these changes Oct 15, 2018

View reviewed changes

k8s-ci-robot assigned timothysc Oct 15, 2018

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

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

timothysc added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Oct 15, 2018

k8s-ci-robot merged commit b11211e into kubernetes:master Oct 15, 2018

kubeadm: modify SetKubernetesVersion #69645

kubeadm: modify SetKubernetesVersion #69645

Uh oh!

Conversation

neolit123 commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neolit123 commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neolit123 commented Oct 11, 2018

Uh oh!

xiangpengzhao left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiangpengzhao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timothysc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neolit123 commented Oct 12, 2018

Uh oh!

xiangpengzhao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neolit123 commented Oct 13, 2018

Uh oh!

timothysc left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

neolit123 commented Oct 11, 2018 •

edited

Loading

neolit123 commented Oct 11, 2018 •

edited

Loading

xiangpengzhao left a comment •

edited

Loading