Change Reconciler implementations to ObjectReconciler

Each implementation was doing the first fetch of its main object a bit
differently. The controller-runtime module can do this since v0.17.0
and Go generics.

Author: cbandy

internal/bridge/crunchybridgecluster/crunchybridgecluster_controller.go

@@ -21,6 +21,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/crunchydata/postgres-operator/internal/bridge"
 	"github.com/crunchydata/postgres-operator/internal/controller/runtime"
@@ -50,8 +51,8 @@ type CrunchyBridgeClusterReconciler struct {
 	}
 }
 
-//+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="crunchybridgeclusters",verbs={list,watch}
-//+kubebuilder:rbac:groups="",resources="secrets",verbs={list,watch}
+//+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="crunchybridgeclusters",verbs={get,list,watch}
+//+kubebuilder:rbac:groups="",resources="secrets",verbs={get,list,watch}
 
 // ManagedReconciler creates a [CrunchyBridgeClusterReconciler] and adds it to m.
 func ManagedReconciler(m ctrl.Manager, newClient func() bridge.ClientInterface) error {
@@ -72,7 +73,7 @@ func ManagedReconciler(m ctrl.Manager, newClient func() bridge.ClientInterface)
 		// Smarter: retry after a certain time for each cluster
 		WatchesRawSource(
 			runtime.NewTickerImmediate(5*time.Minute, event.GenericEvent{},
-				handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, _ client.Object) []ctrl.Request {
+				handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, _ client.Object) []reconcile.Request {
 					var list v1beta1.CrunchyBridgeClusterList
 					_ = reconciler.Reader.List(ctx, &list)
 					return runtime.Requests(initialize.Pointers(list.Items...)...)
@@ -82,11 +83,11 @@ func ManagedReconciler(m ctrl.Manager, newClient func() bridge.ClientInterface)
 		// Watch secrets and filter for secrets mentioned by CrunchyBridgeClusters
 		Watches(
 			&corev1.Secret{},
-			handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, secret client.Object) []ctrl.Request {
+			handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, secret client.Object) []reconcile.Request {
 				return runtime.Requests(reconciler.findCrunchyBridgeClustersForSecret(ctx, client.ObjectKeyFromObject(secret))...)
 			}),
 		).
-		Complete(reconciler)
+		Complete(reconcile.AsReconciler(kubernetes, reconciler))
 }
 
 // The owner reference created by controllerutil.SetControllerReference blocks
@@ -105,47 +106,32 @@ func (r *CrunchyBridgeClusterReconciler) setControllerReference(
 	return controllerutil.SetControllerReference(owner, controlled, runtime.Scheme)
 }
 
-//+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="crunchybridgeclusters",verbs={get,patch,update}
+//+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="crunchybridgeclusters",verbs={patch,update}
 //+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="crunchybridgeclusters/status",verbs={patch,update}
 //+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="crunchybridgeclusters/finalizers",verbs={patch,update}
 //+kubebuilder:rbac:groups="",resources="secrets",verbs={get}
 
 // Reconcile does the work to move the current state of the world toward the
-// desired state described in a [v1beta1.CrunchyBridgeCluster] identified by req.
-func (r *CrunchyBridgeClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+// desired state described in crunchybridgecluster.
+func (r *CrunchyBridgeClusterReconciler) Reconcile(ctx context.Context, crunchybridgecluster *v1beta1.CrunchyBridgeCluster) (ctrl.Result, error) {
+	var err error
 	ctx, span := tracing.Start(ctx, "reconcile-crunchybridgecluster")
 	log := logging.FromContext(ctx)
 	defer span.End()
 
-	// Retrieve the crunchybridgecluster from the client cache, if it exists. A deferred
-	// function below will send any changes to its Status field.
-	//
-	// NOTE: No DeepCopy is necessary here because controller-runtime makes a
-	// copy before returning from its cache.
-	// - https://github.com/kubernetes-sigs/controller-runtime/issues/1235
-	crunchybridgecluster := &v1beta1.CrunchyBridgeCluster{}
-	err := r.Reader.Get(ctx, req.NamespacedName, crunchybridgecluster)
+	// Write any changes to the crunchybridgecluster status on the way out.
+	before := crunchybridgecluster.DeepCopy()
+	defer func() {
+		if !equality.Semantic.DeepEqual(before.Status, crunchybridgecluster.Status) {
+			status := r.StatusWriter.Patch(ctx, crunchybridgecluster, client.MergeFrom(before))
 
-	if err == nil {
-		// Write any changes to the crunchybridgecluster status on the way out.
-		before := crunchybridgecluster.DeepCopy()
-		defer func() {
-			if !equality.Semantic.DeepEqual(before.Status, crunchybridgecluster.Status) {
-				status := r.StatusWriter.Patch(ctx, crunchybridgecluster, client.MergeFrom(before))
-
-				if err == nil && status != nil {
-					err = status
-				} else if status != nil {
-					log.Error(status, "Patching CrunchyBridgeCluster status")
-				}
+			if err == nil && status != nil {
+				err = status
+			} else if status != nil {
+				log.Error(status, "Patching CrunchyBridgeCluster status")
 			}
-		}()
-	} else {
-		// NotFound cannot be fixed by requeuing so ignore it. During background
-		// deletion, we receive delete events from crunchybridgecluster's dependents after
-		// crunchybridgecluster is deleted.
-		return ctrl.Result{}, tracing.Escape(span, client.IgnoreNotFound(err))
-	}
+		}
+	}()
 
 	// Get and validate connection secret for requests
 	key, team, err := r.reconcileBridgeConnectionSecret(ctx, crunchybridgecluster)

internal/controller/pgupgrade/pgupgrade_controller.go

@@ -17,6 +17,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/crunchydata/postgres-operator/internal/config"
 	"github.com/crunchydata/postgres-operator/internal/controller/runtime"
@@ -49,9 +50,9 @@ type PGUpgradeReconciler struct {
 	}
 }
 
-//+kubebuilder:rbac:groups="batch",resources="jobs",verbs={list,watch}
-//+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgupgrades",verbs={list,watch}
-//+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="postgresclusters",verbs={list,watch}
+//+kubebuilder:rbac:groups="batch",resources="jobs",verbs={get,list,watch}
+//+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgupgrades",verbs={get,list,watch}
+//+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="postgresclusters",verbs={get,list,watch}
 
 // ManagedReconciler creates a [PGUpgradeReconciler] and adds it to m.
 func ManagedReconciler(m ctrl.Manager, r registration.Registration) error {
@@ -71,11 +72,11 @@ func ManagedReconciler(m ctrl.Manager, r registration.Registration) error {
 		Owns(&batchv1.Job{}).
 		Watches(
 			v1beta1.NewPostgresCluster(),
-			handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, cluster client.Object) []ctrl.Request {
+			handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, cluster client.Object) []reconcile.Request {
 				return runtime.Requests(reconciler.findUpgradesForPostgresCluster(ctx, client.ObjectKeyFromObject(cluster))...)
 			}),
 		).
-		Complete(reconciler)
+		Complete(reconcile.AsReconciler(kubernetes, reconciler))
 }
 
 //+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgupgrades",verbs={list}
@@ -103,7 +104,6 @@ func (r *PGUpgradeReconciler) findUpgradesForPostgresCluster(
 	return matching
 }
 
-//+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgupgrades",verbs={get}
 //+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgupgrades/status",verbs={patch}
 //+kubebuilder:rbac:groups="batch",resources="jobs",verbs={delete}
 //+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="postgresclusters",verbs={get}
@@ -114,42 +114,26 @@ func (r *PGUpgradeReconciler) findUpgradesForPostgresCluster(
 //+kubebuilder:rbac:groups="",resources="endpoints",verbs={delete}
 
 // Reconcile does the work to move the current state of the world toward the
-// desired state described in a [v1beta1.PGUpgrade] identified by req.
-func (r *PGUpgradeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, err error) {
+// desired state described in upgrade.
+func (r *PGUpgradeReconciler) Reconcile(ctx context.Context, upgrade *v1beta1.PGUpgrade) (result ctrl.Result, err error) {
 	ctx, span := tracing.Start(ctx, "reconcile-pgupgrade")
 	log := logging.FromContext(ctx)
 	defer span.End()
 	defer func(s tracing.Span) { _ = tracing.Escape(s, err) }(span)
 
-	// Retrieve the upgrade from the client cache, if it exists. A deferred
-	// function below will send any changes to its Status field.
-	//
-	// NOTE: No DeepCopy is necessary here because controller-runtime makes a
-	// copy before returning from its cache.
-	// - https://github.com/kubernetes-sigs/controller-runtime/issues/1235
-	upgrade := &v1beta1.PGUpgrade{}
-	err = r.Reader.Get(ctx, req.NamespacedName, upgrade)
-
-	if err == nil {
-		// Write any changes to the upgrade status on the way out.
-		before := upgrade.DeepCopy()
-		defer func() {
-			if !equality.Semantic.DeepEqual(before.Status, upgrade.Status) {
-				status := r.StatusWriter.Patch(ctx, upgrade, client.MergeFrom(before))
-
-				if err == nil && status != nil {
-					err = status
-				} else if status != nil {
-					log.Error(status, "Patching PGUpgrade status")
-				}
+	// Write any changes to the upgrade status on the way out.
+	before := upgrade.DeepCopy()
+	defer func() {
+		if !equality.Semantic.DeepEqual(before.Status, upgrade.Status) {
+			status := r.StatusWriter.Patch(ctx, upgrade, client.MergeFrom(before))
+
+			if err == nil && status != nil {
+				err = status
+			} else if status != nil {
+				log.Error(status, "Patching PGUpgrade status")
 			}
-		}()
-	} else {
-		// NotFound cannot be fixed by requeuing so ignore it. During background
-		// deletion, we receive delete events from upgrade's dependents after
-		// upgrade is deleted.
-		return ctrl.Result{}, client.IgnoreNotFound(err)
-	}
+		}
+	}()
 
 	// Validate the remainder of the upgrade specification. These can likely
 	// move to CEL rules or a webhook when supported.

internal/controller/postgrescluster/cluster_test.go

@@ -18,7 +18,6 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/crunchydata/postgres-operator/internal/controller/runtime"
 	"github.com/crunchydata/postgres-operator/internal/feature"
@@ -100,9 +99,7 @@ func TestCustomLabels(t *testing.T) {
 		})
 
 		// Reconcile the cluster
-		result, err := reconciler.Reconcile(ctx, reconcile.Request{
-			NamespacedName: client.ObjectKeyFromObject(cluster),
-		})
+		result, err := reconciler.Reconcile(ctx, cluster)
 		assert.NilError(t, err)
 		assert.Assert(t, result.Requeue == false)
 	}
@@ -339,9 +336,7 @@ func TestCustomAnnotations(t *testing.T) {
 		})
 
 		// Reconcile the cluster
-		result, err := reconciler.Reconcile(ctx, reconcile.Request{
-			NamespacedName: client.ObjectKeyFromObject(cluster),
-		})
+		result, err := reconciler.Reconcile(ctx, cluster)
 		assert.NilError(t, err)
 		assert.Assert(t, result.Requeue == false)
 	}

internal/controller/postgrescluster/controller.go

@@ -69,29 +69,15 @@ type Reconciler struct {
 }
 
 // +kubebuilder:rbac:groups="",resources="events",verbs={create,patch}
-// +kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="postgresclusters",verbs={get,list,watch}
 // +kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="postgresclusters/status",verbs={patch}
 
-// Reconcile reconciles a ConfigMap in a namespace managed by the PostgreSQL Operator
 func (r *Reconciler) Reconcile(
-	ctx context.Context, request reconcile.Request) (reconcile.Result, error,
+	ctx context.Context, cluster *v1beta1.PostgresCluster) (reconcile.Result, error,
 ) {
 	ctx, span := tracing.Start(ctx, "reconcile-postgrescluster")
 	log := logging.FromContext(ctx)
 	defer span.End()
 
-	// get the postgrescluster from the cache
-	cluster := &v1beta1.PostgresCluster{}
-	if err := r.Reader.Get(ctx, request.NamespacedName, cluster); err != nil {
-		// NotFound cannot be fixed by requeuing so ignore it. During background
-		// deletion, we receive delete events from cluster's dependents after
-		// cluster is deleted.
-		if err = client.IgnoreNotFound(err); err != nil {
-			log.Error(err, "unable to fetch PostgresCluster")
-		}
-		return runtime.ErrorWithBackoff(tracing.Escape(span, err))
-	}
-
 	// Set any defaults that may not have been stored in the API. No DeepCopy
 	// is necessary because controller-runtime makes a copy before returning
 	// from its cache.
@@ -455,6 +441,7 @@ func (r *Reconciler) setOwnerReference(
 // +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources="rolebindings",verbs={get,list,watch}
 // +kubebuilder:rbac:groups="batch",resources="cronjobs",verbs={get,list,watch}
 // +kubebuilder:rbac:groups="policy",resources="poddisruptionbudgets",verbs={get,list,watch}
+// +kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="postgresclusters",verbs={get,list,watch}
 
 // ManagedReconciler creates a [Reconciler] and adds it to m.
 func ManagedReconciler(m manager.Manager, r registration.Registration) error {
@@ -489,5 +476,5 @@ func ManagedReconciler(m manager.Manager, r registration.Registration) error {
 		Watches(&corev1.Pod{}, reconciler.watchPods()).
 		Watches(&appsv1.StatefulSet{},
 			reconciler.controllerRefHandlerFuncs()). // watch all StatefulSets
-		Complete(reconciler))
+		Complete(reconcile.AsReconciler(kubernetes, reconciler)))
 }

internal/controller/postgrescluster/controller_test.go

@@ -166,9 +166,7 @@ var _ = Describe("PostgresCluster Reconciler", func() {
 	reconcile := func(cluster *v1beta1.PostgresCluster) reconcile.Result {
 		ctx := context.Background()
 
-		result, err := test.Reconciler.Reconcile(ctx,
-			reconcile.Request{NamespacedName: client.ObjectKeyFromObject(cluster)},
-		)
+		result, err := test.Reconciler.Reconcile(ctx, cluster)
 		Expect(err).ToNot(HaveOccurred(), func() string {
 			var t interface{ StackTrace() errors.StackTrace }
 			if errors.As(err, &t) {

internal/controller/postgrescluster/instance_test.go

@@ -29,7 +29,6 @@ import (
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/crunchydata/postgres-operator/internal/collector"
 	"github.com/crunchydata/postgres-operator/internal/controller/runtime"
@@ -1233,9 +1232,7 @@ func TestDeleteInstance(t *testing.T) {
 
 	// Reconcile the entire cluster so that we don't have to create all the
 	// resources needed to reconcile a single instance (cm,secrets,svc, etc.)
-	result, err := reconciler.Reconcile(ctx, reconcile.Request{
-		NamespacedName: client.ObjectKeyFromObject(cluster),
-	})
+	result, err := reconciler.Reconcile(ctx, cluster)
 	assert.NilError(t, err)
 	assert.Assert(t, result.Requeue == false)