diff --git a/cluster-autoscaler/FAQ.md b/cluster-autoscaler/FAQ.md index 79292e17c4fc..8d41410deb28 100644 --- a/cluster-autoscaler/FAQ.md +++ b/cluster-autoscaler/FAQ.md @@ -617,6 +617,12 @@ would match the cluster size. This expander is described in more details * `priority` - selects the node group that has the highest priority assigned by the user. It's configuration is described in more details [here](expander/priority/readme.md) + +Multiple expanders may be passed, i.e. +`.cluster-autoscaler --expander=priority,least-waste` + +This will cause the `least-waste` expander to be used as a fallback in the event that the priority expander selects multiple node groups. In general, a list of expanders can be used, where the output of one is passed to the next and the final decision by randomly selecting one. An expander must not appear in the list more than once. + ### Does CA respect node affinity when selecting node groups to scale up? CA respects `nodeSelector` and `requiredDuringSchedulingIgnoredDuringExecution` in nodeAffinity given that you have labelled your node groups accordingly. If there is a pod that cannot be scheduled with either `nodeSelector` or `requiredDuringSchedulingIgnoredDuringExecution` specified, CA will only consider node groups that satisfy those requirements for expansion. diff --git a/cluster-autoscaler/config/autoscaling_options.go b/cluster-autoscaler/config/autoscaling_options.go index a09c5bc3c9ec..aafa7385b3d1 100644 --- a/cluster-autoscaler/config/autoscaling_options.go +++ b/cluster-autoscaler/config/autoscaling_options.go @@ -61,8 +61,8 @@ type AutoscalingOptions struct { NodeGroupAutoDiscovery []string // EstimatorName is the estimator used to estimate the number of needed nodes in scale up. EstimatorName string - // ExpanderName sets the type of node group expander to be used in scale up - ExpanderName string + // ExpanderNames sets the chain of node group expanders to be used in scale up + ExpanderNames string // IgnoreDaemonSetsUtilization is whether CA will ignore DaemonSet pods when calculating resource utilization for scaling down IgnoreDaemonSetsUtilization bool // IgnoreMirrorPodsUtilization is whether CA will ignore Mirror pods when calculating resource utilization for scaling down diff --git a/cluster-autoscaler/core/autoscaler.go b/cluster-autoscaler/core/autoscaler.go index 3735baaac083..64c7ef74e190 100644 --- a/cluster-autoscaler/core/autoscaler.go +++ b/cluster-autoscaler/core/autoscaler.go @@ -17,6 +17,7 @@ limitations under the License. package core import ( + "strings" "time" "k8s.io/autoscaler/cluster-autoscaler/cloudprovider" @@ -101,7 +102,7 @@ func initializeDefaultOptions(opts *AutoscalerOptions) error { opts.CloudProvider = cloudBuilder.NewCloudProvider(opts.AutoscalingOptions) } if opts.ExpanderStrategy == nil { - expanderStrategy, err := factory.ExpanderStrategyFromString(opts.ExpanderName, + expanderStrategy, err := factory.ExpanderStrategyFromStrings(strings.Split(opts.ExpanderNames, ","), opts.CloudProvider, opts.AutoscalingKubeClients, opts.KubeClient, opts.ConfigNamespace) if err != nil { return err diff --git a/cluster-autoscaler/expander/expander.go b/cluster-autoscaler/expander/expander.go index 7f8e3c35def4..40d21b8e2a31 100644 --- a/cluster-autoscaler/expander/expander.go +++ b/cluster-autoscaler/expander/expander.go @@ -50,3 +50,8 @@ type Option struct { type Strategy interface { BestOption(options []Option, nodeInfo map[string]*schedulernodeinfo.NodeInfo) *Option } + +// Filter describes an interface for filtering to equally good options according to some criteria +type Filter interface { + BestOptions(options []Option, nodeInfo map[string]*schedulerframework.NodeInfo) []Option +} diff --git a/cluster-autoscaler/expander/factory/chain.go b/cluster-autoscaler/expander/factory/chain.go new file mode 100644 index 000000000000..eec2ec91a311 --- /dev/null +++ b/cluster-autoscaler/expander/factory/chain.go @@ -0,0 +1,46 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package factory + +import ( + "k8s.io/autoscaler/cluster-autoscaler/expander" + + schedulerframework "k8s.io/kubernetes/pkg/scheduler/framework" +) + +type chainStrategy struct { + filters []expander.Filter + fallback expander.Strategy +} + +func newChainStrategy(filters []expander.Filter, fallback expander.Strategy) expander.Strategy { + return &chainStrategy{ + filters: filters, + fallback: fallback, + } +} + +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) + if len(filteredOptions) == 1 { + return &filteredOptions[0] + } + } + return c.fallback.BestOption(filteredOptions, nodeInfo) +} diff --git a/cluster-autoscaler/expander/factory/chain_test.go b/cluster-autoscaler/expander/factory/chain_test.go new file mode 100644 index 000000000000..b6039269752a --- /dev/null +++ b/cluster-autoscaler/expander/factory/chain_test.go @@ -0,0 +1,133 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package factory + +import ( + "k8s.io/autoscaler/cluster-autoscaler/expander" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + schedulerframework "k8s.io/kubernetes/pkg/scheduler/framework" +) + +type substringTestFilterStrategy struct { + substring string +} + +func newSubstringTestFilterStrategy(substring string) *substringTestFilterStrategy { + return &substringTestFilterStrategy{ + substring: substring, + } +} + +func (s *substringTestFilterStrategy) BestOptions(expansionOptions []expander.Option, nodeInfo map[string]*schedulerframework.NodeInfo) []expander.Option { + var ret []expander.Option + for _, option := range expansionOptions { + if strings.Contains(option.Debug, s.substring) { + ret = append(ret, option) + } + } + return ret + +} + +func (s *substringTestFilterStrategy) BestOption(expansionOptions []expander.Option, nodeInfo map[string]*schedulerframework.NodeInfo) *expander.Option { + ret := s.BestOptions(expansionOptions, nodeInfo) + if len(ret) == 0 { + return nil + } + return &ret[0] +} + +func TestChainStrategy_BestOption(t *testing.T) { + for name, tc := range map[string]struct { + filters []expander.Filter + fallback expander.Strategy + options []expander.Option + expected *expander.Option + }{ + "selects with no filters": { + filters: []expander.Filter{}, + fallback: newSubstringTestFilterStrategy("a"), + options: []expander.Option{ + *newOption("b"), + *newOption("a"), + }, + expected: newOption("a"), + }, + "filters with one filter": { + filters: []expander.Filter{ + newSubstringTestFilterStrategy("a"), + }, + fallback: newSubstringTestFilterStrategy("b"), + options: []expander.Option{ + *newOption("ab"), + *newOption("b"), + }, + expected: newOption("ab"), + }, + "filters with multiple filters": { + filters: []expander.Filter{ + newSubstringTestFilterStrategy("a"), + newSubstringTestFilterStrategy("b"), + }, + fallback: newSubstringTestFilterStrategy("x"), + options: []expander.Option{ + *newOption("xab"), + *newOption("xa"), + *newOption("x"), + }, + expected: newOption("xab"), + }, + "selects from multiple after filters": { + filters: []expander.Filter{ + newSubstringTestFilterStrategy("x"), + }, + fallback: newSubstringTestFilterStrategy("a"), + options: []expander.Option{ + *newOption("xc"), + *newOption("xaa"), + *newOption("xab"), + }, + expected: newOption("xaa"), + }, + "short circuits": { + filters: []expander.Filter{ + newSubstringTestFilterStrategy("a"), + newSubstringTestFilterStrategy("b"), + }, + fallback: newSubstringTestFilterStrategy("x"), + options: []expander.Option{ + *newOption("a"), + }, + expected: newOption("a"), + }, + } { + t.Run(name, func(t *testing.T) { + subject := newChainStrategy(tc.filters, tc.fallback) + actual := subject.BestOption(tc.options, nil) + assert.Equal(t, tc.expected, actual) + }) + } +} + +func newOption(debug string) *expander.Option { + return &expander.Option{ + Debug: debug, + } +} diff --git a/cluster-autoscaler/expander/factory/expander_factory.go b/cluster-autoscaler/expander/factory/expander_factory.go index 9c26520629db..485928032cdb 100644 --- a/cluster-autoscaler/expander/factory/expander_factory.go +++ b/cluster-autoscaler/expander/factory/expander_factory.go @@ -31,30 +31,48 @@ import ( kube_client "k8s.io/client-go/kubernetes" ) -// ExpanderStrategyFromString creates an expander.Strategy according to its name -func ExpanderStrategyFromString(expanderFlag string, cloudProvider cloudprovider.CloudProvider, +// ExpanderStrategyFromStrings creates an expander.Strategy according to the names of the expanders passed in +func ExpanderStrategyFromStrings(expanderFlags []string, cloudProvider cloudprovider.CloudProvider, autoscalingKubeClients *context.AutoscalingKubeClients, kubeClient kube_client.Interface, configNamespace string) (expander.Strategy, errors.AutoscalerError) { - switch expanderFlag { - case expander.RandomExpanderName: - return random.NewStrategy(), nil - case expander.MostPodsExpanderName: - return mostpods.NewStrategy(), nil - case expander.LeastWasteExpanderName: - return waste.NewStrategy(), nil - case expander.PriceBasedExpanderName: - if _, err := cloudProvider.Pricing(); err != nil { - return nil, err + var filters []expander.Filter + seenExpanders := map[string]struct{}{} + strategySeen := false + for i, expanderFlag := range expanderFlags { + if _, ok := seenExpanders[expanderFlag]; ok { + return nil, errors.NewAutoscalerError(errors.InternalError, "Expander %s was specified multiple times, each expander must not be specified more than once", expanderFlag) + } + if strategySeen { + return nil, errors.NewAutoscalerError(errors.InternalError, "Expander %s came after an expander %s that will always return only one result, this is not allowed since %s will never be used", expanderFlag, expanderFlags[i-1], expanderFlag) + } + seenExpanders[expanderFlag] = struct{}{} + + switch expanderFlag { + case expander.RandomExpanderName: + filters = append(filters, random.NewFilter()) + case expander.MostPodsExpanderName: + filters = append(filters, mostpods.NewFilter()) + case expander.LeastWasteExpanderName: + filters = append(filters, waste.NewFilter()) + case expander.PriceBasedExpanderName: + if _, err := cloudProvider.Pricing(); err != nil { + return nil, err + } + filters = append(filters, price.NewFilter(cloudProvider, + price.NewSimplePreferredNodeProvider(autoscalingKubeClients.AllNodeLister()), + price.SimpleNodeUnfitness)) + case expander.PriorityBasedExpanderName: + // It seems other listers do the same here - they never receive the termination msg on the ch. + // This should be currently OK. + stopChannel := make(chan struct{}) + lister := kubernetes.NewConfigMapListerForNamespace(kubeClient, stopChannel, configNamespace) + filters = append(filters, priority.NewFilter(lister.ConfigMaps(configNamespace), autoscalingKubeClients.Recorder)) + default: + return nil, errors.NewAutoscalerError(errors.InternalError, "Expander %s not supported", expanderFlag) + } + if _, ok := filters[len(filters)-1].(expander.Strategy); ok { + strategySeen = true } - return price.NewStrategy(cloudProvider, - price.NewSimplePreferredNodeProvider(autoscalingKubeClients.AllNodeLister()), - price.SimpleNodeUnfitness), nil - case expander.PriorityBasedExpanderName: - // It seems other listers do the same here - they never receive the termination msg on the ch. - // This should be currently OK. - stopChannel := make(chan struct{}) - lister := kubernetes.NewConfigMapListerForNamespace(kubeClient, stopChannel, configNamespace) - return priority.NewStrategy(lister.ConfigMaps(configNamespace), autoscalingKubeClients.Recorder) } - return nil, errors.NewAutoscalerError(errors.InternalError, "Expander %s not supported", expanderFlag) + return newChainStrategy(filters, random.NewStrategy()), nil } diff --git a/cluster-autoscaler/expander/mostpods/mostpods.go b/cluster-autoscaler/expander/mostpods/mostpods.go index 3807b15752bc..82536c1d8b40 100644 --- a/cluster-autoscaler/expander/mostpods/mostpods.go +++ b/cluster-autoscaler/expander/mostpods/mostpods.go @@ -18,21 +18,19 @@ package mostpods import ( "k8s.io/autoscaler/cluster-autoscaler/expander" - "k8s.io/autoscaler/cluster-autoscaler/expander/random" schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo" ) type mostpods struct { - fallbackStrategy expander.Strategy } -// NewStrategy returns a scale up strategy (expander) that picks the node group that can schedule the most pods -func NewStrategy() expander.Strategy { - return &mostpods{random.NewStrategy()} +// NewFilter returns a scale up filter that picks the node group that can schedule the most pods +func NewFilter() expander.Filter { + return &mostpods{} } // BestOption Selects the expansion option that schedules the most pods -func (m *mostpods) BestOption(expansionOptions []expander.Option, nodeInfo map[string]*schedulernodeinfo.NodeInfo) *expander.Option { +func (m *mostpods) BestOptions(expansionOptions []expander.Option, nodeInfo map[string]*schedulernodeinfo.NodeInfo) []expander.Option { var maxPods int var maxOptions []expander.Option @@ -51,5 +49,5 @@ func (m *mostpods) BestOption(expansionOptions []expander.Option, nodeInfo map[s return nil } - return m.fallbackStrategy.BestOption(maxOptions, nodeInfo) + return maxOptions } diff --git a/cluster-autoscaler/expander/price/price.go b/cluster-autoscaler/expander/price/price.go index 1d7a9b25bc18..0beb0f68e16b 100644 --- a/cluster-autoscaler/expander/price/price.go +++ b/cluster-autoscaler/expander/price/price.go @@ -74,11 +74,11 @@ var ( gpuUnfitnessOverride = 1000.0 ) -// NewStrategy returns an expansion strategy that picks nodes based on price and preferred node type. -func NewStrategy(cloudProvider cloudprovider.CloudProvider, +// NewFilter returns an expansion filter that picks nodes based on price and preferred node type. +func NewFilter(cloudProvider cloudprovider.CloudProvider, preferredNodeProvider PreferredNodeProvider, nodeUnfitness NodeUnfitness, -) expander.Strategy { +) expander.Filter { return &priceBased{ cloudProvider: cloudProvider, preferredNodeProvider: preferredNodeProvider, @@ -87,8 +87,8 @@ func NewStrategy(cloudProvider cloudprovider.CloudProvider, } // BestOption selects option based on cost and preferred node type. -func (p *priceBased) BestOption(expansionOptions []expander.Option, nodeInfos map[string]*schedulernodeinfo.NodeInfo) *expander.Option { - var bestOption *expander.Option +func (p *priceBased) BestOptions(expansionOptions []expander.Option, nodeInfos map[string]*schedulernodeinfo.NodeInfo) []expander.Option { + var bestOptions []expander.Option bestOptionScore := 0.0 now := time.Now() then := now.Add(time.Hour) @@ -169,17 +169,21 @@ nextoption: klog.V(5).Infof("Price expander for %s: %s", option.NodeGroup.Id(), debug) - if bestOption == nil || bestOptionScore > optionScore { - bestOption = &expander.Option{ - NodeGroup: option.NodeGroup, - NodeCount: option.NodeCount, - Debug: fmt.Sprintf("%s | price-expander: %s", option.Debug, debug), - Pods: option.Pods, - } + maybeBestOption := expander.Option{ + NodeGroup: option.NodeGroup, + NodeCount: option.NodeCount, + Debug: fmt.Sprintf("%s | price-expander: %s", option.Debug, debug), + Pods: option.Pods, + } + if len(bestOptions) == 0 || bestOptionScore == optionScore { + bestOptions = append(bestOptions, maybeBestOption) + bestOptionScore = optionScore + } else if bestOptionScore > optionScore { + bestOptions = []expander.Option{maybeBestOption} bestOptionScore = optionScore } } - return bestOption + return bestOptions } // buildPod creates a pod with specified resources. diff --git a/cluster-autoscaler/expander/price/price_test.go b/cluster-autoscaler/expander/price/price_test.go index 5b7ff8fd2a24..d17276053828 100644 --- a/cluster-autoscaler/expander/price/price_test.go +++ b/cluster-autoscaler/expander/price/price_test.go @@ -18,6 +18,7 @@ package price import ( "fmt" + "strings" "testing" "time" @@ -60,6 +61,18 @@ func (tpnp *testPreferredNodeProvider) Node() (*apiv1.Node, error) { return tpnp.preferred, nil } +func optionsToDebug(options []expander.Option) []string { + var ret []string + for _, option := range options { + s := strings.Split(option.Debug, " ") + if len(s) == 0 { + s = append(s, "") + } + ret = append(ret, s[0]) + } + return ret +} + func TestPriceExpander(t *testing.T) { n1 := BuildTestNode("n1", 1000, 1000) n2 := BuildTestNode("n2", 4000, 1000) @@ -117,13 +130,13 @@ func TestPriceExpander(t *testing.T) { }, } provider.SetPricingModel(pricingModel) - assert.Contains(t, NewStrategy( + assert.Equal(t, optionsToDebug(NewStrategy( provider, &testPreferredNodeProvider{ preferred: buildNode(2000, units.GiB), }, SimpleNodeUnfitness, - ).BestOption(options, nodeInfosForGroups).Debug, "ng1") + ).BestOptions(options, nodeInfosForGroups)), []string{"ng1"}) // First node group is cheaper, however, the second one is preferred. pricingModel = &testPricingModel{ @@ -138,13 +151,13 @@ func TestPriceExpander(t *testing.T) { }, } provider.SetPricingModel(pricingModel) - assert.Contains(t, NewStrategy( + assert.Equal(t, optionsToDebug(NewStrategy( provider, &testPreferredNodeProvider{ preferred: buildNode(4000, units.GiB), }, SimpleNodeUnfitness, - ).BestOption(options, nodeInfosForGroups).Debug, "ng2") + ).BestOptions(options, nodeInfosForGroups)), []string{"ng2"}) // All node groups accept the same set of pods. Lots of nodes. options1b := []expander.Option{ @@ -175,14 +188,14 @@ func TestPriceExpander(t *testing.T) { }, } provider.SetPricingModel(pricingModel) - assert.Contains(t, NewStrategy( + assert.Equal(t, optionsToDebug(NewStrategy( provider, &testPreferredNodeProvider{ preferred: buildNode(4000, units.GiB), }, SimpleNodeUnfitness, - ).BestOption(options1b, nodeInfosForGroups).Debug, "ng1") + ).BestOptions(options1b, nodeInfosForGroups)), []string{"ng1"}) // Second node group is cheaper pricingModel = &testPricingModel{ @@ -197,13 +210,13 @@ func TestPriceExpander(t *testing.T) { }, } provider.SetPricingModel(pricingModel) - assert.Contains(t, NewStrategy( + assert.Equal(t, optionsToDebug(NewStrategy( provider, &testPreferredNodeProvider{ preferred: buildNode(2000, units.GiB), }, SimpleNodeUnfitness, - ).BestOption(options, nodeInfosForGroups).Debug, "ng2") + ).BestOptions(options, nodeInfosForGroups)), []string{"ng2"}) // First group accept 1 pod and second accepts 2. options2 := []expander.Option{ @@ -234,13 +247,13 @@ func TestPriceExpander(t *testing.T) { provider.SetPricingModel(pricingModel) // Both node groups are equally expensive. However 2 // accept two pods. - assert.Contains(t, NewStrategy( + assert.Equal(t, optionsToDebug(NewStrategy( provider, &testPreferredNodeProvider{ preferred: buildNode(2000, units.GiB), }, SimpleNodeUnfitness, - ).BestOption(options2, nodeInfosForGroups).Debug, "ng2") + ).BestOptions(options2, nodeInfosForGroups)), []string{"ng2"}) // Errors are expected pricingModel = &testPricingModel{ @@ -248,13 +261,13 @@ func TestPriceExpander(t *testing.T) { nodePrice: map[string]float64{}, } provider.SetPricingModel(pricingModel) - assert.Nil(t, NewStrategy( + assert.Empty(t, NewStrategy( provider, &testPreferredNodeProvider{ preferred: buildNode(2000, units.GiB), }, SimpleNodeUnfitness, - ).BestOption(options2, nodeInfosForGroups)) + ).BestOptions(options2, nodeInfosForGroups)) // Add node info for autoprovisioned group. nodeInfosForGroups["autoprovisioned-MT1"] = ni3 @@ -293,13 +306,13 @@ func TestPriceExpander(t *testing.T) { }, } provider.SetPricingModel(pricingModel) - assert.Contains(t, NewStrategy( + assert.Equal(t, optionsToDebug(NewStrategy( provider, &testPreferredNodeProvider{ preferred: buildNode(2000, units.GiB), }, SimpleNodeUnfitness, - ).BestOption(options3, nodeInfosForGroups).Debug, "ng2") + ).BestOptions(options3, nodeInfosForGroups)), []string{"ng2"}) // Choose non-existing group when non-existing is cheaper. pricingModel = &testPricingModel{ @@ -315,11 +328,11 @@ func TestPriceExpander(t *testing.T) { }, } provider.SetPricingModel(pricingModel) - assert.Contains(t, NewStrategy( + assert.Equal(t, optionsToDebug(NewStrategy( provider, &testPreferredNodeProvider{ preferred: buildNode(2000, units.GiB), }, SimpleNodeUnfitness, - ).BestOption(options3, nodeInfosForGroups).Debug, "ng3") + ).BestOptions(options3, nodeInfosForGroups)), []string{"ng3"}) } diff --git a/cluster-autoscaler/expander/priority/priority.go b/cluster-autoscaler/expander/priority/priority.go index f794931f3f72..844a813e5148 100644 --- a/cluster-autoscaler/expander/priority/priority.go +++ b/cluster-autoscaler/expander/priority/priority.go @@ -24,8 +24,6 @@ import ( "gopkg.in/yaml.v2" "k8s.io/autoscaler/cluster-autoscaler/expander" - "k8s.io/autoscaler/cluster-autoscaler/expander/random" - caserrors "k8s.io/autoscaler/cluster-autoscaler/utils/errors" apiv1 "k8s.io/api/core/v1" v1lister "k8s.io/client-go/listers/core/v1" @@ -45,21 +43,19 @@ type priorities map[int][]*regexp.Regexp type priority struct { logRecorder record.EventRecorder - fallbackStrategy expander.Strategy okConfigUpdates int badConfigUpdates int configMapLister v1lister.ConfigMapNamespaceLister } -// NewStrategy returns an expansion strategy that picks node groups based on user-defined priorities -func NewStrategy(configMapLister v1lister.ConfigMapNamespaceLister, - logRecorder record.EventRecorder) (expander.Strategy, caserrors.AutoscalerError) { +// NewFilter returns an expansion filter that picks node groups based on user-defined priorities +func NewFilter(configMapLister v1lister.ConfigMapNamespaceLister, + logRecorder record.EventRecorder) expander.Filter { res := &priority{ - logRecorder: logRecorder, - fallbackStrategy: random.NewStrategy(), - configMapLister: configMapLister, + logRecorder: logRecorder, + configMapLister: configMapLister, } - return res, nil + return res } func (p *priority) reloadConfigMap() (priorities, *apiv1.ConfigMap, error) { @@ -120,7 +116,7 @@ func (p *priority) parsePrioritiesYAMLString(prioritiesYAML string) (priorities, return newPriorities, nil } -func (p *priority) BestOption(expansionOptions []expander.Option, nodeInfo map[string]*schedulernodeinfo.NodeInfo) *expander.Option { +func (p *priority) BestOptions(expansionOptions []expander.Option, nodeInfo map[string]*schedulernodeinfo.NodeInfo) []expander.Option { if len(expansionOptions) <= 0 { return nil } @@ -157,15 +153,15 @@ func (p *priority) BestOption(expansionOptions []expander.Option, nodeInfo map[s } if len(best) == 0 { - msg := "Priority expander: no priorities info found for any of the expansion options. Falling back to random choice." + msg := "Priority expander: no priorities info found for any of the expansion options. No options filtered." p.logConfigWarning(cm, "PriorityConfigMapNoGroupMatched", msg) - return p.fallbackStrategy.BestOption(expansionOptions, nodeInfo) + return expansionOptions } for _, opt := range best { klog.V(2).Infof("priority expander: %s chosen as the highest available", opt.NodeGroup.Id()) } - return p.fallbackStrategy.BestOption(best, nodeInfo) + return best } func (p *priority) groupIDMatchesList(id string, nameRegexpList []*regexp.Regexp) bool { diff --git a/cluster-autoscaler/expander/priority/priority_test.go b/cluster-autoscaler/expander/priority/priority_test.go index 1796ef7a422d..3bcb6c80b752 100644 --- a/cluster-autoscaler/expander/priority/priority_test.go +++ b/cluster-autoscaler/expander/priority/priority_test.go @@ -87,7 +87,7 @@ var ( } ) -func getStrategyInstance(t *testing.T, config string) (expander.Strategy, *record.FakeRecorder, *apiv1.ConfigMap, error) { +func getFilterInstance(t *testing.T, config string) (expander.Filter, *record.FakeRecorder, *apiv1.ConfigMap, error) { cm := &apiv1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: testNamespace, @@ -104,46 +104,40 @@ func getStrategyInstance(t *testing.T, config string) (expander.Strategy, *recor return s, r, cm, err } -func TestPriorityExpanderCorrecltySelectsSingleMatchingOptionOutOfOne(t *testing.T) { - s, _, _, _ := getStrategyInstance(t, config) - ret := s.BestOption([]expander.Option{eoT2Large}, nil) - assert.Equal(t, *ret, eoT2Large) +func TestPriorityExpanderCorrecltyFiltersSingleMatchingOptionOutOfOne(t *testing.T) { + s, _, _, _ := getFilterInstance(t, config) + ret := s.BestOptions([]expander.Option{eoT2Large}, nil) + assert.Equal(t, ret, []expander.Option{eoT2Large}) } -func TestPriorityExpanderCorrecltySelectsSingleMatchingOptionOutOfMany(t *testing.T) { - s, _, _, _ := getStrategyInstance(t, config) - ret := s.BestOption([]expander.Option{eoT2Large, eoM44XLarge}, nil) - assert.Equal(t, *ret, eoM44XLarge) +func TestPriorityExpanderCorrecltyFiltersSingleMatchingOptionOutOfMany(t *testing.T) { + s, _, _, _ := getFilterInstance(t, config) + ret := s.BestOptions([]expander.Option{eoT2Large, eoM44XLarge}, nil) + assert.Equal(t, ret, []expander.Option{eoM44XLarge}) } -func TestPriorityExpanderDoesNotFallBackToRandomWhenHigherPriorityMatches(t *testing.T) { - s, _, _, _ := getStrategyInstance(t, wildcardMatchConfig) - for i := 0; i < 10; i++ { - ret := s.BestOption([]expander.Option{eoT2Large, eoT2Micro}, nil) - assert.Equal(t, *ret, eoT2Large) - } +func TestPriorityExpanderFiltersToHigherPriorityMatch(t *testing.T) { + s, _, _, _ := getFilterInstance(t, wildcardMatchConfig) + ret := s.BestOptions([]expander.Option{eoT2Large, eoT2Micro}, nil) + assert.Equal(t, ret, []expander.Option{eoT2Large}) } -func TestPriorityExpanderCorrecltySelectsOneOfTwoMatchingOptionsOutOfMany(t *testing.T) { - s, _, _, _ := getStrategyInstance(t, config) - for i := 0; i < 10; i++ { - ret := s.BestOption([]expander.Option{eoT2Large, eoT3Large, eoT2Micro}, nil) - assert.True(t, ret.NodeGroup.Id() == eoT2Large.NodeGroup.Id() || ret.NodeGroup.Id() == eoT3Large.NodeGroup.Id()) - } +func TestPriorityExpanderCorrecltyFiltersTwoMatchingOptionsOutOfMany(t *testing.T) { + s, _, _, _ := getFilterInstance(t, config) + ret := s.BestOptions([]expander.Option{eoT2Large, eoT3Large, eoT2Micro}, nil) + assert.Equal(t, ret, []expander.Option{eoT2Large, eoT3Large}) } -func TestPriorityExpanderCorrecltyFallsBackToRandomWhenNoMatches(t *testing.T) { - s, _, _, _ := getStrategyInstance(t, config) - for i := 0; i < 10; i++ { - ret := s.BestOption([]expander.Option{eoT2Large, eoT3Large}, nil) - assert.True(t, ret.NodeGroup.Id() == eoT2Large.NodeGroup.Id() || ret.NodeGroup.Id() == eoT3Large.NodeGroup.Id()) - } +func TestPriorityExpanderCorrecltyFallsBackToAllWhenNoMatches(t *testing.T) { + s, _, _, _ := getFilterInstance(t, config) + ret := s.BestOptions([]expander.Option{eoT2Large, eoT3Large}, nil) + assert.Equal(t, ret, []expander.Option{eoT2Large, eoT3Large}) } func TestPriorityExpanderCorrecltyHandlesConfigUpdate(t *testing.T) { - s, r, cm, _ := getStrategyInstance(t, oneEntryConfig) - ret := s.BestOption([]expander.Option{eoT2Large, eoT3Large, eoM44XLarge}, nil) - assert.Equal(t, *ret, eoT2Large) + s, r, cm, _ := getFilterInstance(t, oneEntryConfig) + ret := s.BestOptions([]expander.Option{eoT2Large, eoT3Large, eoM44XLarge}, nil) + assert.Equal(t, ret, []expander.Option{eoT2Large}) var event string for _, group := range []string{eoT3Large.NodeGroup.Id(), eoM44XLarge.NodeGroup.Id()} { @@ -152,24 +146,24 @@ func TestPriorityExpanderCorrecltyHandlesConfigUpdate(t *testing.T) { } cm.Data[ConfigMapKey] = config - ret = s.BestOption([]expander.Option{eoT2Large, eoT3Large, eoM44XLarge}, nil) + ret = s.BestOptions([]expander.Option{eoT2Large, eoT3Large, eoM44XLarge}, nil) priority := s.(*priority) assert.Equal(t, 2, priority.okConfigUpdates) - assert.Equal(t, *ret, eoM44XLarge) + assert.Equal(t, ret, []expander.Option{eoM44XLarge}) } func TestPriorityExpanderCorrecltySkipsBadChangeConfig(t *testing.T) { - s, r, cm, _ := getStrategyInstance(t, oneEntryConfig) + s, r, cm, _ := getFilterInstance(t, oneEntryConfig) priority := s.(*priority) assert.Equal(t, 0, priority.okConfigUpdates) cm.Data[ConfigMapKey] = "" - ret := s.BestOption([]expander.Option{eoT2Large, eoT3Large, eoM44XLarge}, nil) + ret := s.BestOptions([]expander.Option{eoT2Large, eoT3Large, eoM44XLarge}, nil) assert.Equal(t, 1, priority.badConfigUpdates) event := <-r.Events assert.EqualValues(t, configWarnConfigMapEmpty, event) - assert.Nil(t, ret) + assert.Empty(t, ret) } diff --git a/cluster-autoscaler/expander/random/random.go b/cluster-autoscaler/expander/random/random.go index cf4a5a016220..a117c9ce753f 100644 --- a/cluster-autoscaler/expander/random/random.go +++ b/cluster-autoscaler/expander/random/random.go @@ -26,12 +26,26 @@ import ( type random struct { } +// NewFilter returns an expansion filter that randomly picks between node groups +func NewFilter() expander.Filter { + return &random{} +} + // NewStrategy returns an expansion strategy that randomly picks between node groups func NewStrategy() expander.Strategy { return &random{} } -// RandomExpansion Selects from the expansion options at random +// BestOptions selects from the expansion options at random +func (r *random) BestOptions(expansionOptions []expander.Option, nodeInfo map[string]*schedulernodeinfo.NodeInfo) []expander.Option { + best := r.BestOption(expansionOptions, nodeInfo) + if best == nil { + return nil + } + return []expander.Option{*best} +} + +// BestOption selects from the expansion options at random func (r *random) BestOption(expansionOptions []expander.Option, nodeInfo map[string]*schedulernodeinfo.NodeInfo) *expander.Option { if len(expansionOptions) <= 0 { return nil diff --git a/cluster-autoscaler/expander/waste/waste.go b/cluster-autoscaler/expander/waste/waste.go index b0cf8d3a685b..ddb5a345653c 100644 --- a/cluster-autoscaler/expander/waste/waste.go +++ b/cluster-autoscaler/expander/waste/waste.go @@ -20,22 +20,20 @@ import ( apiv1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/autoscaler/cluster-autoscaler/expander" - "k8s.io/autoscaler/cluster-autoscaler/expander/random" "k8s.io/klog" schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo" ) type leastwaste struct { - fallbackStrategy expander.Strategy } -// NewStrategy returns a strategy that selects the best scale up option based on which node group returns the least waste -func NewStrategy() expander.Strategy { - return &leastwaste{random.NewStrategy()} +// NewFilter returns a filter that selects the best scale up option based on which node group returns the least waste +func NewFilter() expander.Filter { + return &leastwaste{} } // BestOption Finds the option that wastes the least fraction of CPU and Memory -func (l *leastwaste) BestOption(expansionOptions []expander.Option, nodeInfo map[string]*schedulernodeinfo.NodeInfo) *expander.Option { +func (l *leastwaste) BestOptions(expansionOptions []expander.Option, nodeInfo map[string]*schedulernodeinfo.NodeInfo) []expander.Option { var leastWastedScore float64 var leastWastedOptions []expander.Option @@ -70,7 +68,7 @@ func (l *leastwaste) BestOption(expansionOptions []expander.Option, nodeInfo map return nil } - return l.fallbackStrategy.BestOption(leastWastedOptions, nodeInfo) + return leastWastedOptions } func resourcesForPods(pods []*apiv1.Pod) (cpu resource.Quantity, memory resource.Quantity) { diff --git a/cluster-autoscaler/expander/waste/waste_test.go b/cluster-autoscaler/expander/waste/waste_test.go index 11c782503902..8161e7642502 100644 --- a/cluster-autoscaler/expander/waste/waste_test.go +++ b/cluster-autoscaler/expander/waste/waste_test.go @@ -83,8 +83,8 @@ func TestLeastWaste(t *testing.T) { balancedOption := expander.Option{NodeGroup: &FakeNodeGroup{"balanced"}, NodeCount: 1} // Test without any pods, one node info - ret := e.BestOption([]expander.Option{balancedOption}, nodeMap) - assert.Equal(t, *ret, balancedOption) + ret := e.BestOptions([]expander.Option{balancedOption}, nodeMap) + assert.Equal(t, ret, []expander.Option{balancedOption}) pod := &apiv1.Pod{ Spec: apiv1.PodSpec{ @@ -103,20 +103,20 @@ func TestLeastWaste(t *testing.T) { // Test with one pod, one node info balancedOption.Pods = []*apiv1.Pod{pod} - ret = e.BestOption([]expander.Option{balancedOption}, nodeMap) - assert.Equal(t, *ret, balancedOption) + ret = e.BestOptions([]expander.Option{balancedOption}, nodeMap) + assert.Equal(t, ret, []expander.Option{balancedOption}) // Test with one pod, two node infos, one that has lots of RAM one that has less highmemNodeInfo := makeNodeInfo(16*cpuPerPod, 32*memoryPerPod, 100) nodeMap["highmem"] = highmemNodeInfo highmemOption := expander.Option{NodeGroup: &FakeNodeGroup{"highmem"}, NodeCount: 1, Pods: []*apiv1.Pod{pod}} - ret = e.BestOption([]expander.Option{balancedOption, highmemOption}, nodeMap) - assert.Equal(t, *ret, balancedOption) + ret = e.BestOptions([]expander.Option{balancedOption, highmemOption}, nodeMap) + assert.Equal(t, ret, []expander.Option{balancedOption}) // Test with one pod, three node infos, one that has lots of RAM one that has less, and one that has less CPU lowcpuNodeInfo := makeNodeInfo(8*cpuPerPod, 16*memoryPerPod, 100) nodeMap["lowcpu"] = lowcpuNodeInfo lowcpuOption := expander.Option{NodeGroup: &FakeNodeGroup{"lowcpu"}, NodeCount: 1, Pods: []*apiv1.Pod{pod}} - ret = e.BestOption([]expander.Option{balancedOption, highmemOption, lowcpuOption}, nodeMap) - assert.Equal(t, *ret, lowcpuOption) + ret = e.BestOptions([]expander.Option{balancedOption, highmemOption, lowcpuOption}, nodeMap) + assert.Equal(t, ret, []expander.Option{lowcpuOption}) } diff --git a/cluster-autoscaler/main.go b/cluster-autoscaler/main.go index 3e7a1828a02b..44d2f735fea2 100644 --- a/cluster-autoscaler/main.go +++ b/cluster-autoscaler/main.go @@ -148,8 +148,7 @@ var ( estimatorFlag = flag.String("estimator", estimator.BinpackingEstimatorName, "Type of resource estimator to be used in scale up. Available values: ["+strings.Join(estimator.AvailableEstimators, ",")+"]") - expanderFlag = flag.String("expander", expander.RandomExpanderName, - "Type of node group expander to be used in scale up. Available values: ["+strings.Join(expander.AvailableExpanders, ",")+"]") + expanderFlag = flag.String("expander", "", "Type of node group expander to be used in scale up. Available values: ["+strings.Join(expander.AvailableExpanders, ",")+"]. Specifying multiple values separated by commas will call the expanders in succession until there is only one option remaining. Ties still existing after this process are broken randomly.") ignoreDaemonSetsUtilization = flag.Bool("ignore-daemonsets-utilization", false, "Should CA ignore DaemonSet pods when calculating resource utilization for scaling down") @@ -199,7 +198,7 @@ func createAutoscalingOptions() config.AutoscalingOptions { OkTotalUnreadyCount: *okTotalUnreadyCount, ScaleUpFromZero: *scaleUpFromZero, EstimatorName: *estimatorFlag, - ExpanderName: *expanderFlag, + ExpanderNames: *expanderFlag, IgnoreDaemonSetsUtilization: *ignoreDaemonSetsUtilization, IgnoreMirrorPodsUtilization: *ignoreMirrorPodsUtilization, MaxBulkSoftTaintCount: *maxBulkSoftTaintCount,