Make handling Kubernetes versions easier #2506

jackfrancis · 2018-03-21T22:26:01Z

What this PR does / why we need it: Expands version generics and gets rid of static consts so we can more fluidly and easily evolve w/ newer Kubernetes versions going forward.

This PR will mean that to add support for future Kubernetes versions we maintain a single map (AllKubernetesSupportedVersions in pkg/api/common)

Release note:

rationalize Kubernetes version support handling

so we don’t have to define version whitelist twice

jackfrancis · 2018-03-21T22:47:20Z

pkg/api/common/const.go

@@ -53,120 +57,48 @@ const (
 )

 const (
-	// KubernetesVersion1Dot10Dot0RC1 is the string for kubernetes 1.10.0-rc.1


These consts were not adding to readability/maintainability, and runtime optimization is not a concern

jackfrancis · 2018-03-21T22:48:42Z

pkg/api/common/const.go

-	KubernetesVersion1Dot10Dot0RC1:   false,
+var AllKubernetesWindowsSupportedVersions = getAllKubernetesWindowsSupportedVersionsMap()
+
+func getAllKubernetesWindowsSupportedVersionsMap() map[string]bool {


Instead of Windows support being a mostly duplicated whitelist of the larger support matrix, this is now an override/blackslist

jackfrancis · 2018-03-21T22:50:05Z

pkg/api/common/helper_test.go

 		t.Errorf("It is not the default Kubernetes version")
 	}

-	version = GetValidPatchVersion(Kubernetes, "1.6.3")


All of these static tests tied to most recent patch versions were suggesting that there was some significance to testing the most recent patch version in these unit tests. In fact this unit test should be maintained independently of kubernets release version particulars.

jackfrancis · 2018-03-21T22:50:20Z

pkg/api/common/helper_test.go

+			if v != version {
+				t.Errorf("Expected version %s, instead got version %s", version, v)
+			}
+		}
 	}
 }

 func TestGetLatestPatchVersion(t *testing.T) {


ditto above

jackfrancis · 2018-03-21T22:51:51Z

pkg/api/common/helper_test.go

@@ -78,39 +46,31 @@ func Test_RationalizeReleaseAndVersion(t *testing.T) {
 		t.Errorf("It is not the default Kubernetes version")
 	}

+	latest1Dot6Version := GetLatestPatchVersion("1.6", GetAllSupportedKubernetesVersions())


We already have conveniences to grab the latest version in a major.minor release channel, so we don't need to be maintaining them statically to validate "orchestratorRelease" behavior.

jackfrancis · 2018-03-21T22:54:02Z

pkg/api/vlabs/validate.go

-					if o.OrchestratorVersion == common.KubernetesVersion1Dot6Dot6 ||
-						o.OrchestratorVersion == common.KubernetesVersion1Dot6Dot9 ||
-						o.OrchestratorVersion == common.KubernetesVersion1Dot6Dot11 {
+					sv, _ := semver.NewVersion(o.OrchestratorVersion)


Actually validating what our error condition suggests we're validating. :)

jackfrancis · 2018-03-21T22:54:11Z

pkg/api/vlabs/validate.go

-						if o.OrchestratorVersion == common.KubernetesVersion1Dot6Dot6 ||
-							o.OrchestratorVersion == common.KubernetesVersion1Dot6Dot9 ||
-							o.OrchestratorVersion == common.KubernetesVersion1Dot6Dot11 {
+						sv, _ := semver.NewVersion(o.OrchestratorVersion)


jackfrancis · 2018-03-21T22:54:17Z

pkg/api/vlabs/validate.go

-						o.OrchestratorVersion == common.KubernetesVersion1Dot7Dot12 ||
-						o.OrchestratorVersion == common.KubernetesVersion1Dot7Dot13 ||
-						o.OrchestratorVersion == common.KubernetesVersion1Dot7Dot14 {
+					sv, _ := semver.NewVersion(o.OrchestratorVersion)


jackfrancis · 2018-03-21T22:55:04Z

pkg/api/vlabs/validate.go

@@ -235,22 +224,6 @@ func (o *OrchestratorVersionProfile) Validate() error {
 	return o.OrchestratorProfile.Validate(false)
 }

-// ValidateForUpgrade validates upgrade input data


This function is never referenced.

jackfrancis · 2018-03-21T22:56:46Z

pkg/api/vlabs/validate_test.go

@@ -259,10 +259,7 @@ func Test_KubernetesConfig_Validate(t *testing.T) {
 	}

 	// Tests that apply to 1.6 and later releases
-	for _, k8sVersion := range []string{common.KubernetesVersion1Dot6Dot11, common.KubernetesVersion1Dot6Dot12, common.KubernetesVersion1Dot6Dot13,


I can't believe we've been maintaining this string slice literal

jackfrancis · 2018-03-21T22:57:20Z

pkg/api/vlabs/validate_test.go

@@ -274,7 +271,7 @@ func Test_KubernetesConfig_Validate(t *testing.T) {

 	trueVal := true
 	// Tests that apply to 1.8 and later releases
-	for _, k8sVersion := range []string{common.KubernetesVersion1Dot8Dot1} {


Yikes we were never doing what we said

CecileRobertMichon · 2018-03-22T18:11:53Z

Is there any way to make this go away? (and the other 2 const checks)

acs-engine/pkg/api/orchestrators_test.go

Line 129 in 79f6515

Expect(len(list.Properties.Orchestrators)).To(Equal(36))

(I know I'm asking for extra credit, your PR is already great as-is)

CecileRobertMichon · 2018-03-23T00:36:02Z

pkg/api/common/versions.go

 		if cons.Check(sv) {
 			ret = append(ret, v)
 		}
 	}
 	return ret
 }

+// GetVersionsLt returns a list of versions greater than a semver string given a list of versions


Is it just me or should this function be called GetVersionsGt?

scratch that, the comment should be returns a list of versions lesser than a semver string

CecileRobertMichon · 2018-03-23T18:41:40Z

pkg/api/orchestrators_test.go


 	// v20170930 - kubernetes only
 	list, e = GetOrchestratorVersionProfileListV20170930(common.Kubernetes, "")
 	Expect(e).To(BeNil())
-	Expect(len(list.Properties.Orchestrators)).To(Equal(36))
+	Expect(len(list.Properties.Orchestrators)).To(Equal(len(common.GetAllSupportedKubernetesVersions())))


CecileRobertMichon · 2018-03-23T18:42:00Z

pkg/api/orchestrators_test.go


-	// 1.10.0-rc.1 is not upgradable
+	// The latest version is not upgradable


CecileRobertMichon

lgtm

jackfrancis added the orchestrator/k8s label Mar 21, 2018

ghost assigned jackfrancis Mar 21, 2018

ghost added the in progress label Mar 21, 2018

now with fewer consts!

a248dd4

jackfrancis force-pushed the version-rev-rationalization branch from fb7b169 to a248dd4 Compare March 21, 2018 22:32

generalize KubeConfigs

42da2e2

so we don’t have to define version whitelist twice

jackfrancis changed the title ~~WIP: Make handling Kubernetes versions easier~~ Make handling Kubernetes versions easier Mar 21, 2018

jackfrancis commented Mar 21, 2018

View reviewed changes

jackfrancis added 2 commits March 21, 2018 16:04

these are not consts

0755d74

copy/paste errata

813b5d4

jackfrancis added 2 commits March 22, 2018 15:08

no more static orchestrator version num validations!

2abe2ca

whoops we have a straggler here

98302b7

CecileRobertMichon reviewed Mar 23, 2018

View reviewed changes

fix comment

be81858

CecileRobertMichon reviewed Mar 23, 2018

View reviewed changes

pkg/api/orchestrators_test.go

// 1.10.0-rc.1 is not upgradable

// The latest version is not upgradable

Copy link

Contributor

CecileRobertMichon Mar 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

CecileRobertMichon approved these changes Mar 23, 2018

View reviewed changes

jackfrancis merged commit a29c718 into Azure:master Mar 23, 2018

ghost removed the in progress label Mar 23, 2018

jackfrancis deleted the version-rev-rationalization branch March 23, 2018 20:15

jackfrancis added a commit that referenced this pull request Mar 27, 2018

Make handling Kubernetes versions easier (#2506)

b81ad78

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make handling Kubernetes versions easier #2506

Make handling Kubernetes versions easier #2506

jackfrancis commented Mar 21, 2018 •

edited

Loading

jackfrancis Mar 21, 2018

jackfrancis Mar 21, 2018

jackfrancis Mar 21, 2018

jackfrancis Mar 21, 2018

jackfrancis Mar 21, 2018

jackfrancis Mar 21, 2018

jackfrancis Mar 21, 2018

jackfrancis Mar 21, 2018

jackfrancis Mar 21, 2018

jackfrancis Mar 21, 2018

jackfrancis Mar 21, 2018

CecileRobertMichon commented Mar 22, 2018 •

edited

Loading

CecileRobertMichon Mar 23, 2018

CecileRobertMichon Mar 23, 2018

CecileRobertMichon Mar 23, 2018

CecileRobertMichon Mar 23, 2018

CecileRobertMichon left a comment


		// 1.10.0-rc.1 is not upgradable
		// The latest version is not upgradable

Make handling Kubernetes versions easier #2506

Make handling Kubernetes versions easier #2506

Conversation

jackfrancis commented Mar 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon commented Mar 22, 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

CecileRobertMichon left a comment

Choose a reason for hiding this comment

jackfrancis commented Mar 21, 2018 •

edited

Loading

CecileRobertMichon commented Mar 22, 2018 •

edited

Loading