Skip to content

Commit

Permalink
Merge pull request #268 from wking/getInstanceByID-client-side-state-…
Browse files Browse the repository at this point in the history 
…filter

pkg/actuators/machine/utils: Client-side state filtering in getInstanceByID and getInstances
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot authored Nov 14, 2019
2 parents ac01d4f + 382b1df commit 0ee2017
Show file tree
Hide file tree
Showing 6 changed files with 63 additions and 80 deletions.
43 changes: 19 additions & 24 deletions pkg/actuators/machine/actuator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ func TestMachineEvents(t *testing.T) {

mockAWSClient.EXPECT().RunInstances(gomock.Any()).Return(stubReservation("ami-a9acbbd6", "i-02fcb933c5da7085c"), tc.runInstancesErr).AnyTimes()
if tc.describeInstancesOutput == nil {
mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c"), tc.describeInstancesErr).AnyTimes()
mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c", ec2.InstanceStateNameRunning), tc.describeInstancesErr).AnyTimes()
} else {
mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(tc.describeInstancesOutput, tc.describeInstancesErr).AnyTimes()
}
Expand Down Expand Up @@ -546,7 +546,7 @@ func TestActuator(t *testing.T) {
mockAWSClient.EXPECT().RunInstances(gomock.Any()).Return(stubReservation("ami-a9acbbd6", "i-02fcb933c5da7085c"), tc.runInstancesErr).AnyTimes()

if tc.describeInstancesOutput == nil {
mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c"), tc.describeInstancesErr).AnyTimes()
mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c", ec2.InstanceStateNameRunning), tc.describeInstancesErr).AnyTimes()
} else {
mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(tc.describeInstancesOutput, tc.describeInstancesErr).AnyTimes()
}
Expand Down Expand Up @@ -656,7 +656,7 @@ func TestAvailabilityZone(t *testing.T) {
ImageId: aws.String("ami-a9acbbd6"),
InstanceId: aws.String("i-02fcb933c5da7085c"),
State: &ec2.InstanceState{
Name: aws.String("Running"),
Name: aws.String(ec2.InstanceStateNameRunning),
},
LaunchTime: aws.Time(time.Now()),
Placement: &ec2.Placement{
Expand All @@ -675,7 +675,7 @@ func TestAvailabilityZone(t *testing.T) {
ImageId: aws.String("ami-a9acbbd6"),
InstanceId: aws.String("i-02fcb933c5da7085c"),
State: &ec2.InstanceState{
Name: aws.String("Running"),
Name: aws.String(ec2.InstanceStateNameRunning),
Code: aws.Int64(16),
},
LaunchTime: aws.Time(time.Now()),
Expand Down Expand Up @@ -804,7 +804,7 @@ func TestCreate(t *testing.T) {
mockAWSClient.EXPECT().DescribeSecurityGroups(gomock.Any()).Return(nil, fmt.Errorf("describeSecurityGroups error")).AnyTimes()
mockAWSClient.EXPECT().DescribeAvailabilityZones(gomock.Any()).Return(nil, fmt.Errorf("describeAvailabilityZones error")).AnyTimes()
mockAWSClient.EXPECT().DescribeImages(gomock.Any()).Return(nil, fmt.Errorf("describeImages error")).AnyTimes()
mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c"), nil).AnyTimes()
mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c", ec2.InstanceStateNameRunning), nil).AnyTimes()
mockAWSClient.EXPECT().TerminateInstances(gomock.Any()).Return(&ec2.TerminateInstancesOutput{}, nil).AnyTimes()
mockAWSClient.EXPECT().RunInstances(gomock.Any()).Return(stubReservation("ami-a9acbbd6", "i-02fcb933c5da7085c"), nil).AnyTimes()
mockAWSClient.EXPECT().RegisterInstancesWithLoadBalancer(gomock.Any()).Return(nil, nil).AnyTimes()
Expand Down Expand Up @@ -1175,7 +1175,7 @@ func TestGetMachineInstances(t *testing.T) {
}

mockAWSClient.EXPECT().DescribeInstances(request).Return(
stubDescribeInstancesOutput(imageID, instanceID),
stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameRunning),
nil,
).Times(1)

Expand All @@ -1184,7 +1184,7 @@ func TestGetMachineInstances(t *testing.T) {
exists: true,
},
{
testcase: "has-status-search-by-id",
testcase: "has-status-search-by-id-running",
providerStatus: providerconfigv1.AWSMachineProviderStatus{
InstanceID: aws.String(instanceID),
},
Expand All @@ -1196,7 +1196,7 @@ func TestGetMachineInstances(t *testing.T) {
}

mockAWSClient.EXPECT().DescribeInstances(request).Return(
stubDescribeInstancesOutput(imageID, instanceID),
stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameRunning),
nil,
).Times(1)

Expand All @@ -1205,23 +1205,21 @@ func TestGetMachineInstances(t *testing.T) {
exists: true,
},
{
testcase: "has-status-search-by-id and machine is terminated",
testcase: "has-status-search-by-id-terminated",
providerStatus: providerconfigv1.AWSMachineProviderStatus{
InstanceID: aws.String(instanceID),
},
awsClientFunc: func(ctrl *gomock.Controller) awsclient.Client {
mockAWSClient := mockaws.NewMockClient(ctrl)

request := &ec2.DescribeInstancesInput{
first := mockAWSClient.EXPECT().DescribeInstances(&ec2.DescribeInstancesInput{
InstanceIds: aws.StringSlice([]string{instanceID}),
}

mockAWSClient.EXPECT().DescribeInstances(request).Return(
stubTerminatedInstanceDescribeInstancesOutput(imageID, instanceID),
}).Return(
stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated),
nil,
).Times(1)

request2 := &ec2.DescribeInstancesInput{
mockAWSClient.EXPECT().DescribeInstances(&ec2.DescribeInstancesInput{
Filters: []*ec2.Filter{
{
Name: awsTagFilter("Name"),
Expand All @@ -1230,16 +1228,13 @@ func TestGetMachineInstances(t *testing.T) {

clusterFilter(clusterID),
},
}

mockAWSClient.EXPECT().DescribeInstances(request2).Return(
stubTerminatedInstanceDescribeInstancesOutput(imageID, instanceID),
}).Return(
stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated),
nil,
).Times(1)
).Times(1).After(first)

return mockAWSClient
},
exists: false,
},
}

Expand Down Expand Up @@ -1268,12 +1263,12 @@ func TestGetMachineInstances(t *testing.T) {
t.Errorf("Error creating Actuator: %v", err)
}

instance, err := actuator.getMachineInstances(nil, machineCopy)
instances, err := actuator.getMachineInstances(nil, machineCopy)
if err != nil {
t.Errorf("Unexpected error from getMachineInstances: %v", err)
}
if tc.exists != (instance != nil) {
t.Errorf("Expected instance exists: %t, got instance: %v", tc.exists, instance)
if tc.exists != (len(instances) > 0) {
t.Errorf("Expected instance exists: %t, got instances: %v", tc.exists, instances)
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/actuators/machine/awsclient_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func TestAwsClient(t *testing.T) {
ImageId: aws.String("ami-a9acbbd6"),
InstanceId: aws.String("i-02fcb933c5da7085c"),
State: &ec2.InstanceState{
Name: aws.String("Running"),
Name: aws.String(ec2.InstanceStateNameRunning),
},
LaunchTime: aws.Time(time.Now()),
PublicDnsName: aws.String(""),
Expand Down
2 changes: 1 addition & 1 deletion pkg/actuators/machine/instances_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ func TestRunningInstance(t *testing.T) {
ImageId: aws.String("ami-a9acbbd6"),
InstanceId: aws.String("i-02fcb933c5da7085c"),
State: &ec2.InstanceState{
Name: aws.String("Running"),
Name: aws.String(ec2.InstanceStateNameRunning),
},
LaunchTime: aws.Time(time.Now()),
},
Expand Down
28 changes: 4 additions & 24 deletions pkg/actuators/machine/stubs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ func stubInstance(imageID, instanceID string) *ec2.Instance {
ImageId: aws.String(imageID),
InstanceId: aws.String(instanceID),
State: &ec2.InstanceState{
Name: aws.String("Running"),
Name: aws.String(ec2.InstanceStateNameRunning),
Code: aws.Int64(16),
},
LaunchTime: aws.Time(time.Now()),
Expand Down Expand Up @@ -261,7 +261,7 @@ func stubReservation(imageID, instanceID string) *ec2.Reservation {
ImageId: aws.String(imageID),
InstanceId: aws.String(instanceID),
State: &ec2.InstanceState{
Name: aws.String("Running"),
Name: aws.String(ec2.InstanceStateNameRunning),
Code: aws.Int64(16),
},
LaunchTime: aws.Time(time.Now()),
Expand All @@ -273,7 +273,7 @@ func stubReservation(imageID, instanceID string) *ec2.Reservation {
}
}

func stubDescribeInstancesOutput(imageID, instanceID string) *ec2.DescribeInstancesOutput {
func stubDescribeInstancesOutput(imageID, instanceID string, state string) *ec2.DescribeInstancesOutput {
return &ec2.DescribeInstancesOutput{
Reservations: []*ec2.Reservation{
{
Expand All @@ -282,27 +282,7 @@ func stubDescribeInstancesOutput(imageID, instanceID string) *ec2.DescribeInstan
ImageId: aws.String(imageID),
InstanceId: aws.String(instanceID),
State: &ec2.InstanceState{
Name: aws.String("Running"),
Code: aws.Int64(16),
},
LaunchTime: aws.Time(time.Now()),
},
},
},
},
}
}

func stubTerminatedInstanceDescribeInstancesOutput(imageID, instanceID string) *ec2.DescribeInstancesOutput {
return &ec2.DescribeInstancesOutput{
Reservations: []*ec2.Reservation{
{
Instances: []*ec2.Instance{
{
ImageId: aws.String(imageID),
InstanceId: aws.String(instanceID),
State: &ec2.InstanceState{
Name: aws.String(ec2.InstanceStateNameTerminated),
Name: aws.String(state),
Code: aws.Int64(16),
},
LaunchTime: aws.Time(time.Now()),
Expand Down
66 changes: 37 additions & 29 deletions pkg/actuators/machine/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package machine

import (
"fmt"
"strings"

"github.com/golang/glog"

Expand Down Expand Up @@ -87,37 +88,42 @@ func getStoppedInstances(machine *machinev1.Machine, client awsclient.Client) ([

// getExistingInstances returns all instances not terminated
func getExistingInstances(machine *machinev1.Machine, client awsclient.Client) ([]*ec2.Instance, error) {
instances, err := getInstances(machine, client, nil)
if err != nil {
return nil, err
return getInstances(machine, client, existingInstanceStates())
}

func getExistingInstanceByID(id string, client awsclient.Client) (*ec2.Instance, error) {
return getInstanceByID(id, client, existingInstanceStates())
}

func instanceHasAllowedState(instance *ec2.Instance, instanceStateFilter []*string) error {
if instance.InstanceId == nil {
return fmt.Errorf("instance has nil ID")
}

var existingInstances []*ec2.Instance
for key := range instances {
if (instances[key].State) != nil {
if aws.StringValue(instances[key].State.Name) != ec2.InstanceStateNameTerminated {
existingInstances = append(existingInstances, instances[key])
}
}
if instance.State == nil {
return fmt.Errorf("instance %s has nil state", *instance.InstanceId)
}
return existingInstances, nil
}

func getExistingInstanceByID(id string, client awsclient.Client) (*ec2.Instance, error) {
instance, err := getInstanceByID(id, client)
if err != nil {
return nil, err
if len(instanceStateFilter) == 0 {
return nil
}
if instance.State != nil {
if aws.StringValue(instance.State.Name) == ec2.InstanceStateNameTerminated {
return nil, fmt.Errorf("failed to getExistingInstanceByID for instance-id %s, instance is terminated", id)

actualState := aws.StringValue(instance.State.Name)
for _, allowedState := range instanceStateFilter {
if aws.StringValue(allowedState) == actualState {
return nil
}
}
return instance, nil

allowedStates := make([]string, 0, len(instanceStateFilter))
for _, allowedState := range instanceStateFilter {
allowedStates = append(allowedStates, aws.StringValue(allowedState))
}
return fmt.Errorf("instance %s state %q is not in %s", *instance.InstanceId, actualState, strings.Join(allowedStates, ", "))
}

// getInstanceByID returns the instance with the given ID if it exists.
func getInstanceByID(id string, client awsclient.Client) (*ec2.Instance, error) {
func getInstanceByID(id string, client awsclient.Client, instanceStateFilter []*string) (*ec2.Instance, error) {
if id == "" {
return nil, fmt.Errorf("instance-id not specified")
}
Expand All @@ -141,7 +147,9 @@ func getInstanceByID(id string, client awsclient.Client) (*ec2.Instance, error)
return nil, fmt.Errorf("found %d instances for instance-id %s", len(reservation.Instances), id)
}

return reservation.Instances[0], nil
instance := reservation.Instances[0]

return instance, instanceHasAllowedState(instance, instanceStateFilter)
}

// getInstances returns all instances that have a tag matching our machine name,
Expand All @@ -159,12 +167,6 @@ func getInstances(machine *machinev1.Machine, client awsclient.Client, instanceS
},
clusterFilter(clusterID),
}
if instanceStateFilter != nil {
requestFilters = append(requestFilters, &ec2.Filter{
Name: aws.String("instance-state-name"),
Values: instanceStateFilter,
})
}

request := &ec2.DescribeInstancesInput{
Filters: requestFilters,
Expand All @@ -176,9 +178,15 @@ func getInstances(machine *machinev1.Machine, client awsclient.Client, instanceS
}

instances := make([]*ec2.Instance, 0, len(result.Reservations))

for _, reservation := range result.Reservations {
for _, instance := range reservation.Instances {
instances = append(instances, instance)
err := instanceHasAllowedState(instance, instanceStateFilter)
if err != nil {
glog.Errorf("Excluding instance matching %s: %v", machine.Name, err)
} else {
instances = append(instances, instance)
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/client/fake/fake.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func (c *awsClient) DescribeInstances(input *ec2.DescribeInstancesInput) (*ec2.D
ImageId: aws.String("ami-a9acbbd6"),
InstanceId: aws.String("i-02fcb933c5da7085c"),
State: &ec2.InstanceState{
Name: aws.String("Running"),
Name: aws.String(ec2.InstanceStateNameRunning),
Code: aws.Int64(16),
},
LaunchTime: aws.Time(time.Now()),
Expand Down

0 comments on commit 0ee2017

Please sign in to comment.