From 6398c56fa4b1f86295bf09da0635b36d341d2168 Mon Sep 17 00:00:00 2001 From: Enxebre Date: Wed, 5 Feb 2020 16:36:12 +0100 Subject: [PATCH 1/2] Fix deletion to account for any instance state --- pkg/actuators/machine/actuator.go | 11 +++++++---- pkg/actuators/machine/actuator_test.go | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/pkg/actuators/machine/actuator.go b/pkg/actuators/machine/actuator.go index 1dab50fc95..c3c1aa5deb 100644 --- a/pkg/actuators/machine/actuator.go +++ b/pkg/actuators/machine/actuator.go @@ -347,17 +347,20 @@ func (a *Actuator) DeleteMachine(machine *machinev1.Machine) error { return a.handleMachineError(machine, err, deleteEventAction) } - instances, err := getRunningInstances(machine, client) + // Get all instances not terminated. + existingInstances, err := a.getMachineInstances(machine) if err != nil { - glog.Errorf("%s: error getting running instances: %v", machine.Name, err) + glog.Errorf("%s: error getting existing instances: %v", machine.Name, err) return err } - if len(instances) == 0 { + existingLen := len(existingInstances) + glog.Infof("%s: found %d existing instances for machine", machine.Name, existingLen) + if existingLen == 0 { glog.Warningf("%s: no instances found to delete for machine", machine.Name) return nil } - terminatingInstances, err := terminateInstances(client, instances) + terminatingInstances, err := terminateInstances(client, existingInstances) if err != nil { return a.handleMachineError(machine, mapierrors.DeleteMachine(err.Error()), noEventAction) } diff --git a/pkg/actuators/machine/actuator_test.go b/pkg/actuators/machine/actuator_test.go index 6f8b0d45fa..1f268e2bcf 100644 --- a/pkg/actuators/machine/actuator_test.go +++ b/pkg/actuators/machine/actuator_test.go @@ -139,6 +139,24 @@ func TestMachineEvents(t *testing.T) { }, event: "Normal Deleted Deleted machine aws-actuator-testing-machine", }, + { + name: "Delete machine event succeed with pending instances", + machine: machine, + describeInstancesOutput: stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c", ec2.InstanceStateNamePending), + operation: func(actuator *Actuator, machine *machinev1.Machine) { + actuator.DeleteMachine(machine) + }, + event: "Normal Deleted Deleted machine aws-actuator-testing-machine", + }, + { + name: "Delete machine event succeed with stopped instances", + machine: machine, + describeInstancesOutput: stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c", ec2.InstanceStateNameStopped), + operation: func(actuator *Actuator, machine *machinev1.Machine) { + actuator.DeleteMachine(machine) + }, + event: "Normal Deleted Deleted machine aws-actuator-testing-machine", + }, } for _, tc := range cases { From 4cd29b5430fae07de0c9e65618d0ffea8791120a Mon Sep 17 00:00:00 2001 From: Enxebre Date: Wed, 5 Feb 2020 16:36:49 +0100 Subject: [PATCH 2/2] Remove unused client and function getRunningInstance --- pkg/actuators/machine/awsclient.go | 150 ------------------------ pkg/actuators/machine/awsclient_test.go | 88 -------------- pkg/actuators/machine/instances_test.go | 29 ----- pkg/actuators/machine/utils.go | 22 ---- 4 files changed, 289 deletions(-) delete mode 100644 pkg/actuators/machine/awsclient.go delete mode 100644 pkg/actuators/machine/awsclient_test.go diff --git a/pkg/actuators/machine/awsclient.go b/pkg/actuators/machine/awsclient.go deleted file mode 100644 index 96547d914b..0000000000 --- a/pkg/actuators/machine/awsclient.go +++ /dev/null @@ -1,150 +0,0 @@ -package machine - -import ( - "fmt" - - machinev1beta1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1" - awsclient "sigs.k8s.io/cluster-api-provider-aws/pkg/client" -) - -// AwsClientWrapper implements CloudProviderClient for aws e2e framework -type AwsClientWrapper struct { - client awsclient.Client -} - -// NewAwsClientWrapper returns aws client implementation -func NewAwsClientWrapper(client awsclient.Client) *AwsClientWrapper { - return &AwsClientWrapper{client: client} -} - -// GetRunningInstances gets running instances (of a given cloud provider) managed by the machine object -func (client *AwsClientWrapper) GetRunningInstances(machine *machinev1beta1.Machine) ([]interface{}, error) { - runningInstances, err := getRunningInstances(machine, client.client) - if err != nil { - return nil, err - } - - var instances []interface{} - for _, instance := range runningInstances { - instances = append(instances, instance) - } - - return instances, nil -} - -// GetPublicDNSName gets running instance public DNS name -func (client *AwsClientWrapper) GetPublicDNSName(machine *machinev1beta1.Machine) (string, error) { - instance, err := getRunningInstance(machine, client.client) - if err != nil { - return "", err - } - - if *instance.PublicDnsName == "" { - return "", fmt.Errorf("machine instance public DNS name not set") - } - - return *instance.PublicDnsName, nil -} - -// GetPrivateIP gets private IP -func (client *AwsClientWrapper) GetPrivateIP(machine *machinev1beta1.Machine) (string, error) { - instance, err := getRunningInstance(machine, client.client) - if err != nil { - return "", err - } - - if *instance.PrivateIpAddress == "" { - return "", fmt.Errorf("machine instance public DNS name not set") - } - - return *instance.PrivateIpAddress, nil -} - -// GetSecurityGroups gets security groups -func (client *AwsClientWrapper) GetSecurityGroups(machine *machinev1beta1.Machine) ([]string, error) { - instance, err := getRunningInstance(machine, client.client) - if err != nil { - return nil, err - } - var groups []string - for _, groupIdentifier := range instance.SecurityGroups { - if *groupIdentifier.GroupName != "" { - groups = append(groups, *groupIdentifier.GroupName) - } - } - return groups, nil -} - -// GetIAMRole gets IAM role -func (client *AwsClientWrapper) GetIAMRole(machine *machinev1beta1.Machine) (string, error) { - instance, err := getRunningInstance(machine, client.client) - if err != nil { - return "", err - } - if instance.IamInstanceProfile == nil { - return "", err - } - return *instance.IamInstanceProfile.Id, nil -} - -// GetTags gets tags -func (client *AwsClientWrapper) GetTags(machine *machinev1beta1.Machine) (map[string]string, error) { - instance, err := getRunningInstance(machine, client.client) - if err != nil { - return nil, err - } - tags := make(map[string]string, len(instance.Tags)) - for _, tag := range instance.Tags { - tags[*tag.Key] = *tag.Value - } - return tags, nil -} - -// GetSubnet gets subnet -func (client *AwsClientWrapper) GetSubnet(machine *machinev1beta1.Machine) (string, error) { - instance, err := getRunningInstance(machine, client.client) - if err != nil { - return "", err - } - if instance.SubnetId == nil { - return "", err - } - return *instance.SubnetId, nil -} - -// GetAvailabilityZone gets availability zone -func (client *AwsClientWrapper) GetAvailabilityZone(machine *machinev1beta1.Machine) (string, error) { - instance, err := getRunningInstance(machine, client.client) - if err != nil { - return "", err - } - if instance.Placement == nil { - return "", err - } - return *instance.Placement.AvailabilityZone, nil -} - -// GetVolumes gets volumes attached to instance -func (client *AwsClientWrapper) GetVolumes(machine *machinev1beta1.Machine) (map[string]map[string]interface{}, error) { - instance, err := getRunningInstance(machine, client.client) - if err != nil { - return nil, err - } - if instance.BlockDeviceMappings == nil { - return nil, err - } - volumes := make(map[string]map[string]interface{}, len(instance.BlockDeviceMappings)) - for _, blockDeviceMapping := range instance.BlockDeviceMappings { - volume, err := getVolume(client.client, *blockDeviceMapping.Ebs.VolumeId) - if err != nil { - return volumes, err - } - volumes[*blockDeviceMapping.DeviceName] = map[string]interface{}{ - "id": *volume.VolumeId, - "iops": *volume.Iops, - "size": *volume.Size, - "type": *volume.VolumeType, - } - } - return volumes, nil -} diff --git a/pkg/actuators/machine/awsclient_test.go b/pkg/actuators/machine/awsclient_test.go deleted file mode 100644 index 6d393b37c4..0000000000 --- a/pkg/actuators/machine/awsclient_test.go +++ /dev/null @@ -1,88 +0,0 @@ -package machine - -import ( - "fmt" - "testing" - "time" - - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/ec2" - "github.com/golang/mock/gomock" - mockaws "sigs.k8s.io/cluster-api-provider-aws/pkg/client/mock" -) - -func TestAwsClient(t *testing.T) { - machine, err := stubMachine() - if err != nil { - t.Fatalf("Unable to build test machine manifest: %v", err) - } - - cases := []struct { - name string - output *ec2.DescribeInstancesOutput - err error - }{ - { - name: "valid data", - output: &ec2.DescribeInstancesOutput{ - Reservations: []*ec2.Reservation{ - { - Instances: []*ec2.Instance{ - stubInstance("ami-a9acbbd6", "i-02fcb933c5da7085c"), - }, - }, - }, - }, - }, - { - name: "valid data with more nil fields", - output: &ec2.DescribeInstancesOutput{ - Reservations: []*ec2.Reservation{ - { - Instances: []*ec2.Instance{ - { - ImageId: aws.String("ami-a9acbbd6"), - InstanceId: aws.String("i-02fcb933c5da7085c"), - State: &ec2.InstanceState{ - Name: aws.String(ec2.InstanceStateNameRunning), - }, - LaunchTime: aws.Time(time.Now()), - PublicDnsName: aws.String(""), - PrivateIpAddress: aws.String(""), - Tags: []*ec2.Tag{ - { - Key: aws.String("key"), - Value: aws.String("value"), - }, - }, - }, - }, - }, - }, - }, - }, - { - name: "with error", - err: fmt.Errorf("error"), - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - mockCtrl := gomock.NewController(t) - mockAWSClient := mockaws.NewMockClient(mockCtrl) - - mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(tc.output, tc.err).AnyTimes() - - acw := NewAwsClientWrapper(mockAWSClient) - acw.GetRunningInstances(machine) - acw.GetPublicDNSName(machine) - acw.GetPrivateIP(machine) - acw.GetSecurityGroups(machine) - acw.GetIAMRole(machine) - acw.GetTags(machine) - acw.GetSubnet(machine) - acw.GetAvailabilityZone(machine) - }) - } -} diff --git a/pkg/actuators/machine/instances_test.go b/pkg/actuators/machine/instances_test.go index f784cf0332..2554c87908 100644 --- a/pkg/actuators/machine/instances_test.go +++ b/pkg/actuators/machine/instances_test.go @@ -221,35 +221,6 @@ func TestRemoveStoppedMachine(t *testing.T) { } } -func TestRunningInstance(t *testing.T) { - machine, err := stubMachine() - if err != nil { - t.Fatalf("Unable to build test machine manifest: %v", err) - } - - mockCtrl := gomock.NewController(t) - mockAWSClient := mockaws.NewMockClient(mockCtrl) - - // Error describing instances - mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(&ec2.DescribeInstancesOutput{ - Reservations: []*ec2.Reservation{ - { - Instances: []*ec2.Instance{ - { - ImageId: aws.String("ami-a9acbbd6"), - InstanceId: aws.String("i-02fcb933c5da7085c"), - State: &ec2.InstanceState{ - Name: aws.String(ec2.InstanceStateNameRunning), - }, - LaunchTime: aws.Time(time.Now()), - }, - }, - }, - }, - }, nil).AnyTimes() - getRunningInstance(machine, mockAWSClient) -} - func TestLaunchInstance(t *testing.T) { machine, err := stubMachine() if err != nil { diff --git a/pkg/actuators/machine/utils.go b/pkg/actuators/machine/utils.go index 37a37ab51f..a78b7334c6 100644 --- a/pkg/actuators/machine/utils.go +++ b/pkg/actuators/machine/utils.go @@ -47,28 +47,6 @@ func existingInstanceStates() []*string { } } -// getRunningInstance returns the AWS instance for a given machine. If multiple instances match our machine, -// the most recently launched will be returned. If no instance exists, an error will be returned. -func getRunningInstance(machine *machinev1.Machine, client awsclient.Client) (*ec2.Instance, error) { - instances, err := getRunningInstances(machine, client) - if err != nil { - return nil, err - } - if len(instances) == 0 { - return nil, fmt.Errorf("no instance found for machine: %s", machine.Name) - } - - sortInstances(instances) - return instances[0], nil -} - -// getRunningInstances returns all running instances that have a tag matching our machine name, -// and cluster ID. -func getRunningInstances(machine *machinev1.Machine, client awsclient.Client) ([]*ec2.Instance, error) { - runningInstanceStateFilter := []*string{aws.String(ec2.InstanceStateNameRunning)} - return getInstances(machine, client, runningInstanceStateFilter) -} - // getRunningFromInstances returns all running instances from a list of instances. func getRunningFromInstances(instances []*ec2.Instance) []*ec2.Instance { var runningInstances []*ec2.Instance