base reconciler reconciles status (#752)

* base reconciler reconciles status * Update pkg/library/reconcilers/base_reconciler.go Co-authored-by: Jim Fitzpatrick <jfitzpat@redhat.com> --------- Co-authored-by: Jim Fitzpatrick <jfitzpat@redhat.com>
Kuadrant · Jul 22, 2024 · 4f7c0a3 · 4f7c0a3
1 parent 7745a19
commit 4f7c0a3
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 46 deletions.
diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go
@@ -22,13 +22,15 @@ import (
 
 	"github.com/go-logr/logr"
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1"
 	gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
 
 	kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi"
 	"github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant"
+	"github.com/kuadrant/kuadrant-operator/pkg/library/reconcilers"
 	"github.com/kuadrant/kuadrant-operator/pkg/library/utils"
 )
 
@@ -157,9 +159,7 @@ type RateLimitPolicyCommonSpec struct {
 
 // RateLimitPolicyStatus defines the observed state of RateLimitPolicy
 type RateLimitPolicyStatus struct {
-	// ObservedGeneration reflects the generation of the most recently observed spec.
-	// +optional
-	ObservedGeneration int64 `json:"observedGeneration,omitempty"`
+	reconcilers.StatusMeta `json:",inline"`
 
 	// Represents the observations of a foo's current state.
 	// Known .status.conditions.type are: "Available"
@@ -170,25 +170,33 @@ type RateLimitPolicyStatus struct {
 	Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
 }
 
-func (s *RateLimitPolicyStatus) Equals(other *RateLimitPolicyStatus, logger logr.Logger) bool {
-	if s.ObservedGeneration != other.ObservedGeneration {
-		diff := cmp.Diff(s.ObservedGeneration, other.ObservedGeneration)
-		logger.V(1).Info("ObservedGeneration not equal", "difference", diff)
-		return false
-	}
+func RateLimitPolicyStatusMutator(desiredStatus *RateLimitPolicyStatus, logger logr.Logger) reconcilers.StatusMutatorFunc {
+	return func(obj client.Object) (bool, error) {
+		existingRLP, ok := obj.(*RateLimitPolicy)
+		if !ok {
+			return false, fmt.Errorf("unsupported object type %T", obj)
+		}
+
+		opts := cmp.Options{
+			cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime"),
+			cmpopts.IgnoreMapEntries(func(k string, _ any) bool {
+				return k == "lastTransitionTime"
+			}),
+		}
+
+		if cmp.Equal(*desiredStatus, existingRLP.Status, opts) {
+			return false, nil
+		}
 
-	// Marshalling sorts by condition type
-	currentMarshaledJSON, _ := kuadrant.ConditionMarshal(s.Conditions)
-	otherMarshaledJSON, _ := kuadrant.ConditionMarshal(other.Conditions)
-	if string(currentMarshaledJSON) != string(otherMarshaledJSON) {
 		if logger.V(1).Enabled() {
-			diff := cmp.Diff(string(currentMarshaledJSON), string(otherMarshaledJSON))
-			logger.V(1).Info("Conditions not equal", "difference", diff)
+			diff := cmp.Diff(*desiredStatus, existingRLP.Status, opts)
+			logger.V(1).Info("status not equal", "difference", diff)
 		}
-		return false
-	}
 
-	return true
+		existingRLP.Status = *desiredStatus
+
+		return true, nil
+	}
 }
 
 func (s *RateLimitPolicyStatus) GetConditions() []metav1.Condition {
@@ -226,6 +234,9 @@ func (r *RateLimitPolicy) Validate() error {
 	return nil
 }
 
+func (r *RateLimitPolicy) GetObservedGeneration() int64  { return r.Status.GetObservedGeneration() }
+func (r *RateLimitPolicy) SetObservedGeneration(o int64) { r.Status.SetObservedGeneration(o) }
+
 //+kubebuilder:object:root=true
 
 // RateLimitPolicyList contains a list of RateLimitPolicy

diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go
diff --git a/controllers/ratelimitpolicy_status.go b/controllers/ratelimitpolicy_status.go
@@ -2,13 +2,13 @@ package controllers
 
 import (
 	"context"
-	"fmt"
 	"slices"
 
 	"github.com/go-logr/logr"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
 
@@ -19,46 +19,32 @@ import (
 func (r *RateLimitPolicyReconciler) reconcileStatus(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, specErr error) (ctrl.Result, error) {
 	logger, _ := logr.FromContext(ctx)
 	newStatus := r.calculateStatus(rlp, specErr)
-
-	equalStatus := rlp.Status.Equals(newStatus, logger)
-	logger.V(1).Info("Status", "status is different", !equalStatus)
-	logger.V(1).Info("Status", "generation is different", rlp.Generation != rlp.Status.ObservedGeneration)
-	if equalStatus && rlp.Generation == rlp.Status.ObservedGeneration {
-		// Steady state
-		logger.V(1).Info("Status was not updated")
-		return reconcile.Result{}, nil
-	}
-
-	// Save the generation number we acted on, otherwise we might wrongfully indicate
-	// that we've seen a spec update when we retry.
-	// TODO: This can clobber an update if we allow multiple agents to write to the
-	// same status.
-	newStatus.ObservedGeneration = rlp.Generation
-
-	logger.V(1).Info("Updating Status", "sequence no:", fmt.Sprintf("sequence No: %v->%v", rlp.Status.ObservedGeneration, newStatus.ObservedGeneration))
-
-	rlp.Status = *newStatus
-	updateErr := r.Client().Status().Update(ctx, rlp)
-	logger.V(1).Info("Updating Status", "err", updateErr)
-	if updateErr != nil {
+	if err := r.ReconcileResourceStatus(
+		ctx,
+		client.ObjectKeyFromObject(rlp),
+		&kuadrantv1beta2.RateLimitPolicy{},
+		kuadrantv1beta2.RateLimitPolicyStatusMutator(newStatus, logger),
+	); err != nil {
 		// Ignore conflicts, resource might just be outdated.
-		if apierrors.IsConflict(updateErr) {
-			logger.Info("Failed to update status: resource might just be outdated")
+		if apierrors.IsConflict(err) {
+			logger.V(1).Info("Failed to update status: resource might just be outdated")
 			return reconcile.Result{Requeue: true}, nil
 		}
 
-		return reconcile.Result{}, fmt.Errorf("failed to update status: %w", updateErr)
+		return ctrl.Result{}, err
 	}
+
 	return ctrl.Result{}, nil
 }
 
 func (r *RateLimitPolicyReconciler) calculateStatus(rlp *kuadrantv1beta2.RateLimitPolicy, specErr error) *kuadrantv1beta2.RateLimitPolicyStatus {
 	newStatus := &kuadrantv1beta2.RateLimitPolicyStatus{
 		// Copy initial conditions. Otherwise, status will always be updated
-		Conditions:         slices.Clone(rlp.Status.Conditions),
-		ObservedGeneration: rlp.Status.ObservedGeneration,
+		Conditions: slices.Clone(rlp.Status.Conditions),
 	}
 
+	newStatus.SetObservedGeneration(rlp.GetGeneration())
+
 	acceptedCond := kuadrant.AcceptedCondition(rlp, specErr)
 
 	meta.SetStatusCondition(&newStatus.Conditions, *acceptedCond)

diff --git a/pkg/library/reconcilers/base_reconciler.go b/pkg/library/reconcilers/base_reconciler.go
@@ -34,6 +34,32 @@ import (
 	"github.com/kuadrant/kuadrant-operator/pkg/library/utils"
 )
 
+type StatusMeta struct {
+	// ObservedGeneration reflects the generation of the most recently observed spec.
+	// +optional
+	ObservedGeneration int64 `json:"observedGeneration,omitempty"`
+}
+
+func (meta *StatusMeta) GetObservedGeneration() int64  { return meta.ObservedGeneration }
+func (meta *StatusMeta) SetObservedGeneration(o int64) { meta.ObservedGeneration = o }
+
+// StatusMutator is an interface to hold mutator functions for status updates.
+type StatusMutator interface {
+	Mutate(obj client.Object) (bool, error)
+}
+
+// StatusMutatorFunc is a function adaptor for StatusMutators.
+type StatusMutatorFunc func(client.Object) (bool, error)
+
+// Mutate adapts the MutatorFunc to fit through the StatusMutator interface.
+func (s StatusMutatorFunc) Mutate(o client.Object) (bool, error) {
+	if s == nil {
+		return false, nil
+	}
+
+	return s(o)
+}
+
 // MutateFn is a function which mutates the existing object into it's desired state.
 type MutateFn func(existing, desired client.Object) (bool, error)
 
@@ -143,6 +169,36 @@ func (b *BaseReconciler) ReconcileResource(ctx context.Context, obj, desired cli
 	return nil
 }
 
+func (b *BaseReconciler) ReconcileResourceStatus(ctx context.Context, objKey client.ObjectKey, obj client.Object, mutator StatusMutator) error {
+	logger, err := logr.FromContext(ctx)
+	if err != nil {
+		return err
+	}
+
+	if err := b.Client().Get(ctx, objKey, obj); err != nil {
+		return err
+	}
+
+	update, err := mutator.Mutate(obj)
+	if err != nil {
+		return err
+	}
+
+	if !update {
+		// Steady state, early return 🎉
+		logger.V(1).Info("status was not updated")
+		return nil
+	}
+
+	updateErr := b.Client().Status().Update(ctx, obj)
+	logger.V(1).Info("updating status", "err", updateErr)
+	if updateErr != nil {
+		return updateErr
+	}
+
+	return nil
+}
+
 func (b *BaseReconciler) GetResource(ctx context.Context, objKey types.NamespacedName, obj client.Object) error {
 	logger, _ := logr.FromContext(ctx)
 	logger.Info("get object", "kind", strings.Replace(fmt.Sprintf("%T", obj), "*", "", 1), "name", objKey.Name, "namespace", objKey.Namespace)