From d83db006e07c9c71cc63b3bf86a1eaab850c30bf Mon Sep 17 00:00:00 2001 From: Thibault Jamet Date: Fri, 6 Sep 2024 15:09:56 +0200 Subject: [PATCH] fix(appset): Fix perpetual appset reconciliation ScmProvider does not guarantee order of the application resources from the applicationset which causes rapid sync activity for the applicationset as the objects and hence their resourceVersions are updated after each reconcile loop. This then triggers reconciliation of all objects watching the ApplicationSet. In order to prevent this behaviour, ensure that the ApplicationSet reconciler provides an idempotent list of resources, ensuring objects are not updated. Fixes: #19757 Co-authored-by: Fabian Selles Co-authored-by: Ariadna Rouco Signed-off-by: Thibault Jamet --- .../controllers/applicationset_controller.go | 14 ++- .../applicationset_controller_test.go | 102 ++++++++++++++++++ 2 files changed, 114 insertions(+), 2 deletions(-) diff --git a/applicationset/controllers/applicationset_controller.go b/applicationset/controllers/applicationset_controller.go index 1ba523ef74b1d..c6aaf925d6939 100644 --- a/applicationset/controllers/applicationset_controller.go +++ b/applicationset/controllers/applicationset_controller.go @@ -18,6 +18,7 @@ import ( "context" "fmt" "reflect" + "sort" "strings" "time" @@ -1263,13 +1264,22 @@ func (r *ApplicationSetReconciler) migrateStatus(ctx context.Context, appset *ar return nil } +func keys[T any](m map[string]T) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + func (r *ApplicationSetReconciler) updateResourcesStatus(ctx context.Context, logCtx *log.Entry, appset *argov1alpha1.ApplicationSet, apps []argov1alpha1.Application) error { statusMap := status.GetResourceStatusMap(appset) statusMap = status.BuildResourceStatus(statusMap, apps) statuses := []argov1alpha1.ResourceStatus{} - for _, status := range statusMap { - statuses = append(statuses, status) + for _, key := range keys(statusMap) { + statuses = append(statuses, statusMap[key]) } appset.Status.Resources = statuses diff --git a/applicationset/controllers/applicationset_controller_test.go b/applicationset/controllers/applicationset_controller_test.go index f11131dc33ef0..19969fa619bf8 100644 --- a/applicationset/controllers/applicationset_controller_test.go +++ b/applicationset/controllers/applicationset_controller_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "reflect" + "strconv" "strings" "testing" "time" @@ -6137,6 +6138,107 @@ func TestUpdateResourceStatus(t *testing.T) { } } +func generateNAppResourceStatuses(n int) []v1alpha1.ResourceStatus { + var r []v1alpha1.ResourceStatus + for i := 0; i < n; i++ { + r = append(r, v1alpha1.ResourceStatus{ + Name: "app" + strconv.Itoa(i), + Status: v1alpha1.SyncStatusCodeSynced, + Health: &v1alpha1.HealthStatus{ + Status: health.HealthStatusHealthy, + Message: "OK", + }, + }, + ) + } + return r +} + +func generateNHealthyApps(n int) []v1alpha1.Application { + var r []v1alpha1.Application + for i := 0; i < n; i++ { + r = append(r, v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "app" + strconv.Itoa(i), + }, + Status: v1alpha1.ApplicationStatus{ + Sync: v1alpha1.SyncStatus{ + Status: v1alpha1.SyncStatusCodeSynced, + }, + Health: v1alpha1.HealthStatus{ + Status: health.HealthStatusHealthy, + Message: "OK", + }, + }, + }) + } + return r +} + +func TestResourceStatusAreOrdered(t *testing.T) { + scheme := runtime.NewScheme() + err := v1alpha1.AddToScheme(scheme) + require.NoError(t, err) + + err = v1alpha1.AddToScheme(scheme) + require.NoError(t, err) + for _, cc := range []struct { + name string + appSet v1alpha1.ApplicationSet + apps []v1alpha1.Application + expectedResources []v1alpha1.ResourceStatus + }{ + { + name: "Ensures AppSet is always ordered", + appSet: v1alpha1.ApplicationSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "argocd", + }, + Status: v1alpha1.ApplicationSetStatus{ + Resources: []v1alpha1.ResourceStatus{}, + }, + }, + apps: generateNHealthyApps(10), + expectedResources: generateNAppResourceStatuses(10), + }, + } { + + t.Run(cc.name, func(t *testing.T) { + + kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...) + argoDBMock := dbmocks.ArgoDB{} + argoObjs := []runtime.Object{} + + client := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(&cc.appSet).WithObjects(&cc.appSet).Build() + metrics := appsetmetrics.NewFakeAppsetMetrics(client) + + r := ApplicationSetReconciler{ + Client: client, + Scheme: scheme, + Recorder: record.NewFakeRecorder(1), + Generators: map[string]generators.Generator{}, + ArgoDB: &argoDBMock, + ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...), + KubeClientset: kubeclientset, + Metrics: metrics, + } + + err := r.updateResourcesStatus(context.TODO(), log.NewEntry(log.StandardLogger()), &cc.appSet, cc.apps) + require.NoError(t, err, "expected no errors, but errors occurred") + + err = r.updateResourcesStatus(context.TODO(), log.NewEntry(log.StandardLogger()), &cc.appSet, cc.apps) + require.NoError(t, err, "expected no errors, but errors occurred") + + err = r.updateResourcesStatus(context.TODO(), log.NewEntry(log.StandardLogger()), &cc.appSet, cc.apps) + require.NoError(t, err, "expected no errors, but errors occurred") + + assert.Equal(t, cc.expectedResources, cc.appSet.Status.Resources, "expected resources did not match actual") + }) + + } +} + func TestOwnsHandler(t *testing.T) { // progressive syncs do not affect create, delete, or generic ownsHandler := getOwnsHandlerPredicates(true)