Add availability zone support for masters

[ritazh]

What this PR does / why we need it:
Adds availability zones support for masters

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:

If applicable:

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

Release note:

Add availability zones support for k8s masters

cc @khenidak @feiskyer

[codecov]

Codecov Report

Merging #3864 into master will increase coverage by 0.23%.
The diff coverage is 61.72%.

@@            Coverage Diff             @@
##           master    #3864      +/-   ##
==========================================
+ Coverage   56.25%   56.49%   +0.23%     
==========================================
  Files         109      109              
  Lines       16494    16516      +22     
==========================================
+ Hits         9279     9330      +51     
+ Misses       6406     6378      -28     
+ Partials      809      808       -1

[jackfrancis]

Do we have a HasVMSSAgentPool-type method? It'd be more expressive to do:

if properties.HasVMSSAgentPool() {
    setVMSSDefaultsForAgents(properties)
}

[jackfrancis]

Is there a reason we're not doing this in a func like the vmss for agents defaults flow above?

[jackfrancis]

Let's go full java and change this func name to HasZonesForAllAgentPools

[tariq1890]

:'(

[jackfrancis]

FYI got an E2E flake on 1.8:

https://circleci.com/gh/Azure/acs-engine/43044?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

(just in case we get another flake or two we can start to debug more deeply)

[ritazh]

Also added this: https://jenkins.azure-containers.io/view/acs-engine/job/k8s-availability-zones-all/

[ritazh]

@jackfrancis @tariq1890 ready for another round of review

[CecileRobertMichon]

we should use orchestratorRelease in unit tests whenever possible as individual patches will get deprecated as we add new ones (we now support the two latest patches of each minor version)

[CecileRobertMichon]

Is this going to silently override the user's choice if they select storage account? Should we validate + error if the user selects storageAccount + singlePlacement false

[ritazh]

I personally think this is too much detail for end users. so we should set for them but I am ok with either. What would you recommend?

[CecileRobertMichon]

The foundation of acs-engine is that it is highly customizable so I think we should assume that anything that is exposed to the user can and will be customized by users. If we want to set a value for the users we shouldn't expose it to them in the apimodel. I think a simple validation check should be enough in this case. That's my recommendation but I'll let you make the call.

[ritazh]

made it a validation instead of default

[CecileRobertMichon]

same here for overriding LB defaults

[ritazh]

Same here.

[ritazh]

now it's a validation instead of default

[CecileRobertMichon]

Any reason for using 2017-08-01 and not a more recent version like 2018-04-01 that we already use for accelerated networking?

In any case, we have a backlog item to rationalize the default version #3835 so the version will be bumped soon. If the version needs to be 2017-08-01 and is reused in multiple places I would recommend creating a variable for it.

[CecileRobertMichon]

if a.OrchestratorProfile.KubernetesConfig == nil is true it would cause a nil point dereference.

At this point this should never be true (we should be setting KubernetesConfig before we reach this line) but the logic of the if statement is contradictory.

[CecileRobertMichon]

ditto

[CecileRobertMichon]

let's use the helper function helpers.IsFalseBoolPointer here instead of dereferencing the pointer which might be nil

[CecileRobertMichon]

&& a.OrchestratorProfile.KubernetesConfig.LoadBalancerSku != nil

Defined KubernetesConfig but no LoadBalancerSku defined should not fail

[CecileRobertMichon]

The only supported values for MasterProfile.Count are 1, 3 and 5 so this is unreachable code

[CecileRobertMichon]

does this mean that this function should return false if MasterProfile is nil but all agents have AZs? Maybe it should be an || instead

What do you think about making this a single line? :
return (p.MasterProfile == nil || p.MasterProfile.HasAvailabilityZones()) && p.HasZonesForAllAgentPools()

[ritazh]

this function should return true if the cluster contains all profiles with AZs. so in this case, if MasterProfile is nil then it should return false.

[ritazh]

What do you think about making this a single line? :
return (p.MasterProfile == nil || p.MasterProfile.HasAvailabilityZones()) && p.HasZonesForAllAgentPools()

how about

return (p.MasterProfile != nil && p.MasterProfile.HasAvailabilityZones()) && p.HasZonesForAllAgentPools())

[CecileRobertMichon]

So that means it won't be possible to use AZ for agent pool only (AKS) clusters? The way I see this if a cluster only has agents and all its agents use AZ then all the cluster nodes use AZ. If that's not the logic we are trying to achieve let's rename the function to something like MastersAndAgentsUseAvailabilityZones

[ritazh]

AZ for agent pool only (AKS) clusters

That is the next thing we want to focus on after we have enabled AZ for masters and agents as it is done currently.

The way I see this if a cluster only has agents and all its agents use AZ then all the cluster nodes use AZ

This PR currently has validations to ensure all profiles (masters and agents) use AZ in the cluster. We do not allow a mix of AZ and non-AZ when creating a new cluster. Zones need to be explicitly configured hence the requirement to ask the user to configure AZ for both masters and agents. Let me know if you have other questions or recommendations.

---

To avoid confusion, I have added the following for the availabilityZones feature as documented in clusterdefinition.md. Please let me know if any of this is not clear or can be improved.

To protect your cluster from datacenter-level failures, you can enable the Availability Zones feature for your cluster by configuring "availabilityZones" for the master profile and all of the agentPool profiles in the cluster definition. This feature only applies to Kubernetes clusters version 1.12+. Supported values are arrays of strings, each representing a supported availability zone in a region for your subscription. e.g. "availabilityZones": ["1","2"] represents zone 1 and zone 2 can be used. To get supported zones for a region in your subscription, run az vm list-skus --location centralus --query "[?name=='Standard_DS2_v2'].[locationInfo, restrictions"] -o table. You should see values like 'zones': ['2', '3', '1'] appear in the first column. If NotAvailableForSubscription appears in the output, then create an Azure support ticket to enable zones for that region. NOTE: To ensure high availability, each profile must define at least two nodes per zone. e.g. An agent pool profile with 2 zones: "availabilityZones": ["1","2"] must have at least 4 nodes total with "count": 4. When "availabilityZones" is configured, the "loadBalancerSku" will default to Standard as Standard LoadBalancer is required for availability zones.

[CecileRobertMichon]

/lgtm

[acs-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, ritazh

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]

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