Allow specification of multiple expanders #4233

ryanmcnamara · 2021-07-30T20:43:04Z

Multiple expanders can now be specified, expanders now "filter to the
tied for best" instead of "selecting the best" so the output of one
expander is now fed to the input of the next. Each expander may only
be used once to disallow bad configuration. This should not be a change
in functionality as in the event of a tie the random expander is still
used.

k8s-ci-robot · 2021-07-30T20:43:11Z

Welcome @ryanmcnamara!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

ryanmcnamara · 2021-07-30T20:43:26Z

fixes #4206

ryanmcnamara · 2021-07-30T20:44:28Z

Built and deployed this locally and confirmed I was able to use --expander=priority,least-waste and it worked as expected

x13n · 2021-08-02T14:11:12Z

cluster-autoscaler/expander/factory/expander_factory.go

+
+		switch expanderFlag {
+		case expander.RandomExpanderName:
+			filters = append(filters, random.NewFilter())


Shouldn't it be an error to specify an expander that's guaranteed to provide a single option with a fallback? E.g. random,least-waste is not really a reasonable config.

True that the configuration is not a reasonable one, but I felt that catching this case wasn't worth the effort as it would require disambiguating between expanders that

Always return one result and

Those that can return more than one.

My thinking is that since it won't create problems then we can keep the simplicity of allowing this. Happy to be overruled on this though.

Here's what it would look like: https://github.com/ryanmcnamara/autoscaler/compare/rm/expander-chain...ryanmcnamara:rm/expander-chain-demo?expand=1

Yup, something like this. I'm not insisting on this one, but since you're generating an error for one class of invalid configs (duplicated entries), I thought it may make sense to go a step further and disallow bad ordering as well.

Ok addressed in this pr now!

Neat, thanks! Maybe worth adding a comment about this behavior? May be surprising for anyone adding a new expander in the future. Please also squash commits before merging.

x13n · 2021-08-02T14:17:47Z

cluster-autoscaler/expander/factory/chain.go

+func (c *chainStrategy) BestOption(options []expander.Option, nodeInfo map[string]*schedulerframework.NodeInfo) *expander.Option {
+	filteredOptions := options
+	for _, filter := range c.filters {
+		filteredOptions = filter.BestOptions(filteredOptions, nodeInfo)


There is non-trivial logic behind BestOptions call sometime, so maybe worth returning earlier if there's just one option left?

+1, will do

ryanmcnamara · 2021-08-05T17:26:51Z

Looking for a review from a code owner, perhaps @MaciekPytel or if not can you redirect? Thanks!

x13n · 2021-08-13T09:54:11Z

I'm about to go on vacation tomorrow, so to make sure it's not blocked on me in the meantime:

/lgtm
/hold until the commits are squashed

For final approval:
/assign @MaciekPytel

x13n · 2021-09-03T08:36:48Z

Just realized this is still open. Can you squash the commits?

ryanmcnamara · 2021-09-07T14:23:38Z

@x13n done!

x13n · 2021-09-07T14:35:41Z

/lgtm
/hold cancel

mwielgus

/lgtm
/approve

k8s-ci-robot · 2021-09-22T13:33:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mwielgus, ryanmcnamara

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~~ [mwielgus]

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

mwielgus · 2021-09-22T14:03:05Z

Please fix:

Boilerplate header is wrong for: /home/runner/work/autoscaler/autoscaler/go/src/k8s.io/autoscaler/cluster-autoscaler/expander/factory/chain_test.go

Multiple expanders can now be specified, expanders now "filter to the tied for best" instead of "selecting the best" so the output of one expander is now fed to the input of the next. Each expander may only be used once to disallow bad configuration. This should not be a change in functionality as in the event of a tie the random expander is still used.

ryanmcnamara · 2021-09-23T20:32:22Z

@mwielgus done, thanks for the review

x13n · 2021-09-24T08:13:49Z

/lgtm

The github action hooks didn't run the unit tests on the expanders update in kubernetes#4233 (likely the known "tests don't run automatically for new contributor" limitation). NewStrategy became NewFilter, with a BestOptions (plural) method, and returns a Options slice instead of a single Option.

Allow specification of multiple expanders

Allow specification of multiple expanders -- EDIT: change uses of scheduler/schedulerframework to scheduler/nodeinfo, as we are not yet on the version that switches

The github action hooks didn't run the unit tests on the expanders update in kubernetes#4233 (likely the known "tests don't run automatically for new contributor" limitation). NewStrategy became NewFilter, with a BestOptions (plural) method, and returns a Options slice instead of a single Option.

Allow specification of multiple expanders

The github action hooks didn't run the unit tests on the expanders update in kubernetes#4233 (likely the known "tests don't run automatically for new contributor" limitation). NewStrategy became NewFilter, with a BestOptions (plural) method, and returns a Options slice instead of a single Option.

Allow specification of multiple expanders

jonathanbbai · 2022-03-30T22:08:41Z

I don't want to upgrade my cluster to 1.23 but i really like this feature, You think if I upgrade the CA version to 1.23 while on a 1.20 cluster, what are the chances it'll work?

Allow specification of multiple expanders

The github action hooks didn't run the unit tests on the expanders update in kubernetes#4233 (likely the known "tests don't run automatically for new contributor" limitation). NewStrategy became NewFilter, with a BestOptions (plural) method, and returns a Options slice instead of a single Option.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 30, 2021

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 30, 2021

k8s-ci-robot requested review from aleksandra-malinowska and feiskyer July 30, 2021 20:43

x13n reviewed Aug 2, 2021

View reviewed changes

ryanmcnamara force-pushed the rm/expander-chain branch from 1ad750b to 795a8d6 Compare August 3, 2021 14:31

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 13, 2021

k8s-ci-robot assigned x13n Aug 13, 2021

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

k8s-ci-robot assigned MaciekPytel Aug 13, 2021

jbartosik added the area/cluster-autoscaler label Aug 30, 2021

ryanmcnamara force-pushed the rm/expander-chain branch from 417d8ff to 9214395 Compare September 7, 2021 14:23

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 7, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 7, 2021

mwielgus approved these changes Sep 22, 2021

View reviewed changes

k8s-ci-robot assigned mwielgus Sep 22, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2021

ryanmcnamara force-pushed the rm/expander-chain branch from 9214395 to 068af5b Compare September 23, 2021 20:31

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2021

k8s-ci-robot merged commit 82bdc4a into kubernetes:master Sep 24, 2021

bpineau mentioned this pull request Sep 27, 2021

fix expanders unit tests #4354

Merged

This was referenced Nov 9, 2021

Update readme and examples for expander #4454

Closed

Update Huawei Cloud provider examples for expander #4455

Closed

update readme and examples to keep it consistent with the community v… #4461

Merged

evansheng pushed a commit to airbnb/autoscaler that referenced this pull request Dec 3, 2021

Merge pull request kubernetes#4233 from ryanmcnamara/rm/expander-chain

e44c75e

Allow specification of multiple expanders

evansheng mentioned this pull request Dec 3, 2021

backport patch to specify multiple expanders airbnb/autoscaler#11

Closed

evansheng mentioned this pull request Dec 3, 2021

backport allowing specifying multiple expanders patch airbnb/autoscaler#12

Merged

dbonfigli mentioned this pull request Dec 9, 2021

Re-add default expander #4513

Merged

x13n mentioned this pull request Dec 15, 2021

[cluster-autoscaler] Additional expander options #4206

Closed

drmorr0 pushed a commit to airbnb/autoscaler that referenced this pull request Jan 13, 2022

Merge pull request kubernetes#4233 from ryanmcnamara/rm/expander-chain

0bea2d8

Allow specification of multiple expanders

JHeilCoveo mentioned this pull request Jan 17, 2022

Cascading expanders doesn't work #4504

Closed

JoshVee mentioned this pull request Feb 4, 2022

feat: add helm chart support for multiple expanders including "priority" #4665

Merged

evansheng pushed a commit to airbnb/autoscaler that referenced this pull request Mar 24, 2022

Merge pull request kubernetes#4233 from ryanmcnamara/rm/expander-chain

ab8bbe2

Allow specification of multiple expanders

jiancheung pushed a commit to airbnb/autoscaler that referenced this pull request Jul 29, 2022

Merge pull request kubernetes#4233 from ryanmcnamara/rm/expander-chain

40e01a6

Allow specification of multiple expanders

x13n mentioned this pull request Oct 11, 2022

Don't break scale up with priority expander config #5246

Merged

akirillov pushed a commit to airbnb/autoscaler that referenced this pull request Oct 27, 2022

Merge pull request kubernetes#4233 from ryanmcnamara/rm/expander-chain

057e9a0

Allow specification of multiple expanders

akirillov mentioned this pull request Oct 27, 2022

cluster autoscaler patch cluster autoscaler 1.21.3 airbnb0 airbnb/autoscaler#29

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow specification of multiple expanders #4233

Allow specification of multiple expanders #4233

ryanmcnamara commented Jul 30, 2021

k8s-ci-robot commented Jul 30, 2021

ryanmcnamara commented Jul 30, 2021

ryanmcnamara commented Jul 30, 2021

x13n Aug 2, 2021

ryanmcnamara Aug 2, 2021

ryanmcnamara Aug 3, 2021

x13n Aug 4, 2021

ryanmcnamara Aug 5, 2021

x13n Aug 5, 2021

x13n Aug 2, 2021

ryanmcnamara Aug 2, 2021

ryanmcnamara Aug 3, 2021

ryanmcnamara commented Aug 5, 2021

x13n commented Aug 13, 2021 •

edited

Loading

x13n commented Sep 3, 2021

ryanmcnamara commented Sep 7, 2021

x13n commented Sep 7, 2021

mwielgus left a comment

k8s-ci-robot commented Sep 22, 2021

mwielgus commented Sep 22, 2021

ryanmcnamara commented Sep 23, 2021

x13n commented Sep 24, 2021

jonathanbbai commented Mar 30, 2022

Allow specification of multiple expanders #4233

Allow specification of multiple expanders #4233

Conversation

ryanmcnamara commented Jul 30, 2021

k8s-ci-robot commented Jul 30, 2021

ryanmcnamara commented Jul 30, 2021

ryanmcnamara commented Jul 30, 2021

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

ryanmcnamara commented Aug 5, 2021

x13n commented Aug 13, 2021 • edited Loading

x13n commented Sep 3, 2021

ryanmcnamara commented Sep 7, 2021

x13n commented Sep 7, 2021

mwielgus left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 22, 2021

mwielgus commented Sep 22, 2021

ryanmcnamara commented Sep 23, 2021

x13n commented Sep 24, 2021

jonathanbbai commented Mar 30, 2022

x13n commented Aug 13, 2021 •

edited

Loading