feat(api)!: add GitTarget.spec.prune.mode and stop inferring deletions#260
Merged
Conversation
A GitTarget removes a Git document for one of two very different reasons: the
source cluster told us the resource is gone (an explicit DELETE event), or a
resync snapshot did not list it and its absence was INFERRED. The second is only
as trustworthy as the snapshot, and a scope mistake, a source-cluster outage, or
a narrowed RBAC grant all produce a snapshot smaller than reality.
spec.prune.mode controls the two paths separately — never / onEvent / always —
and the effective default is onEvent: observed deletes are still mirrored,
inferred ones are not. `always` restores the previous mark-and-sweep.
The CRD default is not the compatibility mechanism. Kubernetes does not
retro-fill defaults into stored objects, so a GitTarget written before this
field existed reaches the controller with a nil spec.prune forever; every reader
goes through EffectivePruneMode instead, and an existing target becomes safe
without being edited. The field is deliberately mutable, unlike the destination
fields, so a target can be moved to `always` once its scope is confirmed.
The sweep is gated at the PLANNER, not at apply time: a suppressed drop is never
planned, so it cannot reach the plan's action list, its ordering, or the commit.
The explicit delete is gated first in applyDelete, before the document is even
located, so a retention touches no buffer and cannot turn the kustomize oracle
on. A retention must be indistinguishable from the event never arriving.
Two zero values needed deciding, and both fail safe in the direction that
retains:
- manifestanalyzer.SweepMode's zero value RETAINS, so a caller added later
that forgets the field keeps documents rather than deleting them. Every
production caller sets it explicitly anyway.
- PruneMode("") is UNSET, not `never`. Read literally it answers false to both
predicates, which would silently stop mirroring deletes, so every internal
carrier normalizes through OrDefault. An unrecognized value is left alone
and retains on both paths.
Retention is otherwise invisible — no action, no commit, no ResyncStats entry —
so Plan.RetainedOrphans counts it, feeding a throttled log and
gitopsreverser_prune_retained_documents_total. It is the configured outcome, so
nothing here raises a condition or a failure count.
BREAKING CHANGE: a resync no longer deletes Git documents whose resources are
absent from the desired snapshot. Set spec.prune.mode: always on a GitTarget
that needs full desired-state convergence. Deleting a resource in the cluster
still deletes its file. Ships in the same release as the rule-kind scope change,
which it exists to make non-destructive.
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds configurable ChangesGitTarget prune policy
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
PR 5 made a suppressed sweep the default, and a suppressed sweep produces no plan action, no commit, and no ResyncStats entry — deliberately, since a retention must be indistinguishable from the event never arriving. The consequence is that "converged" and "deliberately retaining stale documents" look identical from kubectl, from git log, and from the folder itself. Today the only signal that names the target is a throttled log line: the retention metric is labelled by prune_mode alone, so it cannot answer which GitTarget is keeping anything. This is the plan for closing that, to be implemented in this same PR. It proposes a status.retention roll-up (an observation, never a condition — PR 5's "healthy, not failed" rule still holds), records that the eviction problem is already solved by RenderFidelityGate's epoch mechanism rather than needing a second scope tracker, states the pull-based projection's staleness property instead of hiding it, and separates the cheap metric-label fix from the status work so the two can be sequenced independently. Open questions are listed rather than decided: whether `never` should also report its suppressed explicit deletes, whether zero-vs-absent earns its extra state, whether to enqueue on 0->n given the flake history in this projection class, and whether to ship only the metric fix if closing the release window matters more.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/prune_mode_e2e_test.go`:
- Around line 94-95: Remove the package-wide SetDefaultEventuallyTimeout and
SetDefaultEventuallyPollingInterval calls from the prune-mode spec setup.
Configure the 60-second timeout and 2-second polling interval on the relevant
local Eventually calls, or move the shared defaults to the suite bootstrap so
spec execution order cannot affect them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c50fca73-f0f6-48b5-836e-d616c7d5c5f4
📒 Files selected for processing (39)
api/v1alpha3/gittarget_types.goapi/v1alpha3/prune_policy.goapi/v1alpha3/prune_policy_test.goapi/v1alpha3/zz_generated.deepcopy.gocmd/manifest-analyzer/main.goconfig/crd/bases/configbutler.ai_gittargets.yamldocs/UPGRADING.mddocs/configuration.mddocs/design/watchrule-source-namespace/README.mddocs/design/watchrule-source-namespace/pr5-gittarget-deletion-safety.mddocs/design/watchrule-source-namespace/pr5-retention-visibility.mdinternal/git/acceptance_gate_test.gointernal/git/branch_worker.gointernal/git/branch_worker_split_test.gointernal/git/commit_executor.gointernal/git/fieldpatch_flush_test.gointernal/git/inplace_edit_test.gointernal/git/kustomize_oracle_test.gointernal/git/pending_writes.gointernal/git/placement_test.gointernal/git/plan_flush.gointernal/git/plan_flush_test.gointernal/git/prune_mode_test.gointernal/git/render_fidelity_test.gointernal/git/render_scope_test.gointernal/git/resync_flush.gointernal/git/resync_flush_test.gointernal/git/resync_scope_test.gointernal/git/types.gointernal/git/write_boundary_precondition_test.gointernal/manifestanalyzer/plan.gointernal/manifestanalyzer/plan_test.gointernal/manifestanalyzer/scan.gointernal/manifestanalyzer/scoped_plan_test.gointernal/manifestanalyzer/sweep_policy_test.gointernal/telemetry/exporter.gopkg/manifestanalyzer/folder.gotest/e2e/prune_mode_e2e_test.gotest/e2e/templates/manager/gittarget-prune.tmpl
…kept Two policy-TRANSITION defects found by review of #260, plus the retention visibility half of PR 5. The shipped implementation applied spec.prune.mode correctly wherever it was read; both defects were about what happens when the value CHANGES, in opposite directions. R1 — widening to `always` did not converge a quiet target. The watch set is keyed by what is watched, not by what may be deleted, so a prune edit replaced nothing, and a reconnect resumes from its cursor rather than replaying. The only production path that enqueues a resync is a fresh replay, so a target could sit under `always` indefinitely without sweeping. DeclareForGitTarget now takes the effective mode and forces a replay on the edge INTO a sweeping mode: edge- not level-triggered, and deliberately one-directional. R2 — tightening away from `always` could be outrun by a write already committed locally. A rebase replay re-plans against the rebased worktree under the policy captured at planning time, so it can decide deletions the first apply never made. tightenPendingPruneModes lowers each retained write to the more restrictive of (captured, current) before the replay. Loosening still does not escalate a planned write; an unreadable policy returns an error so the push retries rather than guessing; a deleted GitTarget replays under `never`. R3 — the retention log and metric now name the GitTarget, which both docs already promised. The throttle key was the same defect: a path is not an identity. R4 — the docs claimed a failure mode the code does not have. The gather is fail-closed on a failed list/watch and on a replay cut short before its initial-events bookmark, so an outage stops a sweep rather than shrinking one. The real risk is a snapshot that is complete but scoped wrong. status.retention reports the effective mode and a bounded retained count, epoch-keyed off the render-fidelity epoch so a scope leaving the watch plan takes its count with it. Absent means nothing has reported; zero means a resync found nothing to retain. Gates: lint 0 issues, unit coverage 77.8% (at baseline), e2e 75 passed / 22 skipped / 0 failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`GitTarget.spec.prune.mode` accepted `never` / `onEvent` / `always`. Kubernetes API conventions call for UpperCamelCase enum values — `imagePullPolicy: Always|Never|IfNotPresent`, `persistentVolumeReclaimPolicy: Retain|Delete` — so the values are now `Never` / `OnEvent` / `Always`. Taken now because now is the last moment it is free. The field is new and unreleased, and this branch already carries two breaking API commits, so the rename rides along in a release users must read UPGRADING.md for anyway. After that release it would be a second migration bought for a cosmetic gain, which is a price nobody would pay — leaving the non-conventional spelling permanent. The typed constants (PruneNever, PruneOnEvent, PruneAlways) keep their names, so no Go call site moves. What changes is the wire value: the CRD enum, the CRD default, and every doc, sample and message that quotes one. Found as F12 in docs/future/flux-maintainer-review-status-and-config-model.md, which is updated to record this item as the one taken pre-release. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/watch/retention_rollup_test.go (1)
21-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename tests to follow the
TestFunctionName_Scenarioconvention.As per coding guidelines, tests must follow the naming convention
TestFunctionName_Scenario(t *testing.T). These tests currently use conceptual component names rather than the exact function names being tested.
internal/watch/retention_rollup_test.go#L21-L23: Rename tests to use the function name, e.g.,TestRetentionForGitTarget_SumsEveryScopeorTestMarkTargetRetention_SumsEveryScope.internal/watch/prune_declaration_test.go#L91-L93: Rename the test to use the function name, e.g.,TestRememberGitTargetPruneMode_IsPerGitTarget.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/watch/retention_rollup_test.go` around lines 21 - 23, Rename the test in internal/watch/retention_rollup_test.go:21-23 to use the exact function under test with the _SumsEveryScope scenario suffix, choosing the appropriate symbol such as RetentionForGitTarget or MarkTargetRetention. Also rename the test in internal/watch/prune_declaration_test.go:91-93 to follow the exact function name RememberGitTargetPruneMode with the _IsPerGitTarget suffix.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/watchrule-source-namespace/pr5-gittarget-deletion-safety.md`:
- Around line 126-130: Update the retention-observability paragraph to replace
the ambiguous “no stat” wording with the precise absence of a managed-drop
action, commit, or ResyncStats entry. Preserve that suppressed retention remains
observable through the throttled log line, retention metrics, and
status.retention, consistent with Plan.RetainedOrphans and the linked retention
visibility page.
In `@docs/design/watchrule-source-namespace/pr5-retention-visibility.md`:
- Around line 34-53: Mark the outdated retention-signal discussion as explicitly
pre-Step-0 historical context, or revise it to reflect the implemented target
identity in both the throttled log and metric. Update the “today” claim and
surrounding statements about missing GitTarget identity while preserving the
table’s purpose as historical rationale.
In `@internal/watch/retention_rollup.go`:
- Around line 79-91: Update the comment above priorTotal and priorMode to
acknowledge that resetting scopes on a newer epoch temporarily produces partial
totals, may mark each incoming scope as changed, and can trigger extra
reconciles for multi-scope targets; do not imply that the existing logic fully
debounces same-total epoch replacements.
---
Nitpick comments:
In `@internal/watch/retention_rollup_test.go`:
- Around line 21-23: Rename the test in
internal/watch/retention_rollup_test.go:21-23 to use the exact function under
test with the _SumsEveryScope scenario suffix, choosing the appropriate symbol
such as RetentionForGitTarget or MarkTargetRetention. Also rename the test in
internal/watch/prune_declaration_test.go:91-93 to follow the exact function name
RememberGitTargetPruneMode with the _IsPerGitTarget suffix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ac45046-b078-47d1-bcc3-da13ce89535f
📒 Files selected for processing (34)
api/v1alpha3/gittarget_types.goapi/v1alpha3/prune_policy.goapi/v1alpha3/zz_generated.deepcopy.goconfig/crd/bases/configbutler.ai_gittargets.yamldocs/UPGRADING.mddocs/configuration.mddocs/design/watchrule-source-namespace/README.mddocs/design/watchrule-source-namespace/pr5-gittarget-deletion-safety.mddocs/design/watchrule-source-namespace/pr5-retention-visibility.mddocs/design/watchrule-source-namespace/pr5-review-followups.mddocs/future/flux-maintainer-review-status-and-config-model.mdinternal/controller/gittarget_controller.gointernal/controller/gittarget_source_cluster_test.gointernal/controller/gittarget_status_test.gointernal/git/branch_worker.gointernal/git/placement_test.gointernal/git/prune_mode_test.gointernal/git/prune_replay_test.gointernal/git/render_fidelity_test.gointernal/git/resync_flush.gointernal/git/resync_flush_test.gointernal/git/resync_scope_test.gointernal/git/retention_report_test.gointernal/git/types.gointernal/telemetry/exporter.gointernal/watch/event_router.gointernal/watch/manager.gointernal/watch/materialization.gointernal/watch/prune_declaration.gointernal/watch/prune_declaration_test.gointernal/watch/retention_rollup.gointernal/watch/retention_rollup_test.gointernal/watch/target_watch.gotest/e2e/prune_mode_e2e_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- docs/UPGRADING.md
- docs/configuration.md
- internal/telemetry/exporter.go
- internal/git/resync_scope_test.go
- docs/design/watchrule-source-namespace/README.md
- internal/git/placement_test.go
- internal/git/resync_flush_test.go
- internal/git/resync_flush.go
- internal/git/prune_mode_test.go
- internal/git/render_fidelity_test.go
Follow-up to the enum rename: three test comments and two Ginkgo spec descriptions still named the modes in the old lowercase. No behavior change — these are prose, not values — but a comment that quotes a wire value nobody can apply is worse than no comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 5 of source-namespace addressing, and the
change that makes PR #259's breaking scope rework non-destructive. It closes the do-not-release
window #259 opened.
The problem
A GitTarget removes a Git document for one of two very different reasons:
The second is only as trustworthy as the snapshot, and a scope mistake, a source-cluster outage, or
a narrowed RBAC grant all produce a snapshot smaller than reality. Until now both paths were always
on, so a bad watch scope deleted a tenant's manifests.
The change
GitTarget.spec.prune.modecontrols the two paths separately:neveronEvent— effective defaultalways— previous behaviourDeleting a resource in the cluster still deletes its file. Only the inference changes.
The CRD default is deliberately not the compatibility mechanism: Kubernetes does not retro-fill
defaults into stored objects, so a GitTarget written before this field existed reaches the
controller with a nil
spec.pruneforever. Every reader goes throughEffectivePruneMode()instead, so an existing target becomes safe without being edited. The field is mutable, unlike
the destination fields, so a target can move to
alwaysonce its scope is confirmed rather thanbeing deleted and recreated.
Where the gates sit, and why
cannot reach the plan's action list, its ordering, or the commit path.
applyDelete, before the document is even located, so a retentiontouches no buffer, records no write intent, and cannot turn the kustomize oracle on. A retention
has to be indistinguishable from the event never arriving.
Two zero values, both failing safe toward retaining
manifestanalyzer.SweepMode's zero value retains, so a planner caller added later that forgetsthe field keeps documents rather than deleting them. Every production caller sets it explicitly
regardless.
PruneMode("")is unset, notnever. Read literally it answers false to both predicates,which is
never's behaviour and would silently stop mirroring deletes — so every internal carriernormalizes through
OrDefault(). An unrecognized value (a downgrade from a newer build) is leftalone and retains on both paths.
Surfacing retention
A suppressed drop leaves no other trace — no action, no commit, no
ResyncStatsentry — soPlan.RetainedOrphanscounts it, feeding a throttled log (one per target folder per 10 minutes,since retention is a steady state while resyncs fire per type and namespace) and
gitopsreverser_prune_retained_documents_total. Retention is the configured outcome, so nothinghere raises a condition or a failure count.
Worth a second opinion before merge
Under
onEventthe operator no longer removes a stray managed document a human added to the folderby hand — the drift-heal resync runs through the same planner. This follows from the design rather
than extending it, but "the reverser stopped cleaning up junk I put in the folder" is the same
mechanism as "the reverser did not delete my tenant's manifests when the scope collapsed". It is
recorded in the design doc's Implementation notes and in UPGRADING.md;
alwaysrestores both.Tests
Unit coverage spans the API contract (legacy default, the documented two-path table, unset-is-not-
never, unrecognized-value-retains), the planner (a retained drop is absent from the whole plan,scoped and unscoped alike, and retention is distinguished from an empty scope), and the writer
(both paths under all three modes, plus the unresolvable-target fallback and the log throttle).
Three e2e specs run three GitTargets in one repo, one per policy. Every retention assertion is
paired with a barrier, because "the file is still there" also passes when the pipeline is
asleep:
pruneblock stays absent (so the legacy case is genuinelyexercised rather than silently defaulted),
prune: {}defaults toonEvent, and a value outsidethe enum is rejected.
then is the
nevertarget's surviving copy asserted.force a scoped replay resync. The
alwaystarget's sweep proves a resync ran with the orphan inscope; only then is the default target's retained copy asserted, followed by a fresh ConfigMap to
prove retention is not a stalled pipeline.
Validation
task lint,task test, andtask test-e2eall pass. e2e: 65 passed, 0 failed, with all threenew specs confirmed
passedin the Ginkgo report. Unit coverage is within the ratchet tolerance ofthe committed baseline.
BREAKING CHANGE: a resync no longer deletes Git documents whose resources are absent from the
desired snapshot. Set
spec.prune.mode: alwayson a GitTarget that needs full desired-stateconvergence.
🤖 Generated with Claude Code
Summary by CodeRabbit
GitTarget.spec.prune.modewithnever,onEvent(effective default), andalways.GitTarget.status.retention, plus throttled logs and a retention metric.