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

@@ -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,58 +367,12 @@ 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)
 				r.Recorder.Eventf(machineScope.AWSMachine, corev1.EventTypeWarning, "FailedReleaseHost", "Failed to release dedicated host %s: %v", hostID, 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)
@@ -428,9 +381,6 @@ func (r *AWSMachineReconciler) reconcileDelete(ctx context.Context, machineScope
 				// Mark the condition as succeeded
 				conditions.MarkTrue(machineScope.AWSMachine, infrav1.DedicatedHostReleaseCondition)
 
-				// Clear retry tracking since we succeeded
-				clearHostReleaseRetryTracking(machineScope)
-
 				// Patch the object to persist success state
 				if err := machineScope.PatchObject(); err != nil {
 					machineScope.Error(err, "failed to patch object after successful host release")
@@ -514,7 +464,6 @@ func (r *AWSMachineReconciler) findInstance(machineScope *scope.MachineScope, ec
 	// Parse the ProviderID.
 	pid, err := scope.NewProviderID(machineScope.GetProviderID())
 	if err != nil {
-		//nolint:staticcheck
 		if !errors.Is(err, scope.ErrEmptyProviderID) {
 			return nil, errors.Wrapf(err, "failed to parse Spec.ProviderID")
 		}
@@ -527,7 +476,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 {
 			return nil, err
@@ -1373,94 +1322,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()
-}

config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachines.yaml

@@ -267,6 +267,9 @@ func (s *Service) CreateInstance(ctx context.Context, scope *scope.MachineScope,
 		input.HostID = aws.String(hostID)
 		input.HostAffinity = aws.String("host")
 
+		if scope.AWSMachine.Status.DedicatedHost == nil {
+			scope.AWSMachine.Status.DedicatedHost = &infrav1.DedicatedHostStatus{}
+		}
 		// Update machine status with allocated host ID
 		scope.AWSMachine.Status.DedicatedHost.ID = &hostID
 	} else {
@@ -1301,7 +1304,7 @@ func (s *Service) ensureDedicatedHostAllocation(ctx context.Context, scope *scop
 
 	// Check if a host is already allocated for this machine
 	// Each machine gets its own dedicated host for complete isolation and resource dedication
-	if scope.AWSMachine.Status.DedicatedHost.ID != nil {
+	if scope.AWSMachine.Status.DedicatedHost != nil && scope.AWSMachine.Status.DedicatedHost.ID != nil {
 		existingHostID := aws.ToString(scope.AWSMachine.Status.DedicatedHost.ID)
 		s.scope.Info("Found existing allocated host for machine", "hostID", existingHostID, "machine", scope.Name())
 		return existingHostID, nil
@@ -1312,32 +1315,17 @@ func (s *Service) ensureDedicatedHostAllocation(ctx context.Context, scope *scop
 
 	// Get AZ from the machine's subnet
 	if scope.AWSMachine.Spec.Subnet != nil {
-		var subnet *types.Subnet
-		var err error
-		if scope.AWSMachine.Spec.Subnet.ID != nil {
-			subnet, err = s.getSubnet(scope.AWSMachine.Spec.Subnet.ID)
-			if err != nil {
-				return "", errors.Wrap(err, "failed to get subnet for host allocation")
-			}
-		} else if len(scope.AWSMachine.Spec.Subnet.Filters) > 0 {
-			// Convert CAPA filters to AWS SDK filters
-			awsFilters := make([]types.Filter, len(scope.AWSMachine.Spec.Subnet.Filters))
-			for i, f := range scope.AWSMachine.Spec.Subnet.Filters {
-				awsFilters[i] = types.Filter{
-					Name:   aws.String(f.Name),
-					Values: f.Values,
-				}
-			}
+		subnetID, err := s.findSubnet(scope)
+		if err != nil {
+			return "", errors.Wrap(err, "failed to find subnet for host allocation")
+		}
 
-			subnets, err := s.getFilteredSubnets(awsFilters...)
-			if err != nil {
-				return "", errors.Wrap(err, "failed to get subnet by filters for host allocation")
-			}
-			// if more than one subnet is found, use the first one. they should all share the same AZ.
-			if len(subnets) > 0 {
-				subnet = &subnets[0]
-			}
+		// Get the full subnet object to extract availability zone
+		subnet, err := s.getSubnet(&subnetID)
+		if err != nil {
+			return "", errors.Wrap(err, "failed to get subnet details for host allocation")
 		}
+
 		if subnet != nil && subnet.AvailabilityZone != nil {
 			availabilityZone = subnet.AvailabilityZone
 		}