EvictionController: Retry on re-enablement failures

We will likely encounter transient errors, and we should
not stop deleting the eviction because of that.

Author: fwiesel

internal/controller/eviction_controller.go

@@ -76,7 +76,13 @@ func (r *EvictionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 
 	// Being deleted
 	if !eviction.DeletionTimestamp.IsZero() {
-		err := r.handleFinalizer(ctx, eviction)
+		retry, err := r.handleFinalizer(ctx, eviction)
+		if err != nil {
+			return ctrl.Result{}, err
+		}
+		if retry {
+			return ctrl.Result{RequeueAfter: defaultWaitTime}, err
+		}
 		log.Info("deleted")
 		return ctrl.Result{}, err
 	}
@@ -364,82 +370,98 @@ func (r *EvictionReconciler) evictionReason(eviction *kvmv1.Eviction) string {
 	return fmt.Sprintf("Eviction %v/%v: %v", eviction.Namespace, eviction.Name, eviction.Spec.Reason)
 }
 
-func (r *EvictionReconciler) handleFinalizer(ctx context.Context, eviction *kvmv1.Eviction) error {
+func (r *EvictionReconciler) handleFinalizer(ctx context.Context, eviction *kvmv1.Eviction) (bool, error) {
 	if !controllerutil.ContainsFinalizer(eviction, evictionFinalizerName) {
-		return nil
+		return false, nil
 	}
 
+	// As long as we didn't succeed to re-enable the hypervisor, which includes
+	// - the hypervisor being gone, because it has been torn down
+	// - the hypervisor having been enabled by someone else
 	if !meta.IsStatusConditionTrue(eviction.Status.Conditions, kvmv1.ConditionTypeHypervisorReEnabled) {
-		if err := r.enableHypervisorService(ctx, eviction); err != nil {
-			if errors.Is(err, openstack.ErrNoHypervisor) {
-				log := logger.FromContext(ctx)
-				log.Info("Can't enable host, it is gone")
-			} else {
-				return err
-			}
+		retry, err := r.enableHypervisorService(ctx, eviction)
+		if err != nil || retry {
+			return retry, err
 		}
 	}
 
 	evictionBase := eviction.DeepCopy()
 	controllerutil.RemoveFinalizer(eviction, evictionFinalizerName)
-	return r.Patch(ctx, eviction, client.MergeFromWithOptions(evictionBase, client.MergeFromWithOptimisticLock{}))
+	return false, r.Patch(ctx, eviction, client.MergeFromWithOptions(evictionBase, client.MergeFromWithOptimisticLock{}))
 }
 
-func (r *EvictionReconciler) enableHypervisorService(ctx context.Context, eviction *kvmv1.Eviction) error {
+func (r *EvictionReconciler) enableHypervisorService(ctx context.Context, eviction *kvmv1.Eviction) (bool, error) {
 	log := logger.FromContext(ctx)
 
 	hypervisor, err := openstack.GetHypervisorByName(ctx, r.computeClient, eviction.Spec.Hypervisor, false)
 	if err != nil {
+		changed := false
+		retry := true
 		if errors.Is(err, openstack.ErrNoHypervisor) {
-			meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
+			changed = meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
 				Type:    kvmv1.ConditionTypeHypervisorReEnabled,
 				Status:  metav1.ConditionTrue,
 				Message: "Hypervisor is gone, no need to re-enable",
 				Reason:  kvmv1.ConditionReasonSucceeded,
 			})
+			retry = false
 		} else {
-			// update the condition to reflect the error, but do not fail the reconciliation
-			meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
+			// update the condition to reflect the error, and retry the reconciliation
+			changed = meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
 				Type:    kvmv1.ConditionTypeHypervisorReEnabled,
 				Status:  metav1.ConditionFalse,
 				Message: fmt.Sprintf("failed to get hypervisor due to %s", err),
 				Reason:  kvmv1.ConditionReasonFailed,
 			})
 		}
-		return r.Status().Update(ctx, eviction)
+		if changed {
+			return retry, r.Status().Update(ctx, eviction)
+		} else {
+			return retry, nil
+		}
 	}
 
 	if hypervisor.Service.DisabledReason != r.evictionReason(eviction) {
-		meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
+		changed := meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
 			Type:   kvmv1.ConditionTypeHypervisorReEnabled,
 			Status: metav1.ConditionTrue,
 			Message: fmt.Sprintf("Hypervisor already re-enabled for reason: %s",
 				hypervisor.Service.DisabledReason),
 			Reason: kvmv1.ConditionReasonSucceeded,
 		})
-		return r.Status().Update(ctx, eviction)
+		if changed {
+			return false, r.Status().Update(ctx, eviction)
+		} else {
+			return false, nil
+		}
 	}
 
 	enableService := services.UpdateOpts{Status: services.ServiceEnabled}
 	log.Info("Enabling hypervisor", "id", hypervisor.Service.ID)
 	_, err = services.Update(ctx, r.computeClient, hypervisor.Service.ID, enableService).Extract()
 
+	changed := false
 	if err != nil {
-		meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
+		changed = meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
 			Type:    kvmv1.ConditionTypeHypervisorReEnabled,
 			Status:  metav1.ConditionFalse,
 			Message: fmt.Sprintf("failed to enable hypervisor due to %s", err),
 			Reason:  kvmv1.ConditionReasonFailed,
 		})
 	} else {
-		meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
+		changed = meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
 			Type:    kvmv1.ConditionTypeHypervisorReEnabled,
 			Status:  metav1.ConditionTrue,
 			Message: "Hypervisor re-enabled successfully",
 			Reason:  kvmv1.ConditionReasonSucceeded,
 		})
 	}
-	return r.Status().Update(ctx, eviction)
+
+	if changed {
+		return err != nil, r.Status().Update(ctx, eviction)
+	} else {
+		return err != nil, nil
+	}
 }
 
 // disableHypervisor disables the hypervisor service and adds a finalizer to the eviction