Merge pull request #2957 from sedefsavas/ec2-double-creation

Return error if providerID is set and instance cannot be found

Author: k8s-ci-robot

controllers/awsmachine_controller.go

@@ -323,8 +323,8 @@ func (r *AWSMachineReconciler) reconcileDelete(machineScope *scope.MachineScope,
 	}
 
 	instance, err := r.findInstance(machineScope, ec2Service)
-	if err != nil {
-		machineScope.Error(err, "unable to find instance")
+	if err != nil && err != ec2.ErrInstanceNotFoundByID {
+		machineScope.Error(err, "query to find instance failed")
 		return ctrl.Result{}, err
 	}
 
@@ -426,29 +426,34 @@ func (r *AWSMachineReconciler) reconcileDelete(machineScope *scope.MachineScope,
 	return ctrl.Result{}, nil
 }
 
-// findInstance queries the EC2 apis and retrieves the instance if it exists, returns nil otherwise.
+// findInstance queries the EC2 apis and retrieves the instance if it exists.
+// If providerID is empty, finds instance by tags and if it cannot be found, returns empty instance with nil error.
+// If providerID is set, either finds the instance by ID or returns error.
 func (r *AWSMachineReconciler) findInstance(scope *scope.MachineScope, ec2svc services.EC2MachineInterface) (*infrav1.Instance, error) {
+	var instance *infrav1.Instance
+
 	// Parse the ProviderID.
 	pid, err := noderefutil.NewProviderID(scope.GetProviderID())
-	if err != nil && !errors.Is(err, noderefutil.ErrEmptyProviderID) {
-		return nil, errors.Wrapf(err, "failed to parse Spec.ProviderID")
-	}
-
-	// If the ProviderID is populated, describe the instance using the ID.
-	if err == nil {
-		instance, err := ec2svc.InstanceIfExists(pointer.StringPtr(pid.ID()))
+	if err != nil {
+		if !errors.Is(err, noderefutil.ErrEmptyProviderID) {
+			return nil, errors.Wrapf(err, "failed to parse Spec.ProviderID")
+		}
+		// If the ProviderID is empty, try to query the instance using tags.
+		// If an instance cannot be found, GetRunningInstanceByTags returns empty instance with nil error.
+		instance, err = ec2svc.GetRunningInstanceByTags(scope)
 		if err != nil {
-			return nil, errors.Wrapf(err, "failed to query AWSMachine instance")
+			return nil, errors.Wrapf(err, "failed to query AWSMachine instance by tags")
+		}
+	} else {
+		// If the ProviderID is populated, describe the instance using the ID.
+		// InstanceIfExists() returns error (ErrInstanceNotFoundByID or ErrDescribeInstance) if the instance could not be found.
+		instance, err = ec2svc.InstanceIfExists(pointer.StringPtr(pid.ID()))
+		if err != nil {
+			return nil, err
 		}
-		return instance, nil
-	}
-
-	// If the ProviderID is empty, try to query the instance using tags.
-	instance, err := ec2svc.GetRunningInstanceByTags(scope)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to query AWSMachine instance by tags")
 	}
 
+	// The only case where the instance is nil here is when the providerId is empty and instance could not be found by tags.
 	return instance, nil
 }
 
@@ -501,7 +506,7 @@ func (r *AWSMachineReconciler) reconcileNormal(_ context.Context, machineScope *
 		return ctrl.Result{}, err
 	}
 
-	// Create new instance
+	// Create new instance since providerId is nil and instance could not be found by tags.
 	if instance == nil {
 		// Avoid a flickering condition between InstanceProvisionStarted and InstanceProvisionFailed if there's a persistent failure with createInstance
 		if conditions.GetReason(machineScope.AWSMachine, infrav1.InstanceReadyCondition) != infrav1.InstanceProvisionFailedReason {

pkg/cloud/services/ec2/errors.go

@@ -0,0 +1,27 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package ec2
+
+import "errors"
+
+var (
+	// ErrInstanceNotFoundByID defines an error for when the instance with the provided provider ID is missing.
+	ErrInstanceNotFoundByID = errors.New("failed to find instance by id")
+
+	// ErrDescribeInstance defines an error for when AWS SDK returns error when describing instances.
+	ErrDescribeInstance = errors.New("failed to describe instance by id")
+)

pkg/cloud/services/ec2/instances.go

@@ -75,7 +75,8 @@ func (s *Service) GetRunningInstanceByTags(scope *scope.MachineScope) (*infrav1.
 	return nil, nil
 }
 
-// InstanceIfExists returns the existing instance or nothing if it doesn't exist.
+// InstanceIfExists returns the existing instance by id and errors if it cannot find the instance(ErrInstanceNotFoundByID) or API call fails (ErrDescribeInstance).
+// Returns empty instance with nil error, only when providerID is nil.
 func (s *Service) InstanceIfExists(id *string) (*infrav1.Instance, error) {
 	if id == nil {
 		s.scope.Info("Instance does not have an instance id")
@@ -91,17 +92,20 @@ func (s *Service) InstanceIfExists(id *string) (*infrav1.Instance, error) {
 	out, err := s.EC2Client.DescribeInstances(input)
 	switch {
 	case awserrors.IsNotFound(err):
-		return nil, nil
+		record.Eventf(s.scope.InfraCluster(), "FailedFindInstances", "failed to find instance by providerId %q: %v", *id, err)
+		return nil, ErrInstanceNotFoundByID
 	case err != nil:
 		record.Eventf(s.scope.InfraCluster(), "FailedDescribeInstances", "failed to describe instance %q: %v", *id, err)
-		return nil, errors.Wrapf(err, "failed to describe instance: %q", *id)
+		return nil, ErrDescribeInstance
 	}
 
 	if len(out.Reservations) > 0 && len(out.Reservations[0].Instances) > 0 {
 		return s.SDKToInstance(out.Reservations[0].Instances[0])
+	} else {
+		// Failed to find instance with provider id.
+		record.Eventf(s.scope.InfraCluster(), "FailedFindInstances", "failed to find instance by providerId %q: %v", *id, err)
+		return nil, ErrInstanceNotFoundByID
 	}
-
-	return nil, nil
 }
 
 // CreateInstance runs an ec2 instance.

pkg/cloud/services/ec2/instances_test.go

@@ -61,8 +61,8 @@ func TestInstanceIfExists(t *testing.T) {
 					Return(nil, awserrors.NewNotFound("not found"))
 			},
 			check: func(instance *infrav1.Instance, err error) {
-				if err != nil {
-					t.Fatalf("did not expect error: %v", err)
+				if err == nil {
+					t.Fatalf("expects error when instance could not be found: %v", err)
 				}
 
 				if instance != nil {
@@ -80,8 +80,8 @@ func TestInstanceIfExists(t *testing.T) {
 					Return(nil, awserr.New(awserrors.InvalidInstanceID, "does not exist", nil))
 			},
 			check: func(instance *infrav1.Instance, err error) {
-				if err != nil {
-					t.Fatalf("did not expect error: %v", err)
+				if err == nil {
+					t.Fatalf("expects error when DescribeInstances returns error: %v", err)
 				}
 
 				if instance != nil {