Availability zone support for k8s #3453

sozercan · 2018-07-09T21:50:20Z

What this PR does / why we need it:

Adds support for availability zones infrastructure support for agents
Adds support for singlePlacementGroups for VMSS agent nodes

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1919

Special notes for your reviewer:
Work in progress - do not merge

If applicable:

documentation
unit tests
tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

Release note:

Add availability zone support for k8s agents

//cc @ritazh @kkmsft @khenidak @feiskyer

feiskyer · 2018-08-01T12:53:31Z

We should wait a while for kubernetes/enhancements#586, so as to support zoned AzureDisks as well.

aheumaier · 2018-08-16T13:25:49Z

@sozercan @khenidak Does it make sense to get this two topics separated to get this feature through ?

sozercan · 2018-08-16T17:07:27Z

@aheumaier: @ritazh is working on this PR currently

codecov · 2018-08-16T22:14:41Z

Codecov Report

Merging #3453 into master will increase coverage by 0.04%.
The diff coverage is 57.44%.

@@            Coverage Diff             @@
##           master    #3453      +/-   ##
==========================================
+ Coverage   55.41%   55.46%   +0.04%     
==========================================
  Files         108      108              
  Lines       16102    16146      +44     
==========================================
+ Hits         8923     8955      +32     
- Misses       6416     6425       +9     
- Partials      763      766       +3

sozercan · 2018-08-22T17:53:14Z

pkg/api/vlabs/validate.go

+				if a.AgentPoolProfiles[i].Count < len(a.AgentPoolProfiles[i].AvailabilityZones)*2 {
+					return errors.New("The node count and the number of availability zones provided can result in zone imbalance. To achieve zone balance, each zone should have at least 2 nodes or more. ")
+				}
+			}


can we add a validation for singlePlacementGroups for VMSS only?

CecileRobertMichon · 2018-08-22T21:22:22Z

parts/k8s/kubernetesagentresourcesvmss.t

@@ -38,6 +41,11 @@
      "name": "[variables('{{.Name}}VMSize')]"
    },
    "properties": {
+      {{if UseSinglePlacementGroup .}}
+      "singlePlacementGroup": true,


This could be:
"singlePlacementGroup": {{UseSinglePlacementGroup .}} ,

CecileRobertMichon · 2018-08-22T21:22:43Z

parts/k8s/kuberneteswinagentresourcesvmss.t

@@ -38,6 +41,11 @@
      "name": "[variables('{{.Name}}VMSize')]"
    },
    "properties": {
+      {{if UseSinglePlacementGroup .}}
+      "singlePlacementGroup": true,


CecileRobertMichon · 2018-08-22T21:23:03Z

pkg/acsengine/defaults.go

@@ -555,6 +556,33 @@ func setMasterNetworkDefaults(a *api.Properties, isUpgrade bool) {
 	}
 }

+// setVMSSDefaults
+// singlePlacementGroup = false, the scale set can be composed of multiple placement groups and has a range of 0-1,000 VMs
+// singlePlacementGroup = true,, default value, a scale set is composed of a single placement group, and has a range of 0-100 VMs


extra , here

CecileRobertMichon · 2018-08-22T21:24:53Z

pkg/acsengine/defaults.go

+// singlePlacementGroup = true,, default value, a scale set is composed of a single placement group, and has a range of 0-100 VMs
+// Large scale sets require Azure Managed Disks.
+// https://docs.microsoft.com/en-us/azure/virtual-machine-scale-sets/virtual-machine-scale-sets-placement-groups
+// For availability zones, only standard load balancer is supported.


these comments should be added to the docs instead (https://github.com/Azure/acs-engine/blob/master/docs/clusterdefinition.md)

CecileRobertMichon

/lgtm

…ones

acs-bot · 2018-08-29T21:04:19Z

New changes are detected. LGTM label has been removed.

ritazh · 2018-08-30T17:36:14Z

rebased, jenkins e2e passed. ready for another round of review

jackfrancis · 2018-08-30T18:03:18Z

pkg/acsengine/defaults.go

+				a.OrchestratorProfile.KubernetesConfig.LoadBalancerSku = "Standard"
+				a.OrchestratorProfile.KubernetesConfig.ExcludeMasterFromStandardLB = helpers.PointerToBool(api.DefaultExcludeMasterFromStandardLB)
+			}
+			if profile.SinglePlacementGroup == nil {


This should move up to be above line 570, so that if we ever change DefaultSinglePlacementGroup to false we get the foo in the if false set to managed disk thing.

jackfrancis · 2018-08-30T18:04:38Z

pkg/acsengine/template_generator.go

+			return *profile.SinglePlacementGroup
+		},
+		"HasAvailabilityZones": func(profile *api.AgentPoolProfile) bool {
+			return profile.AvailabilityZones != nil && len(profile.AvailabilityZones) > 0


Let's create a convenience function hasAvailabilityZones that this templat func calls...

jackfrancis · 2018-08-30T18:06:12Z

pkg/acsengine/params.go

@@ -173,6 +173,9 @@ func getParameters(cs *api.ContainerService, generatorCode string, acsengineVers
 	for _, agentProfile := range properties.AgentPoolProfiles {
 		addValue(parametersMap, fmt.Sprintf("%sCount", agentProfile.Name), agentProfile.Count)
 		addValue(parametersMap, fmt.Sprintf("%sVMSize", agentProfile.Name), agentProfile.VMSize)
+		if agentProfile.AvailabilityZones != nil && len(agentProfile.AvailabilityZones) > 0 {


...and then we just call hasAvailabilityZones from here

(sorry, the first part of the elipsis is below ;) )

Basically, that way HasAvailabilityZones in the params template directly correlates to the criteria here. We always want to define the param, and then inject it using this addValue function, according to the exact same criteria.

jackfrancis · 2018-08-30T18:08:03Z

@ritazh Looks great, added some final comments. Thanks so much!

tariq1890 · 2018-08-30T18:05:33Z

pkg/api/vlabs/validate.go

+				}
+
+				if sv.LT(minVersion) {
+					return errors.Errorf("availabilityZone is only available in Kubernetes version 1.12 or greater.")


nit: This should be errors.New

jackfrancis · 2018-08-30T20:04:39Z

/lgtm thanks @ritazh and @sozercan !!!

acs-bot · 2018-08-30T20:04:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, jackfrancis, sozercan

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~~ [CecileRobertMichon,jackfrancis]

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

sozercan added the do-not-merge/work-in-progress label Jul 9, 2018

ghost assigned sozercan Jul 9, 2018

ghost added the in progress label Jul 9, 2018

sozercan force-pushed the availabilityZone branch from 2916c7a to dd607f0 Compare July 10, 2018 00:01

This was referenced Aug 3, 2018

VMSS support for k8s masters #3609

Closed

Support for Standard LB in k8s v1.11 #3515

Merged

feiskyer mentioned this pull request Aug 16, 2018

Multizone support? #1919

Closed

ritazh force-pushed the availabilityZone branch from 79143b1 to 6294350 Compare August 16, 2018 22:06

ghost assigned ritazh Aug 16, 2018

acs-bot added the size/M label Aug 16, 2018

ritazh force-pushed the availabilityZone branch from 6294350 to 3804e7e Compare August 16, 2018 22:11

ritazh force-pushed the availabilityZone branch from 3804e7e to fb067f7 Compare August 16, 2018 22:28

acs-bot added size/L and removed size/M labels Aug 17, 2018

ritazh force-pushed the availabilityZone branch 3 times, most recently from 9cdb28e to b95ef71 Compare August 21, 2018 23:55

ritazh changed the title ~~[WIP] Availability zone support for k8s~~ Availability zone support for k8s Aug 22, 2018

acs-bot removed the do-not-merge/work-in-progress label Aug 22, 2018

ritazh requested review from CecileRobertMichon and jackfrancis August 22, 2018 16:41

sozercan commented Aug 22, 2018

View reviewed changes

CecileRobertMichon reviewed Aug 22, 2018

View reviewed changes

acs-bot assigned CecileRobertMichon Aug 28, 2018

CecileRobertMichon approved these changes Aug 28, 2018

View reviewed changes

acs-bot added lgtm approved labels Aug 28, 2018

sozercan and others added 8 commits August 29, 2018 14:01

Add availability zone support for agent pools

47d4e60

rm zones from master and vmas, add setVMSSDefaults, set default for z…

d2305a9

…ones

Add validation and err check

b0d60bd

Add e2e tests, example for zones

b2258d7

Add validation for singlePlacementGroups

397ff3f

Add unit test, doc, update validation

02f8f79

Add more unit tests

14420bf

Revert version

e6df391

ritazh force-pushed the availabilityZone branch from 62955e6 to e6df391 Compare August 29, 2018 21:04

acs-bot removed the lgtm label Aug 29, 2018

ritazh force-pushed the availabilityZone branch from d334eef to f3f8bd2 Compare August 29, 2018 22:31

Update 1.12.beta workaround

4bfd386

ritazh force-pushed the availabilityZone branch from f3f8bd2 to 4bfd386 Compare August 30, 2018 00:11

jackfrancis reviewed Aug 30, 2018

View reviewed changes

tariq1890 reviewed Aug 30, 2018

View reviewed changes

Address review comments

619cdeb

jackfrancis merged commit 9897327 into Azure:master Aug 30, 2018

ghost removed the in progress label Aug 30, 2018

sozercan deleted the availabilityZone branch August 30, 2018 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Availability zone support for k8s #3453

Availability zone support for k8s #3453

sozercan commented Jul 9, 2018 •

edited by ritazh

Loading

feiskyer commented Aug 1, 2018

aheumaier commented Aug 16, 2018

sozercan commented Aug 16, 2018 •

edited

Loading

codecov bot commented Aug 16, 2018 •

edited

Loading

sozercan Aug 22, 2018

ritazh Aug 22, 2018

CecileRobertMichon Aug 22, 2018

CecileRobertMichon Aug 22, 2018

CecileRobertMichon Aug 22, 2018

CecileRobertMichon Aug 22, 2018

CecileRobertMichon left a comment

acs-bot commented Aug 29, 2018

ritazh commented Aug 30, 2018

jackfrancis Aug 30, 2018

jackfrancis Aug 30, 2018

jackfrancis Aug 30, 2018

jackfrancis commented Aug 30, 2018

tariq1890 Aug 30, 2018

jackfrancis commented Aug 30, 2018

acs-bot commented Aug 30, 2018

Availability zone support for k8s #3453

Availability zone support for k8s #3453

Conversation

sozercan commented Jul 9, 2018 • edited by ritazh Loading

feiskyer commented Aug 1, 2018

aheumaier commented Aug 16, 2018

sozercan commented Aug 16, 2018 • edited Loading

codecov bot commented Aug 16, 2018 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

acs-bot commented Aug 29, 2018

ritazh commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Aug 30, 2018

Choose a reason for hiding this comment

jackfrancis commented Aug 30, 2018

acs-bot commented Aug 30, 2018

sozercan commented Jul 9, 2018 •

edited by ritazh

Loading

sozercan commented Aug 16, 2018 •

edited

Loading

codecov bot commented Aug 16, 2018 •

edited

Loading