wip: addressing feedback

Author: rvanderp3

api/v1beta1/awsmachine_conversion.go

@@ -65,10 +65,6 @@ func (src *AWSMachine) ConvertTo(dstRaw conversion.Hub) error {
 	}
 
 	dst.Status.DedicatedHost = restored.Status.DedicatedHost
-	dst.Status.HostReleaseAttempts = restored.Status.HostReleaseAttempts
-	dst.Status.LastHostReleaseAttempt = restored.Status.LastHostReleaseAttempt
-	dst.Status.HostReleaseFailedReason = restored.Status.HostReleaseFailedReason
-
 	return nil
 }
 

api/v1beta1/zz_generated.conversion.go

@@ -466,18 +466,6 @@ type AWSMachineStatus struct {
 	// This field is populated when DynamicHostAllocation is used.
 	// +optional
 	DedicatedHost *DedicatedHostStatus `json:"dedicatedHost,omitempty"`
-
-	// HostReleaseAttempts tracks the number of attempts to release the dedicated host.
-	// +optional
-	HostReleaseAttempts *int32 `json:"hostReleaseAttempts,omitempty"`
-
-	// LastHostReleaseAttempt tracks the timestamp of the last attempt to release the dedicated host.
-	// +optional
-	LastHostReleaseAttempt *metav1.Time `json:"lastHostReleaseAttempt,omitempty"`
-
-	// HostReleaseFailedReason tracks the reason for the last host release failure.
-	// +optional
-	HostReleaseFailedReason *string `json:"hostReleaseFailedReason,omitempty"`
 }
 
 // DedicatedHostStatus defines the observed state of a dynamically allocated dedicated host
@@ -488,10 +476,6 @@ type DedicatedHostStatus struct {
 	// This field is populated when DynamicHostAllocation is used.
 	// +optional
 	ID *string `json:"id,omitempty"`
-
-	// ReleaseFailureMessage tracks the last failure message for the release host attempt.
-	// +optional
-	ReleaseFailureMessage *string `json:"releaseFailureMessage,omitempty"`
 }
 
 // +kubebuilder:object:root=true

api/v1beta2/awsmachine_types.go

@@ -196,12 +196,6 @@ const (
 	// S3BucketFailedReason is used when any errors occur during reconciliation of an S3 bucket.
 	S3BucketFailedReason = "S3BucketCreationFailed"
 
-	// DedicatedHostReleaseSucceededReason used when the dedicated host is successfully released.
-	DedicatedHostReleaseSucceededReason = "DedicatedHostReleaseSucceeded"
-
 	// DedicatedHostReleaseFailedReason used when the dedicated host release fails.
 	DedicatedHostReleaseFailedReason = "DedicatedHostReleaseFailed"
-
-	// DedicatedHostReleaseRetryingReason used when the dedicated host release is being retried.
-	DedicatedHostReleaseRetryingReason = "DedicatedHostReleaseRetrying"
 )

api/v1beta2/conditions_consts.go

@@ -1298,10 +1298,6 @@ spec:
                       ID tracks the dynamically allocated dedicated host ID.
                       This field is populated when DynamicHostAllocation is used.
                     type: string
-                  releaseFailureMessage:
-                    description: ReleaseFailureMessage tracks the last failure message
-                      for the release host attempt.
-                    type: string
                 type: object
               failureMessage:
                 description: |-
@@ -1341,15 +1337,6 @@ spec:
                   can be added as events to the Machine object and/or logged in the
                   controller's output.
                 type: string
-              hostReleaseAttempts:
-                description: HostReleaseAttempts tracks the number of attempts to
-                  release the dedicated host.
-                format: int32
-                type: integer
-              hostReleaseFailedReason:
-                description: HostReleaseFailedReason tracks the reason for the last
-                  host release failure.
-                type: string
               instanceState:
                 description: InstanceState is the state of the AWS instance for this
                   machine.
@@ -1359,11 +1346,6 @@ spec:
                   Interruptible reports that this machine is using spot instances and can therefore be interrupted by CAPI when it receives a notice that the spot instance is to be terminated by AWS.
                   This will be set to true when SpotMarketOptions is not nil (i.e. this machine is using a spot instance).
                 type: boolean
-              lastHostReleaseAttempt:
-                description: LastHostReleaseAttempt tracks the timestamp of the last
-                  attempt to release the dedicated host.
-                format: date-time
-                type: string
               ready:
                 description: Ready is true when the provider resource is ready.
                 type: boolean

api/v1beta2/zz_generated.deepcopy.go

@@ -32,7 +32,6 @@ import (
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/klog/v2"
@@ -368,74 +367,26 @@ func (r *AWSMachineReconciler) reconcileDelete(ctx context.Context, machineScope
 			machineScope.AWSMachine.Spec.DynamicHostAllocation != nil {
 			hostID := *machineScope.AWSMachine.Status.DedicatedHost.ID
 
-			// Check if we should retry host release
-			shouldRetry, retryAfter := shouldRetryHostRelease(machineScope)
-
-			if shouldRetry {
-				// Mark that we're retrying
-				conditions.MarkFalse(machineScope.AWSMachine, infrav1.DedicatedHostReleaseCondition,
-					infrav1.DedicatedHostReleaseRetryingReason, clusterv1.ConditionSeverityWarning,
-					"Retrying dedicated host release, attempt %d", getHostReleaseAttempts(machineScope))
-
-				// Update retry tracking
-				updateHostReleaseRetryTracking(machineScope)
-
-				// Patch the object to persist retry tracking
-				if err := machineScope.PatchObject(); err != nil {
-					machineScope.Error(err, "failed to patch object with retry tracking")
-					return ctrl.Result{}, err
-				}
-
-				machineScope.Info("Retrying dedicated host release", "hostID", hostID, "attempt", getHostReleaseAttempts(machineScope))
-				return ctrl.Result{RequeueAfter: retryAfter}, nil
-			}
-
 			// Attempt to release the dedicated host
-			machineScope.Info("Releasing dynamically allocated dedicated host", "hostID", hostID, "attempt", getHostReleaseAttempts(machineScope))
+			machineScope.Info("Releasing dynamically allocated dedicated host", "hostID", hostID)
 			if err := ec2Service.ReleaseDedicatedHost(ctx, hostID); err != nil {
-				// Host release failed, set up retry logic
-				machineScope.Error(err, "failed to release dedicated host", "hostID", hostID, "attempt", getHostReleaseAttempts(machineScope))
+				machineScope.Error(err, "failed to release dedicated host", "hostID", hostID)
+				conditions.MarkFalse(machineScope.AWSMachine, infrav1.DedicatedHostReleaseCondition, infrav1.DedicatedHostReleaseFailedReason, clusterv1.ConditionSeverityError, "%s", err.Error())
 				r.Recorder.Eventf(machineScope.AWSMachine, corev1.EventTypeWarning, "FailedReleaseHost", "Failed to release dedicated host %s: %v", hostID, err)
+				return ctrl.Result{}, err
+			}
 
-				// Update failure tracking
-				updateHostReleaseFailureTracking(machineScope, err.Error())
-
-				// Mark the condition as failed
-				conditions.MarkFalse(machineScope.AWSMachine, infrav1.DedicatedHostReleaseCondition,
-					infrav1.DedicatedHostReleaseFailedReason, clusterv1.ConditionSeverityWarning,
-					"Failed to release dedicated host: %v", err)
-
-				// Patch the object to persist failure tracking
-				if err := machineScope.PatchObject(); err != nil {
-					machineScope.Error(err, "failed to patch object with failure tracking")
-					return ctrl.Result{}, err
-				}
-
-				// Check if we've exceeded max retries
-				if hasExceededMaxHostReleaseRetries(machineScope) {
-					machineScope.Error(err, "exceeded maximum retry attempts for dedicated host release", "hostID", hostID, "maxAttempts", getMaxHostReleaseRetries())
-					r.Recorder.Eventf(machineScope.AWSMachine, corev1.EventTypeWarning, "MaxRetriesExceeded", "Exceeded maximum retry attempts for dedicated host %s release", hostID)
-					// Continue with deletion even if host release fails permanently
-				} else {
-					// Return to trigger retry
-					return ctrl.Result{RequeueAfter: getInitialHostReleaseRetryDelay()}, nil
-				}
-			} else {
-				// Host release succeeded
-				machineScope.Info("Successfully released dedicated host", "hostID", hostID)
-				r.Recorder.Eventf(machineScope.AWSMachine, corev1.EventTypeNormal, "SuccessfulReleaseHost", "Released dedicated host %s", hostID)
-
-				// Mark the condition as succeeded
-				conditions.MarkTrue(machineScope.AWSMachine, infrav1.DedicatedHostReleaseCondition)
+			// Host release succeeded
+			machineScope.Info("Successfully released dedicated host", "hostID", hostID)
+			r.Recorder.Eventf(machineScope.AWSMachine, corev1.EventTypeNormal, "SuccessfulReleaseHost", "Released dedicated host %s", hostID)
 
-				// Clear retry tracking since we succeeded
-				clearHostReleaseRetryTracking(machineScope)
+			// Mark the condition as succeeded
+			conditions.MarkTrue(machineScope.AWSMachine, infrav1.DedicatedHostReleaseCondition)
 
-				// Patch the object to persist success state
-				if err := machineScope.PatchObject(); err != nil {
-					machineScope.Error(err, "failed to patch object after successful host release")
-					return ctrl.Result{}, err
-				}
+			// Patch the object to persist success state
+			if err := machineScope.PatchObject(); err != nil {
+				machineScope.Error(err, "failed to patch object after successful host release")
+				return ctrl.Result{}, err
 			}
 		}
 
@@ -527,6 +478,7 @@ func (r *AWSMachineReconciler) findInstance(machineScope *scope.MachineScope, ec
 	} 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.
+
 		//nolint:staticcheck
 		instance, err = ec2svc.InstanceIfExists(ptr.To[string](pid.ID()))
 		if err != nil {
@@ -1373,94 +1325,3 @@ func (r *AWSMachineReconciler) ensureInstanceMetadataOptions(ec2svc services.EC2
 
 	return ec2svc.ModifyInstanceMetadataOptions(instance.ID, machine.Spec.InstanceMetadataOptions)
 }
-
-// getMaxHostReleaseRetries returns the maximum number of retry attempts for dedicated host release.
-func getMaxHostReleaseRetries() int32 {
-	return 5
-}
-
-// getInitialHostReleaseRetryDelay returns the initial delay before the first retry.
-func getInitialHostReleaseRetryDelay() time.Duration {
-	return 30 * time.Second
-}
-
-// getHostReleaseAttempts returns the current number of host release attempts.
-func getHostReleaseAttempts(scope *scope.MachineScope) int32 {
-	if scope.AWSMachine.Status.HostReleaseAttempts == nil {
-		return 0
-	}
-	return *scope.AWSMachine.Status.HostReleaseAttempts
-}
-
-// shouldRetryHostRelease determines if we should retry host release based on retry tracking.
-func shouldRetryHostRelease(scope *scope.MachineScope) (bool, time.Duration) {
-	attempts := getHostReleaseAttempts(scope)
-
-	// If no attempts yet, don't retry
-	if attempts == 0 {
-		return false, 0
-	}
-
-	// Check if we've exceeded max retries
-	if attempts >= getMaxHostReleaseRetries() {
-		return false, 0
-	}
-
-	// Check if enough time has passed since last attempt
-	lastAttempt := scope.AWSMachine.Status.LastHostReleaseAttempt
-	if lastAttempt == nil {
-		return false, 0
-	}
-
-	// Calculate exponential backoff delay
-	baseDelay := getInitialHostReleaseRetryDelay()
-	multiplier := int64(1)
-	for i := int32(1); i < attempts; i++ {
-		multiplier *= 2
-	}
-	backoffDelay := time.Duration(int64(baseDelay) * multiplier)
-
-	// Cap the maximum delay at 5 minutes
-	if backoffDelay > 5*time.Minute {
-		backoffDelay = 5 * time.Minute
-	}
-
-	// Check if enough time has passed
-	timeSinceLastAttempt := time.Since(lastAttempt.Time)
-	if timeSinceLastAttempt < backoffDelay {
-		remainingDelay := backoffDelay - timeSinceLastAttempt
-		return false, remainingDelay
-	}
-
-	return true, backoffDelay
-}
-
-// updateHostReleaseRetryTracking increments the retry attempt counter and updates the timestamp.
-func updateHostReleaseRetryTracking(scope *scope.MachineScope) {
-	attempts := getHostReleaseAttempts(scope) + 1
-	scope.AWSMachine.Status.HostReleaseAttempts = &attempts
-
-	now := time.Now()
-	scope.AWSMachine.Status.LastHostReleaseAttempt = &metav1.Time{Time: now}
-}
-
-// updateHostReleaseFailureTracking updates the failure reason and timestamp.
-func updateHostReleaseFailureTracking(scope *scope.MachineScope, reason string) {
-	scope.AWSMachine.Status.HostReleaseFailedReason = &reason
-
-	// Update the timestamp for the last attempt
-	now := time.Now()
-	scope.AWSMachine.Status.LastHostReleaseAttempt = &metav1.Time{Time: now}
-}
-
-// clearHostReleaseRetryTracking resets all retry tracking fields after successful release.
-func clearHostReleaseRetryTracking(scope *scope.MachineScope) {
-	scope.AWSMachine.Status.HostReleaseAttempts = nil
-	scope.AWSMachine.Status.LastHostReleaseAttempt = nil
-	scope.AWSMachine.Status.HostReleaseFailedReason = nil
-}
-
-// hasExceededMaxHostReleaseRetries checks if we've exceeded the maximum retry attempts.
-func hasExceededMaxHostReleaseRetries(scope *scope.MachineScope) bool {
-	return getHostReleaseAttempts(scope) >= getMaxHostReleaseRetries()
-}