Skip to content

Commit

Permalink
Don't store ASGs at the AWS cloud provider level, let the manager do it
Browse files Browse the repository at this point in the history
  • Loading branch information
Nic Cope committed Nov 30, 2017
1 parent 89eb3ba commit 303795f
Show file tree
Hide file tree
Showing 4 changed files with 133 additions and 180 deletions.
61 changes: 14 additions & 47 deletions cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
apiv1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
"k8s.io/autoscaler/cluster-autoscaler/config/dynamic"
"k8s.io/autoscaler/cluster-autoscaler/utils/errors"
"k8s.io/kubernetes/plugin/pkg/scheduler/schedulercache"
)
Expand All @@ -37,15 +36,13 @@ const (
// awsCloudProvider implements CloudProvider interface.
type awsCloudProvider struct {
awsManager *AwsManager
asgs []*Asg
resourceLimiter *cloudprovider.ResourceLimiter
}

// BuildAwsCloudProvider builds CloudProvider implementation for AWS.
func BuildAwsCloudProvider(awsManager *AwsManager, resourceLimiter *cloudprovider.ResourceLimiter) (cloudprovider.CloudProvider, error) {
aws := &awsCloudProvider{
awsManager: awsManager,
asgs: make([]*Asg, 0),
resourceLimiter: resourceLimiter,
}
return aws, nil
Expand All @@ -57,35 +54,28 @@ func (aws *awsCloudProvider) Cleanup() error {
return nil
}

// addNodeGroup adds node group defined in string spec. Format:
// minNodes:maxNodes:asgName
func (aws *awsCloudProvider) addNodeGroup(spec string) error {
asg, err := buildAsgFromSpec(spec, aws.awsManager)
if err != nil {
return err
}
aws.addAsg(asg)
return nil
}

// addAsg adds and registers an asg to this cloud provider
func (aws *awsCloudProvider) addAsg(asg *Asg) {
aws.asgs = append(aws.asgs, asg)
aws.awsManager.RegisterAsg(asg)
}

// Name returns name of the cloud provider.
func (aws *awsCloudProvider) Name() string {
return ProviderName
}

func (aws *awsCloudProvider) asgs() []*Asg {
infos := aws.awsManager.getAsgs()
asgs := make([]*Asg, len(infos))
for i, info := range infos {
asgs[i] = info.config
}
return asgs
}

// NodeGroups returns all node groups configured for this cloud provider.
func (aws *awsCloudProvider) NodeGroups() []cloudprovider.NodeGroup {
result := make([]cloudprovider.NodeGroup, 0, len(aws.asgs))
for _, asg := range aws.asgs {
result = append(result, asg)
asgs := aws.awsManager.getAsgs()
ngs := make([]cloudprovider.NodeGroup, len(asgs))
for i, asg := range asgs {
ngs[i] = asg.config
}
return result
return ngs
}

// NodeGroupForNode returns the node group for the given node.
Expand Down Expand Up @@ -309,26 +299,3 @@ func (asg *Asg) TemplateNodeInfo() (*schedulercache.NodeInfo, error) {
nodeInfo.SetNode(node)
return nodeInfo, nil
}

func buildAsgFromSpec(value string, awsManager *AwsManager) (*Asg, error) {
spec, err := dynamic.SpecFromString(value, true)

if err != nil {
return nil, fmt.Errorf("failed to parse node group spec: %v", err)
}

asg := buildAsg(awsManager, spec.MinSize, spec.MaxSize, spec.Name)

return asg, nil
}

func buildAsg(awsManager *AwsManager, minSize int, maxSize int, name string) *Asg {
return &Asg{
awsManager: awsManager,
minSize: minSize,
maxSize: maxSize,
AwsRef: AwsRef{
Name: name,
},
}
}
142 changes: 39 additions & 103 deletions cluster-autoscaler/cloudprovider/aws/aws_cloud_provider_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,18 @@ func newTestAwsManagerWithService(service autoScaling) *AwsManager {
}
}

func newTestAwsManagerWithAsgs(t *testing.T, service autoScaling, specs []string) *AwsManager {
m := newTestAwsManagerWithService(service)
for _, spec := range specs {
asg, err := m.buildAsgFromSpec(spec)
if err != nil {
t.Fatalf("bad ASG spec %v: %v", spec, err)
}
m.RegisterAsg(asg)
}
return m
}

func testDescribeAutoScalingGroupsOutput(desiredCap int64, instanceIds ...string) *autoscaling.DescribeAutoScalingGroupsOutput {
return testNamedDescribeAutoScalingGroupsOutput("UNUSED", desiredCap, instanceIds...)
}
Expand Down Expand Up @@ -129,27 +141,13 @@ func TestBuildAwsCloudProvider(t *testing.T) {
assert.NoError(t, err)
}

func TestAddNodeGroup(t *testing.T) {
provider := testProvider(t, testAwsManager)
err := provider.addNodeGroup("bad spec")
assert.Error(t, err)
assert.Equal(t, len(provider.asgs), 0)

err = provider.addNodeGroup("1:5:test-asg")
assert.NoError(t, err)
assert.Equal(t, len(provider.asgs), 1)
}

func TestName(t *testing.T) {
provider := testProvider(t, testAwsManager)
assert.Equal(t, provider.Name(), "aws")
assert.Equal(t, provider.Name(), ProviderName)
}

func TestNodeGroups(t *testing.T) {
provider := testProvider(t, testAwsManager)
assert.Equal(t, len(provider.NodeGroups()), 0)
err := provider.addNodeGroup("1:5:test-asg")
assert.NoError(t, err)
provider := testProvider(t, newTestAwsManagerWithAsgs(t, testService, []string{"1:5:test-asg"}))
assert.Equal(t, len(provider.NodeGroups()), 1)
}

Expand All @@ -160,14 +158,12 @@ func TestNodeGroupForNode(t *testing.T) {
},
}
service := &AutoScalingMock{}
m := newTestAwsManagerWithService(service)
provider := testProvider(t, m)
err := provider.addNodeGroup("1:5:test-asg")
assert.NoError(t, err)
provider := testProvider(t, newTestAwsManagerWithAsgs(t, service, []string{"1:5:test-asg"}))
asgs := provider.asgs()

service.On("DescribeAutoScalingGroupsPages",
&autoscaling.DescribeAutoScalingGroupsInput{
AutoScalingGroupNames: aws.StringSlice([]string{provider.asgs[0].Name}),
AutoScalingGroupNames: aws.StringSlice([]string{asgs[0].Name}),
MaxRecords: aws.Int64(maxRecordsReturnedByAPI),
},
mock.AnythingOfType("func(*autoscaling.DescribeAutoScalingGroupsOutput, bool) bool"),
Expand Down Expand Up @@ -209,35 +205,17 @@ func TestAwsRefFromProviderId(t *testing.T) {
assert.Equal(t, awsRef, &AwsRef{Name: "i-260942b3"})
}

func TestMaxSize(t *testing.T) {
provider := testProvider(t, testAwsManager)
err := provider.addNodeGroup("1:5:test-asg")
assert.NoError(t, err)
assert.Equal(t, len(provider.asgs), 1)
assert.Equal(t, provider.asgs[0].MaxSize(), 5)
}

func TestMinSize(t *testing.T) {
provider := testProvider(t, testAwsManager)
err := provider.addNodeGroup("1:5:test-asg")
assert.NoError(t, err)
assert.Equal(t, len(provider.asgs), 1)
assert.Equal(t, provider.asgs[0].MinSize(), 1)
}

func TestTargetSize(t *testing.T) {
service := &AutoScalingMock{}
m := newTestAwsManagerWithService(service)
provider := testProvider(t, m)
err := provider.addNodeGroup("1:5:test-asg")
assert.NoError(t, err)
provider := testProvider(t, newTestAwsManagerWithAsgs(t, service, []string{"1:5:test-asg"}))
asgs := provider.asgs()

service.On("DescribeAutoScalingGroups", &autoscaling.DescribeAutoScalingGroupsInput{
AutoScalingGroupNames: aws.StringSlice([]string{provider.asgs[0].Name}),
AutoScalingGroupNames: aws.StringSlice([]string{asgs[0].Name}),
MaxRecords: aws.Int64(1),
}).Return(testDescribeAutoScalingGroupsOutput(2, "test-instance-id", "second-test-instance-id"))

targetSize, err := provider.asgs[0].TargetSize()
targetSize, err := asgs[0].TargetSize()
assert.Equal(t, targetSize, 2)
assert.NoError(t, err)

Expand All @@ -246,39 +224,34 @@ func TestTargetSize(t *testing.T) {

func TestIncreaseSize(t *testing.T) {
service := &AutoScalingMock{}
m := newTestAwsManagerWithService(service)
provider := testProvider(t, m)
err := provider.addNodeGroup("1:5:test-asg")
assert.NoError(t, err)
assert.Equal(t, len(provider.asgs), 1)
provider := testProvider(t, newTestAwsManagerWithAsgs(t, service, []string{"1:5:test-asg"}))
asgs := provider.asgs()

service.On("SetDesiredCapacity", &autoscaling.SetDesiredCapacityInput{
AutoScalingGroupName: aws.String(provider.asgs[0].Name),
AutoScalingGroupName: aws.String(asgs[0].Name),
DesiredCapacity: aws.Int64(3),
HonorCooldown: aws.Bool(false),
}).Return(&autoscaling.SetDesiredCapacityOutput{})

service.On("DescribeAutoScalingGroups", &autoscaling.DescribeAutoScalingGroupsInput{
AutoScalingGroupNames: aws.StringSlice([]string{provider.asgs[0].Name}),
AutoScalingGroupNames: aws.StringSlice([]string{asgs[0].Name}),
MaxRecords: aws.Int64(1),
}).Return(testDescribeAutoScalingGroupsOutput(2, "test-instance-id", "second-test-instance-id"))

err = provider.asgs[0].IncreaseSize(1)
err := asgs[0].IncreaseSize(1)
assert.NoError(t, err)
service.AssertNumberOfCalls(t, "SetDesiredCapacity", 1)
service.AssertNumberOfCalls(t, "DescribeAutoScalingGroups", 1)
}

func TestBelongs(t *testing.T) {
service := &AutoScalingMock{}
m := newTestAwsManagerWithService(service)
provider := testProvider(t, m)
err := provider.addNodeGroup("1:5:test-asg")
assert.NoError(t, err)
provider := testProvider(t, newTestAwsManagerWithAsgs(t, service, []string{"1:5:test-asg"}))
asgs := provider.asgs()

service.On("DescribeAutoScalingGroupsPages",
&autoscaling.DescribeAutoScalingGroupsInput{
AutoScalingGroupNames: aws.StringSlice([]string{provider.asgs[0].Name}),
AutoScalingGroupNames: aws.StringSlice([]string{asgs[0].Name}),
MaxRecords: aws.Int64(maxRecordsReturnedByAPI),
},
mock.AnythingOfType("func(*autoscaling.DescribeAutoScalingGroupsOutput, bool) bool"),
Expand All @@ -292,7 +265,7 @@ func TestBelongs(t *testing.T) {
ProviderID: "aws:///us-east-1a/invalid-instance-id",
},
}
_, err = provider.asgs[0].Belongs(invalidNode)
_, err := asgs[0].Belongs(invalidNode)
assert.Error(t, err)
service.AssertNumberOfCalls(t, "DescribeAutoScalingGroupsPages", 1)

Expand All @@ -301,17 +274,19 @@ func TestBelongs(t *testing.T) {
ProviderID: "aws:///us-east-1a/test-instance-id",
},
}
belongs, err := provider.asgs[0].Belongs(validNode)
belongs, err := asgs[0].Belongs(validNode)
assert.Equal(t, belongs, true)
assert.NoError(t, err)
// As "test-instance-id" is already known to be managed by test-asg since the first `Belongs` call,
// No additional DescribAutoScalingGroup call is made
// As "test-instance-id" is already known to be managed by test-asg since
// the first `Belongs` call, No additional DescribAutoScalingGroupsPages
// call is made
service.AssertNumberOfCalls(t, "DescribeAutoScalingGroupsPages", 1)
}

func TestDeleteNodes(t *testing.T) {
service := &AutoScalingMock{}
m := newTestAwsManagerWithService(service)
provider := testProvider(t, newTestAwsManagerWithAsgs(t, service, []string{"1:5:test-asg"}))
asgs := provider.asgs()

service.On("TerminateInstanceInAutoScalingGroup", &autoscaling.TerminateInstanceInAutoScalingGroupInput{
InstanceId: aws.String("test-instance-id"),
Expand All @@ -320,20 +295,16 @@ func TestDeleteNodes(t *testing.T) {
Activity: &autoscaling.Activity{Description: aws.String("Deleted instance")},
})

provider := testProvider(t, m)
err := provider.addNodeGroup("1:5:test-asg")
assert.NoError(t, err)

// Look up the current number of instances...
service.On("DescribeAutoScalingGroups", &autoscaling.DescribeAutoScalingGroupsInput{
AutoScalingGroupNames: aws.StringSlice([]string{provider.asgs[0].Name}),
AutoScalingGroupNames: aws.StringSlice([]string{asgs[0].Name}),
MaxRecords: aws.Int64(1),
}).Return(testDescribeAutoScalingGroupsOutput(2, "test-instance-id", "second-test-instance-id"))

// Refresh the instance to ASG cache...
service.On("DescribeAutoScalingGroupsPages",
&autoscaling.DescribeAutoScalingGroupsInput{
AutoScalingGroupNames: aws.StringSlice([]string{provider.asgs[0].Name}),
AutoScalingGroupNames: aws.StringSlice([]string{asgs[0].Name}),
MaxRecords: aws.Int64(maxRecordsReturnedByAPI),
},
mock.AnythingOfType("func(*autoscaling.DescribeAutoScalingGroupsOutput, bool) bool"),
Expand All @@ -347,7 +318,7 @@ func TestDeleteNodes(t *testing.T) {
ProviderID: "aws:///us-east-1a/test-instance-id",
},
}
err = provider.asgs[0].DeleteNodes([]*apiv1.Node{node})
err := asgs[0].DeleteNodes([]*apiv1.Node{node})
assert.NoError(t, err)
service.AssertNumberOfCalls(t, "TerminateInstanceInAutoScalingGroup", 1)
service.AssertNumberOfCalls(t, "DescribeAutoScalingGroups", 1)
Expand All @@ -363,41 +334,6 @@ func TestGetResourceLimiter(t *testing.T) {
assert.NoError(t, err)
}

func TestId(t *testing.T) {
provider := testProvider(t, testAwsManager)
err := provider.addNodeGroup("1:5:test-asg")
assert.NoError(t, err)
assert.Equal(t, len(provider.asgs), 1)
assert.Equal(t, provider.asgs[0].Id(), "test-asg")
}

func TestDebug(t *testing.T) {
asg := Asg{
awsManager: testAwsManager,
minSize: 5,
maxSize: 55,
}
asg.Name = "test-asg"
assert.Equal(t, asg.Debug(), "test-asg (5:55)")
}

func TestBuildAsg(t *testing.T) {
_, err := buildAsgFromSpec("a", nil)
assert.Error(t, err)
_, err = buildAsgFromSpec("a:b:c", nil)
assert.Error(t, err)
_, err = buildAsgFromSpec("1:", nil)
assert.Error(t, err)
_, err = buildAsgFromSpec("1:2:", nil)
assert.Error(t, err)

asg, err := buildAsgFromSpec("111:222:test-name", nil)
assert.NoError(t, err)
assert.Equal(t, 111, asg.MinSize())
assert.Equal(t, 222, asg.MaxSize())
assert.Equal(t, "test-name", asg.Name)
}

func TestCleanup(t *testing.T) {
provider := testProvider(t, testAwsManager)
err := provider.Cleanup()
Expand Down
Loading

0 comments on commit 303795f

Please sign in to comment.