Improve bundle and GitRepo status error reporting (#4167)

When a bundle reconcile attempt fails, the reconciler now does one of
the following:
* log the error and retry with exponential backoff, when dealing with a
  retryable error such as a failure to perform a CRUD operation on a
  Kubernetes resource
* skip retries and report the error through the bundle's status, setting
  its `Ready` condition to `False`, when the error is non-retryable
  (e.g. configuration error). In such cases, the reconciler newly makes
  use of a `TerminalError` [1], indicating that no retries should take
  place.

Distinguishing retryable errors from non-retryable ones should both
optimise the number of reconciles run for a given resource, and make
non-retryable errors more visible to users, as they would typically be
especially interested in them, since they may well be able to act on
them.

This happens through new, common logic living in
`reconciler/error_handling.go`, encapsulating propagation of errors
through status conditions, and distinctions between retryable and
non-retryable errors.

To illustrate the potential of this new logic, the gitOps controller
partly uses this new logic when reconciling GitRepos, although most of
the work to improve error reporting in that reconciler remains to be
done.

[1]: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/reconcile#TypedReconciler

Author: weyfonk

internal/cmd/controller/gitops/reconciler/gitjob_controller.go

@@ -12,7 +12,6 @@ import (
 	"github.com/go-logr/logr"
 	"github.com/reugn/go-quartz/quartz"
 
-	fleetutil "github.com/rancher/fleet/internal/cmd/controller/errorutil"
 	"github.com/rancher/fleet/internal/cmd/controller/finalize"
 	"github.com/rancher/fleet/internal/cmd/controller/imagescan"
 	"github.com/rancher/fleet/internal/cmd/controller/reconciler"
@@ -28,7 +27,6 @@ import (
 
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -233,10 +231,10 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 
 	res, err := r.manageGitJob(ctx, logger, gitrepo, oldCommit, repoPolled)
 	if err != nil || res.RequeueAfter > 0 {
-		return res, err
+		return res, updateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err)
 	}
 
-	setAcceptedCondition(&gitrepo.Status, nil)
+	reconciler.SetCondition(v1alpha1.GitRepoAcceptedCondition, &gitrepo.Status, nil)
 
 	err = updateStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status)
 	if err != nil {
@@ -262,7 +260,6 @@ func monitorLatestCommit(obj metav1.Object, fetch func() (string, error)) (strin
 
 // manageGitJob is responsible for creating, updating and deleting the GitJob and setting the GitRepo's status accordingly
 func (r *GitJobReconciler) manageGitJob(ctx context.Context, logger logr.Logger, gitrepo *v1alpha1.GitRepo, oldCommit string, repoPolled bool) (reconcile.Result, error) {
-	name := types.NamespacedName{Namespace: gitrepo.Namespace, Name: gitrepo.Name}
 	var job batchv1.Job
 	err := r.Get(ctx, types.NamespacedName{
 		Namespace: gitrepo.Namespace,
@@ -296,7 +293,7 @@ func (r *GitJobReconciler) manageGitJob(ctx context.Context, logger logr.Logger,
 			r.updateGenerationValuesIfNeeded(gitrepo)
 			if err := r.validateExternalSecretExist(ctx, gitrepo); err != nil {
 				r.Recorder.Event(gitrepo, fleetevent.Warning, "FailedValidatingSecret", err.Error())
-				return r.result(gitrepo), updateErrorStatus(ctx, r.Client, name, gitrepo.Status, err)
+				return r.result(gitrepo), fmt.Errorf("error validating external secrets: %w", err)
 			}
 			if err := r.createJobAndResources(ctx, gitrepo, logger); err != nil {
 				gitjobsCreatedFailure.Inc(gitrepo)
@@ -319,7 +316,7 @@ func (r *GitJobReconciler) manageGitJob(ctx context.Context, logger logr.Logger,
 	gitrepo.Status.ObservedGeneration = gitrepo.Generation
 
 	if err = setStatusFromGitjob(ctx, r.Client, gitrepo, &job); err != nil {
-		return r.result(gitrepo), updateErrorStatus(ctx, r.Client, name, gitrepo.Status, err)
+		return r.result(gitrepo), fmt.Errorf("error setting GitRepo status from git job: %w", err)
 	}
 
 	return reconcile.Result{}, nil
@@ -657,19 +654,10 @@ func setStatusFromGitjob(ctx context.Context, c client.Client, gitRepo *v1alpha1
 	return nil
 }
 
-// setAcceptedCondition sets the condition and updates the timestamp, if the condition changed
-func setAcceptedCondition(status *v1alpha1.GitRepoStatus, err error) {
-	cond := condition.Cond(v1alpha1.GitRepoAcceptedCondition)
-	origStatus := status.DeepCopy()
-	cond.SetError(status, "", fleetutil.IgnoreConflict(err))
-	if !equality.Semantic.DeepEqual(origStatus, status) {
-		cond.LastUpdated(status, time.Now().UTC().Format(time.RFC3339))
-	}
-}
-
 // updateErrorStatus sets the condition in the status and tries to update the resource
 func updateErrorStatus(ctx context.Context, c client.Client, req types.NamespacedName, status v1alpha1.GitRepoStatus, orgErr error) error {
-	setAcceptedCondition(&status, orgErr)
+	reconciler.SetCondition(v1alpha1.GitRepoAcceptedCondition, &status, orgErr)
+
 	if statusErr := updateStatus(ctx, c, req, status); statusErr != nil {
 		merr := []error{orgErr, fmt.Errorf("failed to update the status: %w", statusErr)}
 		return errutil.NewAggregate(merr)

internal/cmd/controller/gitops/reconciler/gitjob_test.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"os"
 	"slices"
+	"strings"
 	"testing"
 	"time"
 
@@ -522,16 +523,18 @@ func TestReconcile_Error_WhenGetGitJobErrors(t *testing.T) {
 	mockClient := mocks.NewMockK8sClient(mockCtrl)
 	mockClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(nil)
 
-	mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(1).DoAndReturn(
-		func(ctx context.Context, req types.NamespacedName, gitrepo *fleetv1.GitRepo, opts ...interface{}) error {
-			gitrepo.Name = gitRepo.Name
-			gitrepo.Namespace = gitRepo.Namespace
-			gitrepo.Spec.Repo = "repo"
-			controllerutil.AddFinalizer(gitrepo, finalize.GitRepoFinalizer)
-			gitrepo.Status.Commit = "dd45c7ad68e10307765104fea4a1f5997643020f"
-			return nil
-		},
-	)
+	mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.AssignableToTypeOf(&fleetv1.GitRepo{}), gomock.Any()).
+		Times(2).
+		DoAndReturn(
+			func(ctx context.Context, req types.NamespacedName, gitrepo *fleetv1.GitRepo, opts ...interface{}) error {
+				gitrepo.Name = gitRepo.Name
+				gitrepo.Namespace = gitRepo.Namespace
+				gitrepo.Spec.Repo = "repo"
+				controllerutil.AddFinalizer(gitrepo, finalize.GitRepoFinalizer)
+				gitrepo.Status.Commit = "dd45c7ad68e10307765104fea4a1f5997643020f"
+				return nil
+			},
+		)
 	mockFetcher := gitmocks.NewMockGitFetcher(mockCtrl)
 	commit := "1883fd54bc5dfd225acf02aecbb6cb8020458e33"
 	mockFetcher.EXPECT().LatestCommit(gomock.Any(), gomock.Any(), gomock.Any()).Times(1).Return(commit, nil)
@@ -542,6 +545,20 @@ func TestReconcile_Error_WhenGetGitJobErrors(t *testing.T) {
 		},
 	)
 
+	statusClient := mocks.NewMockSubResourceWriter(mockCtrl)
+	mockClient.EXPECT().Status().Times(1).Return(statusClient)
+	statusClient.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any()).Do(
+		func(ctx context.Context, repo *fleetv1.GitRepo, opts ...interface{}) {
+			c, found := getCondition(repo, fleetv1.GitRepoAcceptedCondition)
+			if !found {
+				t.Errorf("expecting to find the %s condition and could not find it.", fleetv1.GitRepoAcceptedCondition)
+			}
+			if !strings.Contains(c.Message, "GITJOB ERROR") {
+				t.Errorf("expecting message containing [GITJOB ERROR] in condition, got [%s]", c.Message)
+			}
+		},
+	)
+
 	recorderMock := mocks.NewMockEventRecorder(mockCtrl)
 	recorderMock.EXPECT().Event(
 		&gitRepoMatcher{gitRepo},
@@ -606,11 +623,13 @@ func TestReconcile_Error_WhenSecretDoesNotExist(t *testing.T) {
 	mockFetcher.EXPECT().LatestCommit(gomock.Any(), gomock.Any(), gomock.Any()).Times(1).Return(commit, nil)
 
 	// we need to return a NotFound error, so the code tries to create it.
-	mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(1).DoAndReturn(
-		func(ctx context.Context, req types.NamespacedName, job *batchv1.Job, opts ...interface{}) error {
-			return apierrors.NewNotFound(schema.GroupResource{}, "TEST ERROR")
-		},
-	)
+	mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.AssignableToTypeOf(&batchv1.Job{}), gomock.Any()).
+		Times(1).
+		DoAndReturn(
+			func(ctx context.Context, req types.NamespacedName, job *batchv1.Job, opts ...interface{}) error {
+				return apierrors.NewNotFound(schema.GroupResource{}, "TEST ERROR")
+			},
+		)
 
 	mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(1).DoAndReturn(
 		func(ctx context.Context, req types.NamespacedName, job *corev1.Secret, opts ...interface{}) error {
@@ -640,7 +659,7 @@ func TestReconcile_Error_WhenSecretDoesNotExist(t *testing.T) {
 			if !found {
 				t.Errorf("expecting to find the %s condition and could not find it.", fleetv1.GitRepoAcceptedCondition)
 			}
-			if c.Message != "failed to look up HelmSecretNameForPaths, error: SECRET ERROR" {
+			if c.Message != "error validating external secrets: failed to look up HelmSecretNameForPaths, error: SECRET ERROR" {
 				t.Errorf("expecting message [failed to look up HelmSecretNameForPaths, error: SECRET ERROR] in condition, got [%s]", c.Message)
 			}
 		},
@@ -660,7 +679,7 @@ func TestReconcile_Error_WhenSecretDoesNotExist(t *testing.T) {
 	if err == nil {
 		t.Errorf("expecting an error, got nil")
 	}
-	if err.Error() != "failed to look up HelmSecretNameForPaths, error: SECRET ERROR" {
+	if err.Error() != "error validating external secrets: failed to look up HelmSecretNameForPaths, error: SECRET ERROR" {
 		t.Errorf("unexpected error %v", err)
 	}
 }