feat: add "get-versions --azure-env" flag to list custom clouds supported versions #3394

haofan-ms · 2020-06-03T04:29:44Z

Reason for Change:

The aks-engine get-versions command only shows Azure supported versions.
The --azure-env flag will allow users to pass a target custom cloud like Azure Stack and get back supported versions on that target cloud.

Usage: aks-engine get-versions --azure-env AzureStackCloud

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Notes:

jadarsie · 2020-06-03T16:02:32Z

Will take a look

jackfrancis · 2020-06-03T17:39:19Z

pkg/api/common/versions.go

@@ -198,37 +198,99 @@ var AllKubernetesSupportedVersions = map[string]bool{
 	"1.19.0-beta.0":  true,
 }

+// AllKubernetesSupportedVersionsOnAzureStack is a whitelist map of all supported Kubernetes version strings on Azure Stack
+// The bool value indicates if creating new clusters with this version is allowed
+var AllKubernetesSupportedVersionsOnAzureStack = map[string]bool{


Will you be maintaining these version support lists going forward? In practice, this means when the AKS Engine project moves versions forward for public cloud, Azure Stack won't get that support. That seems sane, just want to clarify.

jadarsie

First pass

jadarsie · 2020-06-03T17:45:31Z

cmd/get_versions.go

@@ -44,6 +45,7 @@ func newGetVersionsCmd() *cobra.Command {
 	gvc.orchestrator = "Kubernetes" // orchestrator is always Kubernetes
 	f.StringVar(&gvc.version, "version", "", "Kubernetes version (optional)")
 	f.BoolVar(&gvc.windows, "windows", false, "Kubernetes cluster with Windows nodes (optional)")
+	f.StringVar(&gvc.azureEnv, "azureEnv", "", "The target Azure cloud (default is 'AzurePublicCloud')")


Please keep it consistent with the deploy command by calling this flag azure-env

Initially it was set to be like that, but that failed with a linter check error because of the "-" so I have to change it.

I mean this ... (&gvc.azureEnv, "azure-env", ...

jadarsie · 2020-06-03T17:46:36Z

cmd/get_versions.go

@@ -44,6 +45,7 @@ func newGetVersionsCmd() *cobra.Command {
 	gvc.orchestrator = "Kubernetes" // orchestrator is always Kubernetes
 	f.StringVar(&gvc.version, "version", "", "Kubernetes version (optional)")
 	f.BoolVar(&gvc.windows, "windows", false, "Kubernetes cluster with Windows nodes (optional)")
+	f.StringVar(&gvc.azureEnv, "azureEnv", "", "The target Azure cloud (default is 'AzurePublicCloud')")


The third param is the default value, pass AzurePublicCloud please

jadarsie · 2020-06-03T17:49:10Z

cmd/get_versions.go

@@ -44,6 +45,7 @@ func newGetVersionsCmd() *cobra.Command {
 	gvc.orchestrator = "Kubernetes" // orchestrator is always Kubernetes
 	f.StringVar(&gvc.version, "version", "", "Kubernetes version (optional)")
 	f.BoolVar(&gvc.windows, "windows", false, "Kubernetes cluster with Windows nodes (optional)")
+	f.StringVar(&gvc.azureEnv, "azureEnv", "", "The target Azure cloud (default is 'AzurePublicCloud')")


no need to add (default is 'AzurePublicCloud') at the end, the command package takes care of that if you pass a default value

jadarsie · 2020-06-03T17:50:57Z

pkg/api/apiloader.go

@@ -145,6 +145,7 @@ func (a *Apiloader) LoadContainerServiceForAgentPoolOnlyCluster(
 	hasExistingCS := existingContainerService != nil
 	IsSSHAutoGenerated := false
 	hasWindows := false
+	onAzureStack := false


I think variable name isAzureStack would align better with the code base

jadarsie · 2020-06-03T17:53:50Z

pkg/api/common/versions_test.go

@@ -8,46 +8,72 @@ import (
 )

 func Test_GetAllSupportedKubernetesVersions(t *testing.T) {
-	responseFromGetter := GetAllSupportedKubernetesVersions(true, false)
+	responseFromGetter := GetAllSupportedKubernetesVersions(true, false, false)


Passing a const (notAzureStack or similar) instead of bool false can possibly improve readability

jadarsie · 2020-06-03T18:12:02Z

pkg/api/orchestrators.go

+func isVersionSupported(csOrch *OrchestratorProfile, onAzureStack bool) bool {
 	supported := false
-	for _, version := range versionsMap[csOrch.OrchestratorType] {
+	if onAzureStack {
+		for _, version := range versionsMapAzureStack[csOrch.OrchestratorType] {

-		if version == csOrch.OrchestratorVersion {
-			supported = true
-			break
+			if version == csOrch.OrchestratorVersion {
+				supported = true
+				break
+			}
+		}
+	} else {
+		for _, version := range versionsMap[csOrch.OrchestratorType] {
+
+			if version == csOrch.OrchestratorVersion {
+				supported = true
+				break
+			}
 		}
 	}
 	return supported
 }


This may be easier to read

supported := false versions := versionsMap[csOrch.OrchestratorType] if onAzureStack { versions = versionsMapAzureStack[csOrch.OrchestratorType] } for _, version := range versions { if version == csOrch.OrchestratorVersion { supported = true break } } return supported

jadarsie · 2020-06-03T18:12:59Z

pkg/api/orchestrators.go

 	var err error
+	onAzureStack := (azureEnv == "AzureStackCloud")


The code base usually does a case insensitive comparison for cloud names

jadarsie · 2020-06-03T18:14:10Z

pkg/api/orchestrators.go

@@ -207,7 +225,7 @@ func getKubernetesAvailableUpgradeVersions(orchestratorVersion string, supported

 }

-func dcosInfo(csOrch *OrchestratorProfile, hasWindows bool) ([]*OrchestratorVersionProfile, error) {
+func dcosInfo(csOrch *OrchestratorProfile, hasWindows bool, onAzureStack bool) ([]*OrchestratorVersionProfile, error) {


onAzureStack not used in function

This is because kubernetesInfo, dcosInfo, swarmInfo, dockerceInfo are in the orchestratorsFunc, and they have to follow the structure of orchestratorsFunc (https://github.com/Azure/aks-engine/blob/master/pkg/api/orchestrators.go#L16). Similarly, the hasWindows variable is also not used in these functions, but have to exist.

jadarsie · 2020-06-03T18:14:22Z

pkg/api/orchestrators.go

@@ -255,7 +273,7 @@ func dcosUpgrades(csOrch *OrchestratorProfile) []*OrchestratorProfile {
 	return ret
 }

-func swarmInfo(csOrch *OrchestratorProfile, hasWindows bool) ([]*OrchestratorVersionProfile, error) {
+func swarmInfo(csOrch *OrchestratorProfile, hasWindows bool, onAzureStack bool) ([]*OrchestratorVersionProfile, error) {


same as above

jadarsie · 2020-06-03T18:14:29Z

pkg/api/orchestrators.go

@@ -280,7 +298,7 @@ func swarmInfo(csOrch *OrchestratorProfile, hasWindows bool) ([]*OrchestratorVer
 	}, nil
 }

-func dockerceInfo(csOrch *OrchestratorProfile, hasWindows bool) ([]*OrchestratorVersionProfile, error) {
+func dockerceInfo(csOrch *OrchestratorProfile, hasWindows bool, onAzureStack bool) ([]*OrchestratorVersionProfile, error) {


same as above

jadarsie · 2020-06-03T19:09:51Z

pkg/api/common/versions.go

+	"1.15.12": true,
+	"1.16.8":  false,
+	"1.16.9":  true,
+	"1.16.10": false,


v1.16.10 and v1.17.6 should be true

jadarsie · 2020-06-03T19:11:37Z

pkg/api/common/versions.go

+	"1.16.8":  false,
+	"1.16.9":  true,
+	"1.17.4":  false,
+	"1.17.5":  true,


add v1.16.10 and v1.17.6

jadarsie · 2020-06-03T19:12:17Z

pkg/api/common/versions.go

+	"1.15.10": false,
+	"1.15.11": true,
+	"1.15.12": true,
+	"1.16.8":  false,


Why 1.16.8? do not think we ever supported that one

The way I determine the supported versions is based on the Windows k8s binary URLs (https://github.com/Azure/aks-engine/blob/master/vhd/packer/configure-windows-vhd.ps1#L86-L94) and Linux k8s binaries (https://github.com/Azure/aks-engine/blob/master/vhd/packer/install-dependencies.sh#L349-L364). That may not be the best way, I will update.

codecov · 2020-06-04T05:12:02Z

Codecov Report

Merging #3394 into master will increase coverage by 0.23%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master    #3394      +/-   ##
==========================================
+ Coverage   72.96%   73.20%   +0.23%     
==========================================
  Files         147      147              
  Lines       24978    24967      -11     
==========================================
+ Hits        18226    18276      +50     
+ Misses       5624     5562      -62     
- Partials     1128     1129       +1

Impacted Files	Coverage Δ
pkg/api/common/const.go	`40.00% <ø> (ø)`
pkg/api/apiloader.go	`61.57% <20.00%> (+0.20%)`	⬆️
pkg/api/vlabs/validate.go	`80.22% <80.00%> (-0.15%)`	⬇️
pkg/api/orchestrators.go	`62.26% <82.35%> (+0.68%)`	⬆️
cmd/get_versions.go	`95.34% <100.00%> (+0.11%)`	⬆️
cmd/upgrade.go	`44.32% <100.00%> (ø)`
pkg/api/common/versions.go	`96.84% <100.00%> (+0.29%)`	⬆️
pkg/api/convertertoagentpoolonlyapi.go	`89.93% <100.00%> (ø)`
pkg/api/convertertoapi.go	`93.52% <100.00%> (+0.35%)`	⬆️
pkg/api/defaults.go	`92.58% <100.00%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f52cd34...4233e49. Read the comment docs.

jadarsie · 2020-06-04T16:51:24Z

/lgtm

jackfrancis

lgtm

running some pan-version sanity checks, thank you @haofan-ms!

acs-bot · 2020-06-04T17:06:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haofan-ms, jackfrancis, jadarsie

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:

~~OWNERS~~ [jackfrancis]

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

…rted versions (Azure#3394)

haofan-ms added 4 commits June 2, 2020 20:33

chore: update get-versions for Azure Stack

befff6f

chore: update get-versions for Azure Stack

a4d1e03

Merge branch 'master' into update-get-versions

5eab526

update lint errors

413fd3c

acs-bot added the size/XL label Jun 3, 2020

jadarsie closed this Jun 3, 2020

jadarsie reopened this Jun 3, 2020

jackfrancis reviewed Jun 3, 2020

View reviewed changes

jadarsie reviewed Jun 3, 2020

View reviewed changes

address comments

1e4e5e6

acs-bot added size/L and removed size/XL labels Jun 3, 2020

haofan-ms added 4 commits June 3, 2020 15:42

update support version

7ed9cf2

fix unit-test

e251e76

update default k8s version for Azure Stack

17ce373

fix unit-test failure for Azure Stack

4233e49

mboersma added the azurestack label Jun 4, 2020

jadarsie changed the title ~~chore: update get-versions for Azure Stack~~ feat: add --azure-env flag to get-versions to list custom clouds supported version Jun 4, 2020

jadarsie changed the title ~~feat: add --azure-env flag to get-versions to list custom clouds supported version~~ feat: add "get-versions --azure-env" flag to list custom clouds supported versions Jun 4, 2020

acs-bot assigned jadarsie Jun 4, 2020

acs-bot added the lgtm label Jun 4, 2020

jadarsie requested a review from jackfrancis June 4, 2020 16:52

jackfrancis approved these changes Jun 4, 2020

View reviewed changes

acs-bot added the approved label Jun 4, 2020

mboersma added this to In progress in backlog via automation Jun 4, 2020

jackfrancis merged commit aa2973a into Azure:master Jun 4, 2020

backlog automation moved this from In progress to Done Jun 4, 2020

fmotrifork mentioned this pull request Jun 8, 2020

aks-engine 0.52.0 fishworks/fish-food#768

Merged

haofan-ms deleted the update-get-versions branch August 11, 2020 05:07

penggu pushed a commit to penggu/aks-engine that referenced this pull request Oct 28, 2020

feat: add "get-versions --azure-env" flag to list custom clouds suppo…

05f5efa

…rted versions (Azure#3394)

haofan-ms mentioned this pull request Mar 1, 2021

docs: update get-verions section of Azure Stack documentation #4308

Merged

8 tasks

		var err error
		onAzureStack := (azureEnv == "AzureStackCloud")

feat: add "get-versions --azure-env" flag to list custom clouds supported versions #3394

feat: add "get-versions --azure-env" flag to list custom clouds supported versions #3394

Conversation

haofan-ms commented Jun 3, 2020 • edited by jadarsie

jadarsie commented Jun 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadarsie left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadarsie Jun 3, 2020 • edited

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

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

codecov bot commented Jun 4, 2020 • edited

Codecov Report

jadarsie commented Jun 4, 2020

jackfrancis left a comment

Choose a reason for hiding this comment

acs-bot commented Jun 4, 2020

haofan-ms commented Jun 3, 2020 •

edited by jadarsie

jadarsie Jun 3, 2020 •

edited

codecov bot commented Jun 4, 2020 •

edited