From 88d93a9f16af89ba8a6e91a98e5269d879ee277d Mon Sep 17 00:00:00 2001 From: Ace Eldeib Date: Thu, 17 Oct 2019 04:54:03 -0700 Subject: [PATCH 1/4] feat: azure vmss min/mix autodiscovery --- cluster-autoscaler/FAQ.md | 2 +- .../cloudprovider/azure/azure_manager.go | 123 +++++++++++++++++- .../cloudprovider/azure/azure_util.go | 3 +- 3 files changed, 119 insertions(+), 9 deletions(-) diff --git a/cluster-autoscaler/FAQ.md b/cluster-autoscaler/FAQ.md index 9b1470cd91e..1eafd34d7c1 100644 --- a/cluster-autoscaler/FAQ.md +++ b/cluster-autoscaler/FAQ.md @@ -657,7 +657,7 @@ The following startup parameters are supported for cluster autoscaler: | `ok-total-unready-count` | Number of allowed unready nodes, irrespective of max-total-unready-percentage | 3 | `max-node-provision-time` | Maximum time CA waits for node to be provisioned | 15 minutes | `nodes` | sets min,max size and other configuration data for a node group in a format accepted by cloud provider. Can be used multiple times. Format: :: | "" -| `node-group-auto-discovery` | One or more definition(s) of node group auto-discovery.
A definition is expressed `:[[=]]`
The `aws` and `gce` cloud providers are currently supported. AWS matches by ASG tags, e.g. `asg:tag=tagKey,anotherTagKey`
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`
Can be used multiple times | "" +| `node-group-auto-discovery` | One or more definition(s) of node group auto-discovery.
A definition is expressed `:[[=]]`
The `aws`, `gce`, and `azure` cloud providers are currently supported. AWS matches by ASG tags, e.g. `asg:tag=tagKey,anotherTagKey`
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`
Azure matches by tags on VMSS, e.g. `label:foo=bar`, and will auto-detect `min` and `max` tags on the VMSS to set scaling limits.
Can be used multiple times | "" | `estimator` | Type of resource estimator to be used in scale up | binpacking | `expander` | Type of node group expander to be used in scale up. | random | `write-status-configmap` | Should CA write status information to a configmap | true diff --git a/cluster-autoscaler/cloudprovider/azure/azure_manager.go b/cluster-autoscaler/cloudprovider/azure/azure_manager.go index a113ed99c29..751ca329aa7 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_manager.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_manager.go @@ -43,8 +43,29 @@ const ( // The path of deployment parameters for standard vm. deploymentParametersPath = "/var/lib/azure/azuredeploy.parameters.json" + + vmssTagMin = "min" + vmssTagMax = "max" + autoDiscovererTypeLabel = "label" + labelAutoDiscovererKeyMinNodes = "min" + labelAutoDiscovererKeyMaxNodes = "max" ) +var validLabelAutoDiscovererKeys = strings.Join([]string{ + labelAutoDiscovererKeyMinNodes, + labelAutoDiscovererKeyMaxNodes, +}, ", ") + +// A labelAutoDiscoveryConfig specifies how to autodiscover Azure scale sets. +type labelAutoDiscoveryConfig struct { + // Key-values to match on. + Selector map[string]string + // MinSize specifies the minimum size for all MIGs that match Re. + MinSize int + // MaxSize specifies the maximum size for all MIGs that match Re. + MaxSize int +} + // AzureManager handles Azure communication and data caching. type AzureManager struct { config *Config @@ -53,7 +74,7 @@ type AzureManager struct { asgCache *asgCache lastRefresh time.Time - asgAutoDiscoverySpecs []cloudprovider.LabelAutoDiscoveryConfig + asgAutoDiscoverySpecs []labelAutoDiscoveryConfig explicitlyConfigured map[string]bool } @@ -181,7 +202,7 @@ func CreateAzureManager(configReader io.Reader, discoveryOpts cloudprovider.Node } manager.asgCache = cache - specs, err := discoveryOpts.ParseLabelAutoDiscoverySpecs() + specs, err := parseLabelAutoDiscoverySpecs(discoveryOpts) if err != nil { return nil, err } @@ -335,7 +356,7 @@ func (m *AzureManager) Cleanup() { m.asgCache.Cleanup() } -func (m *AzureManager) getFilteredAutoscalingGroups(filter []cloudprovider.LabelAutoDiscoveryConfig) (asgs []cloudprovider.NodeGroup, err error) { +func (m *AzureManager) getFilteredAutoscalingGroups(filter []labelAutoDiscoveryConfig) (asgs []cloudprovider.NodeGroup, err error) { if len(filter) == 0 { return nil, nil } @@ -359,7 +380,7 @@ func (m *AzureManager) getFilteredAutoscalingGroups(filter []cloudprovider.Label } // listScaleSets gets a list of scale sets and instanceIDs. -func (m *AzureManager) listScaleSets(filter []cloudprovider.LabelAutoDiscoveryConfig) (asgs []cloudprovider.NodeGroup, err error) { +func (m *AzureManager) listScaleSets(filter []labelAutoDiscoveryConfig) (asgs []cloudprovider.NodeGroup, err error) { ctx, cancel := getContextWithCancel() defer cancel() @@ -379,13 +400,38 @@ func (m *AzureManager) listScaleSets(filter []cloudprovider.LabelAutoDiscoveryCo continue } } - spec := &dynamic.NodeGroupSpec{ Name: *scaleSet.Name, MinSize: 1, MaxSize: -1, SupportScaleToZero: scaleToZeroSupportedVMSS, } + + if val, ok := scaleSet.Tags["min"]; ok { + if minSize, err := strconv.Atoi(*val); err == nil { + spec.MinSize = minSize + } else { + return asgs, fmt.Errorf("invalid minimum size specified for vmss: %s", err) + } + } else { + return asgs, fmt.Errorf("no minimum size specified for vmss: %s", err) + } + if val, ok := scaleSet.Tags["max"]; ok { + if maxSize, err := strconv.Atoi(*val); err == nil { + spec.MaxSize = maxSize + } else { + return asgs, fmt.Errorf("invalid maximum size specified for vmss: %s", err) + } + } else { + return asgs, fmt.Errorf("no maximum size specified for vmss: %s", err) + } + if spec.MaxSize < 1 { + return asgs, fmt.Errorf("maximum size must be greater than 1 node") + } + if spec.MaxSize < spec.MinSize { + return asgs, fmt.Errorf("maximum size must be greater than minimum size") + } + asg, _ := NewScaleSet(spec, m) asgs = append(asgs, asg) } @@ -395,7 +441,7 @@ func (m *AzureManager) listScaleSets(filter []cloudprovider.LabelAutoDiscoveryCo // listAgentPools gets a list of agent pools and instanceIDs. // Note: filter won't take effect for agent pools. -func (m *AzureManager) listAgentPools(filter []cloudprovider.LabelAutoDiscoveryConfig) (asgs []cloudprovider.NodeGroup, err error) { +func (m *AzureManager) listAgentPools(filter []labelAutoDiscoveryConfig) (asgs []cloudprovider.NodeGroup, err error) { ctx, cancel := getContextWithCancel() defer cancel() deploy, err := m.azClient.deploymentsClient.Get(ctx, m.config.ResourceGroup, m.config.Deployment) @@ -423,3 +469,68 @@ func (m *AzureManager) listAgentPools(filter []cloudprovider.LabelAutoDiscoveryC return asgs, nil } + +// ParseLabelAutoDiscoverySpecs returns any provided NodeGroupAutoDiscoverySpecs +// parsed into configuration appropriate for ASG autodiscovery. +func parseLabelAutoDiscoverySpecs(o cloudprovider.NodeGroupDiscoveryOptions) ([]labelAutoDiscoveryConfig, error) { + cfgs := make([]labelAutoDiscoveryConfig, len(o.NodeGroupAutoDiscoverySpecs)) + var err error + for i, spec := range o.NodeGroupAutoDiscoverySpecs { + cfgs[i], err = parseLabelAutoDiscoverySpec(spec) + if err != nil { + return nil, err + } + } + return cfgs, nil +} + +// parseLabelAutoDiscoverySpec parses a single spec and returns the corredponding node group spec. +func parseLabelAutoDiscoverySpec(spec string) (labelAutoDiscoveryConfig, error) { + cfg := labelAutoDiscoveryConfig{ + Selector: make(map[string]string), + MinSize: 1, + MaxSize: -1, + } + + tokens := strings.Split(spec, ":") + if len(tokens) != 2 { + return cfg, fmt.Errorf("spec \"%s\" should be discoverer:key=value,key=value", spec) + } + discoverer := tokens[0] + if discoverer != autoDiscovererTypeLabel { + return cfg, fmt.Errorf("unsupported discoverer specified: %s", discoverer) + } + + for _, arg := range strings.Split(tokens[1], ",") { + kv := strings.Split(arg, "=") + if len(kv) != 2 { + return cfg, fmt.Errorf("invalid key=value pair %s", kv) + } + + k, v := kv[0], kv[1] + + switch k { + case labelAutoDiscovererKeyMinNodes: + if minSize, err := strconv.Atoi(v); err == nil { + cfg.MinSize = minSize + } else { + return cfg, fmt.Errorf("invalid minimum nodes: %s", v) + } + case labelAutoDiscovererKeyMaxNodes: + if maxSize, err := strconv.Atoi(v); err == nil { + cfg.MaxSize = maxSize + } else { + return cfg, fmt.Errorf("invalid maximum nodes: %s", v) + } + default: + cfg.Selector[k] = v + } + } + if cfg.MaxSize < 1 { + return cfg, fmt.Errorf("maximum size must be greater than 1 node") + } + if cfg.MaxSize < cfg.MinSize { + return cfg, fmt.Errorf("maximum size must be greater than minimum size") + } + return cfg, nil +} diff --git a/cluster-autoscaler/cloudprovider/azure/azure_util.go b/cluster-autoscaler/cloudprovider/azure/azure_util.go index 0b17996835c..aa6a076dfed 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_util.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_util.go @@ -37,7 +37,6 @@ import ( "golang.org/x/crypto/pkcs12" - "k8s.io/autoscaler/cluster-autoscaler/cloudprovider" "k8s.io/client-go/pkg/version" "k8s.io/klog" ) @@ -494,7 +493,7 @@ func GetVMNameIndex(osType compute.OperatingSystemTypes, vmName string) (int, er return agentIndex, nil } -func matchDiscoveryConfig(labels map[string]*string, configs []cloudprovider.LabelAutoDiscoveryConfig) bool { +func matchDiscoveryConfig(labels map[string]*string, configs []labelAutoDiscoveryConfig) bool { if len(configs) == 0 { return false } From d63067e70b372498b4fe1d480a1a24438ebac234 Mon Sep 17 00:00:00 2001 From: Ace Eldeib Date: Thu, 17 Oct 2019 05:18:55 -0700 Subject: [PATCH 2/4] refactor: move aws discovery config --- .../cloudprovider/aws/auto_scaling_groups.go | 5 +- .../aws/aws_cloud_provider_test.go | 6 +- .../cloudprovider/aws/aws_manager.go | 64 ++++++++++++++++++- .../cloudprovider/aws/aws_manager_test.go | 61 ++++++++++++++++++ 4 files changed, 129 insertions(+), 7 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go b/cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go index 8acffeae69e..fc2a7e5a07c 100644 --- a/cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go +++ b/cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go @@ -23,7 +23,6 @@ import ( "strings" "sync" - "k8s.io/autoscaler/cluster-autoscaler/cloudprovider" "k8s.io/autoscaler/cluster-autoscaler/config/dynamic" "github.com/aws/aws-sdk-go/aws" @@ -44,7 +43,7 @@ type asgCache struct { service autoScalingWrapper interrupt chan struct{} - asgAutoDiscoverySpecs []cloudprovider.ASGAutoDiscoveryConfig + asgAutoDiscoverySpecs []asgAutoDiscoveryConfig explicitlyConfigured map[AwsRef]bool } @@ -72,7 +71,7 @@ type asg struct { Tags []*autoscaling.TagDescription } -func newASGCache(service autoScalingWrapper, explicitSpecs []string, autoDiscoverySpecs []cloudprovider.ASGAutoDiscoveryConfig) (*asgCache, error) { +func newASGCache(service autoScalingWrapper, explicitSpecs []string, autoDiscoverySpecs []asgAutoDiscoveryConfig) (*asgCache, error) { registry := &asgCache{ registeredAsgs: make([]*asg, 0), service: service, diff --git a/cluster-autoscaler/cloudprovider/aws/aws_cloud_provider_test.go b/cluster-autoscaler/cloudprovider/aws/aws_cloud_provider_test.go index 219862211a7..b9df496c467 100644 --- a/cluster-autoscaler/cloudprovider/aws/aws_cloud_provider_test.go +++ b/cluster-autoscaler/cloudprovider/aws/aws_cloud_provider_test.go @@ -79,7 +79,7 @@ var testAwsManager = &AwsManager{ autoScalingService: testService, } -func newTestAwsManagerWithService(service autoScaling, autoDiscoverySpecs []cloudprovider.ASGAutoDiscoveryConfig) *AwsManager { +func newTestAwsManagerWithService(service autoScaling, autoDiscoverySpecs []asgAutoDiscoveryConfig) *AwsManager { wrapper := autoScalingWrapper{service, map[string]string{}} return &AwsManager{ autoScalingService: wrapper, @@ -101,7 +101,7 @@ func newTestAwsManagerWithAsgs(t *testing.T, service autoScaling, specs []string return m } -func newTestAwsManagerWithAutoAsgs(t *testing.T, service autoScaling, specs []string, autoDiscoverySpecs []cloudprovider.ASGAutoDiscoveryConfig) *AwsManager { +func newTestAwsManagerWithAutoAsgs(t *testing.T, service autoScaling, specs []string, autoDiscoverySpecs []asgAutoDiscoveryConfig) *AwsManager { m := newTestAwsManagerWithService(service, autoDiscoverySpecs) m.asgCache.parseExplicitAsgs(specs) return m @@ -167,7 +167,7 @@ func TestNodeGroups(t *testing.T) { func TestAutoDiscoveredNodeGroups(t *testing.T) { service := &AutoScalingMock{} - provider := testProvider(t, newTestAwsManagerWithAutoAsgs(t, service, []string{}, []cloudprovider.ASGAutoDiscoveryConfig{ + provider := testProvider(t, newTestAwsManagerWithAutoAsgs(t, service, []string{}, []asgAutoDiscoveryConfig{ { Tags: map[string]string{"test": ""}, }, diff --git a/cluster-autoscaler/cloudprovider/aws/aws_manager.go b/cluster-autoscaler/cloudprovider/aws/aws_manager.go index 8deab85194a..1d4400b75b4 100644 --- a/cluster-autoscaler/cloudprovider/aws/aws_manager.go +++ b/cluster-autoscaler/cloudprovider/aws/aws_manager.go @@ -51,6 +51,8 @@ const ( maxRecordsReturnedByAPI = 100 maxAsgNamesPerDescribe = 50 refreshInterval = 1 * time.Minute + autoDiscovererTypeASG = "asg" + asgAutoDiscovererKeyTag = "tag" ) // AwsManager is handles aws communication and data caching. @@ -202,7 +204,7 @@ func createAWSManagerInternal( } } - specs, err := discoveryOpts.ParseASGAutoDiscoverySpecs() + specs, err := parseASGAutoDiscoverySpecs(discoveryOpts) if err != nil { return nil, err } @@ -459,3 +461,63 @@ func extractTaintsFromAsg(tags []*autoscaling.TagDescription) []apiv1.Taint { } return taints } + +// An asgAutoDiscoveryConfig specifies how to autodiscover AWS ASGs. +type asgAutoDiscoveryConfig struct { + // Tags to match on. + // Any ASG with all of the provided tag keys will be autoscaled. + Tags map[string]string +} + +// ParseASGAutoDiscoverySpecs returns any provided NodeGroupAutoDiscoverySpecs +// parsed into configuration appropriate for ASG autodiscovery. +func parseASGAutoDiscoverySpecs(o cloudprovider.NodeGroupDiscoveryOptions) ([]asgAutoDiscoveryConfig, error) { + cfgs := make([]asgAutoDiscoveryConfig, len(o.NodeGroupAutoDiscoverySpecs)) + var err error + for i, spec := range o.NodeGroupAutoDiscoverySpecs { + cfgs[i], err = parseASGAutoDiscoverySpec(spec) + if err != nil { + return nil, err + } + } + return cfgs, nil +} + +func parseASGAutoDiscoverySpec(spec string) (asgAutoDiscoveryConfig, error) { + cfg := asgAutoDiscoveryConfig{} + + tokens := strings.Split(spec, ":") + if len(tokens) != 2 { + return cfg, fmt.Errorf("invalid node group auto discovery spec specified via --node-group-auto-discovery: %s", spec) + } + discoverer := tokens[0] + if discoverer != autoDiscovererTypeASG { + return cfg, fmt.Errorf("unsupported discoverer specified: %s", discoverer) + } + param := tokens[1] + kv := strings.SplitN(param, "=", 2) + if len(kv) != 2 { + return cfg, fmt.Errorf("invalid key=value pair %s", kv) + } + k, v := kv[0], kv[1] + if k != asgAutoDiscovererKeyTag { + return cfg, fmt.Errorf("unsupported parameter key \"%s\" is specified for discoverer \"%s\". The only supported key is \"%s\"", k, discoverer, asgAutoDiscovererKeyTag) + } + if v == "" { + return cfg, errors.New("tag value not supplied") + } + p := strings.Split(v, ",") + if len(p) == 0 { + return cfg, fmt.Errorf("invalid ASG tag for auto discovery specified: ASG tag must not be empty") + } + cfg.Tags = make(map[string]string, len(p)) + for _, label := range p { + lp := strings.SplitN(label, "=", 2) + if len(lp) > 1 { + cfg.Tags[lp[0]] = lp[1] + continue + } + cfg.Tags[lp[0]] = "" + } + return cfg, nil +} diff --git a/cluster-autoscaler/cloudprovider/aws/aws_manager_test.go b/cluster-autoscaler/cloudprovider/aws/aws_manager_test.go index afb6edda9e6..cf135ad49fa 100644 --- a/cluster-autoscaler/cloudprovider/aws/aws_manager_test.go +++ b/cluster-autoscaler/cloudprovider/aws/aws_manager_test.go @@ -777,3 +777,64 @@ func flatTagSlice(filters []*autoscaling.Filter) []string { sort.Strings(tags) return tags } + +func TestParseASGAutoDiscoverySpecs(t *testing.T) { + cases := []struct { + name string + specs []string + want []asgAutoDiscoveryConfig + wantErr bool + }{ + { + name: "GoodSpecs", + specs: []string{ + "asg:tag=tag,anothertag", + "asg:tag=cooltag,anothertag", + "asg:tag=label=value,anothertag", + }, + want: []asgAutoDiscoveryConfig{ + {Tags: map[string]string{"tag": "", "anothertag": ""}}, + {Tags: map[string]string{"cooltag": "", "anothertag": ""}}, + {Tags: map[string]string{"label": "value", "anothertag": ""}}, + }, + }, + { + name: "MissingASGType", + specs: []string{"tag=tag,anothertag"}, + wantErr: true, + }, + { + name: "WrongType", + specs: []string{"mig:tag=tag,anothertag"}, + wantErr: true, + }, + { + name: "KeyMissingValue", + specs: []string{"asg:tag="}, + wantErr: true, + }, + { + name: "ValueMissingKey", + specs: []string{"asg:=tag"}, + wantErr: true, + }, + { + name: "KeyMissingSeparator", + specs: []string{"asg:tag"}, + wantErr: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + do := cloudprovider.NodeGroupDiscoveryOptions{NodeGroupAutoDiscoverySpecs: tc.specs} + got, err := parseASGAutoDiscoverySpecs(do) + if tc.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.True(t, assert.ObjectsAreEqualValues(tc.want, got), "\ngot: %#v\nwant: %#v", got, tc.want) + }) + } +} From 95704a7f6c38c74e87e2ed0eaa58cac7fb075047 Mon Sep 17 00:00:00 2001 From: Ace Eldeib Date: Thu, 17 Oct 2019 05:50:40 -0700 Subject: [PATCH 3/4] refactor: move gce discovery config --- .../cloudprovider/gce/gce_manager.go | 99 ++++++++++++++-- .../cloudprovider/gce/gce_manager_test.go | 108 +++++++++++++++++- 2 files changed, 198 insertions(+), 9 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/gce/gce_manager.go b/cluster-autoscaler/cloudprovider/gce/gce_manager.go index c5a651e52b3..60e1d7a8887 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_manager.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_manager.go @@ -17,10 +17,12 @@ limitations under the License. package gce import ( + "errors" "fmt" "io" "os" "regexp" + "strconv" "strings" "time" @@ -41,10 +43,14 @@ import ( ) const ( - refreshInterval = 1 * time.Minute - machinesRefreshInterval = 1 * time.Hour - httpTimeout = 30 * time.Second - scaleToZeroSupported = true + refreshInterval = 1 * time.Minute + machinesRefreshInterval = 1 * time.Hour + httpTimeout = 30 * time.Second + scaleToZeroSupported = true + autoDiscovererTypeMIG = "mig" + migAutoDiscovererKeyPrefix = "namePrefix" + migAutoDiscovererKeyMinNodes = "min" + migAutoDiscovererKeyMaxNodes = "max" ) var ( @@ -54,6 +60,12 @@ var ( "https://www.googleapis.com/auth/service.management.readonly", "https://www.googleapis.com/auth/servicecontrol", } + + validMIGAutoDiscovererKeys = strings.Join([]string{ + migAutoDiscovererKeyPrefix, + migAutoDiscovererKeyMinNodes, + migAutoDiscovererKeyMaxNodes, + }, ", ") ) // GceManager handles GCE communication and data caching. @@ -97,7 +109,7 @@ type gceManagerImpl struct { interrupt chan struct{} regional bool explicitlyConfigured map[GceRef]bool - migAutoDiscoverySpecs []cloudprovider.MIGAutoDiscoveryConfig + migAutoDiscoverySpecs []migAutoDiscoveryConfig } // CreateGceManager constructs GceManager object. @@ -172,7 +184,7 @@ func CreateGceManager(configReader io.Reader, discoveryOpts cloudprovider.NodeGr if err := manager.fetchExplicitMigs(discoveryOpts.NodeGroupSpecs); err != nil { return nil, fmt.Errorf("failed to fetch MIGs: %v", err) } - if manager.migAutoDiscoverySpecs, err = discoveryOpts.ParseMIGAutoDiscoverySpecs(); err != nil { + if manager.migAutoDiscoverySpecs, err = parseMIGAutoDiscoverySpecs(discoveryOpts); err != nil { return nil, err } @@ -312,7 +324,7 @@ func (m *gceManagerImpl) buildMigFromFlag(flag string) (Mig, error) { return m.buildMigFromSpec(s) } -func (m *gceManagerImpl) buildMigFromAutoCfg(link string, cfg cloudprovider.MIGAutoDiscoveryConfig) (Mig, error) { +func (m *gceManagerImpl) buildMigFromAutoCfg(link string, cfg migAutoDiscoveryConfig) (Mig, error) { s := &dynamic.NodeGroupSpec{ Name: link, MinSize: cfg.MinSize, @@ -505,3 +517,76 @@ func parseCustomMachineType(machineType string) (cpu, mem int64, err error) { mem = mem * units.MiB return } + +// parseMIGAutoDiscoverySpecs returns any provided NodeGroupAutoDiscoverySpecs +// parsed into configuration appropriate for MIG autodiscovery. +func parseMIGAutoDiscoverySpecs(o cloudprovider.NodeGroupDiscoveryOptions) ([]migAutoDiscoveryConfig, error) { + cfgs := make([]migAutoDiscoveryConfig, len(o.NodeGroupAutoDiscoverySpecs)) + var err error + for i, spec := range o.NodeGroupAutoDiscoverySpecs { + cfgs[i], err = parseMIGAutoDiscoverySpec(spec) + if err != nil { + return nil, err + } + } + return cfgs, nil +} + +// A migAutoDiscoveryConfig specifies how to autodiscover GCE MIGs. +type migAutoDiscoveryConfig struct { + // Re is a regexp passed using the eq filter to the GCE list API. + Re *regexp.Regexp + // MinSize specifies the minimum size for all MIGs that match Re. + MinSize int + // MaxSize specifies the maximum size for all MIGs that match Re. + MaxSize int +} + +func parseMIGAutoDiscoverySpec(spec string) (migAutoDiscoveryConfig, error) { + cfg := migAutoDiscoveryConfig{} + + tokens := strings.Split(spec, ":") + if len(tokens) != 2 { + return cfg, fmt.Errorf("spec \"%s\" should be discoverer:key=value,key=value", spec) + } + discoverer := tokens[0] + if discoverer != autoDiscovererTypeMIG { + return cfg, fmt.Errorf("unsupported discoverer specified: %s", discoverer) + } + + for _, arg := range strings.Split(tokens[1], ",") { + kv := strings.Split(arg, "=") + if len(kv) != 2 { + return cfg, fmt.Errorf("invalid key=value pair %s", kv) + } + k, v := kv[0], kv[1] + + var err error + switch k { + case migAutoDiscovererKeyPrefix: + if cfg.Re, err = regexp.Compile(fmt.Sprintf("^%s.+", v)); err != nil { + return cfg, fmt.Errorf("invalid instance group name prefix \"%s\" - \"^%s.+\" must be a valid RE2 regexp", v, v) + } + case migAutoDiscovererKeyMinNodes: + if cfg.MinSize, err = strconv.Atoi(v); err != nil { + return cfg, fmt.Errorf("invalid minimum nodes: %s", v) + } + case migAutoDiscovererKeyMaxNodes: + if cfg.MaxSize, err = strconv.Atoi(v); err != nil { + return cfg, fmt.Errorf("invalid maximum nodes: %s", v) + } + default: + return cfg, fmt.Errorf("unsupported key \"%s\" is specified for discoverer \"%s\". Supported keys are \"%s\"", k, discoverer, validMIGAutoDiscovererKeys) + } + } + if cfg.Re == nil || cfg.Re.String() == "^.+" { + return cfg, errors.New("empty instance group name prefix supplied") + } + if cfg.MinSize > cfg.MaxSize { + return cfg, fmt.Errorf("minimum size %d is greater than maximum size %d", cfg.MinSize, cfg.MaxSize) + } + if cfg.MaxSize < 1 { + return cfg, fmt.Errorf("maximum size %d must be at least 1", cfg.MaxSize) + } + return cfg, nil +} diff --git a/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go b/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go index 2c57352a189..2e3e516693e 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go @@ -1140,7 +1140,7 @@ func TestFetchAutoMigsZonal(t *testing.T) { g := newTestGceManager(t, server.URL, regional) min, max := 0, 100 - g.migAutoDiscoverySpecs = []cloudprovider.MIGAutoDiscoveryConfig{ + g.migAutoDiscoverySpecs = []migAutoDiscoveryConfig{ {Re: regexp.MustCompile("UNUSED"), MinSize: min, MaxSize: max}, } @@ -1213,7 +1213,7 @@ func TestFetchAutoMigsRegional(t *testing.T) { g := newTestGceManager(t, server.URL, regional) min, max := 0, 100 - g.migAutoDiscoverySpecs = []cloudprovider.MIGAutoDiscoveryConfig{ + g.migAutoDiscoverySpecs = []migAutoDiscoveryConfig{ {Re: regexp.MustCompile("UNUSED"), MinSize: min, MaxSize: max}, } @@ -1411,3 +1411,107 @@ func validateMigExists(t *testing.T, migs []Mig, zone string, name string, minSi } assert.Failf(t, "Mig not found", "Mig %v not found among %v", ref, allRefs) } + +func TestParseMIGAutoDiscoverySpecs(t *testing.T) { + cases := []struct { + name string + specs []string + want []migAutoDiscoveryConfig + wantErr bool + }{ + { + name: "GoodSpecs", + specs: []string{ + "mig:namePrefix=pfx,min=0,max=10", + "mig:namePrefix=anotherpfx,min=1,max=2", + }, + want: []migAutoDiscoveryConfig{ + {Re: regexp.MustCompile("^pfx.+"), MinSize: 0, MaxSize: 10}, + {Re: regexp.MustCompile("^anotherpfx.+"), MinSize: 1, MaxSize: 2}, + }, + }, + { + name: "MissingMIGType", + specs: []string{"namePrefix=pfx,min=0,max=10"}, + wantErr: true, + }, + { + name: "WrongType", + specs: []string{"asg:namePrefix=pfx,min=0,max=10"}, + wantErr: true, + }, + { + name: "UnknownKey", + specs: []string{"mig:namePrefix=pfx,min=0,max=10,unknown=hi"}, + wantErr: true, + }, + { + name: "NonIntegerMin", + specs: []string{"mig:namePrefix=pfx,min=a,max=10"}, + wantErr: true, + }, + { + name: "NonIntegerMax", + specs: []string{"mig:namePrefix=pfx,min=1,max=donkey"}, + wantErr: true, + }, + { + name: "PrefixDoesNotCompileToRegexp", + specs: []string{"mig:namePrefix=a),min=1,max=10"}, + wantErr: true, + }, + { + name: "KeyMissingValue", + specs: []string{"mig:namePrefix=prefix,min=,max=10"}, + wantErr: true, + }, + { + name: "ValueMissingKey", + specs: []string{"mig:namePrefix=prefix,=0,max=10"}, + wantErr: true, + }, + { + name: "KeyMissingSeparator", + specs: []string{"mig:namePrefix=prefix,min,max=10"}, + wantErr: true, + }, + { + name: "TooManySeparators", + specs: []string{"mig:namePrefix=prefix,min=0,max=10=20"}, + wantErr: true, + }, + { + name: "PrefixIsEmpty", + specs: []string{"mig:namePrefix=,min=0,max=10"}, + wantErr: true, + }, + { + name: "PrefixIsMissing", + specs: []string{"mig:min=0,max=10"}, + wantErr: true, + }, + { + name: "MaxBelowMin", + specs: []string{"mig:namePrefix=prefix,min=10,max=1"}, + wantErr: true, + }, + { + name: "MaxIsZero", + specs: []string{"mig:namePrefix=prefix,min=0,max=0"}, + wantErr: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + do := cloudprovider.NodeGroupDiscoveryOptions{NodeGroupAutoDiscoverySpecs: tc.specs} + got, err := parseMIGAutoDiscoverySpecs(do) + if tc.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.True(t, assert.ObjectsAreEqualValues(tc.want, got), "\ngot: %#v\nwant: %#v", got, tc.want) + }) + } +} From 57e4471bd8fbb96c39ea6042a97a4b05ba3adb3a Mon Sep 17 00:00:00 2001 From: Ace Eldeib Date: Thu, 17 Oct 2019 05:51:27 -0700 Subject: [PATCH 4/4] refactor: common discovery opts --- .../node_group_discovery_options.go | 206 ------------------ .../node_group_discovery_options_test.go | 189 ---------------- 2 files changed, 395 deletions(-) delete mode 100644 cluster-autoscaler/cloudprovider/node_group_discovery_options_test.go diff --git a/cluster-autoscaler/cloudprovider/node_group_discovery_options.go b/cluster-autoscaler/cloudprovider/node_group_discovery_options.go index 5dba4bd3ad0..45877f9bf58 100644 --- a/cluster-autoscaler/cloudprovider/node_group_discovery_options.go +++ b/cluster-autoscaler/cloudprovider/node_group_discovery_options.go @@ -16,32 +16,6 @@ limitations under the License. package cloudprovider -import ( - "errors" - "fmt" - "regexp" - "strconv" - "strings" -) - -const ( - autoDiscovererTypeMIG = "mig" - autoDiscovererTypeASG = "asg" - autoDiscovererTypeLabel = "label" - - migAutoDiscovererKeyPrefix = "namePrefix" - migAutoDiscovererKeyMinNodes = "min" - migAutoDiscovererKeyMaxNodes = "max" - - asgAutoDiscovererKeyTag = "tag" -) - -var validMIGAutoDiscovererKeys = strings.Join([]string{ - migAutoDiscovererKeyPrefix, - migAutoDiscovererKeyMinNodes, - migAutoDiscovererKeyMaxNodes, -}, ", ") - // NodeGroupDiscoveryOptions contains various options to configure how a cloud provider discovers node groups type NodeGroupDiscoveryOptions struct { // NodeGroupSpecs is specified to statically discover node groups listed in it @@ -65,183 +39,3 @@ func (o NodeGroupDiscoveryOptions) AutoDiscoverySpecified() bool { func (o NodeGroupDiscoveryOptions) DiscoverySpecified() bool { return o.StaticDiscoverySpecified() || o.AutoDiscoverySpecified() } - -// ParseMIGAutoDiscoverySpecs returns any provided NodeGroupAutoDiscoverySpecs -// parsed into configuration appropriate for MIG autodiscovery. -func (o NodeGroupDiscoveryOptions) ParseMIGAutoDiscoverySpecs() ([]MIGAutoDiscoveryConfig, error) { - cfgs := make([]MIGAutoDiscoveryConfig, len(o.NodeGroupAutoDiscoverySpecs)) - var err error - for i, spec := range o.NodeGroupAutoDiscoverySpecs { - cfgs[i], err = parseMIGAutoDiscoverySpec(spec) - if err != nil { - return nil, err - } - } - return cfgs, nil -} - -// ParseASGAutoDiscoverySpecs returns any provided NodeGroupAutoDiscoverySpecs -// parsed into configuration appropriate for ASG autodiscovery. -func (o NodeGroupDiscoveryOptions) ParseASGAutoDiscoverySpecs() ([]ASGAutoDiscoveryConfig, error) { - cfgs := make([]ASGAutoDiscoveryConfig, len(o.NodeGroupAutoDiscoverySpecs)) - var err error - for i, spec := range o.NodeGroupAutoDiscoverySpecs { - cfgs[i], err = parseASGAutoDiscoverySpec(spec) - if err != nil { - return nil, err - } - } - return cfgs, nil -} - -// ParseLabelAutoDiscoverySpecs returns any provided NodeGroupAutoDiscoverySpecs -// parsed into configuration appropriate for ASG autodiscovery. -func (o NodeGroupDiscoveryOptions) ParseLabelAutoDiscoverySpecs() ([]LabelAutoDiscoveryConfig, error) { - cfgs := make([]LabelAutoDiscoveryConfig, len(o.NodeGroupAutoDiscoverySpecs)) - var err error - for i, spec := range o.NodeGroupAutoDiscoverySpecs { - cfgs[i], err = parseLabelAutoDiscoverySpec(spec) - if err != nil { - return nil, err - } - } - return cfgs, nil -} - -// A MIGAutoDiscoveryConfig specifies how to autodiscover GCE MIGs. -type MIGAutoDiscoveryConfig struct { - // Re is a regexp passed using the eq filter to the GCE list API. - Re *regexp.Regexp - // MinSize specifies the minimum size for all MIGs that match Re. - MinSize int - // MaxSize specifies the maximum size for all MIGs that match Re. - MaxSize int -} - -func parseMIGAutoDiscoverySpec(spec string) (MIGAutoDiscoveryConfig, error) { - cfg := MIGAutoDiscoveryConfig{} - - tokens := strings.Split(spec, ":") - if len(tokens) != 2 { - return cfg, fmt.Errorf("spec \"%s\" should be discoverer:key=value,key=value", spec) - } - discoverer := tokens[0] - if discoverer != autoDiscovererTypeMIG { - return cfg, fmt.Errorf("unsupported discoverer specified: %s", discoverer) - } - - for _, arg := range strings.Split(tokens[1], ",") { - kv := strings.Split(arg, "=") - if len(kv) != 2 { - return cfg, fmt.Errorf("invalid key=value pair %s", kv) - } - k, v := kv[0], kv[1] - - var err error - switch k { - case migAutoDiscovererKeyPrefix: - if cfg.Re, err = regexp.Compile(fmt.Sprintf("^%s.+", v)); err != nil { - return cfg, fmt.Errorf("invalid instance group name prefix \"%s\" - \"^%s.+\" must be a valid RE2 regexp", v, v) - } - case migAutoDiscovererKeyMinNodes: - if cfg.MinSize, err = strconv.Atoi(v); err != nil { - return cfg, fmt.Errorf("invalid minimum nodes: %s", v) - } - case migAutoDiscovererKeyMaxNodes: - if cfg.MaxSize, err = strconv.Atoi(v); err != nil { - return cfg, fmt.Errorf("invalid maximum nodes: %s", v) - } - default: - return cfg, fmt.Errorf("unsupported key \"%s\" is specified for discoverer \"%s\". Supported keys are \"%s\"", k, discoverer, validMIGAutoDiscovererKeys) - } - } - if cfg.Re == nil || cfg.Re.String() == "^.+" { - return cfg, errors.New("empty instance group name prefix supplied") - } - if cfg.MinSize > cfg.MaxSize { - return cfg, fmt.Errorf("minimum size %d is greater than maximum size %d", cfg.MinSize, cfg.MaxSize) - } - if cfg.MaxSize < 1 { - return cfg, fmt.Errorf("maximum size %d must be at least 1", cfg.MaxSize) - } - return cfg, nil -} - -// An ASGAutoDiscoveryConfig specifies how to autodiscover AWS ASGs. -type ASGAutoDiscoveryConfig struct { - // Tags to match on. - // Any ASG with all of the provided tag keys will be autoscaled. - Tags map[string]string -} - -func parseASGAutoDiscoverySpec(spec string) (ASGAutoDiscoveryConfig, error) { - cfg := ASGAutoDiscoveryConfig{} - - tokens := strings.Split(spec, ":") - if len(tokens) != 2 { - return cfg, fmt.Errorf("invalid node group auto discovery spec specified via --node-group-auto-discovery: %s", spec) - } - discoverer := tokens[0] - if discoverer != autoDiscovererTypeASG { - return cfg, fmt.Errorf("unsupported discoverer specified: %s", discoverer) - } - param := tokens[1] - kv := strings.SplitN(param, "=", 2) - if len(kv) != 2 { - return cfg, fmt.Errorf("invalid key=value pair %s", kv) - } - k, v := kv[0], kv[1] - if k != asgAutoDiscovererKeyTag { - return cfg, fmt.Errorf("unsupported parameter key \"%s\" is specified for discoverer \"%s\". The only supported key is \"%s\"", k, discoverer, asgAutoDiscovererKeyTag) - } - if v == "" { - return cfg, errors.New("tag value not supplied") - } - p := strings.Split(v, ",") - if len(p) == 0 { - return cfg, fmt.Errorf("invalid ASG tag for auto discovery specified: ASG tag must not be empty") - } - cfg.Tags = make(map[string]string, len(p)) - for _, label := range p { - lp := strings.SplitN(label, "=", 2) - if len(lp) > 1 { - cfg.Tags[lp[0]] = lp[1] - continue - } - cfg.Tags[lp[0]] = "" - } - return cfg, nil -} - -// A LabelAutoDiscoveryConfig specifies how to autodiscover Azure scale sets. -type LabelAutoDiscoveryConfig struct { - // Key-values to match on. - Selector map[string]string -} - -func parseLabelAutoDiscoverySpec(spec string) (LabelAutoDiscoveryConfig, error) { - cfg := LabelAutoDiscoveryConfig{ - Selector: make(map[string]string), - } - - tokens := strings.Split(spec, ":") - if len(tokens) != 2 { - return cfg, fmt.Errorf("spec \"%s\" should be discoverer:key=value,key=value", spec) - } - discoverer := tokens[0] - if discoverer != autoDiscovererTypeLabel { - return cfg, fmt.Errorf("unsupported discoverer specified: %s", discoverer) - } - - for _, arg := range strings.Split(tokens[1], ",") { - kv := strings.Split(arg, "=") - if len(kv) != 2 { - return cfg, fmt.Errorf("invalid key=value pair %s", kv) - } - - k, v := kv[0], kv[1] - cfg.Selector[k] = v - } - - return cfg, nil -} diff --git a/cluster-autoscaler/cloudprovider/node_group_discovery_options_test.go b/cluster-autoscaler/cloudprovider/node_group_discovery_options_test.go deleted file mode 100644 index 62cc59aaded..00000000000 --- a/cluster-autoscaler/cloudprovider/node_group_discovery_options_test.go +++ /dev/null @@ -1,189 +0,0 @@ -/* -Copyright 2016 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 cloudprovider - -import ( - "regexp" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestParseMIGAutoDiscoverySpecs(t *testing.T) { - cases := []struct { - name string - specs []string - want []MIGAutoDiscoveryConfig - wantErr bool - }{ - { - name: "GoodSpecs", - specs: []string{ - "mig:namePrefix=pfx,min=0,max=10", - "mig:namePrefix=anotherpfx,min=1,max=2", - }, - want: []MIGAutoDiscoveryConfig{ - {Re: regexp.MustCompile("^pfx.+"), MinSize: 0, MaxSize: 10}, - {Re: regexp.MustCompile("^anotherpfx.+"), MinSize: 1, MaxSize: 2}, - }, - }, - { - name: "MissingMIGType", - specs: []string{"namePrefix=pfx,min=0,max=10"}, - wantErr: true, - }, - { - name: "WrongType", - specs: []string{"asg:namePrefix=pfx,min=0,max=10"}, - wantErr: true, - }, - { - name: "UnknownKey", - specs: []string{"mig:namePrefix=pfx,min=0,max=10,unknown=hi"}, - wantErr: true, - }, - { - name: "NonIntegerMin", - specs: []string{"mig:namePrefix=pfx,min=a,max=10"}, - wantErr: true, - }, - { - name: "NonIntegerMax", - specs: []string{"mig:namePrefix=pfx,min=1,max=donkey"}, - wantErr: true, - }, - { - name: "PrefixDoesNotCompileToRegexp", - specs: []string{"mig:namePrefix=a),min=1,max=10"}, - wantErr: true, - }, - { - name: "KeyMissingValue", - specs: []string{"mig:namePrefix=prefix,min=,max=10"}, - wantErr: true, - }, - { - name: "ValueMissingKey", - specs: []string{"mig:namePrefix=prefix,=0,max=10"}, - wantErr: true, - }, - { - name: "KeyMissingSeparator", - specs: []string{"mig:namePrefix=prefix,min,max=10"}, - wantErr: true, - }, - { - name: "TooManySeparators", - specs: []string{"mig:namePrefix=prefix,min=0,max=10=20"}, - wantErr: true, - }, - { - name: "PrefixIsEmpty", - specs: []string{"mig:namePrefix=,min=0,max=10"}, - wantErr: true, - }, - { - name: "PrefixIsMissing", - specs: []string{"mig:min=0,max=10"}, - wantErr: true, - }, - { - name: "MaxBelowMin", - specs: []string{"mig:namePrefix=prefix,min=10,max=1"}, - wantErr: true, - }, - { - name: "MaxIsZero", - specs: []string{"mig:namePrefix=prefix,min=0,max=0"}, - wantErr: true, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - do := NodeGroupDiscoveryOptions{NodeGroupAutoDiscoverySpecs: tc.specs} - got, err := do.ParseMIGAutoDiscoverySpecs() - if tc.wantErr { - assert.Error(t, err) - return - } - assert.NoError(t, err) - assert.True(t, assert.ObjectsAreEqualValues(tc.want, got), "\ngot: %#v\nwant: %#v", got, tc.want) - }) - } -} - -func TestParseASGAutoDiscoverySpecs(t *testing.T) { - cases := []struct { - name string - specs []string - want []ASGAutoDiscoveryConfig - wantErr bool - }{ - { - name: "GoodSpecs", - specs: []string{ - "asg:tag=tag,anothertag", - "asg:tag=cooltag,anothertag", - "asg:tag=label=value,anothertag", - }, - want: []ASGAutoDiscoveryConfig{ - {Tags: map[string]string{"tag": "", "anothertag": ""}}, - {Tags: map[string]string{"cooltag": "", "anothertag": ""}}, - {Tags: map[string]string{"label": "value", "anothertag": ""}}, - }, - }, - { - name: "MissingASGType", - specs: []string{"tag=tag,anothertag"}, - wantErr: true, - }, - { - name: "WrongType", - specs: []string{"mig:tag=tag,anothertag"}, - wantErr: true, - }, - { - name: "KeyMissingValue", - specs: []string{"asg:tag="}, - wantErr: true, - }, - { - name: "ValueMissingKey", - specs: []string{"asg:=tag"}, - wantErr: true, - }, - { - name: "KeyMissingSeparator", - specs: []string{"asg:tag"}, - wantErr: true, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - do := NodeGroupDiscoveryOptions{NodeGroupAutoDiscoverySpecs: tc.specs} - got, err := do.ParseASGAutoDiscoverySpecs() - if tc.wantErr { - assert.Error(t, err) - return - } - assert.NoError(t, err) - assert.True(t, assert.ObjectsAreEqualValues(tc.want, got), "\ngot: %#v\nwant: %#v", got, tc.want) - }) - } -}