diff --git a/internal/chart/providers/argocd/applications.go b/internal/chart/providers/argocd/applications.go index 43caf58a..0ec109b7 100644 --- a/internal/chart/providers/argocd/applications.go +++ b/internal/chart/providers/argocd/applications.go @@ -65,6 +65,11 @@ type Manager struct { // install). The force-sync path sets a shorter value — re-syncing an existing // platform should not block for an hour. waitTimeout time.Duration + + // groupWait bounds how long syncChildApplications waits for each sync group + // to converge before triggering the next one. Zero means the default + // (defaultGroupWait). Tests set a tiny value for speed. + groupWait time.Duration } // WithWaitTimeout sets a custom WaitForApplications timeout and returns the diff --git a/internal/chart/providers/argocd/sync.go b/internal/chart/providers/argocd/sync.go index 16fdf678..2126ac63 100644 --- a/internal/chart/providers/argocd/sync.go +++ b/internal/chart/providers/argocd/sync.go @@ -3,6 +3,8 @@ package argocd import ( "context" "fmt" + "sort" + "strconv" "strings" "time" @@ -94,10 +96,32 @@ func (m *Manager) RefreshAndSync(ctx context.Context, prune bool) error { // 4) Sync the child Applications too. Syncing only the root updates child // specs but does not roll them out: children that are not auto-sync would // stay OutOfSync and WaitForApplications would then block until its timeout. - // Best-effort — a child that already has an operation in flight is skipped. + // Children labeled with SyncGroupLabel are synced group-by-group, gated on + // health, so dependency layers land in order. Best-effort — a child that + // already has an operation in flight is skipped. return m.syncChildApplications(ctx, prune) } +// SyncGroupLabel is the deploy-ordering label the openframe-oss-tenant app +// templates stamp on every child Application (1=foundation … 5=platform). +// Children sharing a value form a group; groups are synced lowest-first, each +// gated on the previous group reaching Healthy+Synced, so the resource-heavy +// layers don't all land on the cluster at once. +const SyncGroupLabel = "openframe.io/sync-group" + +// defaultSyncGroup mirrors the manifests' template default for apps without an +// explicit group ({{ $app.syncGroup | default "3" }}); a labeled install where +// some app misses the label slots it into the middle group. +const defaultSyncGroup = 3 + +// defaultGroupWait bounds how long each group may take to converge before the +// next group is triggered anyway. The gate is deliberately best-effort: child +// Applications carry `automated` sync policies and may converge on their own +// regardless of trigger order, and WaitForApplications remains the +// authoritative readiness gate after RefreshAndSync — failing the whole +// upgrade on a slow group would only degrade UX without adding safety. +const defaultGroupWait = 5 * time.Minute + // trackingInstanceLabel is ArgoCD's LABEL resource-tracking marker: resources // managed by an Application carry app.kubernetes.io/instance=. Child // Applications created by the app-of-apps therefore carry the root's name. @@ -136,6 +160,12 @@ func trackingOwner(labels, annotations map[string]string) string { // but that may touch Applications that are not OpenFrame-owned (a real risk on // a shared cluster), which the fallback warning makes visible. // +// Children carrying the SyncGroupLabel are synced group-by-group (lowest +// first), each group gated on the previous one converging to Healthy+Synced +// (bounded by groupWait, best-effort — see defaultGroupWait). When NO child +// carries the group label (legacy manifests) all children are synced at once, +// exactly the previous behaviour. +// // Per-child failures no longer vanish (audit F8): individual errors are // counted and surfaced — a warning on partial failure, an error when NOT ONE // child could be synced (previously `app upgrade --sync` would then "succeed" @@ -147,20 +177,20 @@ func (m *Manager) syncChildApplications(ctx context.Context, prune bool) error { return fmt.Errorf("listing applications to sync: %w", err) } - var children []string + var children []unstructured.Unstructured for i := range list.Items { name := list.Items[i].GetName() if name == AppOfAppsName { continue } if trackingOwner(list.Items[i].GetLabels(), list.Items[i].GetAnnotations()) == AppOfAppsName { - children = append(children, name) + children = append(children, list.Items[i]) } } if children == nil { for i := range list.Items { - if name := list.Items[i].GetName(); name != AppOfAppsName { - children = append(children, name) + if list.Items[i].GetName() != AppOfAppsName { + children = append(children, list.Items[i]) } } if len(children) > 0 { @@ -169,7 +199,31 @@ func (m *Manager) syncChildApplications(ctx context.Context, prune bool) error { } } - patched, failed, firstErr := m.syncApplicationsByName(ctx, children, prune) + var patched, failed int + var firstErr error + groups, labeled := groupChildren(children) + if !labeled { + // Legacy manifests without the group label: one ungated pass over all. + patched, failed, firstErr = m.syncApplicationsByName(ctx, groups[0].names, prune) + } else { + for i, g := range groups { + pterm.Info.Printf("Sync group %d: syncing %d application(s): %s\n", g.number, len(g.names), strings.Join(g.names, ", ")) + p, f, e := m.syncApplicationsByName(ctx, g.names, prune) + patched, failed = patched+p, failed+f + if firstErr == nil { + firstErr = e + } + // No gate after the last group — WaitForApplications takes over. + if i == len(groups)-1 { + break + } + if notReady := m.waitGroupReady(ctx, g.names); len(notReady) > 0 { + pterm.Warning.Printf("Sync group %d not fully ready after %s (waiting on: %s); continuing with the next group\n", + g.number, m.groupWaitBudget(), strings.Join(notReady, ", ")) + } + } + } + if failed > 0 && patched == 0 { return fmt.Errorf("could not trigger a sync on any of the %d child applications (first error: %w)", failed, firstErr) } @@ -179,6 +233,84 @@ func (m *Manager) syncChildApplications(ctx context.Context, prune bool) error { return nil } +// syncGroup is one deploy-ordering group: its number and the (sorted) names of +// the child Applications in it. +type syncGroup struct { + number int + names []string +} + +// groupChildren buckets child Applications by their SyncGroupLabel value, +// returning the groups sorted lowest-first and whether ANY child carried the +// label. When labeled is false the single returned group holds every child +// (legacy manifests → caller keeps the ungated single-pass behaviour). A child +// missing the label — or carrying a non-numeric value — on an otherwise +// labeled install falls into defaultSyncGroup, mirroring the template default. +func groupChildren(children []unstructured.Unstructured) (groups []syncGroup, labeled bool) { + byNumber := map[int][]string{} + var all []string + for i := range children { + name := children[i].GetName() + all = append(all, name) + group := defaultSyncGroup + if v := children[i].GetLabels()[SyncGroupLabel]; v != "" { + labeled = true + if n, err := strconv.Atoi(strings.TrimSpace(v)); err == nil { + group = n + } + } + byNumber[group] = append(byNumber[group], name) + } + if !labeled { + return []syncGroup{{number: defaultSyncGroup, names: all}}, false + } + for n, names := range byNumber { + sort.Strings(names) + groups = append(groups, syncGroup{number: n, names: names}) + } + sort.Slice(groups, func(i, j int) bool { return groups[i].number < groups[j].number }) + return groups, true +} + +// groupWaitBudget returns the per-group convergence budget (default when unset). +func (m *Manager) groupWaitBudget() time.Duration { + if m.groupWait > 0 { + return m.groupWait + } + return defaultGroupWait +} + +// waitGroupReady polls until every named Application is Healthy+Synced or the +// group budget elapses, returning the names still not ready (nil when the +// group converged). Read errors abort the wait rather than block the rollout — +// the gate is an ordering optimization, not a correctness barrier. +func (m *Manager) waitGroupReady(ctx context.Context, names []string) []string { + apps := m.dynamicClient.Resource(applicationGVR).Namespace(ArgoCDNamespace) + var notReady []string + pollUntil(ctx, m.groupWaitBudget(), func() bool { + list, err := apps.List(ctx, metav1.ListOptions{}) + if err != nil { + notReady = nil // can't read — stop gating, let the sync proceed + return true + } + status := make(map[string]bool, len(list.Items)) + for i := range list.Items { + obj := list.Items[i].Object + health, _, _ := unstructured.NestedString(obj, "status", "health", "status") + syncStatus, _, _ := unstructured.NestedString(obj, "status", "sync", "status") + status[list.Items[i].GetName()] = health == ArgoCDHealthHealthy && syncStatus == ArgoCDSyncSynced + } + notReady = notReady[:0] + for _, name := range names { + if !status[name] { + notReady = append(notReady, name) + } + } + return len(notReady) == 0 + }) + return notReady +} + // syncApplicationsByName applies the sync-operation patch to each named // Application, returning how many were patched, how many failed, and the first // failure. Lazily initializes the Kubernetes clients like RefreshAndSync. diff --git a/internal/chart/providers/argocd/sync_test.go b/internal/chart/providers/argocd/sync_test.go index b6854fbe..eb807dbf 100644 --- a/internal/chart/providers/argocd/sync_test.go +++ b/internal/chart/providers/argocd/sync_test.go @@ -4,8 +4,10 @@ import ( "context" "strings" "testing" + "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) // TestRefreshAndSync_PatchesAppOfApps proves force-sync applies both the hard @@ -130,6 +132,84 @@ func TestRefreshAndSync_NoClient(t *testing.T) { } } +// appObjInGroup is appObj plus the deploy-ordering SyncGroupLabel. +func appObjInGroup(name, health, sync, group string) *unstructured.Unstructured { + obj := appObj(name, health, sync) + obj.SetLabels(map[string]string{SyncGroupLabel: group}) + return obj +} + +// TestGroupChildren_SortsAndDefaults locks the grouping rules: groups sort +// lowest-first, an unlabeled child on a labeled install falls into +// defaultSyncGroup, and a fully unlabeled install reports labeled=false with +// every child in one group (legacy single-pass behaviour). +func TestGroupChildren_SortsAndDefaults(t *testing.T) { + children := []unstructured.Unstructured{ + *appObjInGroup("tenant", ArgoCDHealthHealthy, ArgoCDSyncSynced, "5"), + *appObjInGroup("kafka", ArgoCDHealthHealthy, ArgoCDSyncSynced, "3"), + *appObjInGroup("zookeeper", ArgoCDHealthHealthy, ArgoCDSyncSynced, "2"), + *appObjInGroup("cassandra", ArgoCDHealthHealthy, ArgoCDSyncSynced, "2"), + *appObj("unlabeled", ArgoCDHealthHealthy, ArgoCDSyncSynced), // → defaultSyncGroup + } + + groups, labeled := groupChildren(children) + if !labeled { + t.Fatal("labeled = false, want true") + } + wantNumbers := []int{2, 3, 5} + if len(groups) != len(wantNumbers) { + t.Fatalf("got %d groups, want %d: %+v", len(groups), len(wantNumbers), groups) + } + for i, n := range wantNumbers { + if groups[i].number != n { + t.Errorf("groups[%d].number = %d, want %d", i, groups[i].number, n) + } + } + if got := strings.Join(groups[0].names, ","); got != "cassandra,zookeeper" { + t.Errorf("group 2 = %q, want cassandra,zookeeper", got) + } + if got := strings.Join(groups[1].names, ","); got != "kafka,unlabeled" { + t.Errorf("group 3 = %q, want kafka,unlabeled (unlabeled must default to group %d)", got, defaultSyncGroup) + } + + // Fully unlabeled install → single group, labeled=false. + groups, labeled = groupChildren(children[4:]) + if labeled || len(groups) != 1 || len(groups[0].names) != 1 { + t.Errorf("unlabeled install: labeled=%v groups=%+v, want one unlabeled group", labeled, groups) + } +} + +// TestRefreshAndSync_GroupedChildrenAllSynced proves the grouped path still +// triggers a sync operation on every labeled child (groups gate the ORDER, +// never drop apps), including a child whose group never converges — the gate +// is best-effort and must not block later groups. +func TestRefreshAndSync_GroupedChildrenAllSynced(t *testing.T) { + m := fakeManager( + appObj(AppOfAppsName, ArgoCDHealthHealthy, ArgoCDSyncSynced), + appObjInGroup("ingress-nginx", ArgoCDHealthHealthy, ArgoCDSyncSynced, "1"), + // Group 2 never reaches Healthy against the controllerless fake: + // the gate must time out (tiny groupWait) and move on, not hang. + appObjInGroup("zookeeper", ArgoCDHealthProgressing, ArgoCDSyncOutOfSync, "2"), + appObjInGroup("kafka", ArgoCDHealthHealthy, ArgoCDSyncSynced, "3"), + ) + m.groupWait = 10 * time.Millisecond + + if err := m.RefreshAndSync(context.Background(), false); err != nil { + t.Fatalf("RefreshAndSync: %v", err) + } + + res := m.dynamicClient.Resource(applicationGVR).Namespace(ArgoCDNamespace) + for _, name := range []string{"ingress-nginx", "zookeeper", "kafka"} { + got, err := res.Get(context.Background(), name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("get %s: %v", name, err) + } + if _, ok := got.Object["operation"]; !ok { + t.Errorf("%s must have a sync .operation set", name) + } + } +} + // unstructuredNestedString reads a nested string value from an unstructured map. func unstructuredNestedString(obj map[string]interface{}, fields ...string) (string, bool, error) { cur := obj