VirtualMachineScaleSets support for Kubernetes #2620

sozercan · 2018-04-06T19:51:22Z

What this PR does / why we need it:

Adds support for Virtual Machine Scale sets for Kubernetes v1.10 agent pools

Which issue this PR fixes:
fixes #2403

If applicable:

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

Release note:

Add Virtual Machine Scale Sets support for Kubernetes v1.10+ agent availability profile

cc @feiskyer @JiangtianLi

feiskyer · 2018-04-07T13:22:04Z

👍 VirtualMachineScaleSets is only supported with kubernetes v1.10 and above versions. So I think we don't need to check backward compatibility.

todoesverso · 2018-04-10T21:03:16Z

I don't really understand why is this documented as supported if there is an open PR. Is that a bug?

https://github.com/Azure/acs-engine/blob/master/docs/clusterdefinition.md#agentpoolprofiles

sozercan · 2018-04-10T22:36:04Z

@todoesverso VMSS is supported for Swarm and DCOS but not for Kubernetes yet (support is added with Kubernetes v1.10)

PoisonousJohn · 2018-04-11T06:25:54Z

@sozercan hey! we're trying to test this PR with kubernetes 1.10.0 and cluster autoscaler. We've got a problem with unregistered nodes.

I0410 05:35:30.594811       1 static_autoscaler.go:184] 1 unregistered nodes present
I0410 05:35:30.594853       1 utils.go:330] Removing unregistered node 
I0410 05:35:30.594952       1 azure_scale_set.go:273] Deleting vmss instances [%!q(*azure.azureRef=&{})]
E0410 05:35:30.594990       1 azure_scale_set.go:293] getLastSegment failed with error: identifier '/' not found in resource name ""
W0410 05:35:30.595004       1 utils.go:353] Failed to remove node : identifier '/' not found in resource name ""
E0410 05:35:30.595027       1 static_autoscaler.go:191] Failed to remove unregistered nodes: identifier '/' not found in resource name ""

Digging through other issues, I found this

kubernetes/autoscaler#775

Is useInstanceMetadata supposed to be false with VMSS and k8s 1.10.0?
We've tried to set useInstanceMetadata to true, but cluster fails to deploy.

feiskyer · 2018-04-11T06:31:51Z

Is useInstanceMetadata supposed to be false with VMSS and k8s 1.10.0?

Yep, this is supported in v1.10. But there are a bug for vmss with autoscaler 1.2.0, the fix is here kubernetes/autoscaler#758. It should be included in v1.2.1.

PoisonousJohn · 2018-04-11T06:40:31Z

@feiskyer oh thx! We'll try it with 1.2.1.

khenidak

few questions/changes but i think it is a great start

khenidak · 2018-04-12T00:02:55Z

examples/kubernetes-vmss/kubernetes.json

+        ]
+      }
+    },
+    "servicePrincipalProfile": {


Let us default our examples to MSI. MSI (and later EMSi when possible)

thanks, updated

khenidak · 2018-04-12T00:05:19Z

parts/k8s/kubernetesmastervars.t

@@ -119,7 +119,11 @@
    "cloudProviderRatelimitQPS": "[parameters('cloudProviderRatelimitQPS')]",
    "cloudProviderRatelimitBucket": "[parameters('cloudProviderRatelimitBucket')]",
    "useManagedIdentityExtension": "{{ UseManagedIdentity }}",
+{{if .HasVirtualMachineScaleSets}}
+    "useInstanceMetadata": "false",


what is the reason for this?

1.10 supports setting useInstanceMetadata to both true and false, so we should keep same logic: "useInstanceMetadata": "{{ UseInstanceMetadata }}"

due to kubernetes/kubernetes#62611, if instanceMetadata is enabled and vmType is set to vmss, master kubelet won't start. If it's disabled, it works fine. One possible (and messy) workaround is to duplicate cloud config on the masters, so master has kubelet vmType as standard and controller-manager has vmss. Better option is to wait until 1.10.2 with above pr merged.

Since this is experimental (we'll add docs later) let's change the above to "useInstanceMetadata": "{{ UseInstanceMetadata }}". Experimenters pre-1.10.2 should specify "useInstanceMetadata": "false" in their api models until then.

This way we don't have to submit a follow-up PR after 1.10.2.

khenidak · 2018-04-12T00:07:21Z

parts/k8s/kubernetesbase.t

@@ -55,6 +55,8 @@
      {{if $index}}, {{end}}
      {{if .IsWindows}}
        {{template "k8s/kuberneteswinagentresourcesvmas.t" .}}
+      {{else if .IsVirtualMachineScaleSets}}


Do we have a reason not support windows as well?

windows agents should be working now

khenidak · 2018-04-12T00:09:02Z

pkg/acsengine/engine.go

 			for _, agentProfile := range cs.Properties.AgentPoolProfiles {
 				if agentProfile.IsAvailabilitySets() {
 					return true
 				}
 			}
 			return false
 		},
+		"AnyAgentUsesVirtualMachineScaleSets": func() bool {


I think we shouldn't allow bootstrapping a mixed mode cluster. Either availability sets or scale sets but not both.

added validation check so mixed mode clusters are not allowed

khenidak · 2018-04-12T00:17:52Z

parts/k8s/kubernetesagentresourcesvmss.t

+              "secrets": "[variables('linuxProfileSecrets')]"
+            {{end}}
+        },
+        "storageProfile": {


There was a PR recently enabling users to provision using their own images. is this included? Used currently by openshift on acs-engine @jim-minter FYI

Yes, seems to be included, thanks

custom images should be included now

khenidak · 2018-04-12T00:19:13Z

parts/k8s/kubernetesagentresourcesvmss.t

+          "osDisk": {
+            "createOption": "FromImage",
+            "caching": "ReadWrite"
+          {{if .IsStorageAccount}}


@jackfrancis I think we should validate against storage accounts for new clusters while allowing them for scale ops. Is this possible?

@khenidak I'm not sure what you mean.

@JackQuincy can you confirm that there's no special validation/template processing context for deployment vs scale?

khenidak · 2018-04-12T00:23:02Z

@kkmsft for autoscaler + VMSS work

feiskyer · 2018-04-19T08:02:55Z

@sozercan Thanks for awesome work. LGTM

@JiangtianLi Could you also have a look at this?

JiangtianLi · 2018-04-20T01:11:53Z

parts/k8s/kuberneteswindowssetup.ps1

    "subnetName": "$global:SubnetName",
    "securityGroupName": "$global:SecurityGroupName",
    "vnetName": "$global:VNetName",
    "routeTableName": "$global:RouteTableName",
    "primaryAvailabilitySetName": "$global:PrimaryAvailabilitySetName",
+    "primaryScaleSetName": "$global:PrimaryScaleSetName",


where is primaryScaleSetName used?

Both vmType and primaryScaleSetName are part of the cloud provider config for Azure for v1.10.

JiangtianLi · 2018-04-20T01:12:05Z

parts/k8s/kuberneteswindowssetup.ps1

@@ -188,11 +186,13 @@ Write-AzureConfig()
    "aadClientSecret": "$AADClientSecret",
    "resourceGroup": "$global:ResourceGroup",
    "location": "$Location",
+    "vmType": "$global:VmType",


where is vmType used?

Same as above

JiangtianLi · 2018-04-20T01:12:51Z

parts/k8s/kuberneteswindowssetup.ps1

@@ -346,7 +345,7 @@ $KubeletCommandLine
 "@
    } else {
        $kubeStartStr += @"
-


thanks for cleaning up. :)

JiangtianLi · 2018-04-20T01:28:06Z

parts/k8s/kuberneteswinagentresourcesvmss.t

@@ -0,0 +1,193 @@
+{{if .IsStorageAccount}}


what is the diff of this file with kubernetesagentresourcesvmss.t?

74c74 < "resourceNameSuffix" : "[variables('winResourceNamePrefix')]", --- > "resourceNameSuffix" : "[variables('nameSuffix')]", 131,134c131,148 < "computerNamePrefix": "[concat(substring(variables('nameSuffix'), 0, 5), 'acs')]", < {{GetKubernetesWindowsAgentCustomData .}} < "adminUsername": "[variables('windowsAdminUsername')]", < "adminPassword": "[variables('windowsAdminPassword')]" --- > "adminUsername": "[variables('username')]", > "computerNamePrefix": "[variables('{{.Name}}VMNamePrefix')]", > {{GetKubernetesAgentCustomData .}} > "linuxConfiguration": { > "disablePasswordAuthentication": "true", > "ssh": { > "publicKeys": [ > { > "keyData": "[parameters('sshRSAPublicKey')]", > "path": "[variables('sshKeyPath')]" > } > ] > } > } > {{if HasLinuxSecrets}} > , > "secrets": "[variables('linuxProfileSecrets')]" > {{end}} 137c151,153 < {{GetDataDisks .}} --- > {{if not (UseAgentCustomImage .)}} > {{GetDataDisks .}} > {{end}} 139,142c155,162 < "offer": "[variables('agentWindowsOffer')]", < "publisher": "[variables('agentWindowsPublisher')]", < "sku": "[variables('agentWindowsSku')]", < "version": "[variables('agentWindowsVersion')]" --- > {{if UseAgentCustomImage .}} > "id": "[resourceId(variables('{{.Name}}osImageResourceGroup'), 'Microsoft.Compute/images', variables('{{.Name}}osImageName'))]" > {{else}} > "offer": "[variables('{{.Name}}osImageOffer')]", > "publisher": "[variables('{{.Name}}osImagePublisher')]", > "sku": "[variables('{{.Name}}osImageSKU')]", > "version": "[variables('{{.Name}}osImageVersion')]" > {{end}} 163,165c183,185 < "publisher": "Microsoft.Compute", < "type": "CustomScriptExtension", < "typeHandlerVersion": "1.8", --- > "publisher": "Microsoft.Azure.Extensions", > "type": "CustomScript", > "typeHandlerVersion": "2.0", 169c189 < "commandToExecute": "[concat('powershell.exe -ExecutionPolicy Unrestricted -command \"', '$arguments = ', variables('singleQuote'),'-MasterIP ',variables('kubernetesAPIServerIP'),' -KubeDnsServiceIp ',variables('kubeDnsServiceIp'),' -MasterFQDNPrefix ',variables('masterFqdnPrefix'),' -Location ',variables('location'),' -AgentKey ',variables('clientPrivateKey'),' -AADClientId ',variables('servicePrincipalClientId'),' -AADClientSecret ',variables('servicePrincipalClientSecret'),variables('singleQuote'), ' ; ', variables('windowsCustomScriptSuffix'), '\" > %SYSTEMDRIVE%\\AzureData\\CustomDataSetupScript.log 2>&1')]" --- > "commandToExecute": "[concat(variables('provisionScriptParametersCommon'),' /usr/bin/nohup /bin/bash -c \"/bin/bash /opt/azure/containers/provision.sh >> /var/log/azure/cluster-provision.log 2>&1\"')]" 178c198 < "type": "ManagedIdentityExtensionForWindows", --- > "type": "ManagedIdentityExtensionForLinux",

JiangtianLi

Thanks for doing this. Generally lgtm. left a few comments.

jackfrancis · 2018-04-23T18:32:17Z

Just one more request @sozercan: could you kindly add a validation that requires instanceMetadata to be false for k8s < 1.10.2? (I know that's basically everything now but that will become useful over time.)

Thanks so much for this, looks great!

khenidak · 2018-04-23T18:53:35Z

@jackfrancis that is a bit of an overkill? we can run imds on 1.8 and 1.9

CecileRobertMichon · 2018-04-23T19:29:01Z

Is the description for availabilityProfile at https://github.com/Azure/acs-engine/blob/master/docs/clusterdefinition.md#agentpoolprofiles still accurate & relevant?

Supported values are VirtualMachineScaleSets (default) and AvailabilitySet. For Kubernetes clusters before k8s version 1.10, use AvailabilitySet. Otherwise, you should use VirtualMachineScaleSets, unless you need features such as dynamic attached disks

sozercan · 2018-04-24T17:14:38Z

@jackfrancis added validation when using VMSS with k8s < v1.10.2

@CecileRobertMichon thanks, updated

jackfrancis · 2018-04-24T17:59:26Z

@khenidak Sorry I wasn't clear, I meant require imds = false for vmss

I see you did that in a recent commit, thanks @sozercan!

jackfrancis

lgtm

ghost assigned sozercan Apr 6, 2018

ghost added the in progress label Apr 6, 2018

sozercan force-pushed the vmss branch from 86bcb43 to ac6a3ef Compare April 9, 2018 18:23

sozercan mentioned this pull request Apr 9, 2018

Kubernetes Cluster Autoscaler (VMSS) addon #2637

Merged

4 tasks

sozercan force-pushed the vmss branch 3 times, most recently from 19a7d05 to 61cc30b Compare April 9, 2018 23:31

khenidak suggested changes Apr 12, 2018

View reviewed changes

sozercan force-pushed the vmss branch 2 times, most recently from 90346d0 to 092547e Compare April 14, 2018 00:33

sozercan added 13 commits April 17, 2018 13:31

initial commit

526b476

master resource

990bb89

vmss name and vnet dependency

af9544a

revert master change

8341d4c

added parameters for vmType and primaryScaleSetName

cca6bf4

instanceMetadata

6e32592

update vmss upgrade policy

11f9b9a

update vmss agent

94f724b

add osdisk and datadisk storage accounts

020496d

add validation

7e6c44e

update example

9a6806f

add managed identity

d1d59ad

windows vmss node

1cb4272

setting hostname dynamically on windows nodes

82a56cb

sozercan force-pushed the vmss branch from 264f17e to 82a56cb Compare April 17, 2018 20:54

add tests

a141b74

sozercan changed the title ~~[WIP] VirtualMachineScaleSets support for Kubernetes~~ VirtualMachineScaleSets support for Kubernetes Apr 17, 2018

khenidak approved these changes Apr 18, 2018

View reviewed changes

sozercan requested a review from jackfrancis April 18, 2018 18:14

sozercan removed the in progress label Apr 18, 2018

JiangtianLi reviewed Apr 20, 2018

View reviewed changes

sozercan mentioned this pull request Apr 23, 2018

availabilityProfile: what to use? #2462

Closed

sozercan added 2 commits April 24, 2018 10:11

add validation for instanceMetadata

993a67e

update docs

b2fa47b

ghost added the in progress label Apr 24, 2018

update tests

8aaac32

jackfrancis approved these changes Apr 24, 2018

View reviewed changes

jackfrancis merged commit 981a72e into Azure:master Apr 24, 2018

ghost removed the in progress label Apr 24, 2018

sozercan deleted the vmss branch April 24, 2018 21:38

CecileRobertMichon mentioned this pull request May 10, 2018

VMSS cluster not being created with acs-engine 0.16.2 #2885

Closed

asw101 mentioned this pull request May 16, 2018

Support for Low-priority VMs on Virtual Machine Scale Sets (VMSS) for Kubernetes #2980

Merged

3 tasks

VirtualMachineScaleSets support for Kubernetes #2620

VirtualMachineScaleSets support for Kubernetes #2620

Conversation

sozercan commented Apr 6, 2018 • edited Loading

feiskyer commented Apr 7, 2018 • edited Loading

todoesverso commented Apr 10, 2018

sozercan commented Apr 10, 2018 • edited Loading

PoisonousJohn commented Apr 11, 2018 • edited Loading

feiskyer commented Apr 11, 2018

PoisonousJohn commented Apr 11, 2018

khenidak 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

sozercan Apr 16, 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

khenidak commented Apr 12, 2018

feiskyer commented Apr 19, 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

sozercan Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

JiangtianLi left a comment

Choose a reason for hiding this comment

jackfrancis commented Apr 23, 2018

khenidak commented Apr 23, 2018

CecileRobertMichon commented Apr 23, 2018

sozercan commented Apr 24, 2018 • edited Loading

jackfrancis commented Apr 24, 2018

jackfrancis left a comment

Choose a reason for hiding this comment

sozercan commented Apr 6, 2018 •

edited

Loading

feiskyer commented Apr 7, 2018 •

edited

Loading

sozercan commented Apr 10, 2018 •

edited

Loading

PoisonousJohn commented Apr 11, 2018 •

edited

Loading

sozercan Apr 16, 2018 •

edited

Loading

sozercan Apr 20, 2018 •

edited

Loading

sozercan commented Apr 24, 2018 •

edited

Loading