Merge pull request #135 from atlassian-labs/issue/KUBE-11964/enable-retries

add retry logic

Author: castlemilk

cmd/manager/main.go

@@ -96,6 +96,7 @@ func main() {
 	metrics.Register(mgr.GetClient(), log, *namespace)
 
 	// Setup the cloud provider
+	// Uses AWS SDK's built-in retry behavior
 	cloudProvider, err := builder.BuildCloudProvider(*cloudProviderName, logger)
 	if err != nil {
 		log.Error(err, "Unable to build cloud provider")

pkg/cloudprovider/aws/aws.go

@@ -59,12 +59,12 @@ func verifyIfErrorOccurred(apiErr error, expectedMessage ...string) (bool, error
 }
 
 func verifyIfErrorOccurredWithDefaults(apiErr error, expectedMessage string) (bool, error) {
-	skip_errs := []string{
+	skipErrs := []string{
 		// default errors we wanted to skip
 		"is not in correct state",
 		expectedMessage,
 	}
-	return verifyIfErrorOccurred(apiErr, skip_errs...)
+	return verifyIfErrorOccurred(apiErr, skipErrs...)
 }
 
 type provider struct {
@@ -134,7 +134,6 @@ func (p *provider) InstancesExist(providerIDs []string) (map[string]interface{},
 			InstanceIds: aws.StringSlice(instanceIDs),
 		},
 	)
-
 	if err != nil {
 		return nil, err
 	}

pkg/cloudprovider/aws/aws_test.go

@@ -438,7 +438,7 @@ func TestInstance_OutOfDate(t *testing.T) {
 				MixedInstancesPolicy: &autoscaling.MixedInstancesPolicy{
 					LaunchTemplate: &autoscaling.LaunchTemplate{
 						LaunchTemplateSpecification: &autoscaling.LaunchTemplateSpecification{
-					                Version: aws.String(launchTemplateLatestVersion),
+							Version: aws.String(launchTemplateLatestVersion),
 						},
 					},
 				},
@@ -453,7 +453,7 @@ func TestInstance_OutOfDate(t *testing.T) {
 				MixedInstancesPolicy: &autoscaling.MixedInstancesPolicy{
 					LaunchTemplate: &autoscaling.LaunchTemplate{
 						LaunchTemplateSpecification: &autoscaling.LaunchTemplateSpecification{
-					                Version: aws.String(launchTemplateLatestVersion),
+							Version: aws.String(launchTemplateLatestVersion),
 						},
 					},
 				},

pkg/cloudprovider/aws/builder.go

@@ -13,15 +13,27 @@ import (
 	"github.com/go-logr/logr"
 )
 
-// NewCloudProvider returns a new AWS cloud provider
+// NewCloudProvider returns a new AWS cloud provider using the AWS SDK's default retry behavior
 func NewCloudProvider(logger logr.Logger) (cloudprovider.CloudProvider, error) {
 	sess, err := session.NewSession()
 	if err != nil {
 		return nil, err
 	}
 
 	var creds *credentials.Credentials
-	config := &aws.Config{Credentials: creds}
+
+	// Configure AWS SDK with default retry logic
+	// The AWS SDK v1 automatically uses client.DefaultRetryer which handles:
+	// - Exponential backoff
+	// - Retries for throttling errors
+	// - Retries for transient network errors
+	// - Retries for 5xx server errors
+	config := &aws.Config{
+		Credentials: creds,
+		// Use AWS SDK's default retry behavior (3 retries with exponential backoff)
+		// This is sufficient for most use cases
+		MaxRetries: aws.Int(3),
+	}
 
 	ec2Service := ec2.New(sess, config)
 	autoScalingService := autoscaling.New(sess, config)
@@ -33,7 +45,7 @@ func NewCloudProvider(logger logr.Logger) (cloudprovider.CloudProvider, error) {
 	}
 
 	// Log the provider we used
-	credValue, err := autoScalingService.Client.Config.Credentials.Get()
+	credValue, err := config.Credentials.Get()
 	if err != nil {
 		return nil, err
 	}

pkg/cloudprovider/builder/builder.go

@@ -11,6 +11,7 @@ import (
 type builderFunc func(logger logr.Logger) (cloudprovider.CloudProvider, error)
 
 // BuildCloudProvider returns a cloud provider based on the provided name
+// Uses the AWS SDK's built-in retry behavior
 func BuildCloudProvider(name string, logger logr.Logger) (cloudprovider.CloudProvider, error) {
 	buildFuncs := map[string]builderFunc{
 		aws.ProviderName: aws.NewCloudProvider,

pkg/controller/cyclenoderequest/transitioner/checks.go

@@ -139,7 +139,7 @@ func (t *CycleNodeRequestTransitioner) makeRequest(httpMethod string, httpClient
 		return 0, nil, err
 	}
 
-	defer resp.Body.Close()
+	defer func() { _ = resp.Body.Close() }()
 
 	bytes, err := io.ReadAll(resp.Body)
 	if err != nil {
@@ -242,7 +242,7 @@ func (t *CycleNodeRequestTransitioner) performInitialHealthChecks(kubeNodes map[
 // performCyclingHealthChecks before terminating an instance selected for termination. Cycling pauses
 // until all health checks pass for the new instance before terminating the old one
 func (t *CycleNodeRequestTransitioner) performCyclingHealthChecks(kubeNodes map[string]corev1.Node) (bool, error) {
-	var allHealthChecksPassed bool = true
+	allHealthChecksPassed := true
 
 	// Find new instsances attached to the nodegroup and perform health checks on them
 	// before terminating the old ones they are replacing
@@ -376,7 +376,7 @@ func (t *CycleNodeRequestTransitioner) sendPreTerminationTrigger(node v1.CycleNo
 // It monitors the progress shutdown progress. Cyclops will wait until this endpoint returns the expected response before
 // proceeding to terminate the node.
 func (t *CycleNodeRequestTransitioner) performPreTerminationHealthChecks(node v1.CycleNodeRequestNode) (bool, error) {
-	var allHealthChecksPassed bool = true
+	allHealthChecksPassed := true
 	nodeHash := getNodeHash(node)
 
 	// Check that the trigger has already been send to the node before performing any health checks

pkg/controller/cyclenoderequest/transitioner/errors.go

@@ -0,0 +1,68 @@
+package transitioner
+
+import (
+	"net"
+	"strings"
+
+	"github.com/aws/aws-sdk-go/aws/awserr"
+)
+
+// isRetryableError determines if an error should trigger a requeue instead of moving to Healing phase
+// Retryable errors typically include network timeouts and transient AWS service errors
+func isRetryableError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	// Check for network errors (timeouts, connection refused, etc.)
+	if netErr, ok := err.(net.Error); ok {
+		if netErr.Timeout() {
+			return true
+		}
+	}
+
+	// Check error string for common network patterns
+	errStr := err.Error()
+	networkPatterns := []string{
+		"i/o timeout",
+		"dial tcp",
+		"connection reset",
+		"connection refused",
+		"connection timeout",
+		"TLS handshake timeout",
+		"broken pipe",
+		"unexpected EOF",
+	}
+
+	for _, pattern := range networkPatterns {
+		if strings.Contains(errStr, pattern) {
+			return true
+		}
+	}
+
+	// Check for AWS SDK transient errors
+	if awsErr, ok := err.(awserr.Error); ok {
+		code := awsErr.Code()
+		// Common transient AWS error codes
+		transientCodes := map[string]bool{
+			"Throttling":                  true,
+			"ThrottlingException":         true,
+			"TooManyRequestsException":    true,
+			"RequestLimitExceeded":        true,
+			"SlowDown":                    true,
+			"ServiceUnavailable":          true,
+			"ServiceUnavailableException": true,
+			"InternalFailure":             true,
+			"InternalError":               true,
+			"InternalServerError":         true,
+			"RequestTimeout":              true,
+			"RequestTimedOut":             true,
+		}
+
+		if transientCodes[code] {
+			return true
+		}
+	}
+
+	return false
+}