Make generic SummarizeAndPatch()

summarizeAndPatch() was used by all the reconcilers with their own
object type. This creates a generic SummarizeAndPatch helper that takes
a conditions.Setter object and performs the same operations. All the
reconcilers are updated to use SummarizeAndPatch(). The process of
summarize and patch can be configured using the HelperOptions.

Introduce ResultProcessor to allow injecting middlewares in the
SummarizeAndPatch process.

Introduce RuntimeResultBuilder to allow defining how the reconciliation
result is computed for specific reconciler. This enabled different
reconcilers to have different meanings of the reconciliation results.

Introduce Conditions in summary package to store all the status
conditions related information of a reconciler. This is passed to
SummarizeAndPatch() to be used for summary and patch calculation.

Remove all the redundant summarizeAndPatch() tests per reconciler.

Add package internal/object containing helpers for interacting with
runtime.Object needed by the generic SummarizeAndPatch().

Add tests for ComputeReconcileResult().

Signed-off-by: Sunny <[email protected]>

Author: darkowlzz

controllers/bucket_controller.go

@@ -37,10 +37,8 @@ import (
 	"golang.org/x/sync/semaphore"
 	"google.golang.org/api/option"
 	corev1 "k8s.io/api/core/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	kuberecorder "k8s.io/client-go/tools/record"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -57,33 +55,33 @@ import (
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
 	serror "github.com/fluxcd/source-controller/internal/error"
 	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
+	"github.com/fluxcd/source-controller/internal/reconcile/summarize"
 	"github.com/fluxcd/source-controller/pkg/sourceignore"
 )
 
-// Status conditions owned by Bucket reconciler.
-var bucketOwnedConditions = []string{
-	sourcev1.ArtifactOutdatedCondition,
-	sourcev1.FetchFailedCondition,
-	meta.ReadyCondition,
-	meta.ReconcilingCondition,
-	meta.StalledCondition,
-}
-
-// Conditions that Ready condition is influenced by in descending order of their
-// priority.
-var bucketReadyDeps = []string{
-	sourcev1.ArtifactOutdatedCondition,
-	sourcev1.FetchFailedCondition,
-	meta.StalledCondition,
-	meta.ReconcilingCondition,
-}
-
-// Negative conditions that Ready condition is influenced by.
-var bucketReadyDepsNegative = []string{
-	sourcev1.ArtifactOutdatedCondition,
-	sourcev1.FetchFailedCondition,
-	meta.StalledCondition,
-	meta.ReconcilingCondition,
+// bucketReadyConditions contains all the conditions information needed
+// for Bucket Ready status conditions summary calculation.
+var bucketReadyConditions = summarize.Conditions{
+	Target: meta.ReadyCondition,
+	Owned: []string{
+		sourcev1.ArtifactOutdatedCondition,
+		sourcev1.FetchFailedCondition,
+		meta.ReadyCondition,
+		meta.ReconcilingCondition,
+		meta.StalledCondition,
+	},
+	Summarize: []string{
+		sourcev1.ArtifactOutdatedCondition,
+		sourcev1.FetchFailedCondition,
+		meta.StalledCondition,
+		meta.ReconcilingCondition,
+	},
+	NegativePolarity: []string{
+		sourcev1.ArtifactOutdatedCondition,
+		sourcev1.FetchFailedCondition,
+		meta.StalledCondition,
+		meta.ReconcilingCondition,
+	},
 }
 
 // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=buckets,verbs=get;list;watch;create;update;patch;delete
@@ -151,7 +149,19 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
 	// Always attempt to patch the object and status after each reconciliation
 	// NOTE: The final runtime result and error are set in this block.
 	defer func() {
-		result, retErr = r.summarizeAndPatch(ctx, obj, patchHelper, recResult, retErr)
+		summarizeHelper := summarize.NewHelper(r.EventRecorder, patchHelper)
+		summarizeOpts := []summarize.Option{
+			summarize.WithConditions(bucketReadyConditions),
+			summarize.WithReconcileResult(recResult),
+			summarize.WithReconcileError(retErr),
+			summarize.WithIgnoreNotFound(),
+			summarize.WithProcessors(
+				summarize.RecordContextualError,
+				summarize.RecordReconcileReq,
+			),
+			summarize.WithResultBuilder(sreconcile.AlwaysRequeueResultBuilder{RequeueAfter: obj.GetInterval().Duration}),
+		}
+		result, retErr = summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...)
 
 		// Always record readiness and duration metrics
 		r.Metrics.RecordReadiness(ctx, obj)
@@ -181,50 +191,6 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
 	return
 }
 
-// summarizeAndPatch analyzes the object conditions to create a summary of the
-// status conditions, computes runtime results and patches the object in the K8s
-// API server.
-func (r *BucketReconciler) summarizeAndPatch(
-	ctx context.Context,
-	obj *sourcev1.Bucket,
-	patchHelper *patch.Helper,
-	res sreconcile.Result,
-	recErr error) (ctrl.Result, error) {
-	sreconcile.RecordContextualError(ctx, r.EventRecorder, obj, recErr)
-
-	// Record the value of the reconciliation request if any.
-	if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok {
-		obj.Status.SetLastHandledReconcileRequest(v)
-	}
-
-	// Compute the reconcile results, obtain patch options and reconcile error.
-	var patchOpts []patch.Option
-	var result ctrl.Result
-	patchOpts, result, recErr = sreconcile.ComputeReconcileResult(obj, obj.GetRequeueAfter(), res, recErr, bucketOwnedConditions)
-
-	// Summarize the Ready condition based on abnormalities that may have been observed.
-	conditions.SetSummary(obj,
-		meta.ReadyCondition,
-		conditions.WithConditions(
-			bucketReadyDeps...,
-		),
-		conditions.WithNegativePolarityConditions(
-			bucketReadyDepsNegative...,
-		),
-	)
-
-	// Finally, patch the resource.
-	if err := patchHelper.Patch(ctx, obj, patchOpts...); err != nil {
-		// Ignore patch error "not found" when the object is being deleted.
-		if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
-			err = kerrors.FilterOut(err, func(e error) bool { return apierrors.IsNotFound(e) })
-		}
-		recErr = kerrors.NewAggregate([]error{recErr, err})
-	}
-
-	return result, recErr
-}
-
 // reconcile steps iterates through the actual reconciliation tasks for objec,
 // it returns early on the first step that returns ResultRequeue or produces an
 // error.

controllers/bucket_controller_test.go

@@ -34,6 +34,7 @@ import (
 	"github.com/darkowlzz/controller-check/status"
 	"github.com/fluxcd/pkg/apis/meta"
 	"github.com/fluxcd/pkg/runtime/conditions"
+	"github.com/fluxcd/pkg/runtime/patch"
 	. "github.com/onsi/gomega"
 	raw "google.golang.org/api/storage/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -125,10 +126,25 @@ func TestBucketReconciler_Reconcile(t *testing.T) {
 	}, timeout).Should(BeTrue())
 
 	// Check if the object status is valid.
-	condns := &status.Conditions{NegativePolarity: bucketReadyDepsNegative}
+	condns := &status.Conditions{NegativePolarity: bucketReadyConditions.NegativePolarity}
 	checker := status.NewChecker(testEnv.Client, testEnv.GetScheme(), condns)
 	checker.CheckErr(ctx, obj)
 
+	// Patch the object with reconcile request annotation.
+	patchHelper, err := patch.NewHelper(obj, testEnv.Client)
+	g.Expect(err).ToNot(HaveOccurred())
+	annotations := map[string]string{
+		meta.ReconcileRequestAnnotation: "now",
+	}
+	obj.SetAnnotations(annotations)
+	g.Expect(patchHelper.Patch(ctx, obj)).ToNot(HaveOccurred())
+	g.Eventually(func() bool {
+		if err := testEnv.Get(ctx, key, obj); err != nil {
+			return false
+		}
+		return obj.Status.LastHandledReconcileAt == "now"
+	}, timeout).Should(BeTrue())
+
 	g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())
 
 	// Wait for Bucket to be deleted

controllers/gitrepository_controller.go

@@ -27,10 +27,8 @@ import (
 	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/fluxcd/pkg/runtime/logger"
 	corev1 "k8s.io/api/core/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	kuberecorder "k8s.io/client-go/tools/record"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
@@ -49,41 +47,41 @@ import (
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
 	serror "github.com/fluxcd/source-controller/internal/error"
 	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
+	"github.com/fluxcd/source-controller/internal/reconcile/summarize"
 	"github.com/fluxcd/source-controller/internal/util"
 	"github.com/fluxcd/source-controller/pkg/git"
 	"github.com/fluxcd/source-controller/pkg/git/strategy"
 	"github.com/fluxcd/source-controller/pkg/sourceignore"
 )
 
-// Status conditions owned by the GitRepository reconciler.
-var gitRepoOwnedConditions = []string{
-	sourcev1.SourceVerifiedCondition,
-	sourcev1.FetchFailedCondition,
-	sourcev1.IncludeUnavailableCondition,
-	sourcev1.ArtifactOutdatedCondition,
-	meta.ReadyCondition,
-	meta.ReconcilingCondition,
-	meta.StalledCondition,
-}
-
-// Conditions that Ready condition is influenced by in descending order of their
-// priority.
-var gitRepoReadyDeps = []string{
-	sourcev1.IncludeUnavailableCondition,
-	sourcev1.SourceVerifiedCondition,
-	sourcev1.FetchFailedCondition,
-	sourcev1.ArtifactOutdatedCondition,
-	meta.StalledCondition,
-	meta.ReconcilingCondition,
-}
-
-// Negative conditions that Ready condition is influenced by.
-var gitRepoReadyDepsNegative = []string{
-	sourcev1.FetchFailedCondition,
-	sourcev1.IncludeUnavailableCondition,
-	sourcev1.ArtifactOutdatedCondition,
-	meta.StalledCondition,
-	meta.ReconcilingCondition,
+// gitRepoReadyConditions contains all the conditions information needed
+// for GitRepository Ready status conditions summary calculation.
+var gitRepoReadyConditions = summarize.Conditions{
+	Target: meta.ReadyCondition,
+	Owned: []string{
+		sourcev1.SourceVerifiedCondition,
+		sourcev1.FetchFailedCondition,
+		sourcev1.IncludeUnavailableCondition,
+		sourcev1.ArtifactOutdatedCondition,
+		meta.ReadyCondition,
+		meta.ReconcilingCondition,
+		meta.StalledCondition,
+	},
+	Summarize: []string{
+		sourcev1.IncludeUnavailableCondition,
+		sourcev1.SourceVerifiedCondition,
+		sourcev1.FetchFailedCondition,
+		sourcev1.ArtifactOutdatedCondition,
+		meta.StalledCondition,
+		meta.ReconcilingCondition,
+	},
+	NegativePolarity: []string{
+		sourcev1.FetchFailedCondition,
+		sourcev1.IncludeUnavailableCondition,
+		sourcev1.ArtifactOutdatedCondition,
+		meta.StalledCondition,
+		meta.ReconcilingCondition,
+	},
 }
 
 // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=gitrepositories,verbs=get;list;watch;create;update;patch;delete
@@ -157,7 +155,19 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	// Always attempt to patch the object and status after each reconciliation
 	// NOTE: The final runtime result and error are set in this block.
 	defer func() {
-		result, retErr = r.summarizeAndPatch(ctx, obj, patchHelper, recResult, retErr)
+		summarizeHelper := summarize.NewHelper(r.EventRecorder, patchHelper)
+		summarizeOpts := []summarize.Option{
+			summarize.WithConditions(gitRepoReadyConditions),
+			summarize.WithReconcileResult(recResult),
+			summarize.WithReconcileError(retErr),
+			summarize.WithIgnoreNotFound(),
+			summarize.WithProcessors(
+				summarize.RecordContextualError,
+				summarize.RecordReconcileReq,
+			),
+			summarize.WithResultBuilder(sreconcile.AlwaysRequeueResultBuilder{RequeueAfter: obj.GetInterval().Duration}),
+		}
+		result, retErr = summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...)
 
 		// Always record readiness and duration metrics
 		r.Metrics.RecordReadiness(ctx, obj)
@@ -189,50 +199,6 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	return
 }
 
-// summarizeAndPatch analyzes the object conditions to create a summary of the
-// status conditions, computes runtime results and patches the object in the K8s
-// API server.
-func (r *GitRepositoryReconciler) summarizeAndPatch(
-	ctx context.Context,
-	obj *sourcev1.GitRepository,
-	patchHelper *patch.Helper,
-	res sreconcile.Result,
-	recErr error) (ctrl.Result, error) {
-	sreconcile.RecordContextualError(ctx, r.EventRecorder, obj, recErr)
-
-	// Record the value of the reconciliation request if any.
-	if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok {
-		obj.Status.SetLastHandledReconcileRequest(v)
-	}
-
-	// Compute the reconcile results, obtain patch options and reconcile error.
-	var patchOpts []patch.Option
-	var result ctrl.Result
-	patchOpts, result, recErr = sreconcile.ComputeReconcileResult(obj, obj.GetRequeueAfter(), res, recErr, gitRepoOwnedConditions)
-
-	// Summarize the Ready condition based on abnormalities that may have been observed.
-	conditions.SetSummary(obj,
-		meta.ReadyCondition,
-		conditions.WithConditions(
-			gitRepoReadyDeps...,
-		),
-		conditions.WithNegativePolarityConditions(
-			gitRepoReadyDepsNegative...,
-		),
-	)
-
-	// Finally, patch the resource.
-	if err := patchHelper.Patch(ctx, obj, patchOpts...); err != nil {
-		// Ignore patch error "not found" when the object is being deleted.
-		if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
-			err = kerrors.FilterOut(err, func(e error) bool { return apierrors.IsNotFound(e) })
-		}
-		recErr = kerrors.NewAggregate([]error{recErr, err})
-	}
-
-	return result, recErr
-}
-
 // reconcile steps iterates through the actual reconciliation tasks for objec,
 // it returns early on the first step that returns ResultRequeue or produces an
 // error.

controllers/gitrepository_controller_test.go

@@ -30,6 +30,7 @@ import (
 	"github.com/fluxcd/pkg/apis/meta"
 	"github.com/fluxcd/pkg/gittestserver"
 	"github.com/fluxcd/pkg/runtime/conditions"
+	"github.com/fluxcd/pkg/runtime/patch"
 	"github.com/fluxcd/pkg/ssh"
 	"github.com/fluxcd/pkg/testserver"
 	"github.com/go-git/go-billy/v5/memfs"
@@ -190,10 +191,25 @@ func TestGitRepositoryReconciler_Reconcile(t *testing.T) {
 	}, timeout).Should(BeTrue())
 
 	// Check if the object status is valid.
-	condns := &status.Conditions{NegativePolarity: gitRepoReadyDepsNegative}
+	condns := &status.Conditions{NegativePolarity: gitRepoReadyConditions.NegativePolarity}
 	checker := status.NewChecker(testEnv.Client, testEnv.GetScheme(), condns)
 	checker.CheckErr(ctx, obj)
 
+	// Patch the object with reconcile request annotation.
+	patchHelper, err := patch.NewHelper(obj, testEnv.Client)
+	g.Expect(err).ToNot(HaveOccurred())
+	annotations := map[string]string{
+		meta.ReconcileRequestAnnotation: "now",
+	}
+	obj.SetAnnotations(annotations)
+	g.Expect(patchHelper.Patch(ctx, obj)).ToNot(HaveOccurred())
+	g.Eventually(func() bool {
+		if err := testEnv.Get(ctx, key, obj); err != nil {
+			return false
+		}
+		return obj.Status.LastHandledReconcileAt == "now"
+	}, timeout).Should(BeTrue())
+
 	g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())
 
 	// Wait for GitRepository to be deleted