From f3509d2f8ac6c9067d53c3a45b6ccc66ab02ead3 Mon Sep 17 00:00:00 2001 From: rumstead <37445536+rumstead@users.noreply.github.com> Date: Fri, 17 Jan 2025 11:03:49 -0500 Subject: [PATCH] fix(appset): dont requeue appsets on status change (#21364) * e2e Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> * fix(appset): don't requeue on status changes Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> * fix spelling Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> * merge in annotation changes Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> * merge in annotation changes Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> * add more tests Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> * lint fix Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> * Update applicationset/controllers/applicationset_controller.go Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com> Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> * fix linting Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> * fix linting Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> --------- Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com> Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com> --- .../controllers/applicationset_controller.go | 122 ++++++--- .../applicationset_controller_test.go | 257 +++++++++++++++--- 2 files changed, 304 insertions(+), 75 deletions(-) diff --git a/applicationset/controllers/applicationset_controller.go b/applicationset/controllers/applicationset_controller.go index 287c599f7fe12..7498d6bbfcc7c 100644 --- a/applicationset/controllers/applicationset_controller.go +++ b/applicationset/controllers/applicationset_controller.go @@ -529,33 +529,6 @@ func ignoreNotAllowedNamespaces(namespaces []string) predicate.Predicate { }) } -// ignoreWhenAnnotationApplicationSetRefreshIsRemoved returns a predicate that ignores updates to ApplicationSet resources -// when the ApplicationSetRefresh annotation is removed -// First reconciliation is triggered when the annotation is added by [webhook.go#refreshApplicationSet] -// Using this predicate we avoid a second reconciliation triggered by the controller himself when the annotation is removed. -func ignoreWhenAnnotationApplicationSetRefreshIsRemoved() predicate.Predicate { - return predicate.Funcs{ - UpdateFunc: func(e event.UpdateEvent) bool { - oldAppset, isAppSet := e.ObjectOld.(*argov1alpha1.ApplicationSet) - if !isAppSet { - return false - } - newAppset, isAppSet := e.ObjectNew.(*argov1alpha1.ApplicationSet) - if !isAppSet { - return false - } - - _, oldHasRefreshAnnotation := oldAppset.Annotations[common.AnnotationApplicationSetRefresh] - _, newHasRefreshAnnotation := newAppset.Annotations[common.AnnotationApplicationSetRefresh] - - if oldHasRefreshAnnotation && !newHasRefreshAnnotation { - return false - } - return true - }, - } -} - func appControllerIndexer(rawObj client.Object) []string { // grab the job object, extract the owner... app := rawObj.(*argov1alpha1.Application) @@ -577,12 +550,13 @@ func (r *ApplicationSetReconciler) SetupWithManager(mgr ctrl.Manager, enableProg return fmt.Errorf("error setting up with manager: %w", err) } - ownsHandler := getOwnsHandlerPredicates(enableProgressiveSyncs) + appOwnsHandler := getApplicationOwnsHandler(enableProgressiveSyncs) + appSetOwnsHandler := getApplicationSetOwnsHandler() return ctrl.NewControllerManagedBy(mgr).WithOptions(controller.Options{ MaxConcurrentReconciles: maxConcurrentReconciliations, - }).For(&argov1alpha1.ApplicationSet{}, builder.WithPredicates(ignoreWhenAnnotationApplicationSetRefreshIsRemoved())). - Owns(&argov1alpha1.Application{}, builder.WithPredicates(ownsHandler)). + }).For(&argov1alpha1.ApplicationSet{}, builder.WithPredicates(appSetOwnsHandler)). + Owns(&argov1alpha1.Application{}, builder.WithPredicates(appOwnsHandler)). WithEventFilter(ignoreNotAllowedNamespaces(r.ApplicationSetNamespaces)). Watches( &corev1.Secret{}, @@ -590,7 +564,6 @@ func (r *ApplicationSetReconciler) SetupWithManager(mgr ctrl.Manager, enableProg Client: mgr.GetClient(), Log: log.WithField("type", "createSecretEventHandler"), }). - // TODO: also watch Applications and respond on changes if we own them. Complete(r) } @@ -1484,7 +1457,7 @@ func syncApplication(application argov1alpha1.Application, prune bool) argov1alp return application } -func getOwnsHandlerPredicates(enableProgressiveSyncs bool) predicate.Funcs { +func getApplicationOwnsHandler(enableProgressiveSyncs bool) predicate.Funcs { return predicate.Funcs{ CreateFunc: func(e event.CreateEvent) bool { // if we are the owner and there is a create event, we most likely created it and do not need to @@ -1521,8 +1494,8 @@ func getOwnsHandlerPredicates(enableProgressiveSyncs bool) predicate.Funcs { if !isApp { return false } - requeue := shouldRequeueApplicationSet(appOld, appNew, enableProgressiveSyncs) - logCtx.WithField("requeue", requeue).Debugf("requeue: %t caused by application %s", requeue, appNew.Name) + requeue := shouldRequeueForApplication(appOld, appNew, enableProgressiveSyncs) + logCtx.WithField("requeue", requeue).Debugf("requeue caused by application %s", appNew.Name) return requeue }, GenericFunc: func(e event.GenericEvent) bool { @@ -1539,13 +1512,13 @@ func getOwnsHandlerPredicates(enableProgressiveSyncs bool) predicate.Funcs { } } -// shouldRequeueApplicationSet determines when we want to requeue an ApplicationSet for reconciling based on an owned +// shouldRequeueForApplication determines when we want to requeue an ApplicationSet for reconciling based on an owned // application change // The applicationset controller owns a subset of the Application CR. // We do not need to re-reconcile if parts of the application change outside the applicationset's control. // An example being, Application.ApplicationStatus.ReconciledAt which gets updated by the application controller. // Additionally, Application.ObjectMeta.ResourceVersion and Application.ObjectMeta.Generation which are set by K8s. -func shouldRequeueApplicationSet(appOld *argov1alpha1.Application, appNew *argov1alpha1.Application, enableProgressiveSyncs bool) bool { +func shouldRequeueForApplication(appOld *argov1alpha1.Application, appNew *argov1alpha1.Application, enableProgressiveSyncs bool) bool { if appOld == nil || appNew == nil { return false } @@ -1578,4 +1551,81 @@ func shouldRequeueApplicationSet(appOld *argov1alpha1.Application, appNew *argov return false } +func getApplicationSetOwnsHandler() predicate.Funcs { + return predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + appSet, isApp := e.Object.(*argov1alpha1.ApplicationSet) + if !isApp { + return false + } + log.WithField("applicationset", appSet.QualifiedName()).Debugln("received create event") + // Always queue a new applicationset + return true + }, + DeleteFunc: func(e event.DeleteEvent) bool { + appSet, isApp := e.Object.(*argov1alpha1.ApplicationSet) + if !isApp { + return false + } + log.WithField("applicationset", appSet.QualifiedName()).Debugln("received delete event") + // Always queue for the deletion of an applicationset + return true + }, + UpdateFunc: func(e event.UpdateEvent) bool { + appSetOld, isAppSet := e.ObjectOld.(*argov1alpha1.ApplicationSet) + if !isAppSet { + return false + } + appSetNew, isAppSet := e.ObjectNew.(*argov1alpha1.ApplicationSet) + if !isAppSet { + return false + } + requeue := shouldRequeueForApplicationSet(appSetOld, appSetNew) + log.WithField("applicationset", appSetNew.QualifiedName()). + WithField("requeue", requeue).Debugln("received update event") + return requeue + }, + GenericFunc: func(e event.GenericEvent) bool { + appSet, isApp := e.Object.(*argov1alpha1.ApplicationSet) + if !isApp { + return false + } + log.WithField("applicationset", appSet.QualifiedName()).Debugln("received generic event") + // Always queue for the generic of an applicationset + return true + }, + } +} + +// shouldRequeueForApplicationSet determines when we need to requeue an applicationset +func shouldRequeueForApplicationSet(appSetOld, appSetNew *argov1alpha1.ApplicationSet) bool { + if appSetOld == nil || appSetNew == nil { + return false + } + // only compare the applicationset spec, annotations, labels and finalizers, specifically avoiding + // the status field. status is owned by the applicationset controller, + // and we do not need to requeue when it does bookkeeping + // NB: the ApplicationDestination comes from the ApplicationSpec being embedded + // in the ApplicationSetTemplate from the generators + if !cmp.Equal(appSetOld.Spec, appSetNew.Spec, cmpopts.EquateEmpty(), cmpopts.EquateComparable(argov1alpha1.ApplicationDestination{})) || + !cmp.Equal(appSetOld.ObjectMeta.GetLabels(), appSetNew.ObjectMeta.GetLabels(), cmpopts.EquateEmpty()) || + !cmp.Equal(appSetOld.ObjectMeta.GetFinalizers(), appSetNew.ObjectMeta.GetFinalizers(), cmpopts.EquateEmpty()) { + return true + } + + // Requeue only when the refresh annotation is newly added to the ApplicationSet. + // Changes to other annotations made simultaneously might be missed, but such cases are rare. + if !cmp.Equal(appSetOld.ObjectMeta.GetAnnotations(), appSetNew.ObjectMeta.GetAnnotations(), cmpopts.EquateEmpty()) { + _, oldHasRefreshAnnotation := appSetOld.Annotations[common.AnnotationApplicationSetRefresh] + _, newHasRefreshAnnotation := appSetNew.Annotations[common.AnnotationApplicationSetRefresh] + + if oldHasRefreshAnnotation && !newHasRefreshAnnotation { + return false + } + return true + } + + return false +} + var _ handler.EventHandler = &clusterSecretEventHandler{} diff --git a/applicationset/controllers/applicationset_controller_test.go b/applicationset/controllers/applicationset_controller_test.go index 254fea1c2e240..5d33dad908517 100644 --- a/applicationset/controllers/applicationset_controller_test.go +++ b/applicationset/controllers/applicationset_controller_test.go @@ -6390,13 +6390,13 @@ func TestResourceStatusAreOrdered(t *testing.T) { } } -func TestOwnsHandler(t *testing.T) { +func TestApplicationOwnsHandler(t *testing.T) { // progressive syncs do not affect create, delete, or generic - ownsHandler := getOwnsHandlerPredicates(true) + ownsHandler := getApplicationOwnsHandler(true) assert.False(t, ownsHandler.CreateFunc(event.CreateEvent{})) assert.True(t, ownsHandler.DeleteFunc(event.DeleteEvent{})) assert.True(t, ownsHandler.GenericFunc(event.GenericEvent{})) - ownsHandler = getOwnsHandlerPredicates(false) + ownsHandler = getApplicationOwnsHandler(false) assert.False(t, ownsHandler.CreateFunc(event.CreateEvent{})) assert.True(t, ownsHandler.DeleteFunc(event.DeleteEvent{})) assert.True(t, ownsHandler.GenericFunc(event.GenericEvent{})) @@ -6576,7 +6576,7 @@ func TestOwnsHandler(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ownsHandler = getOwnsHandlerPredicates(tt.args.enableProgressiveSyncs) + ownsHandler = getApplicationOwnsHandler(tt.args.enableProgressiveSyncs) assert.Equalf(t, tt.want, ownsHandler.UpdateFunc(tt.args.e), "UpdateFunc(%v)", tt.args.e) }) } @@ -6653,7 +6653,9 @@ func TestMigrateStatus(t *testing.T) { } } -func TestIgnoreWhenAnnotationApplicationSetRefreshIsRemoved(t *testing.T) { +func TestApplicationSetOwnsHandlerUpdate(t *testing.T) { + ownsHandler := getApplicationSetOwnsHandler() + buildAppSet := func(annotations map[string]string) *v1alpha1.ApplicationSet { return &v1alpha1.ApplicationSet{ ObjectMeta: metav1.ObjectMeta{ @@ -6663,67 +6665,244 @@ func TestIgnoreWhenAnnotationApplicationSetRefreshIsRemoved(t *testing.T) { } tests := []struct { - name string - oldAppSet crtclient.Object - newAppSet crtclient.Object - reconcileExpected bool + name string + appSetOld crtclient.Object + appSetNew crtclient.Object + want bool }{ + { + name: "Different Spec", + appSetOld: &v1alpha1.ApplicationSet{ + Spec: v1alpha1.ApplicationSetSpec{ + Generators: []v1alpha1.ApplicationSetGenerator{ + {List: &v1alpha1.ListGenerator{}}, + }, + }, + }, + appSetNew: &v1alpha1.ApplicationSet{ + Spec: v1alpha1.ApplicationSetSpec{ + Generators: []v1alpha1.ApplicationSetGenerator{ + {Git: &v1alpha1.GitGenerator{}}, + }, + }, + }, + want: true, + }, + { + name: "Different Annotations", + appSetOld: buildAppSet(map[string]string{"key1": "value1"}), + appSetNew: buildAppSet(map[string]string{"key1": "value2"}), + want: true, + }, + { + name: "Different Labels", + appSetOld: &v1alpha1.ApplicationSet{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"key1": "value1"}, + }, + }, + appSetNew: &v1alpha1.ApplicationSet{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"key1": "value2"}, + }, + }, + want: true, + }, + { + name: "Different Finalizers", + appSetOld: &v1alpha1.ApplicationSet{ + ObjectMeta: metav1.ObjectMeta{ + Finalizers: []string{"finalizer1"}, + }, + }, + appSetNew: &v1alpha1.ApplicationSet{ + ObjectMeta: metav1.ObjectMeta{ + Finalizers: []string{"finalizer2"}, + }, + }, + want: true, + }, + { + name: "No Changes", + appSetOld: &v1alpha1.ApplicationSet{ + Spec: v1alpha1.ApplicationSetSpec{ + Generators: []v1alpha1.ApplicationSetGenerator{ + {List: &v1alpha1.ListGenerator{}}, + }, + }, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"key1": "value1"}, + Labels: map[string]string{"key1": "value1"}, + Finalizers: []string{"finalizer1"}, + }, + }, + appSetNew: &v1alpha1.ApplicationSet{ + Spec: v1alpha1.ApplicationSetSpec{ + Generators: []v1alpha1.ApplicationSetGenerator{ + {List: &v1alpha1.ListGenerator{}}, + }, + }, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"key1": "value1"}, + Labels: map[string]string{"key1": "value1"}, + Finalizers: []string{"finalizer1"}, + }, + }, + want: false, + }, { name: "annotation removed", - oldAppSet: buildAppSet(map[string]string{ + appSetOld: buildAppSet(map[string]string{ argocommon.AnnotationApplicationSetRefresh: "true", }), - newAppSet: buildAppSet(map[string]string{}), - reconcileExpected: false, + appSetNew: buildAppSet(map[string]string{}), + want: false, }, { name: "annotation not removed", - oldAppSet: buildAppSet(map[string]string{ + appSetOld: buildAppSet(map[string]string{ argocommon.AnnotationApplicationSetRefresh: "true", }), - newAppSet: buildAppSet(map[string]string{ + appSetNew: buildAppSet(map[string]string{ argocommon.AnnotationApplicationSetRefresh: "true", }), - reconcileExpected: true, - }, - { - name: "annotation never existed", - oldAppSet: buildAppSet(map[string]string{}), - newAppSet: buildAppSet(map[string]string{}), - reconcileExpected: true, + want: false, }, { name: "annotation added", - oldAppSet: buildAppSet(map[string]string{}), - newAppSet: buildAppSet(map[string]string{ + appSetOld: buildAppSet(map[string]string{}), + appSetNew: buildAppSet(map[string]string{ argocommon.AnnotationApplicationSetRefresh: "true", }), - reconcileExpected: true, + want: true, + }, + { + name: "old object is not an appset", + appSetOld: &v1alpha1.Application{}, + appSetNew: buildAppSet(map[string]string{}), + want: false, + }, + { + name: "new object is not an appset", + appSetOld: buildAppSet(map[string]string{}), + appSetNew: &v1alpha1.Application{}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + requeue := ownsHandler.UpdateFunc(event.UpdateEvent{ + ObjectOld: tt.appSetOld, + ObjectNew: tt.appSetNew, + }) + assert.Equalf(t, tt.want, requeue, "ownsHandler.UpdateFunc(%v, %v)", tt.appSetOld, tt.appSetNew) + }) + } +} + +func TestApplicationSetOwnsHandlerGeneric(t *testing.T) { + ownsHandler := getApplicationSetOwnsHandler() + tests := []struct { + name string + obj crtclient.Object + want bool + }{ + { + name: "Object is ApplicationSet", + obj: &v1alpha1.ApplicationSet{}, + want: true, + }, + { + name: "Object is not ApplicationSet", + obj: &v1alpha1.Application{}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + requeue := ownsHandler.GenericFunc(event.GenericEvent{ + Object: tt.obj, + }) + assert.Equalf(t, tt.want, requeue, "ownsHandler.GenericFunc(%v)", tt.obj) + }) + } +} + +func TestApplicationSetOwnsHandlerCreate(t *testing.T) { + ownsHandler := getApplicationSetOwnsHandler() + tests := []struct { + name string + obj crtclient.Object + want bool + }{ + { + name: "Object is ApplicationSet", + obj: &v1alpha1.ApplicationSet{}, + want: true, + }, + { + name: "Object is not ApplicationSet", + obj: &v1alpha1.Application{}, + want: false, }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + requeue := ownsHandler.CreateFunc(event.CreateEvent{ + Object: tt.obj, + }) + assert.Equalf(t, tt.want, requeue, "ownsHandler.CreateFunc(%v)", tt.obj) + }) + } +} + +func TestApplicationSetOwnsHandlerDelete(t *testing.T) { + ownsHandler := getApplicationSetOwnsHandler() + tests := []struct { + name string + obj crtclient.Object + want bool + }{ { - name: "old object is not an appset", - oldAppSet: &v1alpha1.Application{}, - newAppSet: buildAppSet(map[string]string{}), - reconcileExpected: false, + name: "Object is ApplicationSet", + obj: &v1alpha1.ApplicationSet{}, + want: true, }, { - name: "new object is not an appset", - oldAppSet: buildAppSet(map[string]string{}), - newAppSet: &v1alpha1.Application{}, - reconcileExpected: false, + name: "Object is not ApplicationSet", + obj: &v1alpha1.Application{}, + want: false, }, } - predicate := ignoreWhenAnnotationApplicationSetRefreshIsRemoved() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + requeue := ownsHandler.DeleteFunc(event.DeleteEvent{ + Object: tt.obj, + }) + assert.Equalf(t, tt.want, requeue, "ownsHandler.DeleteFunc(%v)", tt.obj) + }) + } +} +func TestShouldRequeueForApplicationSet(t *testing.T) { + type args struct { + appSetOld *v1alpha1.ApplicationSet + appSetNew *v1alpha1.ApplicationSet + } + tests := []struct { + name string + args args + want bool + }{ + {name: "NilAppSet", args: args{appSetNew: &v1alpha1.ApplicationSet{}, appSetOld: nil}, want: false}, + } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - e := event.UpdateEvent{ - ObjectOld: tt.oldAppSet, - ObjectNew: tt.newAppSet, - } - result := predicate.Update(e) - assert.Equal(t, tt.reconcileExpected, result) + assert.Equalf(t, tt.want, shouldRequeueForApplicationSet(tt.args.appSetOld, tt.args.appSetNew), "shouldRequeueForApplicationSet(%v, %v)", tt.args.appSetOld, tt.args.appSetNew) }) } }