From a03458897a5e88c993d0ba785e976d2a22222c43 Mon Sep 17 00:00:00 2001 From: Marwan Ahmed Date: Mon, 6 Jan 2020 17:59:00 -0800 Subject: [PATCH 1/2] dont attempt to parse min and max counts through discoverer --- .../cloudprovider/azure/azure_manager.go | 39 ++++--------------- 1 file changed, 8 insertions(+), 31 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/azure/azure_manager.go b/cluster-autoscaler/cloudprovider/azure/azure_manager.go index ca20eb525c..52109a1770 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_manager.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_manager.go @@ -63,10 +63,6 @@ var validLabelAutoDiscovererKeys = strings.Join([]string{ 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. @@ -454,7 +450,10 @@ func (m *AzureManager) listScaleSets(filter []labelAutoDiscoveryConfig) (asgs [] 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) + return asgs, fmt.Errorf("no minimum size specified for vmss: %s", *scaleSet.Name) + } + if spec.MinSize < 0 { + return asgs, fmt.Errorf("minimum size must be a non-negative number of nodes") } if val, ok := scaleSet.Tags["max"]; ok { if maxSize, err := strconv.Atoi(*val); err == nil { @@ -463,7 +462,7 @@ func (m *AzureManager) listScaleSets(filter []labelAutoDiscoveryConfig) (asgs [] 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) + return asgs, fmt.Errorf("no maximum size specified for vmss: %s", *scaleSet.Name) } if spec.MaxSize < 1 { return asgs, fmt.Errorf("maximum size must be greater than 1 node") @@ -528,8 +527,6 @@ func parseLabelAutoDiscoverySpecs(o cloudprovider.NodeGroupDiscoveryOptions) ([] func parseLabelAutoDiscoverySpec(spec string) (labelAutoDiscoveryConfig, error) { cfg := labelAutoDiscoveryConfig{ Selector: make(map[string]string), - MinSize: 1, - MaxSize: -1, } tokens := strings.Split(spec, ":") @@ -546,31 +543,11 @@ func parseLabelAutoDiscoverySpec(spec string) (labelAutoDiscoveryConfig, error) 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 k == "" || v == "" { + return cfg, fmt.Errorf("empty value not allowed in key=value tag pairs") } - } - 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") + cfg.Selector[k] = v } return cfg, nil } From 9d7738933a285d03839fc4ca7a6fc54b37e27680 Mon Sep 17 00:00:00 2001 From: Marwan Ahmed Date: Mon, 6 Jan 2020 17:59:59 -0800 Subject: [PATCH 2/2] add test cases for fetching explicit asgs and auto asgs --- .../cloudprovider/azure/azure_fakes.go | 15 ++ .../cloudprovider/azure/azure_manager_test.go | 250 ++++++++++++++++++ 2 files changed, 265 insertions(+) diff --git a/cluster-autoscaler/cloudprovider/azure/azure_fakes.go b/cluster-autoscaler/cloudprovider/azure/azure_fakes.go index 277704fc82..530373ed99 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_fakes.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_fakes.go @@ -285,3 +285,18 @@ func (m *DeploymentsClientMock) Delete(ctx context.Context, resourceGroupName, d return } + +func fakeVMSSWithTags(vmssName string, tags map[string]*string) compute.VirtualMachineScaleSet { + skuName := "Standard_D4_v2" + var vmssCapacity int64 = 3 + + return compute.VirtualMachineScaleSet{ + Name: &vmssName, + Sku: &compute.Sku{ + Capacity: &vmssCapacity, + Name: &skuName, + }, + Tags: tags, + } + +} diff --git a/cluster-autoscaler/cloudprovider/azure/azure_manager_test.go b/cluster-autoscaler/cloudprovider/azure/azure_manager_test.go index cb032b2519..04d4e89ac4 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_manager_test.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_manager_test.go @@ -17,9 +17,11 @@ limitations under the License. package azure import ( + "fmt" "strings" "testing" + "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-10-01/compute" "github.com/stretchr/testify/assert" "k8s.io/autoscaler/cluster-autoscaler/cloudprovider" ) @@ -65,3 +67,251 @@ func TestCreateAzureManagerInvalidConfig(t *testing.T) { _, err := CreateAzureManager(strings.NewReader(invalidAzureCfg), cloudprovider.NodeGroupDiscoveryOptions{}) assert.Error(t, err, "failed to unmarshal config body") } + +func TestFetchExplicitAsgs(t *testing.T) { + min, max, name := 1, 15, "test-asg" + ngdo := cloudprovider.NodeGroupDiscoveryOptions{ + NodeGroupSpecs: []string{ + fmt.Sprintf("%d:%d:%s", min, max, name), + }, + } + + manager := newTestAzureManager(t) + manager.fetchExplicitAsgs(ngdo.NodeGroupSpecs) + + asgs := manager.asgCache.get() + assert.Equal(t, 1, len(asgs)) + assert.Equal(t, name, asgs[0].Id()) + assert.Equal(t, min, asgs[0].MinSize()) + assert.Equal(t, max, asgs[0].MaxSize()) +} + +func TestParseLabelAutoDiscoverySpecs(t *testing.T) { + testCases := []struct { + name string + specs []string + expected []labelAutoDiscoveryConfig + expectedErr bool + }{ + { + name: "ValidSpec", + specs: []string{ + "label:cluster-autoscaler-enabled=true,cluster-autoscaler-name=fake-cluster", + "label:test-tag=test-value,another-test-tag=another-test-value", + }, + expected: []labelAutoDiscoveryConfig{ + {Selector: map[string]string{"cluster-autoscaler-enabled": "true", "cluster-autoscaler-name": "fake-cluster"}}, + {Selector: map[string]string{"test-tag": "test-value", "another-test-tag": "another-test-value"}}, + }, + }, + { + name: "MissingAutoDiscoverLabel", + specs: []string{"test-tag=test-value,another-test-tag"}, + expectedErr: true, + }, + { + name: "InvalidAutoDiscoerLabel", + specs: []string{"invalid:test-tag=test-value,another-test-tag"}, + expectedErr: true, + }, + { + name: "MissingValue", + specs: []string{"label:test-tag="}, + expectedErr: true, + }, + { + name: "MissingKey", + specs: []string{"label:=test-val"}, + expectedErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ngdo := cloudprovider.NodeGroupDiscoveryOptions{NodeGroupAutoDiscoverySpecs: tc.specs} + actual, err := parseLabelAutoDiscoverySpecs(ngdo) + if tc.expectedErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.True(t, assert.ObjectsAreEqualValues(tc.expected, actual), "expected %#v, but found: %#v", tc.expected, actual) + }) + } +} + +func TestListScalesets(t *testing.T) { + manager := newTestAzureManager(t) + vmssTag := "fake-tag" + vmssTagValue := "fake-value" + vmssName := "test-vmss" + + ngdo := cloudprovider.NodeGroupDiscoveryOptions{ + NodeGroupAutoDiscoverySpecs: []string{fmt.Sprintf("label:%s=%s", vmssTag, vmssTagValue)}, + } + specs, err := parseLabelAutoDiscoverySpecs(ngdo) + assert.NoError(t, err) + + testCases := []struct { + name string + specs map[string]string + expected []cloudprovider.NodeGroup + expectedErrString string + }{ + { + name: "ValidMinMax", + specs: map[string]string{"min": "5", "max": "50"}, + expected: []cloudprovider.NodeGroup{&ScaleSet{ + azureRef: azureRef{ + Name: vmssName, + }, + minSize: 5, + maxSize: 50, + manager: manager, + curSize: -1, + }}, + }, + { + name: "InvalidMin", + specs: map[string]string{"min": "some-invalid-string"}, + expectedErrString: "invalid minimum size specified for vmss:", + }, + { + name: "NoMin", + specs: map[string]string{"max": "50"}, + expectedErrString: fmt.Sprintf("no minimum size specified for vmss: %s", vmssName), + }, + { + name: "InvalidMax", + specs: map[string]string{"min": "5", "max": "some-invalid-string"}, + expectedErrString: "invalid maximum size specified for vmss:", + }, + { + name: "NoMax", + specs: map[string]string{"min": "5"}, + expectedErrString: fmt.Sprintf("no maximum size specified for vmss: %s", vmssName), + }, + { + name: "MaxLessThanOne", + specs: map[string]string{"min": "5", "max": "-5"}, + expectedErrString: "maximum size must be greater than 1 node", + }, + { + name: "MinLessThanZero", + specs: map[string]string{"min": "-4", "max": "20"}, + expectedErrString: fmt.Sprintf("minimum size must be a non-negative number of nodes"), + }, + { + name: "MinGreaterThanMax", + specs: map[string]string{"min": "50", "max": "5"}, + expectedErrString: "maximum size must be greater than minimum size", + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tags := make(map[string]*string) + tags[vmssTag] = &vmssTagValue + if val, ok := tc.specs["min"]; ok { + tags["min"] = &val + } + if val, ok := tc.specs["max"]; ok { + tags["max"] = &val + } + + scaleSetClient := &VirtualMachineScaleSetsClientMock{ + FakeStore: map[string]map[string]compute.VirtualMachineScaleSet{ + "test": { + vmssName: fakeVMSSWithTags(vmssName, tags), + }, + }, + } + + manager.azClient.virtualMachineScaleSetsClient = scaleSetClient + asgs, err := manager.listScaleSets(specs) + if tc.expectedErrString != "" { + assert.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedErrString) + return + } + assert.NoError(t, err) + assert.True(t, assert.ObjectsAreEqualValues(tc.expected, asgs), "expected %#v, but found: %#v", tc.expected, asgs) + }) + } +} + +func TestGetFilteredAutoscalingGroupsVmss(t *testing.T) { + vmssName := "test-vmss" + vmssTag := "fake-tag" + vmssTagValue := "fake-value" + min := "1" + minVal := 1 + max := "5" + maxVal := 5 + + ngdo := cloudprovider.NodeGroupDiscoveryOptions{ + NodeGroupAutoDiscoverySpecs: []string{fmt.Sprintf("label:%s=%s", vmssTag, vmssTagValue)}, + } + scaleSetClient := &VirtualMachineScaleSetsClientMock{ + FakeStore: map[string]map[string]compute.VirtualMachineScaleSet{ + "test": { + vmssName: fakeVMSSWithTags(vmssName, map[string]*string{vmssTag: &vmssTagValue, "min": &min, "max": &max}), + }, + }, + } + + manager := newTestAzureManager(t) + manager.azClient.virtualMachineScaleSetsClient = scaleSetClient + specs, err := parseLabelAutoDiscoverySpecs(ngdo) + assert.NoError(t, err) + + asgs, err := manager.getFilteredAutoscalingGroups(specs) + assert.NoError(t, err) + expectedAsgs := []cloudprovider.NodeGroup{&ScaleSet{ + azureRef: azureRef{ + Name: vmssName, + }, + minSize: minVal, + maxSize: maxVal, + manager: manager, + curSize: -1, + }} + assert.True(t, assert.ObjectsAreEqualValues(expectedAsgs, asgs), "expected %#v, but found: %#v", expectedAsgs, asgs) +} + +func TestFetchAutoAsgsVmss(t *testing.T) { + vmssName := "test-vmss" + vmssTag := "fake-tag" + vmssTagValue := "fake-value" + minString := "1" + minVal := 1 + maxString := "5" + maxVal := 5 + + ngdo := cloudprovider.NodeGroupDiscoveryOptions{ + NodeGroupAutoDiscoverySpecs: []string{fmt.Sprintf("label:%s=%s", vmssTag, vmssTagValue)}, + } + scaleSetClient := &VirtualMachineScaleSetsClientMock{ + FakeStore: map[string]map[string]compute.VirtualMachineScaleSet{ + "test": { + vmssName: fakeVMSSWithTags(vmssName, map[string]*string{vmssTag: &vmssTagValue, "min": &minString, "max": &maxString}), + }, + }, + } + + manager := newTestAzureManager(t) + manager.azClient.virtualMachineScaleSetsClient = scaleSetClient + specs, err := parseLabelAutoDiscoverySpecs(ngdo) + assert.NoError(t, err) + manager.asgAutoDiscoverySpecs = specs + + // assert cache is empty before fetching auto asgs + asgs := manager.asgCache.get() + assert.Equal(t, 0, len(asgs)) + + manager.fetchAutoAsgs() + asgs = manager.asgCache.get() + assert.Equal(t, 1, len(asgs)) + assert.Equal(t, vmssName, asgs[0].Id()) + assert.Equal(t, minVal, asgs[0].MinSize()) + assert.Equal(t, maxVal, asgs[0].MaxSize()) +}