Default min/max sizes for Azure VMSSs #6863

rrangith · 2024-05-22T20:28:08Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Currently the autoscaler will ignore a VMSS if no min/max size is provided. This PR changes the behaviour so it will instead default to a min and max as part of the autodiscovery tags, similar to how GCP does it

autoscaler/cluster-autoscaler/main.go

Line 182 in cf606a1

    
           "GCE matches by IG name prefix, and requires you to specify min and max nodes per IG, e.g. `mig:namePrefix=pfx,min=0,max=10` "+

. The min and max are optional for Azure and used as default values if a VMSS is missing its min/max tags

Azure has a limit of 50 tags per resource https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/tag-resources#limitations. This change will allow people to save 2 tag spaces

What does this PR do

We will read scaleSet.Tags["min"] (which is the current behaviour), and then else if that is not defined, we will check to see if there is a default minSize (new behaviour), and then if neither are defined we will ignore the vmss (current behaviour).

So if a default is not specified, then the current behaviour will be maintained. And the explicit min/max tags will always be given the higher assignment precedence since even if the default is specified, we will still use the the explicit tag value

Testing

I built an image including these changes and tested in an environment. I confirmed that when you don't specify a default min/max the behaviour stays the same. And then when I defined a default min/max I confirmed that now VMSSs missing an explicit min/max tag were now considered and used the default. I also confirmed that the explicit min/max tag would take precedence over the default value

Which issue(s) this PR fixes:

Fixes #6864

Does this PR introduce a user-facing change?

You can now optionally specify a default `min` and `max` size for Azure VMSSs through the auto discovery tags. Explicit `min` and `max` tags on VMSSs will still be given priority over the default.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2024-05-22T20:28:17Z

Hi @rrangith. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

rrangith · 2024-06-10T14:12:59Z

/assign @jackfrancis @feiskyer

I presented this at the SIG Autoscaling meeting and they suggested I reach out to you. Can I please get a review?

jackfrancis · 2024-06-10T21:34:43Z

cluster-autoscaler/cloudprovider/azure/azure_manager.go

@@ -327,6 +329,8 @@ func (m *AzureManager) getFilteredScaleSets(filter []labelAutoDiscoveryConfig) (
 				klog.Warningf("ignoring vmss %q because of invalid minimum size specified for vmss: %s", *scaleSet.Name, err)
 				continue
 			}
+		} else if cfgMinSize >= 0 {


Should we invert the precedence here and have the previous min/max foo (scaleSet.Tags["min"] and scaleSet.Tags["max"] have the higher assignment precedence? I think that might be a better back-compat story.

I'm not sure I follow. We will read scaleSet.Tags["min"] (which is the current behaviour), and then else if that is not defined, we will check to see if there is a default cfgMinSize (new behaviour), and then if neither are defined we will ignore the vmss (current behaviour).

So if a default is not specified, then the current behaviour will be maintained. And the explicit tags will always be given the higher assignment precedence since even if the default is specified, we will still use the the explicit tag value

Ah yes my bad, thanks!

jackfrancis · 2024-06-10T21:40:38Z

cluster-autoscaler/cloudprovider/azure/azure_autodiscovery.go

-func matchDiscoveryConfig(labels map[string]*string, configs []labelAutoDiscoveryConfig) bool {
+// returns true if the VMSS's tags match the autodiscovery configs
+// if there are multiple min/max sizes defined, return the highest min value and the lowest max value
+func matchDiscoveryConfig(labels map[string]*string, configs []labelAutoDiscoveryConfig) (bool, int, int) {


This func (and the caller flow is starting to get a little opaque...)

Would returning a struct make this a bit more readable from the consumer side and make the func more maintainable? Is there a good chance that the configs we grab out of this will never grow beyond min + max?

(Love the new UT cases though!)

sure I can change that to a struct!

jackfrancis · 2024-06-10T22:53:42Z

/ok-to-test

rrangith · 2024-06-11T00:49:57Z

/retest-required

rrangith · 2024-06-11T02:05:47Z

/retest

jackfrancis · 2024-06-11T14:45:34Z

@rrangith have you built images from these source changes and tested in an Azure environment? I'm confident the E2E failures are not related to this PR, but would like to get some coverage before landing this.

rrangith · 2024-06-11T17:52:00Z

@rrangith have you built images from these source changes and tested in an Azure environment? I'm confident the E2E failures are not related to this PR, but would like to get some coverage before landing this.

@jackfrancis yes, I built an image including these changes and tested in an environment. I confirmed that when you don't specify a default min/max the behaviour stays the same. And then when I defined a default min/max I confirmed that now VMSSs missing an explicit min/max tag were now considered and used the default. I also confirmed that the explicit min/max tag would take precedence over the default value

return a struct

k8s-ci-robot · 2024-06-11T19:04:47Z

@rrangith: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-autoscaler-e2e-azure	`333d438`	link	false	`/test pull-cluster-autoscaler-e2e-azure`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

jackfrancis

/lgtm
/approve

Thank you @rrangith!

rrangith · 2024-07-01T13:59:08Z

/retest

gjtempleton · 2024-07-01T14:29:11Z

Thanks for this, changes outside of the Azure cloudprovider implementation all look good to me.

/approve

k8s-ci-robot · 2024-07-01T14:29:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gjtempleton, jackfrancis, rrangith

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:

~~cluster-autoscaler/OWNERS~~ [gjtempleton]

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

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label May 22, 2024

k8s-ci-robot requested a review from nilo19 May 22, 2024 20:28

k8s-ci-robot added the area/cluster-autoscaler label May 22, 2024

k8s-ci-robot requested a review from x13n May 22, 2024 20:28

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/provider/azure Issues or PRs related to azure provider labels May 22, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 22, 2024

rrangith mentioned this pull request May 22, 2024

Default Azure VMSS min size to 0 if not provided #6858

Closed

k8s-ci-robot assigned feiskyer and jackfrancis Jun 10, 2024

jackfrancis reviewed Jun 10, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 10, 2024

Default min/max sizes for Azure VMSSs

333d438

return a struct

rrangith force-pushed the azure-default-sizes branch from c938b65 to 333d438 Compare June 11, 2024 17:55

jackfrancis approved these changes Jun 13, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 1, 2024

k8s-ci-robot merged commit 3c6dd26 into kubernetes:master Jul 1, 2024
6 of 7 checks passed

comtalyst mentioned this pull request Jul 4, 2024

refactor: upstream most of Azure managed CAS changes in cloudprovider/azure for 1.29 #6991

Merged

rrangith mentioned this pull request Jul 10, 2024

Default min/max sizes for Azure VMSSs DataDog/autoscaler#117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default min/max sizes for Azure VMSSs #6863

Default min/max sizes for Azure VMSSs #6863

rrangith commented May 22, 2024 •

edited

Loading

k8s-ci-robot commented May 22, 2024

rrangith commented Jun 10, 2024

jackfrancis Jun 10, 2024

rrangith Jun 10, 2024

jackfrancis Jun 10, 2024

jackfrancis Jun 10, 2024

rrangith Jun 10, 2024

jackfrancis commented Jun 10, 2024

rrangith commented Jun 11, 2024

rrangith commented Jun 11, 2024

jackfrancis commented Jun 11, 2024

rrangith commented Jun 11, 2024 •

edited

Loading

k8s-ci-robot commented Jun 11, 2024

jackfrancis left a comment

rrangith commented Jul 1, 2024

gjtempleton commented Jul 1, 2024

k8s-ci-robot commented Jul 1, 2024

Default min/max sizes for Azure VMSSs #6863

Default min/max sizes for Azure VMSSs #6863

Conversation

rrangith commented May 22, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

What does this PR do

Testing

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented May 22, 2024

rrangith commented Jun 10, 2024

jackfrancis Jun 10, 2024

Choose a reason for hiding this comment

rrangith Jun 10, 2024

Choose a reason for hiding this comment

jackfrancis Jun 10, 2024

Choose a reason for hiding this comment

jackfrancis Jun 10, 2024

Choose a reason for hiding this comment

rrangith Jun 10, 2024

Choose a reason for hiding this comment

jackfrancis commented Jun 10, 2024

rrangith commented Jun 11, 2024

rrangith commented Jun 11, 2024

jackfrancis commented Jun 11, 2024

rrangith commented Jun 11, 2024 • edited Loading

k8s-ci-robot commented Jun 11, 2024

jackfrancis left a comment

Choose a reason for hiding this comment

rrangith commented Jul 1, 2024

gjtempleton commented Jul 1, 2024

k8s-ci-robot commented Jul 1, 2024

rrangith commented May 22, 2024 •

edited

Loading

rrangith commented Jun 11, 2024 •

edited

Loading