From a2689a42d2e15c514116542dfe61548e8df6a09f Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Thu, 14 Aug 2025 22:49:36 -0500 Subject: [PATCH] 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. --- .../crunchybridgecluster_controller.go | 56 +++++++------------ .../pgupgrade/pgupgrade_controller.go | 56 +++++++------------ .../postgrescluster/cluster_test.go | 9 +-- .../controller/postgrescluster/controller.go | 19 +------ .../postgrescluster/controller_test.go | 4 +- .../postgrescluster/instance_test.go | 5 +- .../standalone_pgadmin/controller.go | 32 ++++------- 7 files changed, 60 insertions(+), 121 deletions(-) diff --git a/internal/bridge/crunchybridgecluster/crunchybridgecluster_controller.go b/internal/bridge/crunchybridgecluster/crunchybridgecluster_controller.go index 98f3897c01..8a3280f512 100644 --- a/internal/bridge/crunchybridgecluster/crunchybridgecluster_controller.go +++ b/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) diff --git a/internal/controller/pgupgrade/pgupgrade_controller.go b/internal/controller/pgupgrade/pgupgrade_controller.go index 653ea9e55e..e0864604f5 100644 --- a/internal/controller/pgupgrade/pgupgrade_controller.go +++ b/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. diff --git a/internal/controller/postgrescluster/cluster_test.go b/internal/controller/postgrescluster/cluster_test.go index c56947a834..5ea4ace886 100644 --- a/internal/controller/postgrescluster/cluster_test.go +++ b/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) } diff --git a/internal/controller/postgrescluster/controller.go b/internal/controller/postgrescluster/controller.go index 09ddf15834..7d015c4012 100644 --- a/internal/controller/postgrescluster/controller.go +++ b/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))) } diff --git a/internal/controller/postgrescluster/controller_test.go b/internal/controller/postgrescluster/controller_test.go index a6f237b81a..95358d488b 100644 --- a/internal/controller/postgrescluster/controller_test.go +++ b/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) { diff --git a/internal/controller/postgrescluster/instance_test.go b/internal/controller/postgrescluster/instance_test.go index 8a028913e5..f00267974b 100644 --- a/internal/controller/postgrescluster/instance_test.go +++ b/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) diff --git a/internal/controller/standalone_pgadmin/controller.go b/internal/controller/standalone_pgadmin/controller.go index fe205dcaf6..7e3d0c8355 100644 --- a/internal/controller/standalone_pgadmin/controller.go +++ b/internal/controller/standalone_pgadmin/controller.go @@ -14,10 +14,12 @@ import ( "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" - ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/crunchydata/postgres-operator/internal/controller/runtime" "github.com/crunchydata/postgres-operator/internal/logging" @@ -48,7 +50,7 @@ type PGAdminReconciler struct { Recorder record.EventRecorder } -//+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgadmins",verbs={list,watch} +//+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgadmins",verbs={get,list,watch} //+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="postgresclusters",verbs={list,watch} //+kubebuilder:rbac:groups="",resources="persistentvolumeclaims",verbs={list,watch} //+kubebuilder:rbac:groups="",resources="secrets",verbs={list,watch} @@ -56,7 +58,7 @@ type PGAdminReconciler struct { //+kubebuilder:rbac:groups="apps",resources="statefulsets",verbs={list,watch} // ManagedReconciler creates a [PGAdminReconciler] and adds it to m. -func ManagedReconciler(m ctrl.Manager) error { +func ManagedReconciler(m manager.Manager) error { exec, err := runtime.NewPodExecutor(m.GetConfig()) kubernetes := client.WithFieldOwner(m.GetClient(), naming.ControllerPGAdmin) recorder := m.GetEventRecorderFor(naming.ControllerPGAdmin) @@ -69,7 +71,7 @@ func ManagedReconciler(m ctrl.Manager) error { Writer: kubernetes, } - return errors.Join(err, ctrl.NewControllerManagedBy(m). + return errors.Join(err, builder.ControllerManagedBy(m). For(&v1beta1.PGAdmin{}). Owns(&corev1.ConfigMap{}). Owns(&corev1.PersistentVolumeClaim{}). @@ -78,39 +80,29 @@ func ManagedReconciler(m ctrl.Manager) error { Owns(&corev1.Service{}). 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.findPGAdminsForPostgresCluster(ctx, cluster)...) }), ). 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.findPGAdminsForSecret(ctx, client.ObjectKeyFromObject(secret))...) }), ). - Complete(reconciler)) + Complete(reconcile.AsReconciler(kubernetes, reconciler))) } -//+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgadmins",verbs={get} //+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgadmins/status",verbs={patch} -// Reconcile which aims to move the current state of the pgAdmin closer to the -// desired state described in a [v1beta1.PGAdmin] identified by request. -func (r *PGAdminReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +// Reconcile moves the current state of pgAdmin closer to the state described in its specification. +func (r *PGAdminReconciler) Reconcile(ctx context.Context, pgAdmin *v1beta1.PGAdmin) (reconcile.Result, error) { var err error ctx, span := tracing.Start(ctx, "reconcile-pgadmin") log := logging.FromContext(ctx) defer span.End() - pgAdmin := &v1beta1.PGAdmin{} - if err := r.Reader.Get(ctx, req.NamespacedName, pgAdmin); err != nil { - // NotFound cannot be fixed by requeuing so ignore it. During background - // deletion, we receive delete events from pgadmin's dependents after - // pgadmin is deleted. - return ctrl.Result{}, tracing.Escape(span, client.IgnoreNotFound(err)) - } - // Write any changes to the pgadmin status on the way out. before := pgAdmin.DeepCopy() defer func() { @@ -163,7 +155,7 @@ func (r *PGAdminReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct log.V(1).Info("Reconciled pgAdmin") } - return ctrl.Result{}, tracing.Escape(span, err) + return reconcile.Result{}, tracing.Escape(span, err) } // The owner reference created by controllerutil.SetControllerReference blocks