kubeadm: simplify minimum Kubernetes version bumps #71946

bart0sh · 2018-12-11T10:30:29Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Replaced hardcoded "v0.12.0" strings with new
MinimalKubernetesVersion constant.

This should help with a regular release version bumps.

Special notes for your reviewer:

This is a first step towards fixing kubernetes/kubeadm#1260

Does this PR introduce a user-facing change?:

NONE

rosti

Thanks for taking this up @bart0sh !
We can cover a few more cases with this PR (all v1.12.x cases).
A few of the spots don't depend on the current version. It's OK to keep them wired to a string as that may make code more readable.

cmd/kubeadm/app/apis/kubeadm/v1alpha3/conversion_test.go

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

cmd/kubeadm/app/componentconfigs/config_test.go

cmd/kubeadm/app/util/version_test.go

rosti · 2018-12-11T13:51:20Z

cmd/kubeadm/app/constants/constants.go

@@ -334,6 +334,9 @@ const (
 	// CoreDNSVersion is the version of CoreDNS to be deployed if it is used
 	CoreDNSVersion = "1.2.6"

+	// MinimumKubernetesVersion specifies minimum Kubernetes version supported by kubeadm
+	MinimumKubernetesVersion = "v1.12.0"


We can probably trim this to v1.12, so we can cover more cases with this PR (like v1.12.5).

I'd prefer to do it in a follow-up PR as this one is already big and could cause rebase conflicts if not merged soon. wdyt?

better to have it in the same PR, IMO.

ok, will do

but i wonder what other cases we need to do?

I think I understood the idea. I don't understand why it has to be implemented as one PR.

my vote would be for:

MinimumKubernetesVersionWithoutPatch = ... MinimumKubernetesVersion = ...

in terms of the example change in:
https://github.com/kubernetes/kubernetes/pull/68649/files

we might as well add more constants for the sake of making it more easy.
e.g. Minor + 1

that's my 2c.

I was thinking about having all changes, that involve 1.12.x in a single PR (currently we replaced only 1.12.0 and left some 1.12.x stuff, where x != 0, for another PR).

I am OK with moving 1.13 and 1.14 stuff in other PR(s). That way it will be more reviewable.

@rosti

currently we replaced only 1.12.0 and left some 1.12.x stuff

Nope, almost everything is covered with MinimumKubernetesVersion and MinimumKubernetesVersionWithoutPatch. You probably missed my updates.

Below is what's left currently. Can you point me out what of this should be included into this PR?

$ grep -r v1.12 . | grep -v ':\s*\/\/' Binary file ./kubeadm matches ./app/constants/constants.go: MinimumKubernetesVersionWithoutPatch = "v1.12" Binary file ./app/cmd/phases/phases.test matches ./app/cmd/phases/util_test.go: input: "v1.12.0", ./app/cmd/phases/util_test.go: output: "v1.12.0", ./app/cmd/config_test.go: ":v1.12.1", ./app/cmd/config_test.go: kubernetesVersion: v1.12.1 ./app/componentconfigs/config_test.go: k8sVersion := version.MustParseGeneric("v1.12.0") ./app/phases/uploadconfig/uploadconfig_test.go: KubernetesVersion: "v1.12.10", Binary file ./app/util/upgrade/upgrade.test matches ./app/util/config/testdata/conversion/master/v1alpha3.yaml:kubernetesVersion: v1.12.2 ./app/util/config/testdata/conversion/master/v1alpha3.yaml:unifiedControlPlaneImage: "k8s.gcr.io/hyperkube:v1.12.2" ./app/util/config/testdata/conversion/master/v1beta1.yaml:kubernetesVersion: v1.12.2 ./app/util/config/testdata/conversion/master/internal.yaml:KubernetesVersion: v1.12.2 ./app/util/config/common.go: "kubeadm.k8s.io/v1alpha2": "v1.12", ./app/util/version_test.go: remoteVersion: "v1.12.0", ./app/util/version_test.go: remoteVersion: "v1.12.1", ./app/util/version_test.go: output: "v1.12.1", ./app/util/version_test.go: clientVersion: "v1.12.0", ./app/util/version_test.go: remoteVersion: "v1.12.0", ./app/util/etcd/etcd.go: klog.V(1).Infoln("etcd manifest created by kubeadm v1.12") ./app/apis/kubeadm/v1alpha3/conversion_test.go: KubernetesVersion: "v1.12.2", ./app/apis/kubeadm/v1alpha3/conversion_test.go: UnifiedControlPlaneImage: "k8s.gcr.io/hyperkube:v1.12.2", ./app/apis/kubeadm/v1alpha3/conversion_test.go: KubernetesVersion: "v1.12.0", ./app/apis/kubeadm/v1alpha3/conversion_test.go: UnifiedControlPlaneImage: "k8s.gcr.io/hyperkube:v1.12.2", ./app/apis/kubeadm/v1alpha3/conversion_test.go: KubernetesVersion: "v1.12.2", ./app/apis/kubeadm/v1alpha3/conversion_test.go: UnifiedControlPlaneImage: "k8s.gcr.io/hyperkube:v1.12.2", ./app/apis/kubeadm/v1alpha3/conversion_test.go: KubernetesVersion: "v1.12.2", ./app/apis/kubeadm/v1alpha3/conversion_test.go: UnifiedControlPlaneImage: "k8s.gcr.io/otherimage:v1.12.2",

Sorry @bart0sh , I missed the updates. Will take a look.

rosti · 2018-12-11T13:56:26Z

/priority important-longterm
/assign @neolit123

neolit123

i think this is good.
thanks @bart0sh

neolit123 · 2018-12-11T14:46:31Z

/approve
/hold
/assign @fabriziopandini
for another set of eyes.

rosti · 2018-12-13T14:30:50Z

/test pull-kubernetes-godeps

bart0sh · 2018-12-13T15:57:51Z

/test pull-kubernetes-godeps

cblecker · 2018-12-13T23:26:51Z

/test pull-kubernetes-godeps

rosti

Thanks @bart0sh ! Just a single one missing:

kubernetes/cmd/kubeadm/test/cmd/init_test.go

Line 105 in 1c04caa

args: "--kubernetes-version=1.12.0",

Then we are ready to LGTM the 1.12.x portion.

bart0sh · 2018-12-14T15:03:41Z

@rosti Thank you for the review. Updated. lgtm, please as I'm concerned about possible conflicts.

bart0sh · 2018-12-19T15:14:53Z

@neolit123, @fabriziopandini, @rosti I've replaced string concatenations with using With* methods as @fabriziopandini suggested. Please, review again. Thanks.

neolit123 · 2018-12-19T15:22:43Z

@bart0sh
the staging/src/k8s.io/apimachinery/pkg/util/version/version.go helper changes LGTM, but FYI this can drag this PR a lot in time as it enters the scope of another SIG.

rosti

Thanks for the update @bart0sh !
Actually I like this approach better, but as @neolit123 we need to sell the idea to SIG API Machinery and that could take time or even not happen at all.
A few unnecessary String() calls can be omitted, but I can live with them.
I give you LGTM on the kubeadm side of things.
/lgtm

cmd/kubeadm/app/cmd/alpha/kubelet.go

cmd/kubeadm/app/cmd/config_test.go

cmd/kubeadm/test/util.go

bart0sh · 2018-12-19T17:16:08Z

@rosti, @neolit123 Thank you for the review!

As for the change of Version API, I tried to avoid it in the previous version of this patch. Then @fabriziopandini suggested this approach and I reworked the patch. I still have previous version, so please suggest which way we want to go.

In my opinion we should go faster way(previous patch) and submit Version API change as a separate PR. When/if it's accepted I'll be happy to submit new PR for kubeadm code that uses changed API.

Thoughts?

Added WithMajor, WithMinor, WithPatch and WithPreRelease methods to the Version API. These methods return copy of the Version object with one changed property(Major, Minor, Patch or preRelease).

Replaced hardcoded "v0.12.0" strings with MinimumControlPlaneVersion and MinimumKubeletVersion global variables. This should help with a regular release version bumps.

fabriziopandini

@bart0sh thanks for this news version!
/approve
/lgtm

Let's wait for sig-api-machinery opinion

bart0sh · 2018-12-20T09:35:44Z

@fabriziopandini @neolit123 Is anything required from me? Should I contact someone or should we assign someone from sig-api-machinery to this PR?

neolit123 · 2018-12-20T15:10:27Z

@bart0sh the likelyhood of this making it in before NY is minimal due to the api-machinery change.

/assign @caesarxuchao

bart0sh · 2018-12-20T15:39:23Z

@neolit123 so, should we wait or use previous version of this PR and make separate PR for api change as I proposed earlier?

neolit123 · 2018-12-20T16:08:46Z

@bart0sh

as per @fabriziopandini 's comment:

Let's wait for sig-api-machinery opinion

neolit123 · 2018-12-20T16:39:26Z

cmd/kubeadm/app/cmd/alpha/kubelet.go

-		kubeadm alpha phase kubelet config download --kubelet-version v1.12.0
-		`)
+		kubeadm alpha phase kubelet config download --kubelet-version %s
+		`, constants.MinimumKubeletVersion))


@fabriziopandini @rosti @bart0sh @rojkov

instead of the usage of this constant here i think we should just have a CurrentKubernetesMinorVersion (or a similar naming) that is v1.13.0.
we can then use this version in such CLI examples and also have it as the current version for unit tests.
WDYT?

would that Current... constant require manual updating on every release? that does not seem like a good idea

we already bump these on each release:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/constants/constants.go#L380-L383

in an ideal world:

an app runtime should fail if the build system did not add versioning to the binary.

skew versions and current versions should be obtained in variables based of the client version.

unit tests should not be bound to internet or client versioning functionality.

a default version should not be fetched from the internet by default.

Considering that this PR IMO is already achieving the goals to simplify version bumps, which was the original scope of the underlying issue, I'm not sure it is a good idea to conflate in this PR other goals like fixing version discovery for unit test or shifting to a dynamic approach for handling version skew policy. Might be follow up PRs are more appropriate in this case

then again, using constants.MinimumKubeletVersion as an example in the CLI might introduce problems. this is a version that is currently 1.12 instead of 1.13.

The idea of CurrentKubernetesVersion is good one, but outside of the scope of this PR. It can be done in some of the followup PRs though. For a start we can bump this version manually at the end of the release cycle, but ideally this should be done automatically by the build system.

caesarxuchao · 2018-12-21T01:24:14Z

The apimachinery change lgtm.

/approve

k8s-ci-robot · 2018-12-21T01:24:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bart0sh, caesarxuchao, fabriziopandini, neolit123

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,neolit123]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [caesarxuchao]

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

rosti · 2018-12-21T15:17:10Z

I think, that we can safely cancel the hold now and let this merge.
@neolit123 @fabriziopandini WDYT?

bart0sh · 2018-12-21T15:19:50Z

in my opinion it's ok to merge it. If there are any issues with it I'll fix it. It's reviewed and approved by many people already.

neolit123 · 2018-12-21T16:55:31Z

/hold cancel

bart0sh · 2018-12-21T19:42:13Z

/retest

fejta-bot · 2018-12-22T03:20:25Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-ci-robot requested review from kad and rosti December 11, 2018 10:31

bart0sh force-pushed the PR0045-kubeadm-1260-simplify-minimum-kubernetes-version-bumps branch from 4379c6f to 0f1f856 Compare December 11, 2018 10:41

rosti reviewed Dec 11, 2018

View reviewed changes

k8s-ci-robot assigned neolit123 Dec 11, 2018

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 11, 2018

bart0sh force-pushed the PR0045-kubeadm-1260-simplify-minimum-kubernetes-version-bumps branch from 0f1f856 to 8b2bf33 Compare December 11, 2018 14:12

neolit123 approved these changes Dec 11, 2018

View reviewed changes

k8s-ci-robot assigned fabriziopandini Dec 11, 2018

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 11, 2018

bart0sh force-pushed the PR0045-kubeadm-1260-simplify-minimum-kubernetes-version-bumps branch from 8b2bf33 to 9e8c9c5 Compare December 11, 2018 15:07

rosti approved these changes Dec 14, 2018

View reviewed changes

bart0sh force-pushed the PR0045-kubeadm-1260-simplify-minimum-kubernetes-version-bumps branch from 9e8c9c5 to 5d1cdfd Compare December 14, 2018 15:02

bart0sh force-pushed the PR0045-kubeadm-1260-simplify-minimum-kubernetes-version-bumps branch 2 times, most recently from 7af175b to c6f5cca Compare December 19, 2018 15:13

rosti approved these changes Dec 19, 2018

View reviewed changes

cmd/kubeadm/app/cmd/alpha/kubelet.go Outdated Show resolved Hide resolved

cmd/kubeadm/app/cmd/alpha/kubelet.go Outdated Show resolved Hide resolved

cmd/kubeadm/app/cmd/config_test.go Outdated Show resolved Hide resolved

cmd/kubeadm/test/util.go Outdated Show resolved Hide resolved

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

bart0sh force-pushed the PR0045-kubeadm-1260-simplify-minimum-kubernetes-version-bumps branch from c6f5cca to f4e0eeb Compare December 19, 2018 17:37

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

bart0sh added 2 commits December 19, 2018 20:41

version: add 3 methods

af07603

Added WithMajor, WithMinor, WithPatch and WithPreRelease methods to the Version API. These methods return copy of the Version object with one changed property(Major, Minor, Patch or preRelease).

kubeadm: simplify minimum Kubernetes version bumps

b40018d

Replaced hardcoded "v0.12.0" strings with MinimumControlPlaneVersion and MinimumKubeletVersion global variables. This should help with a regular release version bumps.

bart0sh force-pushed the PR0045-kubeadm-1260-simplify-minimum-kubernetes-version-bumps branch from f4e0eeb to b40018d Compare December 19, 2018 18:41

fabriziopandini approved these changes Dec 20, 2018

View reviewed changes

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 assigned caesarxuchao Dec 20, 2018

neolit123 reviewed Dec 20, 2018

View reviewed changes

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 21, 2018

k8s-ci-robot merged commit 190f6d8 into kubernetes:master Dec 22, 2018

kubeadm: simplify minimum Kubernetes version bumps #71946

kubeadm: simplify minimum Kubernetes version bumps #71946

Conversation

bart0sh commented Dec 11, 2018

rosti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bart0sh Dec 11, 2018 • edited Loading

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

rosti commented Dec 11, 2018

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Dec 11, 2018

rosti commented Dec 13, 2018

bart0sh commented Dec 13, 2018

cblecker commented Dec 13, 2018

rosti left a comment

Choose a reason for hiding this comment

bart0sh commented Dec 14, 2018

bart0sh commented Dec 19, 2018

neolit123 commented Dec 19, 2018

rosti left a comment

Choose a reason for hiding this comment

bart0sh commented Dec 19, 2018

fabriziopandini left a comment

Choose a reason for hiding this comment

bart0sh commented Dec 20, 2018

neolit123 commented Dec 20, 2018

bart0sh commented Dec 20, 2018

neolit123 commented Dec 20, 2018

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

caesarxuchao commented Dec 21, 2018

k8s-ci-robot commented Dec 21, 2018

rosti commented Dec 21, 2018

bart0sh commented Dec 21, 2018

neolit123 commented Dec 21, 2018

bart0sh commented Dec 21, 2018

fejta-bot commented Dec 22, 2018

bart0sh Dec 11, 2018 •

edited

Loading