From 5980690374c21797f58519696fef14169b4e9735 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Wed, 13 Nov 2019 20:56:00 -0800 Subject: [PATCH 1/3] pkg/actuators/machine/stubs: Add 'state' argument to stubDescribeInstancesOutput So we can create pending, terminated, etc. instances for testing. This should scale better than stub{State}InstanceDescribeInstancesOutput from openshift/cluster-api-provider-aws@c8e341f22f (Stop retieving instances by state on API call, 2019-11-14, openshift/cluster-api-provider-aws#269). --- pkg/actuators/machine/actuator_test.go | 18 +++++++++--------- pkg/actuators/machine/stubs.go | 24 ++---------------------- 2 files changed, 11 insertions(+), 31 deletions(-) diff --git a/pkg/actuators/machine/actuator_test.go b/pkg/actuators/machine/actuator_test.go index 0e65699b5e..d0816a4971 100644 --- a/pkg/actuators/machine/actuator_test.go +++ b/pkg/actuators/machine/actuator_test.go @@ -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() } @@ -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() } @@ -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{ @@ -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()), @@ -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() @@ -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) @@ -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) @@ -1217,7 +1217,7 @@ func TestGetMachineInstances(t *testing.T) { } mockAWSClient.EXPECT().DescribeInstances(request).Return( - stubTerminatedInstanceDescribeInstancesOutput(imageID, instanceID), + stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated), nil, ).Times(1) @@ -1233,7 +1233,7 @@ func TestGetMachineInstances(t *testing.T) { } mockAWSClient.EXPECT().DescribeInstances(request2).Return( - stubTerminatedInstanceDescribeInstancesOutput(imageID, instanceID), + stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated), nil, ).Times(1) diff --git a/pkg/actuators/machine/stubs.go b/pkg/actuators/machine/stubs.go index 4f0c673470..be750d644f 100644 --- a/pkg/actuators/machine/stubs.go +++ b/pkg/actuators/machine/stubs.go @@ -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{ { @@ -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()), From e4e94b1e0845317f135aaae4537f9a7e2d6d6dc2 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Wed, 13 Nov 2019 12:13:01 -0800 Subject: [PATCH 2/3] pkg/actuators/machine/utils: Client-side state filtering in getInstanceByID openshift/cluster-api-provider-aws@c8e341f22f (Stop retieving instances by state on API call, 2019-11-14, openshift/cluster-api-provider-aws#269) did part of this. This commit: * Restores the instanceStateFilter argument to getInstanceByID, to avoid having two definitions (existingInstanceStates and a local != Terminated in getExistingInstanceByID) that define what it means to exist. This will also give us logged error messages like: instance i-123 state terminated is not in running, pending, stopped, stopping, shutting-down which will make it easier to find and fix omissions in existingInstanceStates. * Adds a -running suffix to has-status-search-by-id, so it's easier to distinguish from the terminated case. * Converts to the hyphenated has-status-search-by-id-terminated, instead of mixing hyphens and spaces in a case name. * Uses inline argument for the DescribeInstances mocks, so we don't have to use request and request2. These aren't so big that inlining them makes the mock setup unreadable. * Returns an empty set of instances in the instance-listing fallback call in has-status-search-by-id-terminated (which we now require to happen after the by-ID call). The list call is filtered by state, so it wouldn't return a terminated instance. --- pkg/actuators/machine/actuator_test.go | 21 ++++++--------- pkg/actuators/machine/utils.go | 37 +++++++++++++++++--------- 2 files changed, 33 insertions(+), 25 deletions(-) diff --git a/pkg/actuators/machine/actuator_test.go b/pkg/actuators/machine/actuator_test.go index d0816a4971..2dbf847bd2 100644 --- a/pkg/actuators/machine/actuator_test.go +++ b/pkg/actuators/machine/actuator_test.go @@ -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), }, @@ -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( + }).Return( stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated), nil, ).Times(1) - request2 := &ec2.DescribeInstancesInput{ + mockAWSClient.EXPECT().DescribeInstances(&ec2.DescribeInstancesInput{ Filters: []*ec2.Filter{ { Name: awsTagFilter("Name"), @@ -1230,16 +1228,13 @@ func TestGetMachineInstances(t *testing.T) { clusterFilter(clusterID), }, - } - - mockAWSClient.EXPECT().DescribeInstances(request2).Return( - stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated), + }).Return( + &ec2.DescribeInstancesOutput{}, nil, - ).Times(1) + ).Times(1).After(first) return mockAWSClient }, - exists: false, }, } diff --git a/pkg/actuators/machine/utils.go b/pkg/actuators/machine/utils.go index 8d1298e2c9..3b095dba12 100644 --- a/pkg/actuators/machine/utils.go +++ b/pkg/actuators/machine/utils.go @@ -18,6 +18,7 @@ package machine import ( "fmt" + "strings" "github.com/golang/glog" @@ -104,20 +105,11 @@ func getExistingInstances(machine *machinev1.Machine, client awsclient.Client) ( } func getExistingInstanceByID(id string, client awsclient.Client) (*ec2.Instance, error) { - instance, err := getInstanceByID(id, client) - if err != nil { - return nil, err - } - 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) - } - } - return instance, nil + return getInstanceByID(id, client, existingInstanceStates()) } // 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") } @@ -141,7 +133,28 @@ 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] + + if len(instanceStateFilter) == 0 { + return instance, nil + } + + if instance.State == nil { + return nil, fmt.Errorf("instance %s has nil state", id) + } + + actualState := aws.StringValue(instance.State.Name) + for _, allowedState := range instanceStateFilter { + if aws.StringValue(allowedState) == actualState { + return instance, nil + } + } + + allowedStates := make([]string, 0, len(instanceStateFilter)) + for _, allowedState := range instanceStateFilter { + allowedStates = append(allowedStates, aws.StringValue(allowedState)) + } + return instance, fmt.Errorf("instance %s state %q is not in %s", id, actualState, strings.Join(allowedStates, ", ")) } // getInstances returns all instances that have a tag matching our machine name, From 382b1df2c0cb371d9ec7d36edd1c904e1c0dfee2 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Thu, 14 Nov 2019 09:35:57 -0800 Subject: [PATCH 3/3] pkg/actuators/machine/utils: Client-side state filtering in getInstances Like openshift/cluster-api-provider-aws@e4e94b1e08 (pkg/actuators/machine/utils: Client-side state filtering in getInstanceByID, 2019-11-13, openshift/cluster-api-provider-aws#268), but for getInstances. The cluster/name filtering is already enough to limit the list response to a reasonable size, so we can perform the state filtering locally in order to produce useful error messages. Also fix a bug in TestGetMachineInstances about the expected return type of getMachineInstances; it's a slice, not a single instance. Also, fix a few "Running" -> InstanceStateNameRunning bugs; the AWS string is "running", but we shouldn't care either way. --- pkg/actuators/machine/actuator_test.go | 8 +-- pkg/actuators/machine/awsclient_test.go | 2 +- pkg/actuators/machine/instances_test.go | 2 +- pkg/actuators/machine/stubs.go | 4 +- pkg/actuators/machine/utils.go | 75 ++++++++++++------------- pkg/client/fake/fake.go | 2 +- 6 files changed, 44 insertions(+), 49 deletions(-) diff --git a/pkg/actuators/machine/actuator_test.go b/pkg/actuators/machine/actuator_test.go index 2dbf847bd2..0c75396504 100644 --- a/pkg/actuators/machine/actuator_test.go +++ b/pkg/actuators/machine/actuator_test.go @@ -1229,7 +1229,7 @@ func TestGetMachineInstances(t *testing.T) { clusterFilter(clusterID), }, }).Return( - &ec2.DescribeInstancesOutput{}, + stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated), nil, ).Times(1).After(first) @@ -1263,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) } }) } diff --git a/pkg/actuators/machine/awsclient_test.go b/pkg/actuators/machine/awsclient_test.go index 64e6c08f08..6d393b37c4 100644 --- a/pkg/actuators/machine/awsclient_test.go +++ b/pkg/actuators/machine/awsclient_test.go @@ -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(""), diff --git a/pkg/actuators/machine/instances_test.go b/pkg/actuators/machine/instances_test.go index e659706c1e..f784cf0332 100644 --- a/pkg/actuators/machine/instances_test.go +++ b/pkg/actuators/machine/instances_test.go @@ -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()), }, diff --git a/pkg/actuators/machine/stubs.go b/pkg/actuators/machine/stubs.go index be750d644f..f4e28396ca 100644 --- a/pkg/actuators/machine/stubs.go +++ b/pkg/actuators/machine/stubs.go @@ -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()), @@ -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()), diff --git a/pkg/actuators/machine/utils.go b/pkg/actuators/machine/utils.go index 3b095dba12..5d1efc03be 100644 --- a/pkg/actuators/machine/utils.go +++ b/pkg/actuators/machine/utils.go @@ -88,24 +88,38 @@ 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) + } + + if len(instanceStateFilter) == 0 { + return nil + } + + actualState := aws.StringValue(instance.State.Name) + for _, allowedState := range instanceStateFilter { + if aws.StringValue(allowedState) == actualState { + return nil } } - return existingInstances, nil -} -func getExistingInstanceByID(id string, client awsclient.Client) (*ec2.Instance, error) { - return getInstanceByID(id, client, existingInstanceStates()) + 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. @@ -135,26 +149,7 @@ func getInstanceByID(id string, client awsclient.Client, instanceStateFilter []* instance := reservation.Instances[0] - if len(instanceStateFilter) == 0 { - return instance, nil - } - - if instance.State == nil { - return nil, fmt.Errorf("instance %s has nil state", id) - } - - actualState := aws.StringValue(instance.State.Name) - for _, allowedState := range instanceStateFilter { - if aws.StringValue(allowedState) == actualState { - return instance, nil - } - } - - allowedStates := make([]string, 0, len(instanceStateFilter)) - for _, allowedState := range instanceStateFilter { - allowedStates = append(allowedStates, aws.StringValue(allowedState)) - } - return instance, fmt.Errorf("instance %s state %q is not in %s", id, actualState, strings.Join(allowedStates, ", ")) + return instance, instanceHasAllowedState(instance, instanceStateFilter) } // getInstances returns all instances that have a tag matching our machine name, @@ -172,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, - }) - } // Query instances with our machine's name, and in running/pending state. request := &ec2.DescribeInstancesInput{ @@ -190,9 +179,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) + } } } diff --git a/pkg/client/fake/fake.go b/pkg/client/fake/fake.go index bb9c6ce5ab..d4377fd9c6 100644 --- a/pkg/client/fake/fake.go +++ b/pkg/client/fake/fake.go @@ -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()),