Scaling Windows agent pools for Availability Sets and VMSS #2859

danigian · 2018-05-04T17:31:18Z

What this PR does / why we need it:
This code implements the scaling for Windows agent pools (both for Availability Sets and Virtual Machine Scale Sets)

Which issue this PR fixes:
fixes #1972 #2649

Special notes for your reviewer:

If applicable:

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

Release note:

* Forced apimodel validation to avoid incongruences (fixes #2025) * Added more details about required fields while deploying (fixes #2491)

This reverts commit 0eb7ece.

…wsscale

sozercan · 2018-05-06T02:40:16Z

For VMSS, you should be able to scale using portal or CLI, shouldn't need ./acs-engine scale

danigian · 2018-05-07T07:37:57Z

@sozercan I agree.
I thought that having a unified experience with acs-engine scale for AS and VMSS could have been a good idea

qmalexander · 2018-05-16T16:37:59Z

So @danigian . How is it going with this?

This updates client-go to v7.0.0 and adds the needed dependencies on `k8s.io/api`. Also fixes a small issue with conflicting `uuid` library versions

make build-vendor passed!

…dowsscale

This allows a cluster to scale even if the user has added a subnet to the vnet

danigian · 2018-05-21T12:19:31Z

@CecileRobertMichon waiting for PR #2954 to be approved as it solves the issues I had (#2869).
In order to test it, I merged changes from that PR into my branch and right now I have both AS and VMSS scaling (out and in) working again for Windows Agent Pools.

CecileRobertMichon · 2018-05-21T16:53:11Z

That's great news thank you @danigian! cc: @jackfrancis

CecileRobertMichon · 2018-05-21T18:01:14Z

@Damaggio #2954 just got merged so let's move this forward 🙂 The unit tests are failing because of lint / go import errors so I'll let you fix those.

danigian · 2018-05-23T08:00:41Z

@CecileRobertMichon everything should be ok now
I tested the following scenarios:

AvailabilitySets Scale-out and Scale-in (both in hybrid and windows only clusters) with and without pods already assigned to the nodes
VirtualMachineScaleSets Scale-out and Scale-in (as the previous line)

Please do more tests if you want.

Thanks

codecov · 2018-05-24T18:43:09Z

Codecov Report

Merging #2859 into master will decrease coverage by 0.01%.
The diff coverage is 32.35%.

@@            Coverage Diff             @@
##           master    #2859      +/-   ##
==========================================
- Coverage   51.59%   51.57%   -0.02%     
==========================================
  Files          97       97              
  Lines       14834    14861      +27     
==========================================
+ Hits         7654     7665      +11     
- Misses       6482     6496      +14     
- Partials      698      700       +2

Impacted Files	Coverage Δ
pkg/operations/kubernetesupgrade/upgradecluster.go	`46.85% <0%> (ø)`	⬆️
cmd/scale.go	`12.68% <5.55%> (-0.25%)`	⬇️
pkg/armhelpers/utils/util.go	`63.09% <71.42%> (+1.66%)`	⬆️

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 3482257...a351ae0. Read the comment docs.

CecileRobertMichon · 2018-05-24T23:18:26Z

Running back compat scale + upgrade tests

CecileRobertMichon · 2018-05-25T16:47:48Z

cmd/scale.go

@@ -78,6 +79,7 @@ func newScaleCmd() *cobra.Command {
 	f.BoolVar(&sc.classicMode, "classic-mode", false, "enable classic parameters and outputs")
 	f.StringVar(&sc.agentPoolToScale, "node-pool", "", "node pool to scale")
 	f.StringVar(&sc.masterFQDN, "master-FQDN", "", "FQDN for the master load balancer, Needed to scale down Kubernetes agent pools")
+	f.BoolVar(&sc.windowsPool, "windows", false, "true if windows pool")


Is it necessary to have a windows flag? Would it be possible to determine if the agentpool is windows based on the names of the VMs?

If there is no other way or it is complicated, it's fine for now but we should add a section in docs about scaling windows agent pools and mention the flag there.

Will work on this now as it should not be a problem removing the flag, using instead the OsType of the OsDisk object in the StorageProfile (should be better than using rather the name which could change in time). In a first moment I added the flag because I thought that could have been useful, but actually it is not

@CecileRobertMichon I ended up with using StorageProfile.ImageReference.Publisher to understand if it is a Windows Agent Pool as the OsType property in the OsDisk Object is not available when using VMSS.
Everything should work as before, but without the windows flag.

Please review this new commit

…s Agent Pool in Scaling scenario

CecileRobertMichon · 2018-05-29T20:35:56Z

cmd/scale.go

 				continue
 			}

+			osPublisher := *vm.StorageProfile.ImageReference.Publisher


I think it'd be cleaner to use *vm.StorageProfile.OsDisk.OsType == Windows instead

It should be cleaner, however I decided to use the osPublisher as it is the same check done for VMSS (which are not defining OsDisk.OsType).
Do you want me to change this?

Ah that makes sense. All good then.

CecileRobertMichon · 2018-05-29T20:37:40Z

cmd/scale.go

-			if sc.windowsPool {
-				_, _, poolindex, err = utils.WindowsVMSSNameParts(*vmss.Name)
+			osPublisher := *vmss.VirtualMachineProfile.StorageProfile.ImageReference.Publisher
+			if strings.EqualFold(osPublisher, "MicrosoftWindowsServer") {


Maybe we can have a bool var isWindows defined above and re-use it here instead of checking again

I am not sure about what you mean here, sorry. The check happens only one time: when the vmss corresponding to the agent pool, we want to scale, is found.

My bad, I read the code too fast the first time and didn't realize this was VMSS.

CecileRobertMichon · 2018-05-30T17:49:12Z

I just ran an scale on \examples\windows\kubernetes.json and got the following error:

+ docker run --rm -v /var/lib/jenkins/workspace/k8s-scale:/go/src/github.com/Azure/acs-engine -w /go/src/github.com/Azure/acs-engine -e RESOURCE_GROUP=kubernetes-northcentralus-66441 -e REGION=northcentralus quay.io/deis/go-dev:v1.12.0 ./bin/acs-engine scale --subscription-id 3014546b-7d1c-4f80-8523-f24a9976fe6a --deployment-dir _output/kubernetes-northcentralus-66441 --location northcentralus --resource-group kubernetes-northcentralus-66441 --master-FQDN kubernetes-northcentralus-66441.northcentralus.cloudapp.azure.com --node-pool windowspool2 --new-node-count 5 --auth-method client_secret --client-id xxx --client-secret xxx
INFO[0000] validating...                                
INFO[0003] Name suffix: %s 20789360                     
panic: runtime error: index out of range

goroutine 1 [running]:
github.com/Azure/acs-engine/cmd.(*scaleCmd).run(0xc4203ab800, 0xc4203f2d80, 0xc4203a1cc0, 0x0, 0x14, 0x0, 0x0)
	/go/src/github.com/Azure/acs-engine/cmd/scale.go:240 +0x2c9f
github.com/Azure/acs-engine/cmd.newScaleCmd.func1(0xc4203f2d80, 0xc4203a1cc0, 0x0, 0x14, 0x0, 0x0)
	/go/src/github.com/Azure/acs-engine/cmd/scale.go:69 +0x52
github.com/Azure/acs-engine/vendor/github.com/spf13/cobra.(*Command).execute(0xc4203f2d80, 0xc4203a1b80, 0x14, 0x14, 0xc4203f2d80, 0xc4203a1b80)
	/go/src/github.com/Azure/acs-engine/vendor/github.com/spf13/cobra/command.go:647 +0x3e4
github.com/Azure/acs-engine/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc4203f2000, 0xc4203f2d80, 0xc4203f2b40, 0xc4203f2900)
	/go/src/github.com/Azure/acs-engine/vendor/github.com/spf13/cobra/command.go:726 +0x2d4
github.com/Azure/acs-engine/vendor/github.com/spf13/cobra.(*Command).Execute(0xc4203f2000, 0xc4200e6008, 0x10e79f3)
	/go/src/github.com/Azure/acs-engine/vendor/github.com/spf13/cobra/command.go:685 +0x2b
main.main()
	/go/src/github.com/Azure/acs-engine/main.go:12 +0x74

Am I missing something?

danigian · 2018-05-31T08:54:02Z

@CecileRobertMichon i can't reproduce the error. This is what I've got now:

root@186d526c1672:/gopath/src/github.com/Azure/acs-engine# ./bin/acs-engine deploy --api-model damaggioscale.json -l northcentralus -g ncusrg --auth-method client_secret --client-id xxx --client-secret xxx --subscription-id xxx
INFO[0013] Starting ARM Deployment (ncusrg-1688597576). This will take some time... 
INFO[0577] Finished ARM Deployment (ncusrg-1688597576). Succeeded 

root@186d526c1672:/gopath/src/github.com/Azure/acs-engine# ./bin/acs-engine scale --deployment-dir _output/damaggioscale -l northcentralus -g ncusrg --master-FQDN damaggioscale.northcentralus.cloudapp.azure.com --node-pool windowspool2 --new-node-count 5 --auth-method client_secret --client-id xxx --client-secret xxx --subscription-id xxx
INFO[0000] validating...                                
INFO[0004] Name suffix: %s 36123647                     
0
1
INFO[0004] Found no resources with type Microsoft.Network/routeTables in the template.  source="scaling command line"
INFO[0004] Starting ARM Deployment (ncusrg-1312361338). This will take some time... 
INFO[0546] Finished ARM Deployment (ncusrg-1312361338). Succeeded

This is what I got from a deployment point of view

and this is what I see in kubectl get no

Let me know

Update: I was also able to correctly run the upgrade on this cluster

CecileRobertMichon · 2018-05-31T20:47:09Z

Update: Jenkins test was broken, tested back compat manually and it passed. This should be ready to go pending e2e and code review from @jackfrancis.

/lgtm
/approve

jackfrancis · 2018-05-31T22:27:43Z

/approve

acs-bot · 2018-05-31T22:27:46Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

CecileRobertMichon · 2018-06-01T16:20:29Z

Thank you @danigian!! 🎉

danigian and others added 9 commits April 25, 2018 11:01

acs-engine deploy: model validation and required fields in --help

22d2e88

* Forced apimodel validation to avoid incongruences (fixes #2025) * Added more details about required fields while deploying (fixes #2491)

WIP - Adding windows agent pool scaling

0eb7ece

Revert "WIP - Adding windows agent pool scaling" - wrong branch

d1334c2

This reverts commit 0eb7ece.

WIP - Windows Scaling on Availability Sets and VMSS

788cc3d

WIP - Naming format modified for vmss windows scaling

cc683e4

Merge remote-tracking branch 'Azure/master' into windowsscale

1a2b0c1

Merge remote-tracking branch 'Azure/master' into windowsscale

73a0d15

Merge remote-tracking branch 'Azure/patch-release-v0.16.2' into windo…

b0449f2

…wsscale

Fixing tests

ec744d3

ghost assigned danigian May 4, 2018

ghost added the in progress label May 4, 2018

thomastaylor312 and others added 2 commits May 18, 2018 12:47

feat(*): Bumps client-go to v7.0.0

6e6f716

This updates client-go to v7.0.0 and adds the needed dependencies on `k8s.io/api`. Also fixes a small issue with conflicting `uuid` library versions

testing re-vendor health

1bc2fbe

make build-vendor passed!

acs-bot added the size/XXL label May 21, 2018

danigian and others added 4 commits May 21, 2018 10:12

Merge remote-tracking branch 'Azure/master' into windowsscale

f30fc5f

Merge remote-tracking branch 'thomastaylor312/feat/bump_k8s' into win…

50804d3

…dowsscale

removing the vnet from scale templates (#2994)

52262b5

This allows a cluster to scale even if the user has added a subnet to the vnet

Fix missing DefaultKubernetesClusterSubnet

8935085

danigian and others added 2 commits May 22, 2018 17:53

Merge remote-tracking branch 'Azure/master' into windowsscale

204dad1

dos2unix

77d62dc

acs-bot added size/XL and removed size/XXL labels May 22, 2018

Clean and go fmt

a1d8388

acs-bot removed the size/XL label May 22, 2018

Clean and go fmt

c405133

acs-bot added size/M and removed size/XXL labels May 22, 2018

Fix util_test.go

69dbe85

CecileRobertMichon changed the title ~~[WIP] Scaling Windows agent pools for Availability Sets and VMSS~~ Scaling Windows agent pools for Availability Sets and VMSS May 24, 2018

CecileRobertMichon reviewed May 25, 2018

View reviewed changes

Using StorageProfile.ImageReference.Publisher to understand if Window…

c0d5f84

…s Agent Pool in Scaling scenario

CecileRobertMichon reviewed May 29, 2018

View reviewed changes

lint

a351ae0

ghost assigned CecileRobertMichon May 30, 2018

CecileRobertMichon added the DO-NOT-MERGE label May 30, 2018

This comment has been minimized.

Sign in to view

acs-bot added the lgtm label May 31, 2018

CecileRobertMichon removed the DO-NOT-MERGE label May 31, 2018

acs-bot added the approved label May 31, 2018

CecileRobertMichon merged commit c02ec75 into Azure:master Jun 1, 2018

ghost removed the in progress label Jun 1, 2018

CecileRobertMichon mentioned this pull request Jun 5, 2018

panic: runtime error: index out of range #2649

Closed

jim-minter mentioned this pull request Jun 7, 2018

fix scale command on OpenShift #3210

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scaling Windows agent pools for Availability Sets and VMSS #2859

Scaling Windows agent pools for Availability Sets and VMSS #2859

danigian commented May 4, 2018 •

edited by CecileRobertMichon

Loading

sozercan commented May 6, 2018 •

edited

Loading

danigian commented May 7, 2018

qmalexander commented May 16, 2018

danigian commented May 21, 2018

CecileRobertMichon commented May 21, 2018

CecileRobertMichon commented May 21, 2018

danigian commented May 23, 2018

codecov bot commented May 24, 2018 •

edited

Loading

CecileRobertMichon commented May 24, 2018

CecileRobertMichon May 25, 2018

CecileRobertMichon May 25, 2018

danigian May 28, 2018

danigian May 28, 2018 •

edited

Loading

CecileRobertMichon May 29, 2018

danigian May 30, 2018

CecileRobertMichon May 30, 2018

CecileRobertMichon May 29, 2018

danigian May 30, 2018

CecileRobertMichon May 30, 2018

CecileRobertMichon commented May 30, 2018 •

edited

Loading

danigian commented May 31, 2018 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

CecileRobertMichon commented May 31, 2018 •

edited

Loading

jackfrancis commented May 31, 2018

acs-bot commented May 31, 2018

CecileRobertMichon commented Jun 1, 2018

Scaling Windows agent pools for Availability Sets and VMSS #2859

Scaling Windows agent pools for Availability Sets and VMSS #2859

Conversation

danigian commented May 4, 2018 • edited by CecileRobertMichon Loading

sozercan commented May 6, 2018 • edited Loading

danigian commented May 7, 2018

qmalexander commented May 16, 2018

danigian commented May 21, 2018

CecileRobertMichon commented May 21, 2018

CecileRobertMichon commented May 21, 2018

danigian commented May 23, 2018

codecov bot commented May 24, 2018 • edited Loading

Codecov Report

CecileRobertMichon commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danigian May 28, 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

CecileRobertMichon commented May 30, 2018 • edited Loading

danigian commented May 31, 2018 • edited Loading

This comment has been minimized.

This comment has been minimized.

CecileRobertMichon commented May 31, 2018 • edited Loading

jackfrancis commented May 31, 2018

acs-bot commented May 31, 2018

CecileRobertMichon commented Jun 1, 2018

danigian commented May 4, 2018 •

edited by CecileRobertMichon

Loading

sozercan commented May 6, 2018 •

edited

Loading

codecov bot commented May 24, 2018 •

edited

Loading

danigian May 28, 2018 •

edited

Loading

CecileRobertMichon commented May 30, 2018 •

edited

Loading

danigian commented May 31, 2018 •

edited

Loading

CecileRobertMichon commented May 31, 2018 •

edited

Loading