Skip to content
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

feat(appset): Add a cache layer for Argo Projects to speed-up application validation #18703

Merged
merged 8 commits into from
Sep 30, 2024
+78 −161
Merged

feat(appset): Add a cache layer for Argo Projects to speed-up application validation #18703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
caede62
feat(appset): Add a cache layer for Argo Projects to speed-up applica…
dacofr Jun 17, 2024
726144e
Use local client rather than custom cache
dacofr Jul 5, 2024
ffc89b3
Clean go.mod
dacofr Jul 5, 2024
6850646
Merge master
dacofr Jul 8, 2024
e37652e
Fix after merging master
dacofr Sep 11, 2024
a039995
Initialize appProject variable inside loop
dacofr Sep 11, 2024
4e20d64
Remove unused ArgoAppClientset field
dacofr Sep 25, 2024
add382e
Fix linter issue
dacofr Sep 25, 2024
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
9 changes: 5 additions & 4 deletions applicationset/controllers/applicationset_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ import (
"github.com/argoproj/argo-cd/v2/util/db"

argov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
appclientset "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned"
argoutil "github.com/argoproj/argo-cd/v2/util/argo"
"github.com/argoproj/argo-cd/v2/util/argo/normalizers"

Expand All @@ -79,7 +78,6 @@ type ApplicationSetReconciler struct {
Recorder record.EventRecorder
Generators map[string]generators.Generator
ArgoDB db.ArgoDB
ArgoAppClientset appclientset.Interface
KubeClientset kubernetes.Interface
Policy argov1alpha1.ApplicationsSyncPolicy
EnablePolicyOverride bool
Expand All @@ -97,6 +95,7 @@ type ApplicationSetReconciler struct {
// +kubebuilder:rbac:groups=argoproj.io,resources=applicationsets/status,verbs=get;update;patch

func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
startReconcile := time.Now()
logCtx := log.WithField("applicationset", req.NamespacedName)

var applicationSetInfo argov1alpha1.ApplicationSet
Expand Down Expand Up @@ -334,7 +333,7 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque
requeueAfter = ReconcileRequeueOnValidationError
}

logCtx.WithField("requeueAfter", requeueAfter).Info("end reconcile")
logCtx.WithField("requeueAfter", requeueAfter).Info("end reconcile in ", time.Since(startReconcile))

return ctrl.Result{
RequeueAfter: requeueAfter,
Expand Down Expand Up @@ -472,7 +471,9 @@ func (r *ApplicationSetReconciler) validateGeneratedApplications(ctx context.Con
errorsByIndex[i] = fmt.Errorf("ApplicationSet %s contains applications with duplicate name: %s", applicationSetInfo.Name, app.Name)
continue
}
_, err := r.ArgoAppClientset.ArgoprojV1alpha1().AppProjects(r.ArgoCDNamespace).Get(ctx, app.Spec.GetProject(), metav1.GetOptions{})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was the only non-test use of ArgoAppClientset. Can you go ahead and delete that field and clean up associated tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍 ArgoAppClientset field is deleted.
Job 'unit test with -race for Go packages' is failed, not sure if it's related to my changes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR updated with master and jobs are 🟢

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @crenshaw-dev 😃
Do you see any other improvements/changes related to this PR ? Do you think, this one can be merged ? 🙏


appProject := &argov1alpha1.AppProject{}
err := r.Client.Get(ctx, types.NamespacedName{Name: app.Spec.Project, Namespace: r.ArgoCDNamespace}, appProject)
if err != nil {
if apierr.IsNotFound(err) {
errorsByIndex[i] = fmt.Errorf("application references project %s which does not exist", app.Spec.Project)
Expand Down
Loading