diff --git a/hack/backupstrategy-crd-gate.bats b/hack/backupstrategy-crd-gate.bats index 0f68a1ca4..fac48f35e 100644 --- a/hack/backupstrategy-crd-gate.bats +++ b/hack/backupstrategy-crd-gate.bats @@ -137,9 +137,9 @@ $tpl: $obj does not gate on crdEstablished for $crd" # The controller's DefaultObjectsGate tells an object rendered with a stale S3 # endpoint (publishing.host changed after the render) by the # backups.cozystack.io/endpoint annotation, because the endpoint itself sits at -# a different path in every kind. An object the chart renders without it is -# forced once and then never compared again, so a new template that forgets it -# would silently keep a stale endpoint. +# a different path in every kind. An object the chart renders without it +# counts as stale on every check, so a new template that forgets it would +# force this release every MinForceInterval forever. @test "every rendered Strategy CR and the Velero BSL carry the rendered endpoint annotation" { tmp=$(mktemp -d) diff --git a/internal/backupcontroller/default_objects_gate.go b/internal/backupcontroller/default_objects_gate.go index 9b6fc0ede..9aea96f31 100644 --- a/internal/backupcontroller/default_objects_gate.go +++ b/internal/backupcontroller/default_objects_gate.go @@ -194,12 +194,6 @@ type DefaultObjectsGate struct { // MinForceInterval for no reason. lastForce time.Time lastCredentialsForce time.Time - // renderForced records that a forced render of HelmRelease was requested - // (the annotations were patched; whether the upgrade succeeds is not - // observed). That render stamps endpointAnnotation on every object the - // chart renders, so from then on an object without it (one rendered by a - // chart that predates the annotation) is not counted as stale again. - renderForced bool } // errHelmReleaseSuspended reports that the target release has @@ -423,10 +417,7 @@ func (g *DefaultObjectsGate) Check(ctx context.Context) ([]string, bool, error) sort.Strings(objects) _, objectsForced, err := g.force(ctx, g.HelmRelease, &g.lastForce, objects) needed = append(needed, objects...) - if objectsForced { - forced = true - g.renderForced = true - } + forced = forced || objectsForced errs = append(errs, err) } return needed, forced, errors.Join(errs...) @@ -563,7 +554,9 @@ type defaultObjectsState struct { } // staleEndpoint reports an existing object rendered with another endpoint -// than want, formatted for logs. An empty want disables the comparison. +// than want, formatted for logs. An empty want disables the comparison. An +// object without the annotation (rendered by a chart that predates it) stays +// stale until a render stamps it: requesting one does not prove it landed. func (g *DefaultObjectsGate) staleEndpoint(state *defaultObjectsState, key string, obj *unstructured.Unstructured, want string) { if want == "" { return @@ -571,9 +564,7 @@ func (g *DefaultObjectsGate) staleEndpoint(state *defaultObjectsState, key strin have, ok := obj.GetAnnotations()[endpointAnnotation] switch { case !ok: - if !g.renderForced { - state.stale = append(state.stale, key+" (no endpoint annotation)") - } + state.stale = append(state.stale, key+" (no endpoint annotation)") case have != want: state.stale = append(state.stale, fmt.Sprintf("%s (endpoint %q, want %q)", key, have, want)) } diff --git a/internal/backupcontroller/default_objects_gate_test.go b/internal/backupcontroller/default_objects_gate_test.go index bed60053f..e287c9872 100644 --- a/internal/backupcontroller/default_objects_gate_test.go +++ b/internal/backupcontroller/default_objects_gate_test.go @@ -1037,13 +1037,13 @@ func TestCheckIgnoresObjectEndpointWithoutSecretEndpoint(t *testing.T) { } } -// TestCheckForcesOnceForUnannotatedObjects covers objects rendered by a chart -// that predates the endpoint annotation. Their endpoint is unknown, so they -// are treated as stale until one forced render has gone through, which stamps -// the annotation. A force that did not go through (throttled) does not count, -// and once one has, an object still without the annotation (one the chart -// does not render) must not force again every MinForceInterval. -func TestCheckForcesOnceForUnannotatedObjects(t *testing.T) { +// TestCheckKeepsForcingForUnannotatedObjects covers objects rendered by a +// chart that predates the endpoint annotation. Their endpoint is unknown, so +// they count as stale until a render stamps the annotation. Having requested +// a render proves nothing: one that failed leaves them unannotated, and the +// next unthrottled check must force again rather than fall silent, so the +// force counter keeps reporting the render that does not land. +func TestCheckKeepsForcingForUnannotatedObjects(t *testing.T) { base := time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC) objs := []runtime.Object{helmReleaseObject(), credentialsHelmReleaseObject()} for _, o := range allDefaultObjects() { @@ -1065,27 +1065,11 @@ func TestCheckForcesOnceForUnannotatedObjects(t *testing.T) { g.now = func() time.Time { return base.Add(time.Hour) } missing, forced, err := g.Check(context.Background()) - if err != nil || forced || len(missing) != 0 { - t.Fatalf("after the forced render: missing = %v, forced = %v, err = %v; want a no-op", missing, forced, err) + if err != nil || !forced || len(missing) != 7 { + t.Fatalf("after a render that left them unannotated: missing = %v, forced = %v, err = %v; want 7 reported and forced again", missing, forced, err) } - if again := forceAnnotations(t, dyn)["reconcile.fluxcd.io/forceAt"]; again != first { - t.Errorf("forced again for unannotated objects: %q, then %q", first, again) - } -} - -// TestCheckStillForcesStaleAfterUnannotatedForce pins that the once-only -// treatment covers the missing annotation alone: an annotation that names the -// wrong endpoint still forces after that. -func TestCheckStillForcesStaleAfterUnannotatedForce(t *testing.T) { - objs := []runtime.Object{helmReleaseObject(), credentialsHelmReleaseObject()} - for _, o := range allDefaultObjects() { - objs = append(objs, withEndpoint(o.(*unstructured.Unstructured), "https://s3.old.example.com")) - } - g, _ := newGate(t, []client.Object{sourceSecret("bucket-1a2b"), cozyDefaultBackupClass()}, objs...) - g.renderForced = true - - if _, forced, err := g.Check(context.Background()); err != nil || !forced { - t.Fatalf("forced = %v, err = %v; want forced", forced, err) + if again := forceAnnotations(t, dyn)["reconcile.fluxcd.io/forceAt"]; again == first { + t.Errorf("not forced again for unannotated objects: forceAt still %q", first) } }