fix: applicationset reduce redundant reconciles (#12457)

applicationset/controllers/applicationset_controller.go

@@ -17,6 +17,7 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"reflect"
 	"time"
 
 	log "github.com/sirupsen/logrus"
@@ -29,9 +30,12 @@ import (
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/record"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
 	"github.com/argoproj/argo-cd/v2/applicationset/generators"
@@ -533,7 +537,7 @@ func (r *ApplicationSetReconciler) SetupWithManager(mgr ctrl.Manager) error {
 
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&argov1alpha1.ApplicationSet{}).
-		Owns(&argov1alpha1.Application{}).
+		Owns(&argov1alpha1.Application{}, builder.WithPredicates(ownsHandler)).
 		Watches(
 			&source.Kind{Type: &corev1.Secret{}},
 			&clusterSecretEventHandler{
@@ -1312,4 +1316,67 @@ func syncApplication(application argov1alpha1.Application, prune bool) (argov1al
 	return application, nil
 }
 
+var ownsHandler = 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
+		// re-reconcile
+		log.Debugln("received create event from owning an application")
+		return false
+	},
+	DeleteFunc: func(e event.DeleteEvent) bool {
+		log.Debugln("received delete event from owning an application")
+		return true
+	},
+	UpdateFunc: func(e event.UpdateEvent) bool {
+		log.Debugln("received update event from owning an application")
+		appOld, isApp := e.ObjectOld.(*argov1alpha1.Application)
+		if !isApp {
+			return false
+		}
+		appNew, isApp := e.ObjectNew.(*argov1alpha1.Application)
+		if !isApp {
+			return false
+		}
+		requeue := shouldRequeueApplicationSet(appOld, appNew)
+		log.Debugf("requeue: %t caused by application %s\n", requeue, appNew.Name)
+		return requeue
+	},
+	GenericFunc: func(e event.GenericEvent) bool {
+		log.Debugln("received generic event from owning an application")
+		return true
+	},
+}
+
+// shouldRequeueApplicationSet 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) bool {
+	if appOld == nil || appNew == nil {
+		return false
+	}
+
+	// the applicationset controller owns the application spec, labels, annotations, and finalizers on the applications
+	if !reflect.DeepEqual(appOld.Spec, appNew.Spec) ||
+		!reflect.DeepEqual(appOld.ObjectMeta.GetAnnotations(), appNew.ObjectMeta.GetAnnotations()) ||
+		!reflect.DeepEqual(appOld.ObjectMeta.GetLabels(), appNew.ObjectMeta.GetLabels()) ||
+		!reflect.DeepEqual(appOld.ObjectMeta.GetFinalizers(), appNew.ObjectMeta.GetFinalizers()) {
+		return true
+	}
+
+	// progressive syncs use the application status for updates. if they differ, requeue to trigger the next progression
+	if appOld.Status.Health.Status != appNew.Status.Health.Status || appOld.Status.Sync.Status != appNew.Status.Sync.Status {
+		return true
+	}
+
+	if appOld.Status.OperationState != nil && appNew.Status.OperationState != nil &&
+		appOld.Status.OperationState.Phase != appNew.Status.OperationState.Phase {
+		return true
+	}
+
+	return false
+}
+
 var _ handler.EventHandler = &clusterSecretEventHandler{}

applicationset/controllers/applicationset_controller_test.go

@@ -24,6 +24,7 @@ import (
 	crtclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/event"
 
 	"github.com/argoproj/argo-cd/v2/applicationset/generators"
 	"github.com/argoproj/argo-cd/v2/applicationset/utils"
@@ -4838,3 +4839,105 @@ func TestUpdateApplicationSetApplicationStatusProgress(t *testing.T) {
 		})
 	}
 }
+
+func TestOwnsHandler(t *testing.T) {
+	assert.False(t, ownsHandler.CreateFunc(event.CreateEvent{}))
+	assert.True(t, ownsHandler.DeleteFunc(event.DeleteEvent{}))
+	assert.True(t, ownsHandler.GenericFunc(event.GenericEvent{}))
+
+	now := metav1.Now()
+	type args struct {
+		e event.UpdateEvent
+	}
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{name: "SameApplicationReconciledAtDiff", args: args{e: event.UpdateEvent{
+			ObjectOld: &argov1alpha1.Application{Status: argov1alpha1.ApplicationStatus{ReconciledAt: &now}},
+			ObjectNew: &argov1alpha1.Application{Status: argov1alpha1.ApplicationStatus{ReconciledAt: &now}},
+		}}, want: false},
+		{name: "SameApplicationResourceVersionDiff", args: args{e: event.UpdateEvent{
+			ObjectOld: &argov1alpha1.Application{ObjectMeta: metav1.ObjectMeta{
+				ResourceVersion: "foo",
+			}},
+			ObjectNew: &argov1alpha1.Application{ObjectMeta: metav1.ObjectMeta{
+				ResourceVersion: "bar",
+			}},
+		}}, want: false},
+		{name: "ApplicationHealthStatusDiff", args: args{e: event.UpdateEvent{
+			ObjectOld: &argov1alpha1.Application{Status: argov1alpha1.ApplicationStatus{
+				Health: argov1alpha1.HealthStatus{
+					Status: "Unknown",
+				},
+			}},
+			ObjectNew: &argov1alpha1.Application{Status: argov1alpha1.ApplicationStatus{
+				Health: argov1alpha1.HealthStatus{
+					Status: "Healthy",
+				},
+			}},
+		}}, want: true},
+		{name: "ApplicationSyncStatusDiff", args: args{e: event.UpdateEvent{
+			ObjectOld: &argov1alpha1.Application{Status: argov1alpha1.ApplicationStatus{
+				Sync: argov1alpha1.SyncStatus{
+					Status: "OutOfSync",
+				},
+			}},
+			ObjectNew: &argov1alpha1.Application{Status: argov1alpha1.ApplicationStatus{
+				Sync: argov1alpha1.SyncStatus{
+					Status: "Synced",
+				},
+			}},
+		}}, want: true},
+		{name: "ApplicationOperationStateDiff", args: args{e: event.UpdateEvent{
+			ObjectOld: &argov1alpha1.Application{Status: argov1alpha1.ApplicationStatus{
+				OperationState: &argov1alpha1.OperationState{
+					Phase: "foo",
+				},
+			}},
+			ObjectNew: &argov1alpha1.Application{Status: argov1alpha1.ApplicationStatus{
+				OperationState: &argov1alpha1.OperationState{
+					Phase: "bar",
+				},
+			}},
+		}}, want: true},
+		{name: "SameApplicationGeneration", args: args{e: event.UpdateEvent{
+			ObjectOld: &argov1alpha1.Application{ObjectMeta: metav1.ObjectMeta{
+				Generation: 1,
+			}},
+			ObjectNew: &argov1alpha1.Application{ObjectMeta: metav1.ObjectMeta{
+				Generation: 2,
+			}},
+		}}, want: false},
+		{name: "DifferentApplicationSpec", args: args{e: event.UpdateEvent{
+			ObjectOld: &argov1alpha1.Application{Spec: argov1alpha1.ApplicationSpec{Project: "default"}},
+			ObjectNew: &argov1alpha1.Application{Spec: argov1alpha1.ApplicationSpec{Project: "not-default"}},
+		}}, want: true},
+		{name: "DifferentApplicationLabels", args: args{e: event.UpdateEvent{
+			ObjectOld: &argov1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}},
+			ObjectNew: &argov1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"bar": "foo"}}},
+		}}, want: true},
+		{name: "DifferentApplicationAnnotations", args: args{e: event.UpdateEvent{
+			ObjectOld: &argov1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"foo": "bar"}}},
+			ObjectNew: &argov1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"bar": "foo"}}},
+		}}, want: true},
+		{name: "DifferentApplicationFinalizers", args: args{e: event.UpdateEvent{
+			ObjectOld: &argov1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Finalizers: []string{"argo"}}},
+			ObjectNew: &argov1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Finalizers: []string{"none"}}},
+		}}, want: true},
+		{name: "NotAnAppOld", args: args{e: event.UpdateEvent{
+			ObjectOld: &argov1alpha1.AppProject{},
+			ObjectNew: &argov1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"bar": "foo"}}},
+		}}, want: false},
+		{name: "NotAnAppNew", args: args{e: event.UpdateEvent{
+			ObjectOld: &argov1alpha1.Application{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}},
+			ObjectNew: &argov1alpha1.AppProject{},
+		}}, want: false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equalf(t, tt.want, ownsHandler.UpdateFunc(tt.args.e), "UpdateFunc(%v)", tt.args.e)
+		})
+	}
+}