Skip to content

Commit

Permalink
[0.10] Backport GitRepo status resources improvements (#3033)
Browse files Browse the repository at this point in the history 
* Partial backport of #2997

Refactor grutils/gitops controller:
* remove grutil package and unexport functions
* move all gitrepo utils into reconciler, who is the only consumer

* Avoid modifying summary directly

* Avoid listing BundleDeployments twice

* Backport #3027

GitRepo resources list doesn't list resources multiple times

* Add comment to merge

* Fix summary import
  • Loading branch information
@aruiz14
aruiz14 authored Oct 30, 2024
1 parent df7140c commit 6a570fc
Show file tree
Hide file tree
Showing 8 changed files with 306 additions and 100 deletions.
8 changes: 4 additions & 4 deletions internal/cmd/controller/grutil/gitjob.go → ...md/controller/gitops/reconciler/gitjob.go
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
// Copyright (c) 2021-2023 SUSE LLC

package grutil
package reconciler

import (
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func NewServiceAccount(namespace string, name string) *corev1.ServiceAccount {
func newServiceAccount(namespace string, name string) *corev1.ServiceAccount {
return &corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand All @@ -17,7 +17,7 @@ func NewServiceAccount(namespace string, name string) *corev1.ServiceAccount {
}
}

func NewRole(namespace string, name string) *rbacv1.Role {
func newRole(namespace string, name string) *rbacv1.Role {
return &rbacv1.Role{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand All @@ -43,7 +43,7 @@ func NewRole(namespace string, name string) *rbacv1.Role {
}
}

func NewRoleBinding(namespace string, name string) *rbacv1.RoleBinding {
func newRoleBinding(namespace string, name string) *rbacv1.RoleBinding {
return &rbacv1.RoleBinding{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand Down
39 changes: 18 additions & 21 deletions internal/cmd/controller/gitops/reconciler/gitjob_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
"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"
Expand Down Expand Up @@ -151,10 +150,10 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr

// Restrictions / Overrides, gitrepo reconciler is responsible for setting error in status
oldStatus := gitrepo.Status.DeepCopy()
_, err := grutil.AuthorizeAndAssignDefaults(ctx, r.Client, gitrepo)
_, err := authorizeAndAssignDefaults(ctx, r.Client, gitrepo)
if err != nil {
r.Recorder.Event(gitrepo, fleetevent.Warning, "FailedToApplyRestrictions", err.Error())
return ctrl.Result{}, grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, *oldStatus, err)
return ctrl.Result{}, updateErrorStatus(ctx, r.Client, req.NamespacedName, *oldStatus, err)
}

if !gitrepo.DeletionTimestamp.IsZero() {
Expand Down Expand Up @@ -207,29 +206,27 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
return res, err
}

err = grutil.SetStatusFromBundleDeployments(ctx, r.Client, gitrepo)
err = SetStatusFromBundleDeployments(ctx, r.Client, gitrepo)
if err != nil {
return result(repoPolled, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err)
return result(repoPolled, gitrepo), updateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err)
}

err = grutil.SetStatusFromBundles(ctx, r.Client, gitrepo)
err = SetStatusFromBundles(ctx, r.Client, gitrepo)
if err != nil {
return result(repoPolled, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err)
return result(repoPolled, gitrepo), 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)
if err = UpdateDisplayState(gitrepo); err != nil {
return result(repoPolled, gitrepo), 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)
SetCondition(&gitrepo.Status, nil)

err = grutil.UpdateStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status)
err = updateStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status)
if err != nil {
logger.Error(err, "Reconcile failed final update to git repo status", "status", gitrepo.Status)

Expand Down Expand Up @@ -273,7 +270,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 result(repoPolled, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, name, gitrepo.Status, err)
return result(repoPolled, gitrepo), updateErrorStatus(ctx, r.Client, name, gitrepo.Status, err)
}
if err := r.createJobAndResources(ctx, gitrepo, logger); err != nil {
return result(repoPolled, gitrepo), err
Expand All @@ -293,8 +290,8 @@ func (r *GitJobReconciler) manageGitJob(ctx context.Context, logger logr.Logger,

gitrepo.Status.ObservedGeneration = gitrepo.Generation

if err = grutil.SetStatusFromGitjob(ctx, r.Client, gitrepo, &job); err != nil {
return result(repoPolled, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, name, gitrepo.Status, err)
if err = setStatusFromGitjob(ctx, r.Client, gitrepo, &job); err != nil {
return result(repoPolled, gitrepo), updateErrorStatus(ctx, r.Client, name, gitrepo.Status, err)
}

return reconcile.Result{}, nil
Expand Down Expand Up @@ -387,23 +384,23 @@ func (r *GitJobReconciler) addGitRepoFinalizer(ctx context.Context, nsName types
func (r *GitJobReconciler) createJobRBAC(ctx context.Context, gitrepo *v1alpha1.GitRepo) error {
// No update needed, values are the same. So we ignore AlreadyExists.
saName := name.SafeConcatName("git", gitrepo.Name)
sa := grutil.NewServiceAccount(gitrepo.Namespace, saName)
sa := newServiceAccount(gitrepo.Namespace, saName)
if err := controllerutil.SetControllerReference(gitrepo, sa, r.Scheme); err != nil {
return err
}
if err := r.Create(ctx, sa); err != nil && !errors.IsAlreadyExists(err) {
return err
}

role := grutil.NewRole(gitrepo.Namespace, saName)
role := newRole(gitrepo.Namespace, saName)
if err := controllerutil.SetControllerReference(gitrepo, role, r.Scheme); err != nil {
return err
}
if err := r.Create(ctx, role); err != nil && !errors.IsAlreadyExists(err) {
return err
}

rb := grutil.NewRoleBinding(gitrepo.Namespace, saName)
rb := newRoleBinding(gitrepo.Namespace, saName)
if err := controllerutil.SetControllerReference(gitrepo, rb, r.Scheme); err != nil {
return err
}
Expand All @@ -415,7 +412,7 @@ func (r *GitJobReconciler) createJobRBAC(ctx context.Context, gitrepo *v1alpha1.
}

func (r *GitJobReconciler) createTargetsConfigMap(ctx context.Context, gitrepo *v1alpha1.GitRepo) error {
configMap, err := grutil.NewTargetsConfigMap(gitrepo)
configMap, err := newTargetsConfigMap(gitrepo)
if err != nil {
return err
}
Expand Down Expand Up @@ -663,7 +660,7 @@ func (r *GitJobReconciler) newJobSpec(ctx context.Context, gitrepo *v1alpha1.Git
}

// compute configmap, needed because its name contains a hash
configMap, err := grutil.NewTargetsConfigMap(gitrepo)
configMap, err := newTargetsConfigMap(gitrepo)
if err != nil {
return nil, err
}
Expand Down
26 changes: 14 additions & 12 deletions internal/cmd/controller/grutil/status.go → ...oller/gitops/reconciler/gitrepo_status.go
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package grutil
package reconciler

import (
"context"
Expand Down Expand Up @@ -73,7 +73,7 @@ func SetStatusFromBundleDeployments(ctx context.Context, c client.Client, gitrep
return err
}

gitrepo.Status.Summary = fleet.BundleSummary{}
var bundleSummary fleet.BundleSummary

sort.Slice(list.Items, func(i, j int) bool {
return list.Items[i].UID < list.Items[j].UID
Expand All @@ -87,8 +87,8 @@ func SetStatusFromBundleDeployments(ctx context.Context, c client.Client, gitrep
for _, bd := range list.Items {
bd := bd // fix gosec warning regarding "Implicit memory aliasing in for loop"
state := summary.GetDeploymentState(&bd)
summary.IncrementState(&gitrepo.Status.Summary, bd.Name, state, summary.MessageFromDeployment(&bd), bd.Status.ModifiedStatus, bd.Status.NonReadyStatus)
gitrepo.Status.Summary.DesiredReady++
summary.IncrementState(&bundleSummary, bd.Name, state, summary.MessageFromDeployment(&bd), bd.Status.ModifiedStatus, bd.Status.NonReadyStatus)
bundleSummary.DesiredReady++
if fleet.StateRank[state] > fleet.StateRank[maxState] {
maxState = state
message = summary.MessageFromDeployment(&bd)
Expand All @@ -99,16 +99,18 @@ func SetStatusFromBundleDeployments(ctx context.Context, c client.Client, gitrep
maxState = ""
message = ""
}

gitrepo.Status.Summary = bundleSummary
gitrepo.Status.Display.State = string(maxState)
gitrepo.Status.Display.Message = message
gitrepo.Status.Display.Error = len(message) > 0

setResources(list, gitrepo)

return nil
}

// SetStatusFromGitjob sets the status fields relative to the given job in the gitRepo
func SetStatusFromGitjob(ctx context.Context, c client.Client, gitRepo *fleet.GitRepo, job *batchv1.Job) error {
// setStatusFromGitjob sets the status fields relative to the given job in the gitRepo
func setStatusFromGitjob(ctx context.Context, c client.Client, gitRepo *fleet.GitRepo, job *batchv1.Job) error {
obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(job)
if err != nil {
return err
Expand Down Expand Up @@ -194,20 +196,20 @@ func SetCondition(status *fleet.GitRepoStatus, err error) {
}
}

// 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 fleet.GitRepoStatus, orgErr error) error {
// 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 fleet.GitRepoStatus, orgErr error) error {
SetCondition(&status, orgErr)
if statusErr := UpdateStatus(ctx, c, req, status); statusErr != nil {
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)
}
return orgErr
}

// UpdateStatus updates the status for the GitRepo resource. It retries on
// updateStatus updates the status for the GitRepo resource. It retries on
// conflict. If the status was updated successfully, it also collects (as in
// updates) metrics for the resource GitRepo resource.
func UpdateStatus(ctx context.Context, c client.Client, req types.NamespacedName, status fleet.GitRepoStatus) error {
func updateStatus(ctx context.Context, c client.Client, req types.NamespacedName, status fleet.GitRepoStatus) error {
return retry.RetryOnConflict(retry.DefaultRetry, func() error {
t := &fleet.GitRepo{}
err := c.Get(ctx, req, t)
Expand Down
Loading

0 comments on commit 6a570fc

Please sign in to comment.