Update the requeue logic for cases where the status could not be updated

Author: johscheuer

controllers/backup_controller.go

@@ -111,7 +111,7 @@ func (r *FoundationDBBackupReconciler) Reconcile(
 			continue
 		}
 
-		return processRequeue(req, subReconciler, backup, r.Recorder, backupLog)
+		return processResult(processRequeue(req, subReconciler, backup, r.Recorder, backupLog))
 	}
 
 	if backup.Status.Generations.Reconciled < originalGeneration {

controllers/cluster_controller.go

@@ -30,8 +30,8 @@ import (
 	"syscall"
 	"time"
 
-	"k8s.io/utils/ptr"
 	"k8s.io/apimachinery/pkg/api/equality"
+	"k8s.io/utils/ptr"
 
 	"github.com/FoundationDB/fdb-kubernetes-operator/v2/pkg/fdbadminclient"
 	"github.com/FoundationDB/fdb-kubernetes-operator/v2/pkg/podmanager"
@@ -181,15 +181,23 @@ func (r *FoundationDBClusterReconciler) Reconcile(
 	ctx context.Context,
 	request ctrl.Request,
 ) (ctrl.Result, error) {
+	return processResult(r.doReconcile(ctx, request))
+}
+
+// doReconcile will run the reconcile logic for the FoundationDBClusterReconciler.
+func (r *FoundationDBClusterReconciler) doReconcile(
+	ctx context.Context,
+	request ctrl.Request,
+) (*ctrl.Result, error) {
 	cluster := &fdbv1beta2.FoundationDBCluster{}
 
 	err := r.Get(ctx, request.NamespacedName, cluster)
 	if err != nil {
 		if k8serrors.IsNotFound(err) {
-			return ctrl.Result{}, nil
+			return nil, nil
 		}
 		// Error reading the object - requeue the request.
-		return ctrl.Result{}, err
+		return nil, err
 	}
 
 	clusterLog := globalControllerLogger.WithValues(
@@ -206,6 +214,7 @@ func (r *FoundationDBClusterReconciler) Reconcile(
 
 	originalStatus := cluster.Status.DeepCopy()
 	startTime := time.Now()
+	result := &ctrl.Result{}
 	defer func() {
 		// If the cluster.Status has changed compared to the original Status, we have to update the status.
 		// See: https://github.com/kubernetes-sigs/kubebuilder/issues/592
@@ -216,6 +225,10 @@ func (r *FoundationDBClusterReconciler) Reconcile(
 			err = r.updateOrApply(ctx, cluster)
 			if err != nil {
 				clusterLog.Error(err, "Error updating cluster clusterStatus")
+				// If no requeue is planned, we should target a requeue to ensure that the status will be updated.
+				if result.IsZero() || result.RequeueAfter == 0 {
+					result.RequeueAfter = 2 * time.Second
+				}
 			}
 		}
 
@@ -232,33 +245,33 @@ func (r *FoundationDBClusterReconciler) Reconcile(
 	if cluster.Spec.Skip {
 		clusterLog.Info("Skipping cluster with skip value true", "skip", cluster.Spec.Skip)
 		// Don't requeue
-		return ctrl.Result{}, nil
+		return nil, nil
 	}
 
 	err = internal.NormalizeClusterSpec(cluster, r.DeprecationOptions)
 	if err != nil {
-		return ctrl.Result{}, err
+		return nil, nil
 	}
 
 	err = cluster.Validate()
 	if err != nil {
 		r.Recorder.Event(cluster, corev1.EventTypeWarning, "ClusterSpec not valid", err.Error())
-		return ctrl.Result{}, fmt.Errorf("ClusterSpec is not valid: %w", err)
+		return nil, fmt.Errorf("ClusterSpec is not valid: %w", err)
 	}
 
 	adminClient, err := r.getAdminClient(clusterLog, cluster)
 	if err != nil {
-		return ctrl.Result{}, err
+		return nil, err
 	}
 	defer func() {
 		_ = adminClient.Close()
 	}()
 	supportedVersion, err := adminClient.VersionSupported(cluster.Spec.Version)
 	if err != nil {
-		return ctrl.Result{}, err
+		return nil, err
 	}
 	if !supportedVersion {
-		return ctrl.Result{}, fmt.Errorf("version %s is not supported", cluster.Spec.Version)
+		return nil, fmt.Errorf("version %s is not supported", cluster.Spec.Version)
 	}
 
 	// When using DNS entries in the cluster file, we want to make sure to create pods if required before doing any
@@ -336,13 +349,13 @@ func (r *FoundationDBClusterReconciler) Reconcile(
 						process, err := os.FindProcess(os.Getpid())
 						if err != nil {
 							fmt.Printf("Error finding process: %v\n", err)
-							return ctrl.Result{RequeueAfter: 5 * time.Second}, err
+							return &ctrl.Result{RequeueAfter: 5 * time.Second}, err
 						}
 
 						err = process.Signal(syscall.SIGTERM)
 						if err != nil {
 							fmt.Printf("Error sending signal: %v\n", err)
-							return ctrl.Result{RequeueAfter: 5 * time.Second}, err
+							return &ctrl.Result{RequeueAfter: 5 * time.Second}, err
 						}
 					}
 				}
@@ -399,7 +412,7 @@ func (r *FoundationDBClusterReconciler) Reconcile(
 			delayedRequeueDuration = 2 * time.Second
 		}
 
-		return ctrl.Result{RequeueAfter: delayedRequeueDuration}, nil
+		return &ctrl.Result{RequeueAfter: delayedRequeueDuration}, nil
 	}
 
 	clusterLog.Info("Reconciliation complete", "generation", cluster.Status.Generations.Reconciled)
@@ -410,7 +423,7 @@ func (r *FoundationDBClusterReconciler) Reconcile(
 		fmt.Sprintf("Reconciled generation %d", cluster.Status.Generations.Reconciled),
 	)
 
-	return ctrl.Result{}, nil
+	return result, nil
 }
 
 // runClusterSubReconciler will start the subReconciler and will log the duration of the subReconciler.

controllers/controllers.go

@@ -64,7 +64,7 @@ func processRequeue(
 	object runtime.Object,
 	recorder record.EventRecorder,
 	logger logr.Logger,
-) (ctrl.Result, error) {
+) (*ctrl.Result, error) {
 	curLog := logger.WithValues(
 		"reconciler",
 		fmt.Sprintf("%T", subReconciler),
@@ -90,9 +90,23 @@ func processRequeue(
 	recorder.Event(object, corev1.EventTypeNormal, "ReconciliationTerminatedEarly", requeue.message)
 	if err != nil {
 		curLog.Error(err, "Error in reconciliation")
-		return ctrl.Result{}, err
+		return nil, err
 	}
 	curLog.Info("Reconciliation terminated early", "message", requeue.message)
 
-	return ctrl.Result{RequeueAfter: requeue.delay}, nil
+	return &ctrl.Result{RequeueAfter: requeue.delay}, nil
+}
+
+// processResult will return a ctrl.Result and error based on the input, e.g. if the result is nil, it will ensure
+// that an empty ctrl.Result is returned.
+func processResult(result *ctrl.Result, err error) (ctrl.Result, error) {
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	if result.IsZero() {
+		return ctrl.Result{}, nil
+	}
+
+	return *result, nil
 }

controllers/restore_controller.go

@@ -89,7 +89,8 @@ func (r *FoundationDBRestoreReconciler) Reconcile(
 		if req == nil {
 			continue
 		}
-		return processRequeue(req, subReconciler, restore, r.Recorder, restoreLog)
+
+		return processResult(processRequeue(req, subReconciler, restore, r.Recorder, restoreLog))
 	}
 
 	if restore.Status.State != fdbv1beta2.CompletedFoundationDBRestoreState {