From 62de718a20e1377d5a8702876077762ed9a37f27 Mon Sep 17 00:00:00 2001 From: Mario Manno Date: Mon, 12 Aug 2024 12:44:10 +0200 Subject: [PATCH] Refactor gitjops reconciler to remove complexity warning * simplify names * order imports into groups * extract the job handling into a function --- internal/cmd/controller/gitops/operator.go | 16 +-- .../gitops/reconciler/gitjob_controller.go | 112 ++++++++++-------- .../gitops/reconciler/gitjob_test.go | 5 +- 3 files changed, 71 insertions(+), 62 deletions(-) diff --git a/internal/cmd/controller/gitops/operator.go b/internal/cmd/controller/gitops/operator.go index f0eb5a7140..54befd8d86 100644 --- a/internal/cmd/controller/gitops/operator.go +++ b/internal/cmd/controller/gitops/operator.go @@ -8,15 +8,9 @@ import ( "strconv" "time" - command "github.com/rancher/fleet/internal/cmd" - "github.com/rancher/fleet/internal/cmd/controller/gitops/reconciler" - "github.com/rancher/fleet/internal/metrics" - fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" - "github.com/rancher/fleet/pkg/git" - "github.com/rancher/fleet/pkg/version" - "github.com/rancher/fleet/pkg/webhook" "github.com/reugn/go-quartz/quartz" "github.com/spf13/cobra" + "golang.org/x/sync/errgroup" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -28,7 +22,13 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" - "golang.org/x/sync/errgroup" + command "github.com/rancher/fleet/internal/cmd" + "github.com/rancher/fleet/internal/cmd/controller/gitops/reconciler" + "github.com/rancher/fleet/internal/metrics" + fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" + "github.com/rancher/fleet/pkg/git" + "github.com/rancher/fleet/pkg/version" + "github.com/rancher/fleet/pkg/webhook" ) var ( diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go index 11cf6b28e3..e0fff3016c 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go +++ b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go @@ -11,19 +11,20 @@ import ( "time" "github.com/go-logr/logr" + "github.com/reugn/go-quartz/quartz" + "github.com/rancher/fleet/internal/cmd/controller/finalize" "github.com/rancher/fleet/internal/cmd/controller/grutil" "github.com/rancher/fleet/internal/cmd/controller/imagescan" "github.com/rancher/fleet/internal/metrics" "github.com/rancher/fleet/internal/ociwrapper" v1alpha1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" + fleetevent "github.com/rancher/fleet/pkg/event" "github.com/rancher/fleet/pkg/sharding" - "github.com/rancher/wrangler/v3/pkg/condition" - "github.com/reugn/go-quartz/quartz" + "github.com/rancher/wrangler/v3/pkg/condition" "github.com/rancher/wrangler/v3/pkg/name" - fleetevent "github.com/rancher/fleet/pkg/event" batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -182,29 +183,68 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, nil } - commitBefore := gitrepo.Status.Commit - gitPollerWasExecuted, err := r.checkPollingTask(ctx, gitrepo) + oldCommit := gitrepo.Status.Commit + repoPolled, err := r.repoPolled(ctx, gitrepo) if err != nil { r.Recorder.Event(gitrepo, fleetevent.Warning, "FailedToCheckCommit", err.Error()) - } else if gitPollerWasExecuted && commitBefore != gitrepo.Status.Commit { + } else if repoPolled && oldCommit != gitrepo.Status.Commit { r.Recorder.Event(gitrepo, fleetevent.Normal, "GotNewCommit", gitrepo.Status.Commit) } // From this point onwards we have to take into account if the poller // task was executed. // If so, we need to return a Result with EnqueueAfter set. + res, err := r.manageGitJob(ctx, logger, gitrepo, oldCommit, repoPolled) + if err != nil { + return res, err + } + + err = grutil.SetStatusFromBundleDeployments(ctx, r.Client, gitrepo) + if err != nil { + return result(repoPolled, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err) + } + + err = grutil.SetStatusFromBundles(ctx, r.Client, gitrepo) + if err != nil { + return result(repoPolled, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err) + } + + if err = grutil.UpdateDisplayState(gitrepo); err != nil { + return result(repoPolled, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err) + } + + grutil.SetStatusFromResourceKey(ctx, r.Client, gitrepo) + + gitrepo.Status.Display.ReadyBundleDeployments = fmt.Sprintf("%d/%d", + gitrepo.Status.Summary.Ready, + gitrepo.Status.Summary.DesiredReady) + + grutil.SetCondition(&gitrepo.Status, nil) + + err = grutil.UpdateStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status) + if err != nil { + logger.V(1).Error(err, "Reconcile failed final update to git repo status", "status", gitrepo.Status) + + return result(repoPolled, gitrepo), err + } + + return result(repoPolled, gitrepo), nil +} + +// 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{ + err := r.Get(ctx, types.NamespacedName{ Namespace: gitrepo.Namespace, Name: jobName(gitrepo), }, &job) if err != nil && !errors.IsNotFound(err) { err = fmt.Errorf("error retrieving git job: %w", err) r.Recorder.Event(gitrepo, fleetevent.Warning, "FailedToGetGitJob", err.Error()) - return reconcileResult(gitPollerWasExecuted, gitrepo), err + return result(repoPolled, gitrepo), err } - // Gitjob handling if errors.IsNotFound(err) { if gitrepo.Spec.DisablePolling { commit, err := r.GitFetcher.LatestCommit(ctx, gitrepo, r.Client) @@ -215,7 +255,7 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr if err != nil { r.Recorder.Event(gitrepo, fleetevent.Warning, "Failed", err.Error()) } else { - if gitPollerWasExecuted && commitBefore != gitrepo.Status.Commit { + if repoPolled && oldCommit != gitrepo.Status.Commit { r.Recorder.Event(gitrepo, fleetevent.Normal, "GotNewCommit", gitrepo.Status.Commit) } } @@ -224,67 +264,36 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr if gitrepo.Status.Commit != "" { if err := r.validateExternalSecretExist(ctx, gitrepo); err != nil { r.Recorder.Event(gitrepo, fleetevent.Warning, "FailedValidatingSecret", err.Error()) - return reconcileResult(gitPollerWasExecuted, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err) + return result(repoPolled, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, name, gitrepo.Status, err) } logger.V(1).Info("Creating Git job resources") if err := r.createJobRBAC(ctx, gitrepo); err != nil { - return reconcileResult(gitPollerWasExecuted, gitrepo), fmt.Errorf("failed to create RBAC resources for git job: %w", err) + return result(repoPolled, gitrepo), fmt.Errorf("failed to create RBAC resources for git job: %w", err) } if err := r.createTargetsConfigMap(ctx, gitrepo); err != nil { - return reconcileResult(gitPollerWasExecuted, gitrepo), fmt.Errorf("failed to create targets config map for git job: %w", err) + return result(repoPolled, gitrepo), fmt.Errorf("failed to create targets config map for git job: %w", err) } if err := r.createJob(ctx, gitrepo); err != nil { - return reconcileResult(gitPollerWasExecuted, gitrepo), fmt.Errorf("error creating git job: %w", err) + return result(repoPolled, gitrepo), fmt.Errorf("error creating git job: %w", err) } r.Recorder.Event(gitrepo, fleetevent.Normal, "Created", "GitJob was created") } } else if gitrepo.Status.Commit != "" { if err = r.deleteJobIfNeeded(ctx, gitrepo, &job); err != nil { - return reconcileResult(gitPollerWasExecuted, gitrepo), fmt.Errorf("error deleting git job: %w", err) + return result(repoPolled, gitrepo), fmt.Errorf("error deleting git job: %w", err) } } gitrepo.Status.ObservedGeneration = gitrepo.Generation - // Refresh the status if err = grutil.SetStatusFromGitjob(ctx, r.Client, gitrepo, &job); err != nil { - return reconcileResult(gitPollerWasExecuted, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err) - } - - err = grutil.SetStatusFromBundleDeployments(ctx, r.Client, gitrepo) - if err != nil { - return reconcileResult(gitPollerWasExecuted, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err) - } - - err = grutil.SetStatusFromBundles(ctx, r.Client, gitrepo) - if err != nil { - return reconcileResult(gitPollerWasExecuted, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err) - } - - if err = grutil.UpdateDisplayState(gitrepo); err != nil { - return reconcileResult(gitPollerWasExecuted, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err) - } - - grutil.SetStatusFromResourceKey(ctx, r.Client, gitrepo) - - gitrepo.Status.Display.ReadyBundleDeployments = fmt.Sprintf("%d/%d", - gitrepo.Status.Summary.Ready, - gitrepo.Status.Summary.DesiredReady) - - grutil.SetCondition(&gitrepo.Status, nil) - - err = grutil.UpdateStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status) - if err != nil { - logger.V(1).Error(err, "Reconcile failed final update to git repo status", "status", gitrepo.Status) - - return reconcileResult(gitPollerWasExecuted, gitrepo), err + return result(repoPolled, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, name, gitrepo.Status, err) } - return reconcileResult(gitPollerWasExecuted, gitrepo), nil + return reconcile.Result{}, nil } func (r *GitJobReconciler) cleanupGitRepo(ctx context.Context, logger logr.Logger, gitrepo *v1alpha1.GitRepo) error { - // Clean up logger.V(1).Info("Gitrepo deleted, deleting bundle, image scans") metrics.GitRepoCollector.Delete(gitrepo.Name, gitrepo.Namespace) @@ -1038,7 +1047,8 @@ func bundleStatusChangedPredicate() predicate.Funcs { } } -func (r *GitJobReconciler) checkPollingTask(ctx context.Context, gitrepo *v1alpha1.GitRepo) (bool, error) { +// repoPolled returns true if the git poller was executed and the repo should still be polled. +func (r *GitJobReconciler) repoPolled(ctx context.Context, gitrepo *v1alpha1.GitRepo) (bool, error) { if gitrepo.Spec.DisablePolling { return false, nil } @@ -1081,8 +1091,8 @@ func getPollingIntervalDuration(gitrepo *v1alpha1.GitRepo) time.Duration { return gitrepo.Spec.PollingInterval.Duration } -func reconcileResult(gitPollerWasExecuted bool, gitrepo *v1alpha1.GitRepo) reconcile.Result { - if gitPollerWasExecuted { +func result(repoPolled bool, gitrepo *v1alpha1.GitRepo) reconcile.Result { + if repoPolled { return reconcile.Result{RequeueAfter: getPollingIntervalDuration(gitrepo)} } return reconcile.Result{} diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_test.go b/internal/cmd/controller/gitops/reconciler/gitjob_test.go index 58e7564d49..d4dcf720ea 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_test.go +++ b/internal/cmd/controller/gitops/reconciler/gitjob_test.go @@ -16,7 +16,6 @@ import ( "github.com/rancher/fleet/internal/cmd/controller/finalize" "github.com/rancher/fleet/internal/mocks" fleetv1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" - v1alpha1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" "github.com/rancher/wrangler/v3/pkg/genericcondition" fleetevent "github.com/rancher/fleet/pkg/event" @@ -35,7 +34,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) -func getCondition(gitrepo *v1alpha1.GitRepo, condType string) (genericcondition.GenericCondition, bool) { +func getCondition(gitrepo *fleetv1.GitRepo, condType string) (genericcondition.GenericCondition, bool) { for _, cond := range gitrepo.Status.Conditions { if cond.Type == condType { return cond, true @@ -1269,7 +1268,7 @@ func TestCheckforPollingTask(t *testing.T) { Clock: ClockMock{t: test.timeNow}, GitFetcher: fetcher, } - res, err := r.checkPollingTask(context.TODO(), test.gitrepo) + res, err := r.repoPolled(context.TODO(), test.gitrepo) if res != test.expectedResult { t.Errorf("unexpected result. Expecting %t, got %t", test.expectedResult, res) }