Skip to content

fix(applicationset_controller): requeue by ApplicationStatus changes #23043

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

Merged
merged 2 commits into from
May 19, 2025
Merged

fix(applicationset_controller): requeue by ApplicationStatus changes #23043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c46f6a3
fix(applicationset_controller): requeue by ApplicationStatus changes
dshmelev May 16, 2025
d1bb394
Update applicationset/controllers/applicationset_controller.go
dshmelev May 19, 2025
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
16 changes: 12 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 @@ -553,7 +553,7 @@ func (r *ApplicationSetReconciler) SetupWithManager(mgr ctrl.Manager, enableProg
}

appOwnsHandler := getApplicationOwnsHandler(enableProgressiveSyncs)
appSetOwnsHandler := getApplicationSetOwnsHandler()
appSetOwnsHandler := getApplicationSetOwnsHandler(enableProgressiveSyncs)

return ctrl.NewControllerManagedBy(mgr).WithOptions(controller.Options{
MaxConcurrentReconciles: maxConcurrentReconciliations,
Expand Down Expand Up @@ -1550,7 +1550,7 @@ func shouldRequeueForApplication(appOld *argov1alpha1.Application, appNew *argov
return false
}

func getApplicationSetOwnsHandler() predicate.Funcs {
func getApplicationSetOwnsHandler(enableProgressiveSyncs bool) predicate.Funcs {
return predicate.Funcs{
CreateFunc: func(e event.CreateEvent) bool {
appSet, isApp := e.Object.(*argov1alpha1.ApplicationSet)
Expand Down Expand Up @@ -1579,7 +1579,7 @@ func getApplicationSetOwnsHandler() predicate.Funcs {
if !isAppSet {
return false
}
requeue := shouldRequeueForApplicationSet(appSetOld, appSetNew)
requeue := shouldRequeueForApplicationSet(appSetOld, appSetNew, enableProgressiveSyncs)
log.WithField("applicationset", appSetNew.QualifiedName()).
WithField("requeue", requeue).Debugln("received update event")
return requeue
Expand All @@ -1597,10 +1597,18 @@ func getApplicationSetOwnsHandler() predicate.Funcs {
}

// shouldRequeueForApplicationSet determines when we need to requeue an applicationset
func shouldRequeueForApplicationSet(appSetOld, appSetNew *argov1alpha1.ApplicationSet) bool {
func shouldRequeueForApplicationSet(appSetOld, appSetNew *argov1alpha1.ApplicationSet, enableProgressiveSyncs bool) bool {
if appSetOld == nil || appSetNew == nil {
return false
}

// Requeue if any ApplicationStatus.Status changed for Progressive sync strategy
if enableProgressiveSyncs {
if !cmp.Equal(appSetOld.Status.ApplicationStatus, appSetNew.Status.ApplicationStatus, cmpopts.EquateEmpty()) {
return true
}
}

// 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
Expand Down
114 changes: 80 additions & 34 deletions applicationset/controllers/applicationset_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6678,8 +6678,6 @@ func TestMigrateStatus(t *testing.T) {
}

func TestApplicationSetOwnsHandlerUpdate(t *testing.T) {
ownsHandler := getApplicationSetOwnsHandler()

buildAppSet := func(annotations map[string]string) *v1alpha1.ApplicationSet {
return &v1alpha1.ApplicationSet{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -6689,10 +6687,11 @@ func TestApplicationSetOwnsHandlerUpdate(t *testing.T) {
}

tests := []struct {
name string
appSetOld crtclient.Object
appSetNew crtclient.Object
want bool
name string
appSetOld crtclient.Object
appSetNew crtclient.Object
enableProgressiveSyncs bool
want bool
}{
{
name: "Different Spec",
Expand All @@ -6710,13 +6709,15 @@ func TestApplicationSetOwnsHandlerUpdate(t *testing.T) {
},
},
},
want: true,
enableProgressiveSyncs: false,
want: true,
},
{
name: "Different Annotations",
appSetOld: buildAppSet(map[string]string{"key1": "value1"}),
appSetNew: buildAppSet(map[string]string{"key1": "value2"}),
want: true,
name: "Different Annotations",
appSetOld: buildAppSet(map[string]string{"key1": "value1"}),
appSetNew: buildAppSet(map[string]string{"key1": "value2"}),
enableProgressiveSyncs: false,
want: true,
},
{
name: "Different Labels",
Expand All @@ -6730,7 +6731,8 @@ func TestApplicationSetOwnsHandlerUpdate(t *testing.T) {
Labels: map[string]string{"key1": "value2"},
},
},
want: true,
enableProgressiveSyncs: false,
want: true,
},
{
name: "Different Finalizers",
Expand All @@ -6744,7 +6746,8 @@ func TestApplicationSetOwnsHandlerUpdate(t *testing.T) {
Finalizers: []string{"finalizer2"},
},
},
want: true,
enableProgressiveSyncs: false,
want: true,
},
{
name: "No Changes",
Expand Down Expand Up @@ -6772,15 +6775,17 @@ func TestApplicationSetOwnsHandlerUpdate(t *testing.T) {
Finalizers: []string{"finalizer1"},
},
},
want: false,
enableProgressiveSyncs: false,
want: false,
},
{
name: "annotation removed",
appSetOld: buildAppSet(map[string]string{
argocommon.AnnotationApplicationSetRefresh: "true",
}),
appSetNew: buildAppSet(map[string]string{}),
want: false,
appSetNew: buildAppSet(map[string]string{}),
enableProgressiveSyncs: false,
want: false,
},
{
name: "annotation not removed",
Expand All @@ -6790,43 +6795,48 @@ func TestApplicationSetOwnsHandlerUpdate(t *testing.T) {
appSetNew: buildAppSet(map[string]string{
argocommon.AnnotationApplicationSetRefresh: "true",
}),
want: false,
enableProgressiveSyncs: false,
want: false,
},
{
name: "annotation added",
appSetOld: buildAppSet(map[string]string{}),
appSetNew: buildAppSet(map[string]string{
argocommon.AnnotationApplicationSetRefresh: "true",
}),
want: true,
enableProgressiveSyncs: false,
want: true,
},
{
name: "old object is not an appset",
appSetOld: &v1alpha1.Application{},
appSetNew: buildAppSet(map[string]string{}),
want: false,
name: "old object is not an appset",
appSetOld: &v1alpha1.Application{},
appSetNew: buildAppSet(map[string]string{}),
enableProgressiveSyncs: false,
want: false,
},
{
name: "new object is not an appset",
appSetOld: buildAppSet(map[string]string{}),
appSetNew: &v1alpha1.Application{},
want: false,
name: "new object is not an appset",
appSetOld: buildAppSet(map[string]string{}),
appSetNew: &v1alpha1.Application{},
enableProgressiveSyncs: false,
want: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ownsHandler := getApplicationSetOwnsHandler(tt.enableProgressiveSyncs)
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)
assert.Equalf(t, tt.want, requeue, "ownsHandler.UpdateFunc(%v, %v, %t)", tt.appSetOld, tt.appSetNew, tt.enableProgressiveSyncs)
})
}
}

func TestApplicationSetOwnsHandlerGeneric(t *testing.T) {
ownsHandler := getApplicationSetOwnsHandler()
ownsHandler := getApplicationSetOwnsHandler(false)
tests := []struct {
name string
obj crtclient.Object
Expand Down Expand Up @@ -6855,7 +6865,7 @@ func TestApplicationSetOwnsHandlerGeneric(t *testing.T) {
}

func TestApplicationSetOwnsHandlerCreate(t *testing.T) {
ownsHandler := getApplicationSetOwnsHandler()
ownsHandler := getApplicationSetOwnsHandler(false)
tests := []struct {
name string
obj crtclient.Object
Expand Down Expand Up @@ -6884,7 +6894,7 @@ func TestApplicationSetOwnsHandlerCreate(t *testing.T) {
}

func TestApplicationSetOwnsHandlerDelete(t *testing.T) {
ownsHandler := getApplicationSetOwnsHandler()
ownsHandler := getApplicationSetOwnsHandler(false)
tests := []struct {
name string
obj crtclient.Object
Expand Down Expand Up @@ -6914,19 +6924,55 @@ func TestApplicationSetOwnsHandlerDelete(t *testing.T) {

func TestShouldRequeueForApplicationSet(t *testing.T) {
type args struct {
appSetOld *v1alpha1.ApplicationSet
appSetNew *v1alpha1.ApplicationSet
appSetOld *v1alpha1.ApplicationSet
appSetNew *v1alpha1.ApplicationSet
enableProgressiveSyncs bool
}
tests := []struct {
name string
args args
want bool
}{
{name: "NilAppSet", args: args{appSetNew: &v1alpha1.ApplicationSet{}, appSetOld: nil}, want: false},
{
name: "NilAppSet",
args: args{
appSetNew: &v1alpha1.ApplicationSet{},
appSetOld: nil,
enableProgressiveSyncs: false,
},
want: false,
},
{
name: "ApplicationSetApplicationStatusChanged",
args: args{
appSetOld: &v1alpha1.ApplicationSet{
Status: v1alpha1.ApplicationSetStatus{
ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Status: "Healthy",
},
},
},
},
appSetNew: &v1alpha1.ApplicationSet{
Status: v1alpha1.ApplicationSetStatus{
ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Status: "Waiting",
},
},
},
},
enableProgressiveSyncs: true,
},
want: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equalf(t, tt.want, shouldRequeueForApplicationSet(tt.args.appSetOld, tt.args.appSetNew), "shouldRequeueForApplicationSet(%v, %v)", tt.args.appSetOld, tt.args.appSetNew)
assert.Equalf(t, tt.want, shouldRequeueForApplicationSet(tt.args.appSetOld, tt.args.appSetNew, tt.args.enableProgressiveSyncs), "shouldRequeueForApplicationSet(%v, %v)", tt.args.appSetOld, tt.args.appSetNew)
})
}
}
Expand Down
Loading