Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1798596: take into consideration all instance states for deletion #291

Merged
merged 2 commits into from
Feb 6, 2020
Merged

Bug 1798596: take into consideration all instance states for deletion #291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6398c56
Fix deletion to account for any instance state
enxebre Feb 5, 2020
4cd29b5
Remove unused client and function getRunningInstance
enxebre Feb 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions pkg/actuators/machine/actuator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we resolve the conflict this creates with #290? Within #290 once this is merged is probably easiest right? cc @alexander-demichev

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! let's hold the other ones and let this move forwards as it's an important bug fix that we want to back port to 4.3 right away.

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)
}
Expand Down
18 changes: 18 additions & 0 deletions pkg/actuators/machine/actuator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
150 changes: 0 additions & 150 deletions pkg/actuators/machine/awsclient.go
View file
Open in desktop

This file was deleted.

88 changes: 0 additions & 88 deletions pkg/actuators/machine/awsclient_test.go
View file
Open in desktop

This file was deleted.

29 changes: 0 additions & 29 deletions pkg/actuators/machine/instances_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
22 changes: 0 additions & 22 deletions pkg/actuators/machine/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down