Skip to content

Improve bundle and GitRepo status error reporting #4167

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
weyfonk merged 25 commits into from
Oct 14, 2025
Merged

Improve bundle and GitRepo status error reporting #4167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
b1093f7
Propagate error to GitRepo status when failing to create git job
weyfonk Sep 24, 2025
ee3d0a4
Reorder bundle controller methods and functions
weyfonk Sep 24, 2025
387bfb8
Return reconcile error for incorrect Helm chart versions
weyfonk Sep 25, 2025
38b9f31
Update bundle status with OCI storage manifest handling errors
weyfonk Sep 25, 2025
4aa5bb1
Fail bundle reconcile when bundle targeting fails
weyfonk Sep 25, 2025
fbbe88b
Propagate failures to write Content resources
weyfonk Sep 25, 2025
e89f338
Propagate values secrets errors to bundle status
weyfonk Sep 25, 2025
232380c
Propagate values secret load error to bundle status
weyfonk Sep 25, 2025
eca8369
Propagate failure to add bundle finalizer to bundle status
weyfonk Sep 25, 2025
be3ecd9
Propagate OCI storage access secret error to bundle status
weyfonk Sep 25, 2025
4288e01
Propagate partitions handling errors to bundle status
weyfonk Sep 25, 2025
b27cfeb
Propagate access secrets cloning errors to bundle status
weyfonk Sep 25, 2025
ee5be28
Isolate bundle status patch checks into separate function
weyfonk Sep 26, 2025
9b5694c
Skip status update for finalizer errors
weyfonk Oct 6, 2025
3f9ecc6
Exclude retriable storage errors from bundle status updates
weyfonk Oct 6, 2025
c1728d6
Fix error message for OCI reference build failure
weyfonk Oct 6, 2025
45383bf
Treat options secret CRUD errors as retryable
weyfonk Oct 7, 2025
643e6ae
Distinguish retryable errors when resolving OCI reference
weyfonk Oct 7, 2025
e00432c
Distinguish retriable errors when cloning secrets
weyfonk Oct 7, 2025
1ef2c37
Prevent bundle reconcile retries in non-retryable error cases
weyfonk Oct 7, 2025
cea28e2
Use terminal errors to prevent retries in non-retryable cases
weyfonk Oct 7, 2025
829bcce
Factor retryable error checks into separate function
weyfonk Oct 9, 2025
ab6889e
Remove unused parameter
weyfonk Oct 9, 2025
01be466
Use common logic for condition errors and retryable checks
weyfonk Oct 9, 2025
62a3c22
Apply new common logic to non-error condition setting
weyfonk Oct 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 6 additions & 18 deletions internal/cmd/controller/gitops/reconciler/gitjob_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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)
Comment thread
0xavi0 marked this conversation as resolved.
}

setAcceptedCondition(&gitrepo.Status, nil)
reconciler.SetCondition(v1alpha1.GitRepoAcceptedCondition, &gitrepo.Status, nil)

err = updateStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status)
if err != nil {
Expand All @@ -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,
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down
53 changes: 36 additions & 17 deletions internal/cmd/controller/gitops/reconciler/gitjob_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"
"os"
"slices"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -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)
Expand All @@ -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},
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
},
Expand All @@ -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)
}
}
Expand Down
Loading