Add constructors for valid reconcile.Result values

The result list of Reconciler.Reconcile is a pair of types with multiple
fields and special values. Some combinations are confusing, ignored, or
cause warnings at runtime. The following values can be combined eighteen
ways:

    Result.Requeue = { true, false }
    Result.RequeueAfter = { negative, zero, positive }
    error = { nil, non-nil, terminal }

These constructors provide names and documentation for four of the valid
combinations.

Author: cbandy

internal/bridge/crunchybridgecluster/crunchybridgecluster_controller.go

@@ -33,6 +33,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/event"
 
 	"github.com/crunchydata/postgres-operator/internal/bridge"
+	"github.com/crunchydata/postgres-operator/internal/controller/runtime"
 	pgoRuntime "github.com/crunchydata/postgres-operator/internal/controller/runtime"
 	"github.com/crunchydata/postgres-operator/internal/naming"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
@@ -152,11 +153,6 @@ func (r *CrunchyBridgeClusterReconciler) Reconcile(ctx context.Context, req ctrl
 		return ctrl.Result{}, err
 	} else if result != nil {
 		if log := log.V(1); log.Enabled() {
-			if result.RequeueAfter > 0 {
-				// RequeueAfter implies Requeue, but set both to make the next
-				// log message more clear.
-				result.Requeue = true
-			}
 			log.Info("deleting", "result", fmt.Sprintf("%+v", *result))
 		}
 		return *result, err
@@ -238,7 +234,7 @@ func (r *CrunchyBridgeClusterReconciler) Reconcile(ctx context.Context, req ctrl
 	// TODO(crunchybridgecluster): Do we want the operator to interrupt
 	// upgrades created through the GUI/API?
 	if len(crunchybridgecluster.Status.OngoingUpgrade) != 0 {
-		return ctrl.Result{RequeueAfter: 3 * time.Minute}, nil
+		return runtime.RequeueWithoutBackoff(3 * time.Minute), nil
 	}
 
 	// Check if there's an upgrade difference for the three upgradeable fields that hit the upgrade endpoint
@@ -268,7 +264,7 @@ func (r *CrunchyBridgeClusterReconciler) Reconcile(ctx context.Context, req ctrl
 	log.Info("Reconciled")
 	// TODO(crunchybridgecluster): do we always want to requeue? Does the Watch mean we
 	// don't need this, or do we want both?
-	return ctrl.Result{RequeueAfter: 3 * time.Minute}, nil
+	return runtime.RequeueWithoutBackoff(3 * time.Minute), nil
 }
 
 // reconcileBridgeConnectionSecret looks for the Bridge connection secret specified by the cluster,
@@ -418,7 +414,7 @@ func (r *CrunchyBridgeClusterReconciler) handleCreateCluster(ctx context.Context
 		Message:            "The condition of the upgrade(s) is unknown.",
 	})
 
-	return ctrl.Result{RequeueAfter: 3 * time.Minute}
+	return runtime.RequeueWithoutBackoff(3 * time.Minute)
 }
 
 // handleGetCluster handles getting the cluster details from Bridge and
@@ -579,7 +575,7 @@ func (r *CrunchyBridgeClusterReconciler) handleUpgrade(ctx context.Context,
 		})
 	}
 
-	return ctrl.Result{RequeueAfter: 3 * time.Minute}
+	return runtime.RequeueWithoutBackoff(3 * time.Minute)
 }
 
 // handleUpgradeHA handles upgrades that hit the
@@ -626,7 +622,7 @@ func (r *CrunchyBridgeClusterReconciler) handleUpgradeHA(ctx context.Context,
 		})
 	}
 
-	return ctrl.Result{RequeueAfter: 3 * time.Minute}
+	return runtime.RequeueWithoutBackoff(3 * time.Minute)
 }
 
 // handleUpdate handles upgrades that hit the "PATCH /clusters/<id>" endpoint
@@ -671,7 +667,7 @@ func (r *CrunchyBridgeClusterReconciler) handleUpdate(ctx context.Context,
 			clusterUpdate.ClusterName, *clusterUpdate.IsProtected),
 	})
 
-	return ctrl.Result{RequeueAfter: 3 * time.Minute}
+	return runtime.RequeueWithoutBackoff(3 * time.Minute)
 }
 
 // GetSecretKeys gets the secret and returns the expected API key and team id

internal/bridge/crunchybridgecluster/crunchybridgecluster_controller_test.go

@@ -197,7 +197,7 @@ func TestHandleCreateCluster(t *testing.T) {
 		cluster.Namespace = ns
 
 		controllerResult := reconciler.handleCreateCluster(ctx, testApiKey, testTeamId, cluster)
-		assert.Equal(t, controllerResult, ctrl.Result{RequeueAfter: 3 * time.Minute})
+		assert.Equal(t, controllerResult.RequeueAfter, 3*time.Minute)
 		assert.Equal(t, cluster.Status.ID, "0")
 
 		readyCondition := meta.FindStatusCondition(cluster.Status.Conditions, v1beta1.ConditionReady)
@@ -484,7 +484,7 @@ func TestHandleUpgrade(t *testing.T) {
 		cluster.Spec.Plan = "standard-16" // originally "standard-8"
 
 		controllerResult := reconciler.handleUpgrade(ctx, testApiKey, cluster)
-		assert.Equal(t, controllerResult, ctrl.Result{RequeueAfter: 3 * time.Minute})
+		assert.Equal(t, controllerResult.RequeueAfter, 3*time.Minute)
 		upgradingCondition := meta.FindStatusCondition(cluster.Status.Conditions, v1beta1.ConditionUpgrading)
 		if assert.Check(t, upgradingCondition != nil) {
 			assert.Equal(t, upgradingCondition.Status, metav1.ConditionTrue)
@@ -506,7 +506,7 @@ func TestHandleUpgrade(t *testing.T) {
 		cluster.Spec.PostgresVersion = 16 // originally "15"
 
 		controllerResult := reconciler.handleUpgrade(ctx, testApiKey, cluster)
-		assert.Equal(t, controllerResult, ctrl.Result{RequeueAfter: 3 * time.Minute})
+		assert.Equal(t, controllerResult.RequeueAfter, 3*time.Minute)
 		upgradingCondition := meta.FindStatusCondition(cluster.Status.Conditions, v1beta1.ConditionUpgrading)
 		if assert.Check(t, upgradingCondition != nil) {
 			assert.Equal(t, upgradingCondition.Status, metav1.ConditionTrue)
@@ -528,7 +528,7 @@ func TestHandleUpgrade(t *testing.T) {
 		cluster.Spec.Storage = resource.MustParse("15Gi") // originally "10Gi"
 
 		controllerResult := reconciler.handleUpgrade(ctx, testApiKey, cluster)
-		assert.Equal(t, controllerResult, ctrl.Result{RequeueAfter: 3 * time.Minute})
+		assert.Equal(t, controllerResult.RequeueAfter, 3*time.Minute)
 		upgradingCondition := meta.FindStatusCondition(cluster.Status.Conditions, v1beta1.ConditionUpgrading)
 		if assert.Check(t, upgradingCondition != nil) {
 			assert.Equal(t, upgradingCondition.Status, metav1.ConditionTrue)
@@ -592,7 +592,7 @@ func TestHandleUpgradeHA(t *testing.T) {
 		cluster.Spec.IsHA = true // originally "false"
 
 		controllerResult := reconciler.handleUpgradeHA(ctx, testApiKey, cluster)
-		assert.Equal(t, controllerResult, ctrl.Result{RequeueAfter: 3 * time.Minute})
+		assert.Equal(t, controllerResult.RequeueAfter, 3*time.Minute)
 		upgradingCondition := meta.FindStatusCondition(cluster.Status.Conditions, v1beta1.ConditionUpgrading)
 		if assert.Check(t, upgradingCondition != nil) {
 			assert.Equal(t, upgradingCondition.Status, metav1.ConditionTrue)
@@ -613,7 +613,7 @@ func TestHandleUpgradeHA(t *testing.T) {
 		cluster.Status.ID = "2345"
 
 		controllerResult := reconciler.handleUpgradeHA(ctx, testApiKey, cluster)
-		assert.Equal(t, controllerResult, ctrl.Result{RequeueAfter: 3 * time.Minute})
+		assert.Equal(t, controllerResult.RequeueAfter, 3*time.Minute)
 		upgradingCondition := meta.FindStatusCondition(cluster.Status.Conditions, v1beta1.ConditionUpgrading)
 		if assert.Check(t, upgradingCondition != nil) {
 			assert.Equal(t, upgradingCondition.Status, metav1.ConditionTrue)
@@ -672,7 +672,7 @@ func TestHandleUpdate(t *testing.T) {
 		cluster.Spec.ClusterName = "new-cluster-name" // originally "hippo-cluster"
 
 		controllerResult := reconciler.handleUpdate(ctx, testApiKey, cluster)
-		assert.Equal(t, controllerResult, ctrl.Result{RequeueAfter: 3 * time.Minute})
+		assert.Equal(t, controllerResult.RequeueAfter, 3*time.Minute)
 		upgradingCondition := meta.FindStatusCondition(cluster.Status.Conditions, v1beta1.ConditionUpgrading)
 		if assert.Check(t, upgradingCondition != nil) {
 			assert.Equal(t, upgradingCondition.Status, metav1.ConditionTrue)
@@ -690,7 +690,7 @@ func TestHandleUpdate(t *testing.T) {
 		cluster.Spec.IsProtected = true // originally "false"
 
 		controllerResult := reconciler.handleUpdate(ctx, testApiKey, cluster)
-		assert.Equal(t, controllerResult, ctrl.Result{RequeueAfter: 3 * time.Minute})
+		assert.Equal(t, controllerResult.RequeueAfter, 3*time.Minute)
 		upgradingCondition := meta.FindStatusCondition(cluster.Status.Conditions, v1beta1.ConditionUpgrading)
 		if assert.Check(t, upgradingCondition != nil) {
 			assert.Equal(t, upgradingCondition.Status, metav1.ConditionTrue)

internal/bridge/crunchybridgecluster/delete.go

@@ -22,6 +22,8 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
+	"github.com/crunchydata/postgres-operator/internal/controller/runtime"
+	"github.com/crunchydata/postgres-operator/internal/initialize"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
@@ -58,7 +60,7 @@ func (r *CrunchyBridgeClusterReconciler) handleDelete(
 			}
 
 			if !deletedAlready {
-				return &ctrl.Result{RequeueAfter: 1 * time.Second}, err
+				return initialize.Pointer(runtime.RequeueWithoutBackoff(time.Second)), err
 			}
 
 			// Remove finalizer if deleted already

internal/bridge/crunchybridgecluster/delete_test.go

@@ -85,7 +85,7 @@ func TestHandleDeleteCluster(t *testing.T) {
 		cluster.Status.ID = "1234"
 		controllerResult, err = reconciler.handleDelete(ctx, cluster, "9012")
 		assert.NilError(t, err)
-		assert.Equal(t, *controllerResult, ctrl.Result{RequeueAfter: 1 * time.Second})
+		assert.Equal(t, controllerResult.RequeueAfter, 1*time.Second)
 		assert.Equal(t, len(testBridgeClient.Clusters), 1)
 		assert.Equal(t, testBridgeClient.Clusters[0].ClusterName, "bridge-cluster-2")
 

internal/bridge/installation.go

@@ -131,13 +131,15 @@ func (r *InstallationReconciler) Reconcile(
 		result.RequeueAfter, err = r.reconcile(ctx, secret)
 	}
 
-	// TODO: Check for corev1.NamespaceTerminatingCause after
-	// k8s.io/[email protected]; see https://issue.k8s.io/108528.
+	// Nothing can be written to a deleted namespace.
+	if err != nil && apierrors.HasStatusCause(err, corev1.NamespaceTerminatingCause) {
+		return runtime.ErrorWithoutBackoff(err)
+	}
 
 	// Write conflicts are returned as errors; log and retry with backoff.
 	if err != nil && apierrors.IsConflict(err) {
 		logging.FromContext(ctx).Info("Requeue", "reason", err)
-		err, result.Requeue, result.RequeueAfter = nil, true, 0
+		return runtime.RequeueWithBackoff(), nil
 	}
 
 	return result, err

internal/controller/pgupgrade/pgupgrade_controller.go

@@ -31,6 +31,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 
 	"github.com/crunchydata/postgres-operator/internal/config"
+	"github.com/crunchydata/postgres-operator/internal/controller/runtime"
 	"github.com/crunchydata/postgres-operator/internal/registration"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
@@ -493,17 +494,15 @@ func (r *PGUpgradeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		}
 
 		// Requeue to verify that Patroni endpoints are deleted
-		return ctrl.Result{Requeue: true}, err // FIXME
+		return runtime.RequeueWithBackoff(), err // FIXME
 	}
 
 	// TODO: write upgradeJob back to world? No, we will wake and see it when it
 	// has some progress. OTOH, whatever we just wrote has the latest metadata.generation.
 	// TODO: consider what it means to "re-use" the same PGUpgrade for more than
 	// one postgres version. Should the job name include the version number?
 
-	log.Info("Reconciled", "requeue", err != nil ||
-		result.Requeue ||
-		result.RequeueAfter > 0)
+	log.Info("Reconciled", "requeue", !result.IsZero() || err != nil)
 	return
 }