diff --git a/pkg/bundler/deployer/argocd/argocd.go b/pkg/bundler/deployer/argocd/argocd.go index e7bb20045..4cf6d06ec 100644 --- a/pkg/bundler/deployer/argocd/argocd.go +++ b/pkg/bundler/deployer/argocd/argocd.go @@ -108,6 +108,15 @@ type ApplicationData struct { // CascadeDelete adds ResourcesFinalizer to the rendered Application. // See #1628. CascadeDelete bool + + // ApplyOutOfSyncOnly adds ApplyOutOfSyncOnly=true to spec.syncPolicy.syncOptions. + // Set only for the -readiness folder's Application: paired with the Job-level + // Replace=true,Force=true annotation (see gatemanifest.jobMetadataAnnotations), + // this prevents ArgoCD from deleting/recreating the readiness-gate Job on every + // sync when nothing actually changed — Replace+Force alone forces a + // delete-and-recreate unconditionally. See #2367 and the CodeRabbit finding on + // PR #2408. + ApplyOutOfSyncOnly bool } // AppOfAppsData contains data for rendering the App of Apps manifest. @@ -790,13 +799,22 @@ func waveForFolder(f localformat.Folder, level int) int { return base case f.Parent + "-post": return base + 2 - case f.Parent + "-readiness": - return base + 3 - default: // primary: Name == Parent - return base + 1 + default: + if isReadinessFolder(f) { + return base + 3 + } + return base + 1 // primary: Name == Parent } } +// isReadinessFolder reports whether f is the injected -readiness folder for +// its parent component. Shared by waveForFolder (sync-wave banding) and +// buildApplicationData (ApplyOutOfSyncOnly scoping) so the two can't +// silently diverge on what counts as a readiness folder. See #2367. +func isReadinessFolder(f localformat.Folder) bool { + return f.Name == f.Parent+"-readiness" +} + // buildApplicationData constructs ApplicationData for a single folder. The // FolderKind drives the Application shape — KindLocalHelm sets IsLocalChart // (path-based single-source); KindUpstreamHelm leaves it empty (multi-source @@ -811,12 +829,13 @@ func waveForFolder(f localformat.Folder, level int) int { func buildApplicationData(comp recipe.ComponentRef, f localformat.Folder, syncWave int, repoURL, targetRevision string, values map[string]any, inline bool) (ApplicationData, error) { chart := comp.EffectiveChart() data := ApplicationData{ - Name: f.Name, - Namespace: comp.Namespace, - SyncWave: syncWave, - RepoURL: repoURL, - TargetRevision: targetRevision, - BundleDir: f.Dir, + Name: f.Name, + Namespace: comp.Namespace, + SyncWave: syncWave, + RepoURL: repoURL, + TargetRevision: targetRevision, + BundleDir: f.Dir, + ApplyOutOfSyncOnly: isReadinessFolder(f), } switch f.Kind { case localformat.KindLocalHelm: diff --git a/pkg/bundler/deployer/argocd/argocd_test.go b/pkg/bundler/deployer/argocd/argocd_test.go index 60b2f9dc2..9c17c5267 100644 --- a/pkg/bundler/deployer/argocd/argocd_test.go +++ b/pkg/bundler/deployer/argocd/argocd_test.go @@ -2343,3 +2343,128 @@ func TestGenerate_ChildNameLimits(t *testing.T) { }) } } + +// TestBuildApplicationData_ApplyOutOfSyncOnly is the regression-proofing +// test for the SCOPING of ApplicationData.ApplyOutOfSyncOnly, not just its +// presence: it exercises all four folder kinds for the same parent +// component so a test that only checked the readiness case could not pass +// if the predicate degenerated to unconditionally true. Paired with the +// Job-level Replace=true,Force=true annotation in +// pkg/bundler/gatemanifest/manifest.go, ApplyOutOfSyncOnly (set only for +// the readiness Application) stops ArgoCD from delete-and-recreating the +// readiness-gate Job on every no-op resync. See #2367. +func TestBuildApplicationData_ApplyOutOfSyncOnly(t *testing.T) { + comp := recipe.ComponentRef{ + Name: "gpu-operator", + Source: "https://helm.ngc.nvidia.com/nvidia", + Chart: "gpu-operator", + Version: "v25.3.3", + } + + tests := []struct { + name string + folder localformat.Folder + want bool + }{ + { + name: "primary folder", + folder: localformat.Folder{Name: "gpu-operator", Dir: "001-gpu-operator", Kind: localformat.KindUpstreamHelm, Parent: "gpu-operator"}, + want: false, + }, + { + name: "-pre folder", + folder: localformat.Folder{Name: "gpu-operator-pre", Dir: "001-gpu-operator-pre", Kind: localformat.KindLocalHelm, Parent: "gpu-operator"}, + want: false, + }, + { + name: "-post folder", + folder: localformat.Folder{Name: "gpu-operator-post", Dir: "003-gpu-operator-post", Kind: localformat.KindLocalHelm, Parent: "gpu-operator"}, + want: false, + }, + { + name: "-readiness folder", + folder: localformat.Folder{Name: "gpu-operator-readiness", Dir: "004-gpu-operator-readiness", Kind: localformat.KindLocalHelm, Parent: "gpu-operator"}, + want: true, + }, + { + // Adversarial: a primary folder whose NAME merely contains + // "-readiness" (not the synthetic suffix pattern relative to + // its own Parent) must not be scoped in. f.Parent+"-readiness" + // = "foo-readiness-readiness" != "foo-readiness", so the + // predicate correctly evaluates false. + name: "primary folder whose name happens to contain -readiness", + folder: localformat.Folder{Name: "foo-readiness", Dir: "005-foo-readiness", Kind: localformat.KindUpstreamHelm, Parent: "foo-readiness"}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + data, err := buildApplicationData(comp, tt.folder, 0, "https://github.com/example/repo.git", "main", nil, false) + if err != nil { + t.Fatalf("buildApplicationData() error = %v", err) + } + if data.ApplyOutOfSyncOnly != tt.want { + t.Errorf("ApplyOutOfSyncOnly = %v, want %v (folder=%+v)", data.ApplyOutOfSyncOnly, tt.want, tt.folder) + } + }) + } +} + +// TestGenerate_ApplyOutOfSyncOnlySyncOptions asserts the rendered +// application.yaml for a -readiness folder carries the exact +// "- ApplyOutOfSyncOnly=true" syncOptions entry, and that a non-readiness +// folder's rendered application.yaml does not mention ApplyOutOfSyncOnly +// anywhere. See #2367. +func TestGenerate_ApplyOutOfSyncOnlySyncOptions(t *testing.T) { + ctx := context.Background() + outputDir := t.TempDir() + + recipeResult := &recipe.RecipeResult{} + recipeResult.Metadata.Version = testVersion + recipeResult.ComponentRefs = []recipe.ComponentRef{ + { + Name: "gpu-operator", + Namespace: "gpu-operator", + Chart: "gpu-operator", + Version: "v25.3.3", + Type: recipe.ComponentTypeHelm, + Source: "https://helm.ngc.nvidia.com/nvidia", + }, + } + recipeResult.DeploymentOrder = []string{"gpu-operator"} + + g := &Generator{ + RecipeResult: recipeResult, + ComponentValues: map[string]map[string]any{"gpu-operator": {}}, + Version: "v0.0.0-test", + RepoURL: "https://github.com/example/aicr-bundles.git", + TargetRevision: "main", + ComponentReadiness: map[string]map[string][]byte{ + "gpu-operator": { + "readiness.yaml": []byte("apiVersion: batch/v1\nkind: Job\nmetadata:\n" + + " name: gpu-operator-readiness-gate\n namespace: {{ .Release.Namespace }}\n"), + }, + }, + } + + if _, err := g.Generate(ctx, outputDir); err != nil { + t.Fatalf("Generate() error = %v", err) + } + + primary, err := os.ReadFile(filepath.Join(outputDir, "001-gpu-operator", "application.yaml")) + if err != nil { + t.Fatalf("read primary application.yaml: %v", err) + } + if strings.Contains(string(primary), "ApplyOutOfSyncOnly") { + t.Errorf("primary Application must not mention ApplyOutOfSyncOnly:\n%s", primary) + } + + readiness, err := os.ReadFile(filepath.Join(outputDir, "002-gpu-operator-readiness", "application.yaml")) + if err != nil { + t.Fatalf("read readiness application.yaml: %v", err) + } + if !strings.Contains(string(readiness), "- ApplyOutOfSyncOnly=true") { + t.Errorf("readiness Application must contain \"- ApplyOutOfSyncOnly=true\":\n%s", readiness) + } +} diff --git a/pkg/bundler/deployer/argocd/templates/application.yaml.tmpl b/pkg/bundler/deployer/argocd/templates/application.yaml.tmpl index aa29400c2..d887c665c 100644 --- a/pkg/bundler/deployer/argocd/templates/application.yaml.tmpl +++ b/pkg/bundler/deployer/argocd/templates/application.yaml.tmpl @@ -56,3 +56,12 @@ spec: # per-Application template branching and a confusing mix of sync # strategies in a single bundle. - ServerSideApply=true +{{- if .ApplyOutOfSyncOnly }} + # ApplyOutOfSyncOnly: scoped to the readiness-gate Application only. + # Paired with the Job's Replace=true,Force=true annotation + # (pkg/bundler/gatemanifest/manifest.go), this skips resources ArgoCD's + # diff already considers in-sync so a no-op resync does not + # delete-and-recreate the readiness-gate Job (and rerun its checks) + # when nothing changed. See #2367. + - ApplyOutOfSyncOnly=true +{{- end }} diff --git a/pkg/bundler/deployer/argocd/testdata/readiness_gate/002-gpu-operator-readiness/application.yaml b/pkg/bundler/deployer/argocd/testdata/readiness_gate/002-gpu-operator-readiness/application.yaml index 26cc57258..58b417720 100644 --- a/pkg/bundler/deployer/argocd/testdata/readiness_gate/002-gpu-operator-readiness/application.yaml +++ b/pkg/bundler/deployer/argocd/testdata/readiness_gate/002-gpu-operator-readiness/application.yaml @@ -31,3 +31,10 @@ spec: # per-Application template branching and a confusing mix of sync # strategies in a single bundle. - ServerSideApply=true + # ApplyOutOfSyncOnly: scoped to the readiness-gate Application only. + # Paired with the Job's Replace=true,Force=true annotation + # (pkg/bundler/gatemanifest/manifest.go), this skips resources ArgoCD's + # diff already considers in-sync so a no-op resync does not + # delete-and-recreate the readiness-gate Job (and rerun its checks) + # when nothing changed. See #2367. + - ApplyOutOfSyncOnly=true diff --git a/pkg/bundler/deployer/argocd/testdata/readiness_gate/002-gpu-operator-readiness/templates/readiness.yaml b/pkg/bundler/deployer/argocd/testdata/readiness_gate/002-gpu-operator-readiness/templates/readiness.yaml index 7c229dcbc..d22c7fe34 100644 --- a/pkg/bundler/deployer/argocd/testdata/readiness_gate/002-gpu-operator-readiness/templates/readiness.yaml +++ b/pkg/bundler/deployer/argocd/testdata/readiness_gate/002-gpu-operator-readiness/templates/readiness.yaml @@ -59,7 +59,7 @@ metadata: name: gpu-operator-readiness-gate namespace: gpu-operator annotations: - argocd.argoproj.io/sync-options: Replace=true + argocd.argoproj.io/sync-options: Replace=true,Force=true spec: backoffLimit: 6 template: diff --git a/pkg/bundler/deployer/argocdhelm/argocdhelm_test.go b/pkg/bundler/deployer/argocdhelm/argocdhelm_test.go index aaa26ebfd..5ccff53fa 100644 --- a/pkg/bundler/deployer/argocdhelm/argocdhelm_test.go +++ b/pkg/bundler/deployer/argocdhelm/argocdhelm_test.go @@ -1949,6 +1949,63 @@ func TestBundleGolden_ReadinessGate(t *testing.T) { } } +// TestGenerate_ApplyOutOfSyncOnlySyncOptions is the empirical proof that +// transformApplication's generic map[string]any YAML round-trip preserves +// the ApplyOutOfSyncOnly=true syncOptions entry the delegated argocd.Generator +// adds to the readiness folder's Application, and that it stays scoped to +// only that folder's transformed Helm-chart-template output. See #2367. +func TestGenerate_ApplyOutOfSyncOnlySyncOptions(t *testing.T) { + ctx := context.Background() + outputDir := t.TempDir() + + rr := newRecipeResult("v1.0.0", []recipe.ComponentRef{ + { + Name: "gpu-operator", + Namespace: "gpu-operator", + Chart: "gpu-operator", + Version: "v25.3.3", + Type: recipe.ComponentTypeHelm, + Source: "https://helm.ngc.nvidia.com/nvidia", + }, + }) + rr.DeploymentOrder = []string{"gpu-operator"} + + g := &Generator{ + RecipeResult: rr, + ComponentValues: map[string]map[string]any{ + "gpu-operator": {"driver": map[string]any{"version": "580"}}, + }, + Version: "v0.0.0-test", + RepoURL: "https://github.com/example/aicr-bundles.git", + TargetRevision: "main", + ComponentReadiness: map[string]map[string][]byte{ + "gpu-operator": { + "readiness.yaml": readinessGateManifest(t, config.DeployerArgoCDHelm), + }, + }, + } + + if _, err := g.Generate(ctx, outputDir); err != nil { + t.Fatalf("Generate() error = %v", err) + } + + primary, err := os.ReadFile(filepath.Join(outputDir, "templates", "gpu-operator.yaml")) + if err != nil { + t.Fatalf("read primary template: %v", err) + } + if strings.Contains(string(primary), "ApplyOutOfSyncOnly") { + t.Errorf("primary child template must not mention ApplyOutOfSyncOnly:\n%s", primary) + } + + readiness, err := os.ReadFile(filepath.Join(outputDir, "templates", "gpu-operator-readiness.yaml")) + if err != nil { + t.Fatalf("read readiness template: %v", err) + } + if !strings.Contains(string(readiness), "ApplyOutOfSyncOnly=true") { + t.Errorf("readiness child template must contain \"ApplyOutOfSyncOnly=true\":\n%s", readiness) + } +} + // TestHelmTemplate_RendersWithSetRepoURL is the live-render counterpart to // the golden tests: goldens freeze the pre-render template bytes, this // test verifies that running `helm template` against the generated bundle diff --git a/pkg/bundler/deployer/argocdhelm/testdata/readiness_gate/002-gpu-operator-readiness/templates/readiness.yaml b/pkg/bundler/deployer/argocdhelm/testdata/readiness_gate/002-gpu-operator-readiness/templates/readiness.yaml index 7c229dcbc..d22c7fe34 100644 --- a/pkg/bundler/deployer/argocdhelm/testdata/readiness_gate/002-gpu-operator-readiness/templates/readiness.yaml +++ b/pkg/bundler/deployer/argocdhelm/testdata/readiness_gate/002-gpu-operator-readiness/templates/readiness.yaml @@ -59,7 +59,7 @@ metadata: name: gpu-operator-readiness-gate namespace: gpu-operator annotations: - argocd.argoproj.io/sync-options: Replace=true + argocd.argoproj.io/sync-options: Replace=true,Force=true spec: backoffLimit: 6 template: diff --git a/pkg/bundler/deployer/argocdhelm/testdata/readiness_gate/templates/gpu-operator-readiness.yaml b/pkg/bundler/deployer/argocdhelm/testdata/readiness_gate/templates/gpu-operator-readiness.yaml index 93ccdc5ea..8e11cd768 100644 --- a/pkg/bundler/deployer/argocdhelm/testdata/readiness_gate/templates/gpu-operator-readiness.yaml +++ b/pkg/bundler/deployer/argocdhelm/testdata/readiness_gate/templates/gpu-operator-readiness.yaml @@ -28,3 +28,4 @@ spec: syncOptions: - CreateNamespace=true - ServerSideApply=true + - ApplyOutOfSyncOnly=true diff --git a/pkg/bundler/gatemanifest/manifest.go b/pkg/bundler/gatemanifest/manifest.go index c81a758e1..3f18c830a 100644 --- a/pkg/bundler/gatemanifest/manifest.go +++ b/pkg/bundler/gatemanifest/manifest.go @@ -173,8 +173,30 @@ func jobMetadataAnnotations(deployer config.DeployerType) string { helm.sh/hook: post-install,post-upgrade helm.sh/hook-delete-policy: before-hook-creation` case config.DeployerArgoCD, config.DeployerArgoCDHelm: + // The Job's spec.selector and spec.template.metadata.labels are + // server-generated/immutable, and the rendered manifest correctly + // omits them. Plain Replace=true maps to `kubectl replace`, which the + // API server rejects on any upgrade that changes the Job spec (e.g. + // an image tag bump), leaving the Application permanently + // OutOfSync. Force=true is ArgoCD's documented option to + // delete-and-recreate when a replace fails. This alone would + // delete-and-recreate the Job on EVERY sync, including no-op + // resyncs where nothing changed — see the ApplyOutOfSyncOnly=true + // entry this deployer adds to the readiness folder's + // Application-level spec.syncPolicy.syncOptions + // (pkg/bundler/deployer/argocd/argocd.go's buildApplicationData), + // which excludes already-in-sync resources from a sync operation + // and stops the needless rerun. The two mechanisms are + // complementary: Job-level Replace+Force handles genuine spec + // diffs (e.g. an image tag bump); Application-level + // ApplyOutOfSyncOnly prevents unnecessary reruns when there is no + // diff at all. Deliberately NOT using a Helm-style sync hook + // (helm.sh/hook) here: per + // pkg/bundler/deployer/localformat/hooks.go's stripHelmHooks doc, + // hook-annotated resources are excluded from ArgoCD's normal drift + // detection, so an image-tag-only bump could silently go undetected. return ` annotations: - argocd.argoproj.io/sync-options: Replace=true` + argocd.argoproj.io/sync-options: Replace=true,Force=true` case config.DeployerFlux, config.DeployerHelmfile: return "" default: diff --git a/pkg/bundler/gatemanifest/manifest_test.go b/pkg/bundler/gatemanifest/manifest_test.go index f4eedef3d..5302ca2a2 100644 --- a/pkg/bundler/gatemanifest/manifest_test.go +++ b/pkg/bundler/gatemanifest/manifest_test.go @@ -39,7 +39,7 @@ func TestRender(t *testing.T) { "kind: ServiceAccount", "kind: ClusterRole", "kind: Job", - "argocd.argoproj.io/sync-options: Replace=true", + "argocd.argoproj.io/sync-options: Replace=true,Force=true", "backoffLimit: 6", "customresourcedefinitions", `resources: ["*"]`, @@ -111,6 +111,48 @@ func TestRender_HelmHooks(t *testing.T) { } } +func TestRender_ArgoCDSyncOptions(t *testing.T) { + // Replace=true alone maps to `kubectl replace`, which the API server + // rejects on any upgrade that changes the Job spec because + // spec.selector/spec.template.metadata.labels are immutable, leaving the + // Application permanently OutOfSync (#2367). Force=true makes ArgoCD + // delete-and-recreate on replace failure instead. Both ArgoCD deployer + // branches (native and Helm-rendered) must emit the same annotation. + // This Job-level annotation is only half the fix: see + // TestBuildApplicationData_ApplyOutOfSyncOnly and + // TestGenerate_ApplyOutOfSyncOnlySyncOptions in + // pkg/bundler/deployer/argocd for the Application-level + // ApplyOutOfSyncOnly=true entry that stops Force=true from + // delete-and-recreating the Job on every no-op resync. + tests := []struct { + name string + deployer config.DeployerType + }{ + {"argocd", config.DeployerArgoCD}, + {"argocd-helm", config.DeployerArgoCDHelm}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := Render("gpu-operator", "img:tag", []byte(validReadinessTestYAML), tt.deployer) + if err != nil { + t.Fatalf("Render: %v", err) + } + s := string(got) + const want = "argocd.argoproj.io/sync-options: Replace=true,Force=true" + if !strings.Contains(s, want) { + t.Errorf("manifest for deployer %v missing %q", tt.deployer, want) + } + // Must not use a Helm-style sync hook: hook-annotated resources + // are excluded from ArgoCD's normal drift detection (see + // pkg/bundler/deployer/localformat/hooks.go's stripHelmHooks + // doc), which could let an image-tag-only bump go undetected. + if strings.Contains(s, "helm.sh/hook") { + t.Errorf("ArgoCD deployer %v manifest must not use a Helm sync hook", tt.deployer) + } + }) + } +} + func TestRender_EmptyComponentName(t *testing.T) { if _, err := Render("", "img:tag", []byte("x"), config.DeployerHelm); err == nil { t.Fatal("expected error for empty component name") diff --git a/pkg/bundler/readiness_test.go b/pkg/bundler/readiness_test.go index ed61be036..848efaedb 100644 --- a/pkg/bundler/readiness_test.go +++ b/pkg/bundler/readiness_test.go @@ -143,8 +143,8 @@ func TestCollectComponentReadiness(t *testing.T) { t.Fatal("readiness manifest key missing") } s := string(body) - if !strings.Contains(s, "argocd.argoproj.io/sync-options: Replace=true") { - t.Errorf("missing Replace=true:\n%s", s) + if !strings.Contains(s, "argocd.argoproj.io/sync-options: Replace=true,Force=true") { + t.Errorf("missing Replace=true,Force=true:\n%s", s) } if !strings.Contains(s, "ghcr.io/nvidia/aicr-gate:v0.13.0") { t.Errorf("missing normalized gate image tag:\n%s", s)