Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Kubernetes Cluster Autoscaler (VMSS) addon #2637

Merged
merged 17 commits into from
May 9, 2018

Conversation

sozercan
Copy link
Member

@sozercan sozercan commented Apr 9, 2018

What this PR does / why we need it:

This PR adds Azure Virtual Machine Scale Set cluster autoscaler (https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler/cloudprovider/azure) as an add-on for acs-engine

It automatically pulls in and converts to base64 all required values for credentials (such as clientId, clientSecret, subscriptionId, tentantId, etc) and other required values (scale set name, resource group, etc) from the deployment.

User can override minNodes (default to 1), maxNodes (default to 5), image, limits, and requests.

Documentation: https://github.com/sozercan/acs-engine/blob/autoscaler-vmss/examples/addons/cluster-autoscaler/README.md

Special notes for your reviewer:
Merge after #2620

If applicable:

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

Release note:

Added cluster-autoscaler addon for Virtual Machine Scale Sets

@ghost ghost assigned sozercan Apr 9, 2018
@ghost ghost added the in progress label Apr 9, 2018
@sozercan sozercan changed the title [WIP] Cluster Autoscaler (VMSS) addon [WIP] Kubernetes Cluster Autoscaler (VMSS) addon Apr 9, 2018
@sozercan sozercan force-pushed the autoscaler-vmss branch 7 times, most recently from 00d9e3c to 88bf274 Compare April 10, 2018 05:01
@sozercan sozercan force-pushed the autoscaler-vmss branch 4 times, most recently from 7f77e01 to abd494d Compare April 17, 2018 23:19
@khenidak
Copy link
Contributor

@kkmsft who is working on Autoscaler for AKS RP. Can we align both work streams?

@sozercan sozercan force-pushed the autoscaler-vmss branch from 4ad9539 to ba254e5 Compare May 2, 2018 01:55
@sozercan sozercan changed the title [WIP] Kubernetes Cluster Autoscaler (VMSS) addon Kubernetes Cluster Autoscaler (VMSS) addon May 2, 2018
@sozercan sozercan force-pushed the autoscaler-vmss branch from ba254e5 to d93d408 Compare May 2, 2018 03:39
@codecov
Copy link

codecov bot commented May 2, 2018

Codecov Report

Merging #2637 into master will decrease coverage by 0.09%.
The diff coverage is 56.09%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2637     +/-   ##
=========================================
- Coverage   49.61%   49.51%   -0.1%     
=========================================
  Files          91       91             
  Lines       13854    13939     +85     
=========================================
+ Hits         6873     6902     +29     
- Misses       6344     6394     +50     
- Partials      637      643      +6
Impacted Files Coverage Δ
pkg/acsengine/addons.go 100% <100%> (ø) ⬆️
pkg/api/types.go 30.2% <100%> (+2.92%) ⬆️
pkg/acsengine/k8s_versions.go 100% <100%> (ø) ⬆️
pkg/api/vlabs/validate.go 43.96% <16.66%> (-0.54%) ⬇️
pkg/acsengine/engine.go 63.22% <22.22%> (-1.11%) ⬇️
pkg/acsengine/defaults.go 73.7% <71.42%> (-0.04%) ⬇️
pkg/operations/kubernetesupgrade/upgrader.go 56.66% <0%> (-0.84%) ⬇️

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 25049c1...90a3bcd. Read the comment docs.

@mpalumbo7
Copy link
Contributor

What would it take to extend this to support auto-scaling multiple agent pools? At the moment it only scales the first one, but the cluster-autoscaler supports multiple agent pools.

@feiskyer
Copy link
Member

feiskyer commented May 3, 2018

@ewok2030 You can add more agent pools by adding

- --nodes=<kubernetesClusterAutoscalerMinNodes>:<kubernetesClusterAutoscalerMaxNodes>:<kubernetesClusterAutoscalerVMSSName>
- --nodes=<kubernetesClusterAutoscalerMinNodes>:<kubernetesClusterAutoscalerMaxNodes>:<kubernetesClusterAutoscalerVMSSName>

@sozercan
Copy link
Member Author

sozercan commented May 3, 2018

@ewok2030 like @feiskyer said, you can edit the yaml (in /etc/kubernetes/addons/ on the master) to include multiple agent pools, but that functionality isn't there in the add-on definition logic yet (it'll automatically include the primaryScaleSet which is the first agent pool). I have been thinking about adding some extra configuration options to include multiple agent pools.

@sozercan sozercan force-pushed the autoscaler-vmss branch 3 times, most recently from 855bd92 to adebbec Compare May 7, 2018 20:33
@sozercan sozercan requested a review from jackfrancis May 7, 2018 20:35
@sozercan
Copy link
Member Author

sozercan commented May 7, 2018

@jackfrancis ready for review when you get a chance

@@ -104,8 +104,8 @@ const (
DefaultACIConnectorAddonName = "aci-connector"
// DefaultDashboardAddonName is the name of the kubernetes-dashboard addon deployment
DefaultDashboardAddonName = "kubernetes-dashboard"
// DefaultACIConnectorImage defines the ACI Connector deployment version on Kubernetes Clusters
DefaultACIConnectorImage = "virtual-kubelet:latest"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we want to delete this const?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, this one is no longer used (since it moved to k8s_versions), thought i would clean it up

"dockerEngine": "1.13.*",
"dashboard": "kubernetes-dashboard-amd64:v1.8.3",
"exechealthz": "exechealthz-amd64:1.2",
"addon-resizer": "addon-resizer:1.7",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we're downgrading addon-resizer to 1.7 because 1.8.1 is not mutually compatible with cluster-autoscaler?

Can we be confident that we won't re-introduce this bug:

#2430

Which was fixed here:

#2753

(the addon-resizer manfest definition was changed with the above PR, not just the version; perhaps the changes in how addon-resizer was expressed is the important part and not the upgrade to 1.8.1?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't mean to downgrade addon-resizer, probably didn't notice when rebasing. reverted that change.

@jackfrancis
Copy link
Member

This looks great, couple questions. Let's add validation unit tests (see pkg/api/vlabs/validate_test.go) to cover your new validations.

Thanks so much for this!!

@sozercan sozercan force-pushed the autoscaler-vmss branch from adebbec to 9cfc760 Compare May 7, 2018 22:13
@sozercan
Copy link
Member Author

sozercan commented May 9, 2018

@jackfrancis thanks! added validation unit tests. should be good to review when you get a chance

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm pending E2E

@jackfrancis jackfrancis merged commit ad8c1da into Azure:master May 9, 2018
@ghost ghost removed the awaiting review label May 9, 2018
@sozercan sozercan deleted the autoscaler-vmss branch May 9, 2018 23:29
@robsonpeixoto
Copy link

How to change the mix/max after the cluster been created?

@mpalumbo7
Copy link
Contributor

At this time, it’ll have to be done in the same way a second agent pool may be added: by SSH’ing into each master node an editing the addon YAML.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants