Merge pull request #83 from racheljpg/addTerminatedState

OCPBUGS-15255: Add terminated as a handled state so terminated instances don't get stuck

Author: openshift-merge-bot[bot]

pkg/actuators/machine/reconciler.go

@@ -42,8 +42,16 @@ func (r *Reconciler) create() error {
 
 	if instances, err := r.getMachineInstances(); err == nil && len(instances) > 0 {
 		klog.Infof("%s: found existing instance %s for machine", r.machine.Name, aws.StringValue(instances[0].InstanceId))
+
+		if r.checkIfInstanceTerminated(instances[0]) {
+			// The instance exists, but is in a terminated state.
+			// This means the instance was terminated prior to MAPI realizing it existed.
+			// We should fail the machine in this scenario else we end up in a forever loop.
+			return machinecontroller.InvalidMachineConfiguration("Instance %s is in a terminated state", aws.StringValue(instances[0].InstanceId))
+		}
+
 		// If we got here, then Exists failed to find the instance, and we were asked to create a new instance.
-		// The instance already exists, so requeue and start the reconcile again, Exists should pass now.
+		// The instance already exists, and isn't terminated, so requeue and start the reconcile again, Exists should pass now.
 		// Don't bother updating the status, Update will configure everything on the next reconcile.
 		return fmt.Errorf("%s: Possible eventual-consistency discrepancy; returning an error to requeue", r.machine.Name)
 	}
@@ -122,7 +130,7 @@ func (r *Reconciler) create() error {
 func (r *Reconciler) delete() error {
 	klog.Infof("%s: deleting machine", r.machine.Name)
 
-	// Get all instances not terminated.
+	// Get all instances (including terminated, so that we can handle a terminated state)
 	existingInstances, err := r.getMachineInstances()
 	if err != nil {
 		metrics.RegisterFailedInstanceDelete(&metrics.MachineLabels{
@@ -141,23 +149,29 @@ func (r *Reconciler) delete() error {
 		return nil
 	}
 
-	if err = r.removeFromLoadBalancers(existingInstances); err != nil {
-		metrics.RegisterFailedInstanceDelete(&metrics.MachineLabels{
-			Name:      r.machine.Name,
-			Namespace: r.machine.Namespace,
-			Reason:    "failed to remove instance from load balancers",
-		})
-		return fmt.Errorf("failed to remove instance from load balancers: %w", err)
-	}
+	isTerminated := r.checkIfInstanceTerminated(existingInstances[0])
+	var terminatingInstances []*ec2.InstanceStateChange
+
+	if !isTerminated {
+		if err := r.removeFromLoadBalancers(existingInstances); err != nil {
+			metrics.RegisterFailedInstanceDelete(&metrics.MachineLabels{
+				Name:      r.machine.Name,
+				Namespace: r.machine.Namespace,
+				Reason:    "failed to remove instance from load balancers",
+			})
+			return fmt.Errorf("failed to remove instance from load balancers: %w", err)
+		}
+
+		terminatingInstances, err = terminateInstances(r.awsClient, existingInstances)
+		if err != nil {
+			metrics.RegisterFailedInstanceDelete(&metrics.MachineLabels{
+				Name:      r.machine.Name,
+				Namespace: r.machine.Namespace,
+				Reason:    "failed to delete instances",
+			})
+			return fmt.Errorf("failed to delete instaces: %w", err)
+		}
 
-	terminatingInstances, err := terminateInstances(r.awsClient, existingInstances)
-	if err != nil {
-		metrics.RegisterFailedInstanceDelete(&metrics.MachineLabels{
-			Name:      r.machine.Name,
-			Namespace: r.machine.Namespace,
-			Reason:    "failed to delete instances",
-		})
-		return fmt.Errorf("failed to delete instaces: %w", err)
 	}
 
 	if r.machine.Annotations == nil {
@@ -168,6 +182,8 @@ func (r *Reconciler) delete() error {
 		if terminatingInstances[0] != nil && terminatingInstances[0].CurrentState != nil && terminatingInstances[0].CurrentState.Name != nil {
 			r.machine.Annotations[machinecontroller.MachineInstanceStateAnnotationName] = aws.StringValue(terminatingInstances[0].CurrentState.Name)
 		}
+	} else if isTerminated {
+		r.machine.Annotations[machinecontroller.MachineInstanceStateAnnotationName] = ec2.InstanceStateNameTerminated
 	}
 
 	klog.Infof("Deleted machine %v", r.machine.Name)
@@ -183,7 +199,7 @@ func (r *Reconciler) update() error {
 		return fmt.Errorf("%v: failed validating machine provider spec: %v", r.machine.GetName(), err)
 	}
 
-	// Get all instances not terminated.
+	// Get all instances
 	existingInstances, err := r.getMachineInstances()
 	if err != nil {
 		metrics.RegisterFailedInstanceUpdate(&metrics.MachineLabels{
@@ -297,6 +313,13 @@ func (r *Reconciler) exists() (bool, error) {
 		return false, nil
 	}
 
+	if r.checkIfInstanceTerminated(existingInstances[0]) {
+		// The instance exists, but is in a terminated state.
+		// For the purposes of exists, this machine should not be considered to exist.
+		// If the machine is already provisioned, it will go failed.
+		return false, nil
+	}
+
 	return existingInstances[0] != nil, err
 }
 
@@ -472,6 +495,18 @@ func (r *Reconciler) requeueIfInstancePending(instance *ec2.Instance) error {
 	return nil
 }
 
+// Check if an instance is terminated so that we can handle it appropriately
+func (r *Reconciler) checkIfInstanceTerminated(instance *ec2.Instance) bool {
+
+	if instance != nil && instance.State != nil &&
+		aws.StringValue(instance.State.Name) == ec2.InstanceStateNameTerminated {
+		klog.Infof("%s: Instance state terminated", r.machine.Name)
+		return true
+	}
+
+	return false
+}
+
 func (r *Reconciler) getMachineInstances() ([]*ec2.Instance, error) {
 	// If there is a non-empty instance ID, search using that, otherwise
 	// fallback to filtering based on tags.

pkg/actuators/machine/reconciler_test.go

@@ -636,6 +636,25 @@ func TestExists(t *testing.T) {
 				return mockAWSClient
 			},
 		},
+		{
+			name: "Successfully find terminated instance",
+			machine: func() *machinev1beta1.Machine {
+				machine, err := stubMachine()
+				if err != nil {
+					t.Fatalf("unable to build stub machine: %v", err)
+				}
+
+				return machine
+			},
+			existsResult:  false,
+			expectedError: nil,
+			awsClient: func(ctrl *gomock.Controller) awsclient.Client {
+				mockCtrl := gomock.NewController(t)
+				mockAWSClient := mockaws.NewMockClient(mockCtrl)
+				mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("test-ami", "test-id", ec2.InstanceStateNameTerminated, "1.1.1.1"), nil).AnyTimes()
+				return mockAWSClient
+			},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -900,6 +919,24 @@ func TestDelete(t *testing.T) {
 				return mockAWSClient
 			},
 		},
+		{
+			name: "Does not try to delete an instance if it's already in a terminated state",
+			machine: func() *machinev1beta1.Machine {
+				machine, err := stubMachine()
+				if err != nil {
+					t.Fatalf("unable to build stub machine: %v", err)
+				}
+
+				return machine
+			},
+			expectedError: nil,
+			awsClient: func(ctrl *gomock.Controller) awsclient.Client {
+				mockCtrl := gomock.NewController(t)
+				mockAWSClient := mockaws.NewMockClient(mockCtrl)
+				mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("test-ami", "test-id", ec2.InstanceStateNameTerminated, "1.1.1.1"), nil).Times(1)
+				return mockAWSClient
+			},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -1005,29 +1042,16 @@ func TestGetMachineInstances(t *testing.T) {
 			awsClientFunc: func(ctrl *gomock.Controller) awsclient.Client {
 				mockAWSClient := mockaws.NewMockClient(ctrl)
 
-				first := mockAWSClient.EXPECT().DescribeInstances(&ec2.DescribeInstancesInput{
+				mockAWSClient.EXPECT().DescribeInstances(&ec2.DescribeInstancesInput{
 					InstanceIds: aws.StringSlice([]string{instanceID}),
 				}).Return(
 					stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated, "192.168.0.10"),
 					nil,
 				).Times(1)
 
-				mockAWSClient.EXPECT().DescribeInstances(&ec2.DescribeInstancesInput{
-					Filters: []*ec2.Filter{
-						{
-							Name:   awsTagFilter("Name"),
-							Values: aws.StringSlice([]string{machine.Name}),
-						},
-
-						clusterFilter(clusterID),
-					},
-				}).Return(
-					stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated, "192.168.0.10"),
-					nil,
-				).Times(1).After(first)
-
 				return mockAWSClient
 			},
+			exists: true,
 		},
 	}
 

pkg/actuators/machine/utils.go

@@ -39,14 +39,16 @@ import (
 const upstreamMachineClusterIDLabel = "sigs.k8s.io/cluster-api-cluster"
 
 // existingInstanceStates returns the list of states an EC2 instance can be in
-// while being considered "existing", i.e. mostly anything but "Terminated".
+// while being considered "existing"
+// terminated is also handled so that an instance can't get stuck in terminated
 func existingInstanceStates() []*string {
 	return []*string{
 		aws.String(ec2.InstanceStateNameRunning),
 		aws.String(ec2.InstanceStateNamePending),
 		aws.String(ec2.InstanceStateNameStopped),
 		aws.String(ec2.InstanceStateNameStopping),
 		aws.String(ec2.InstanceStateNameShuttingDown),
+		aws.String(ec2.InstanceStateNameTerminated),
 	}
 }
 
@@ -68,7 +70,7 @@ func getStoppedInstances(machine *machinev1beta1.Machine, client awsclient.Clien
 	return getInstances(machine, client, stoppedInstanceStateFilter)
 }
 
-// getExistingInstances returns all instances not terminated
+// getExistingInstances returns all instances
 func getExistingInstances(machine *machinev1beta1.Machine, client awsclient.Client) ([]*ec2.Instance, error) {
 	return getInstances(machine, client, existingInstanceStates())
 }