diff --git a/.coverage-baseline b/.coverage-baseline index 9274442d4..7a60a9108 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -78.9 +79.0 diff --git a/api/v1alpha3/gitprovider_types.go b/api/v1alpha3/gitprovider_types.go index 830b7c905..b421b9356 100644 --- a/api/v1alpha3/gitprovider_types.go +++ b/api/v1alpha3/gitprovider_types.go @@ -141,6 +141,20 @@ type GitProviderStatus struct { // +optional SigningPublicKey string `json:"signingPublicKey,omitempty"` + // Branches lists the branches this repository's GitTargets are configured to write, with how + // many reference each. It answers what the repository is being used for, so it includes a + // branch whose targets are blocked or suspended and excludes a branch nothing references any + // more. + // + // It is not the branches that exist on the remote, and not the workers currently running: it + // moves only when somebody edits a GitTarget. An empty list beside Ready=True is the state + // this exists to make legible — configured and unused, which looks nothing like broken and + // previously took a GitTarget listing to tell apart. + // +optional + // +listType=map + // +listMapKey=name + Branches []GitProviderBranchStatus `json:"branches,omitempty"` + // LastVerifiedAt is when the credential and the repository were last proved together: the // connectivity check listed the remote's refs with this provider's credential and the remote // answered. It is never cleared — a failure is already carried by Ready=False with a reason — @@ -153,6 +167,22 @@ type GitProviderStatus struct { LastVerifiedAt *metav1.Time `json:"lastVerifiedAt,omitempty"` } +// GitProviderBranchStatus is one branch of this repository that GitTargets are configured to +// write, and how many of them write it. +type GitProviderBranchStatus struct { + // Name is the branch. + // +required + // +kubebuilder:validation:MinLength=1 + Name string `json:"name"` + + // GitTargets is how many GitTargets are configured to write this branch. More than one means + // they share a branch worker, and each still reports its own folder's health on its own + // conditions. + // +required + // +kubebuilder:validation:Minimum=0 + GitTargets int32 `json:"gitTargets"` +} + // CommitSpec configures the commit identity and signing a GitProvider uses. Message formatting // lives on the GitTarget (spec.commit.message), because it describes the folder rather than the // connection. diff --git a/api/v1alpha3/zz_generated.deepcopy.go b/api/v1alpha3/zz_generated.deepcopy.go index db2c35d9e..6e08abeac 100644 --- a/api/v1alpha3/zz_generated.deepcopy.go +++ b/api/v1alpha3/zz_generated.deepcopy.go @@ -549,6 +549,21 @@ func (in *GitProvider) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *GitProviderBranchStatus) DeepCopyInto(out *GitProviderBranchStatus) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GitProviderBranchStatus. +func (in *GitProviderBranchStatus) DeepCopy() *GitProviderBranchStatus { + if in == nil { + return nil + } + out := new(GitProviderBranchStatus) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *GitProviderList) DeepCopyInto(out *GitProviderList) { *out = *in @@ -626,6 +641,11 @@ func (in *GitProviderStatus) DeepCopyInto(out *GitProviderStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.Branches != nil { + in, out := &in.Branches, &out.Branches + *out = make([]GitProviderBranchStatus, len(*in)) + copy(*out, *in) + } if in.LastVerifiedAt != nil { in, out := &in.LastVerifiedAt, &out.LastVerifiedAt *out = (*in).DeepCopy() diff --git a/cmd/main.go b/cmd/main.go index bba74bfdd..39b86a51c 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -183,7 +183,6 @@ func main() { // healthy; the resync path already reports its own refusals through the router. workerManager.SetPathRefusalReporter(watchMgr.ReportGitPathRefusal) workerManager.SetLayoutReporter(watchMgr.ReportLayoutResolved) - workerManager.SetRemoteReporter(watchMgr.ReportRemoteObserved) // The read-only twin of the refusal reporter: the refresher re-reads an idle target's folder, // and a folder somebody else broke in Git produces no Kubernetes event to notice it by. workerManager.SetScanAcceptanceReporter(watchMgr.ReportGitPathScan) diff --git a/config/crd/bases/configbutler.ai_gitproviders.yaml b/config/crd/bases/configbutler.ai_gitproviders.yaml index b9d2783a1..6e16622b1 100644 --- a/config/crd/bases/configbutler.ai_gitproviders.yaml +++ b/config/crd/bases/configbutler.ai_gitproviders.yaml @@ -174,6 +174,42 @@ spec: status: description: status defines the observed state of GitProvider properties: + branches: + description: |- + Branches lists the branches this repository's GitTargets are configured to write, with how + many reference each. It answers what the repository is being used for, so it includes a + branch whose targets are blocked or suspended and excludes a branch nothing references any + more. + + It is not the branches that exist on the remote, and not the workers currently running: it + moves only when somebody edits a GitTarget. An empty list beside Ready=True is the state + this exists to make legible — configured and unused, which looks nothing like broken and + previously took a GitTarget listing to tell apart. + items: + description: |- + GitProviderBranchStatus is one branch of this repository that GitTargets are configured to + write, and how many of them write it. + properties: + gitTargets: + description: |- + GitTargets is how many GitTargets are configured to write this branch. More than one means + they share a branch worker, and each still reports its own folder's health on its own + conditions. + format: int32 + minimum: 0 + type: integer + name: + description: Name is the branch. + minLength: 1 + type: string + required: + - gitTargets + - name + type: object + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map conditions: description: |- Conditions report repository validation and commit configuration readiness: diff --git a/docs/INDEX.md b/docs/INDEX.md index 1084fe54c..f8d92e935 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -98,6 +98,14 @@ lives in the window where nothing has been published. The one part that stood on shipped — the e2e rule barrier now requires `observedGeneration == generation` before it accepts `StreamsRunning=True`. The page carries the trigger conditions for picking the rest up. +[`repository-status-surface.md`](design/repository-status-surface.md) fixes which object reports +what about a mirror, and at what rate, with worked examples for two `GitTarget`s sharing one +`GitProvider`, a `GitProvider` nothing mirrors to yet, and one whose credential cannot write. Its +load-bearing distinction is delivery against publication: an observation reaches every target on a +branch at once and in memory, while the status write it produces is sampled. It also settles how a +wrong destination is found out without cloning it, and names the one kind of wrong that nothing can +report at all. + **The live workstream** is [`design/support-boundary/`](design/support-boundary/) — editing existing GitOps folders through the Kubernetes API. Start at [`support-contract.md`](design/support-boundary/support-contract.md) — **the single page that diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 616a4b5e6..14e8980db 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,43 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## `GitTarget.status.remote` is sampled, and a `GitProvider` lists its branches + +**Not breaking for any field, and a change of promise for one.** `status.remote.revision` is a +**sampled** value: a target publishes what it holds when it reconciles, and writes the stanza at +most once a minute, so a push followed immediately by a `kubectl get` can still show the previous +revision. It used to be written on every change, which +meant ten folders sharing a branch turned one commit into ten status writes — each an etcd write +that invalidates the cached copy every watcher of the type holds. + +Nothing about what is *proved* changed. An observation still reaches every `GitTarget` on the +branch the instant a push or a fetch proves it, and the tuple stays internally consistent: one +observation from one moment, never the newest revision beside a stale clock. Two things are still +immediate — the first observation, and taking back a revision that names a repository the target no +longer points at. + +### What to change + +**Automation that reads `status.remote.revision` to confirm its own push has landed needs a +timeout of minutes, not seconds**, or should read Git instead: that field answers "where is my branch", +not "did my write go through". A `CommitRequest`'s own status is what answers the second question. + +### What you gain + +`GitProvider.status.branches` lists the branches the namespace's `GitTarget`s are configured to +write, and how many reference each: + +```yaml +status: + branches: + - name: main + gitTargets: 2 +``` + +`Ready=True` beside `branches: []` now reads as **configured and unused** rather than looking the +same as a repository nothing can reach. It follows the configuration, not the remote: a suspended +or blocked target still counts, and a branch that only exists on the remote never appears. + ## Every duration in the API is a Go duration string **Breaking for `CommitRequest.spec.closeDelaySeconds`, which no longer exists.** Every time-valued @@ -116,7 +153,8 @@ against your Git host and an idle target generates no Git traffic. ### What you gain `GitTarget.status.remote` publishes where the branch is, when that was last proved, and whether a -push or a fetch proved it — renewed by every push, so it is current on an active target at no cost. +push or a fetch proved it — proved again by every push, at no cost, and published on the sampled +cadence described above. `GitProvider.status.lastVerifiedAt` does the same for the credential. Both have a `Verified` printer column under `kubectl get -o wide`. diff --git a/docs/api-first-publication.md b/docs/api-first-publication.md index c041a3558..0896e116d 100644 --- a/docs/api-first-publication.md +++ b/docs/api-first-publication.md @@ -263,10 +263,12 @@ commits behind the retained writes are already unreachable at that point, and a the success path would miss exactly that case. The cost of setting it too eagerly is one fetch on the next cycle. -A fourth piece of state is not a flag: the trust above is bound to the repository it was gained -against. A `GitProvider` is repointed by deleting and recreating it, which does not restart the -worker, so trust from the previous repository must not be carried into the new one. Carrying it -forward would skip establishing the new checkout entirely. +A fourth piece of state is not a flag: the trust above, like everything else the worker holds, is +about one repository, and the worker carries that repository's identity. A `GitProvider` is +repointed by deleting and recreating it, which does not restart the worker — so the worker goes on +using its own repository's URL, and the reconcile replaces it with one built for the new +repository. Trust from the previous repository is never carried into the new one, because the two +are never held by the same worker. ## Story 3: save now, and know what reached Git diff --git a/docs/architecture.md b/docs/architecture.md index e6256d1de..509777190 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -76,12 +76,14 @@ when adding code here:** anything that resolves, commits, or concludes without r advertisement must either fetch, or refuse to conclude. See [inbound push notification](design/push-notification-and-reconcile-trigger.md) §3. -That trust belongs to **one repository**. A worker is keyed by -`(GitProvider namespace, GitProvider name, branch)`, while `spec.url` is immutable -and repointed by deleting and recreating the `GitProvider`, so the same worker can meet a new -repository without anything restarting it. Each remote has its own clone, so trust carried across -that change would skip establishing the new checkout entirely; `noteRemoteIdentity` drops it -instead. +That trust belongs to **one repository**, and so does the worker holding it. A worker is keyed by +`(GitProvider namespace, GitProvider name, branch)`, which names no repository, while `spec.url` is +immutable and repointed by deleting and recreating the `GitProvider`. So the worker also carries +the **repository identity** it was created for: the provider's UID paired with its URL. Every +repository operation uses that URL rather than whatever the provider names when the cycle reads it, +and the reconcile compares the identity on each tick. A provider naming a different repository does +not move the worker; it **replaces** it, and the replaced worker takes its clone, its trust and its +queued work with it. Nothing has to remember to invalidate a field. **An idle target re-proves where its branch is, periodically.** A target that is writing learns it for free: the push session reads the remote's advertisement and the server names the hash it @@ -1134,6 +1136,56 @@ reconciled only after the replay completes. ## Git write architecture +### Who owns which question + +Three objects meet at a write, and each answers a different question. Keeping them apart is what +stops a fact being published by something that cannot actually prove it. + +| Question | Proved by | Published on | +| --- | --- | --- | +| Can this repository be reached with this credential? | the `GitProvider` controller's connectivity check | `GitProvider.status`: `Ready`, `lastVerifiedAt` | +| Which repository is this? | the `GitProvider` object's identity (`metadata.uid` with `spec.url`) | nothing: it is carried on the branch worker and compared, not reported | +| Where is branch B on the remote, and when was that proved? | the branch worker, on its queue | `GitTarget.status.remote`, sampled by each `GitTarget` on the branch | +| What is this repository being used for? | the `GitTarget`s that reference it | `GitProvider.status.branches` | +| Is this folder accepted, streaming and matching live? | the watch plane, per `GitTarget` | the `GitTarget`'s conditions and `status.placement` | + +A branch worker sits at the intersection and belongs wholly to neither object: `GitTarget`s decide +which branches are needed, `GitProvider` decides which repository those branches are in. So the +worker is **keyed** by `(provider namespace, provider, branch)`, all of which a `GitTarget` carries, +and **identified** by the repository it is about, which only the `GitProvider` carries. A provider +that names a different repository does not move a live worker; it replaces it, and the replacement +starts with no clone, no base trust and no observation. + +`GitTarget.status.remote` is a **branch** fact published per target, and it is worth being exact +about that, because the reverse reading is tempting and wrong. The revision and its `lastVerifiedAt` +are the branch worker's single observation, shared by every `GitTarget` on that branch: a push by +one target proves it for all of them. So a fresh `lastVerifiedAt` says the branch tip was recently +proved. It does **not** say that this target's folder was scanned, that its documents were +published, or that its streams are running. Those are the conditions' job. + +**Delivery is not publication**, and the split is deliberate. Delivery is immediate and costs +nothing: the observation is recorded once, where it is proved, and every target on the branch reads +the same tuple from that moment. Publication is a status write (an etcd write that invalidates +every watcher's cached copy of the type), so it is sampled: each target publishes what it holds +when it reconciles, writes the stanza at most once a minute however often that is, and nothing in +the data plane wakes a target because a branch moved. + +So two targets on one branch can show different times. That is publication lag, not a difference in +what was proved, and each tuple is internally consistent: one observation from one moment, never +the newest revision beside a stale clock. + +### Everything about a branch happens on that branch's queue + +Reading or writing a branch's repository state is the branch worker's job, and the worker does it on +one FIFO queue, on one goroutine. Live events, resyncs, `CommitRequest` attaches and periodic +refreshes all enter the same queue and are served in order. Nothing else opens a connection on a +branch's behalf, and nothing else touches its clone. That is what makes "the base is trusted" a +statement one goroutine can hold. + +The `GitProvider` controller's connectivity check is the one thing outside that rule, and it is +outside it deliberately: it answers a question about the repository that has to be answerable when +no branch worker exists at all, including before any `GitTarget` has been created. + ### BranchWorker - **Source**: [internal/git/branch_worker.go](../internal/git/branch_worker.go) @@ -1157,13 +1209,17 @@ bursts. Heal resyncs that arrive during a window are deferred and drained at the ### Local clones and conflict retry Local clones live under -`/tmp/gitops-reverser-workers/{provider namespace}/{provider}/{branch}/repos/{url-digest}`. The leading -segments are exactly the `(provider namespace, provider, branch)` tuple that identifies the -[BranchWorker](../internal/git/branch_worker.go), and the final `{url-digest}` segment is a short digest -of `GitProvider.spec.url` (a truncated SHA-256 of the remote URL, -[`repoCacheKey`](../internal/git/branch_worker.go)), **not** a commit hash. It only disambiguates distinct -remote URLs under the same branch and is stable across commits, so the worker reuses one clone for the life -of the branch. [PushAtomic](../internal/git/git_atomic_push.go) checks the remote ref before pushing. If the +`/tmp/gitops-reverser-workers/{provider namespace}/{provider}/{branch-component}/repos/{url-digest}`. The +leading segments are exactly the `(provider namespace, provider, branch)` tuple that identifies the +[BranchWorker](../internal/git/branch_worker.go). `{branch-component}` is the branch rendered as **one** +path component ([`branchPathComponent`](../internal/git/branch_worker.go)): a readable prefix for whoever +is looking at the directory, plus a digest of the raw name. Branch names contain slashes, and joining one +in would make `release/v1`'s directory a child of `release`'s, which is data loss the moment a retired +worker deletes its own tree. The final `{url-digest}` segment is a short digest of `GitProvider.spec.url` +(a truncated SHA-256 of the remote URL, [`repoCacheKey`](../internal/git/branch_worker.go)), **not** a +commit hash. A worker is about exactly one repository (see below), so that segment no longer separates +two destinations inside one worker: it is stable for the worker's whole life, and the worker reuses one +clone throughout. [PushAtomic](../internal/git/git_atomic_push.go) checks the remote ref before pushing. If the remote diverged it smart fetches the latest tip, hard resets the local clone, replays the retained pending writes against the fresh tip (refreshing commit hashes), and retries up to the attempt limit. This is valid because every pending write is rebuilt from sanitized API state; nothing depends on locally edited files. diff --git a/docs/bi-directional.md b/docs/bi-directional.md index 552331947..2d863e72b 100644 --- a/docs/bi-directional.md +++ b/docs/bi-directional.md @@ -309,8 +309,9 @@ not exist yet, so what a moved branch costs depends on what the target is doing: folder in Git and the condition clears within about ten seconds. - **Healthy and idle:** it finds out on its own, within `--git-refresh-interval` (10m by default) plus the 5-minute reconcile tick that carries the request. The target spends one ref - advertisement, fetches only if the branch actually moved, and republishes `status.remote` and - `status.placement`. If that push left content the writer cannot handle, the same read sets + advertisement, fetches only if the branch actually moved, and republishes `status.placement`; + `status.remote` follows on a later tick, since it is sampled rather than pushed. If that push + left content the writer cannot handle, the same read sets `GitPathAccepted=False` — which moves the target into the refused row above, where it re-checks every ten seconds until somebody fixes the folder. The refresh writes no content of its own. diff --git a/docs/configuration.md b/docs/configuration.md index 21af1b866..dc0fc8bbd 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -173,6 +173,33 @@ spec: - main ``` +### What this repository is being used for (`status.branches`) + +`status.branches` lists the branches the `GitTarget`s in this namespace are configured to write, +with how many reference each: + +```yaml +status: + branches: + - name: main + gitTargets: 2 + - name: release + gitTargets: 1 +``` + +It is a statement about **configuration**, not about the remote and not about what is running: a +suspended or blocked `GitTarget` still says what the repository is for, and a branch that exists on +the remote but nothing writes never appears. It is re-derived on the provider's own reconcile tick, +so a `GitTarget` edit reaches it within one steady interval. + +`Ready=True` beside `branches: []` is the state it exists to make readable: **configured and +unused**, which looks nothing like broken, and which otherwise takes a `GitTarget` listing to tell +apart. + +More than one `gitTargets` on a branch means those folders share one branch worker, one clone and +one queue. They still report their own health on their own conditions: one folder being refused +says nothing about its siblings. + ### `GitProvider.spec.secretRef`: the credentials Secret The referenced Secret holds the Git credentials. The examples use the **Kubernetes-native** keys, @@ -832,9 +859,23 @@ status: Two things renew it. A **push** does, for free: the push session reads the remote's advertisement and the server names the hash it accepted, on the connection the push was making anyway. So a -target that is publishing never needs anything else, and its revision moves with each push rather -than lagging an interval behind. An **idle** target is asked to re-prove it on its reconcile tick; -see [`--git-refresh-interval`](#keeping-an-idle-target-fresh---git-refresh-interval) below. +target that is publishing never needs anything else. An **idle** target is asked to re-prove it on +its reconcile tick; see +[`--git-refresh-interval`](#keeping-an-idle-target-fresh---git-refresh-interval) below. + +**What is proved and what is published are two different rates.** An observation of a branch is +one fact, and it reaches every `GitTarget` on that branch the moment it is proved, in memory and +for nothing. Writing it to status is not free: ten folders sharing a branch would turn one commit +into ten status writes, each invalidating every watcher's cached copy of the type. So each target +publishes what it holds **when it reconciles** (no commit wakes anybody), and writes this stanza +at most once a minute however often it reconciles. Push and read status immediately, and you may +see the previous revision until that target's next tick. + +Two corrections are never held back, because they are not throughput: the **first** observation, +and **taking back** a revision that names a repository the target no longer points at (a recreated +`GitProvider` with a different URL). And none of this promises anything about the branch itself: an +unreachable remote yields no observation at all, which is exactly what the pair `revision` + +`lastVerifiedAt` is there to say. `GitProvider.status.lastVerifiedAt` is the same word for the connection rather than the branch: when the credential and the repository were last proved together. It is never cleared, so diff --git a/docs/design/repository-status-surface.md b/docs/design/repository-status-surface.md new file mode 100644 index 000000000..0a6de6591 --- /dev/null +++ b/docs/design/repository-status-surface.md @@ -0,0 +1,249 @@ +# Status surfaces for a repository and its folders + +> **design**: steps 1 and 2 are implemented; steps 3 and 4 are not. +> Index: [`../INDEX.md`](../INDEX.md) + +Two objects describe one mirror. A `GitProvider` is a repository; a `GitTarget` is one folder on +one branch of it. Between them sits a branch worker, which is not an API object and must not +become one. This page fixes what each surface reports, and at what rate. + +## The surface + +| Information | Where it lives | +| --- | --- | +| Branches configured, and how many folders reference each | `GitProvider.status.branches[{name, gitTargets}]` (new) | +| Whether the credential may write | `GitProvider.status` condition `Writable` (new) | +| Configuration and health verdicts | the existing conditions on both objects | +| The latest sampled branch observation | the existing `GitTarget.status.remote` | +| Queue depth, pushes, failures, retries, drops | the existing metrics | + +Nothing else. No worker totals, no pending-write counts, no per-branch clocks, and no commit SHA as +a metric label. + +`branches` is **the branches that configured `GitTarget`s reference**, including ones that are +blocked or suspended. It is not the branches that exist on the remote, and not the workers +currently running: it answers "what is this repository being used for", which is a question about +configuration and moves only when somebody edits a `GitTarget`. Keyed by name, sorted, patched only +when the contents change. + +## Delivery is not publication + +An observation of a branch is one fact shared by every `GitTarget` on it. Two separate decisions +follow, and conflating them is the mistake this section exists to correct. + +**Delivery** is in memory and immediate. A confirmed look at branch B, by a push or by a fetch, is +available to every target on B at once, carrying the time it was proved. Distributing a +fact is not re-proving it, and it costs nothing. + +**Publication** is a status write, and it has to be bounded. Fanning a revision out to ten targets +that share a branch turns one push into ten status patches, each an etcd write that invalidates the +cached copy held by every watcher of the type. Moving revisions from the `GitProvider` to its +`GitTarget`s does not reduce that. The revision belongs on the `GitTarget` because that is where +the question is asked, which is a different argument; on write volume alone it is worse. + +So the `remote` tuple is **sampled** rather than published on every change. Two things stay prompt, +because they are health rather than throughput: a condition transition, and clearing `remote` when +the repository identity no longer matches. + +### What publication promises, and what it does not + +The sampling rule is deliberately the plainest one available, because every extra scheduling +guarantee has to be threaded through every early return the reconcile has: + +- **A `GitTarget` publishes what it holds when it reconciles**, on its own cadence. Nothing in the + data plane wakes a target because a branch moved, so a commit costs no reconciles at all; the + lag on a converged target is therefore up to one `RequeueSteadyInterval`, the same freshness + every other projection on this object has. +- **On top of that sits one floor**, `RemotePublicationInterval`, so a target reconciling in a + tight loop still writes this stanza at most once a minute. It is measured from the last WRITE, + which is kept in memory and never published: the observation's own timestamp cannot measure it, + because an observation proved just before a write and one proved just after are seconds apart + while the writes are not. +- **The lag is stated against the latest OBSERVATION, never against the branch.** No bound against + the actual branch is available at any price: an unreachable remote yields no observation at all, + and the pair (`revision`, `lastVerifiedAt`) is precisely what says so. + +Two corrections skip the floor, because they are not throughput: the first observation, and a +revision that names a repository the target has left. The second follows what THIS target +published (also kept in memory) rather than the latest shared observation, which a sibling on the +same branch may already have renewed against the new repository. + +Removing the revision bypass from +[`remoteStatusIsNews`](../../internal/controller/gittarget_remote.go) is part of it. That function +also treated a changed `verifiedBy` as news, which it is not: `Push` to `Fetch` is the same answer +proved a second way, and the watch plane already ignores it for that reason. News is "a different +revision, or the same one proved later". + +**This changes a promise, so it is worth stating plainly.** `status.remote.revision` is sampled, so +an operator who pushes and immediately reads status may see the previous revision until the target +next reconciles. The alternative is a status field that moves at commit rate, which is what the +[status rate rule](../spec/status-conditions-guide.md) rejects. + +## Example: two `GitTarget`s on one `GitProvider` + +`apps` and `infra` write different folders of the same branch, so they share one branch worker. + +```yaml +apiVersion: configbutler.ai/v1alpha3 +kind: GitProvider +metadata: {name: platform, namespace: shop} +status: + lastVerifiedAt: "2026-09-24T18:04:11Z" + branches: + - name: main + gitTargets: 2 + conditions: + - type: Ready + status: "True" + reason: Succeeded + message: Repository is reachable with the configured credential +``` + +```yaml +# GitTarget shop/apps: published after its own push +status: + streams: {summary: 5/5, total: 5, ready: 5, replaying: 0, blocked: 0} + placement: {mode: KustomizeRoot, renderRoot: clusters/prod} + remote: + revision: 4f2b9c1e8a77d3f0b6e5c4a39182d7e0fa6c5b41 + lastVerifiedAt: "2026-09-24T18:04:09Z" + verifiedBy: Push + conditions: [{type: Ready, status: "True", reason: Succeeded}] +``` + +```yaml +# GitTarget shop/infra: same branch, and has not published since +status: + streams: {summary: 3/3, total: 3, ready: 3, replaying: 0, blocked: 0} + remote: + revision: 9c07ab53d1e64f82c5b0937ae41d268fbb3a90e7 + lastVerifiedAt: "2026-09-24T17:58:40Z" + verifiedBy: Fetch + conditions: [{type: Ready, status: "True", reason: Succeeded}] +``` + +`infra` holds an **older revision together with the older time that proved it**. The tuple is always +internally consistent: one observation from one moment, never the newest revision with a stale +clock. That is what makes it readable, and sampling has to preserve it. + +Both targets keep their own conditions, and that is why one worker cannot summarize the branch: +`apps` can be `Ready=True` while `infra` sits at `GitPathAccepted=False` over content in its folder. + +## Example: an empty `GitProvider` + +A repository is declared, nothing mirrors to it yet. No branch worker exists, and none should. + +```yaml +status: + lastVerifiedAt: "2026-09-24T18:04:11Z" + branches: [] + conditions: + - type: Ready + status: "True" + reason: Succeeded + message: Repository is reachable with the configured credential +``` + +`Ready=True` with `branches: []` is the state this surface most needs to distinguish: **configured +and unused** looks nothing like **broken**, and today an operator has to list `GitTarget`s to tell +them apart. + +## Example: a wrong `GitProvider` + +The URL resolves, the credential reads, and it has no write scope. + +```yaml +status: + lastVerifiedAt: "2026-09-24T18:04:11Z" + branches: + - name: main + gitTargets: 2 + conditions: + - type: Writable + status: "False" + reason: WriteAccessDenied + message: The remote refused a write session for this credential + - type: Ready + status: "False" + reason: WriteAccessDenied + message: The remote refused a write session for this credential +``` + +Their `status.remote` keeps **advancing**: reads still work, so the refresher goes on proving where +the branch is. A stale `lastVerifiedAt` is not the signal here and must not be read as one. Writes +failing is carried by the condition and by the push metrics. Remote freshness and publication +success are separate questions, and they want separate alerts. + +### Why `Writable` is a condition of its own + +Folding the verdict into `Ready` alone would say "this provider is broken" without saying how, and +it would make the check a change to what `Ready` means rather than something added beside it. The +house pattern already answers this: a `GitTarget` publishes `StreamsRunning`, `GitPathAccepted` and +`RenderMatchesLive` as conditions in their own right AND contributes each to the trio through one +documented precedence, so the axis is readable on its own and the roll-up stays honest. + +`Writable` follows that shape: + +- `False` only on an explicit refusal. A probe that could not run leaves it `Unknown`, which by this + project's convention does not downgrade `Ready`, so a transient failure never turns a healthy + provider red. +- Not latched, with one asymmetry that has to be written down because it is where the two rules + above collide. **A failed probe never clears a proven denial.** `False` moves to `Unknown` only + when nothing has been proven yet; once the remote has refused a write session, the condition + stays `False` until a probe SUCCEEDS. Otherwise a repository whose token was revoked would go + green the moment the network flapped, and green here means "this destination works" to every + consumer, including the `Ready` gate step 4 adds. Uncertainty may withhold an affirmative + verdict; it may not overturn a proven negative one. +- Write access comes back when a token is regranted, and the condition follows: the successful + probe is the evidence, and it is the only thing that clears the denial. +- It gets a printer column, because "reachable but not writable" is the case an operator cannot + currently see at all. + +**Whether it also gates `Ready` is a separate decision, and the answer is yes, eventually.** There +is no such thing as a legitimately read-only destination here: every folder is mirrored by pushing. +A provider that cannot be written to cannot do its job, and a green `Ready` beside it would be the +kind of half-truth that teaches people to ignore conditions. Gating also costs nothing to plumb, +because a `GitTarget` already projects its provider's readiness as `GitProviderReady`; without the +gate, every consumer would have to learn a second condition and project that too. + +So: publish `Writable` first, and flip the `Ready` gate as its own release with an upgrade note, +since a provider that reads but cannot write goes from `True` to `False` the day it lands. + +## Finding out that a destination is wrong + +**No copy is needed.** Cloning answers folder questions, which belong to the `GitTarget` and already +have `GitPathAccepted`. Everything about the destination is answerable from a handshake that +transfers no objects. + +Reading is what is checked today: it rules out a bad URL, a dead credential and a missing +repository, and it yields the branch tips as a side effect. It does not prove we can write, and that +is the gap worth closing: a read-only token looks healthy right up to the first push. + +A write probe opens a push session and sends nothing. This was tried by hand against GitHub and +behaved as expected. Three limits keep it a **separate change** rather than part of this one: + +- It proves the server would open a session. Branch protection and pre-receive hooks are applied + when refs are updated, so a green probe is necessary and not sufficient. A red one is definitive. +- It cannot replace the read probe. Git lets the two services hide refs differently, so a write + advertisement is not guaranteed to carry the same branches, and an absent branch there cannot be + read as "this branch does not exist". +- It reports its own condition rather than only a `Ready` reason, for the reasons above. + +**One kind of wrong cannot be found out at all.** A URL that resolves, reads and writes may still be +a fork, a stale mirror, or somebody else's repository. Nothing on the wire identifies a repository, +so only the operator knows which one they meant. This is why the branch worker's identity is +`{GitProvider UID, URL}`: it stops state **crossing** between repositories, and it cannot stop a +destination being mistaken in the first place. + +## Order of work + +1. **Done.** The inventory: `branches[{name, gitTargets}]`, derived from the configured + `GitTarget`s on the provider's own steady tick. No watch on `GitTarget`: this reconcile also + proves the credential against the remote, so an edge per edit would spend a round trip and move + `lastVerifiedAt` to answer a question about configuration. +2. **Done.** Shared delivery plus sampled publication. The observation is recorded once, where it + is proved, and kept per branch by the worker manager, so it outlives the worker that proved it + and the per-target projection is gone. Publication follows the rule above. +3. The write probe, publishing `Writable` and nothing else. +4. `Writable` gates `Ready`, with the upgrade note. Separate, because it is the only step that + changes what an existing field means. diff --git a/internal/controller/constants.go b/internal/controller/constants.go index 60001b00a..aff0f2e30 100644 --- a/internal/controller/constants.go +++ b/internal/controller/constants.go @@ -160,6 +160,19 @@ const ( // a single 5-minute fallback for the GitProvider, GitTarget, WatchRule, and // ClusterWatchRule reconcilers. The fast stream-settle loop below is separate. RequeueSteadyInterval = 5 * time.Minute + // RemotePublicationInterval bounds how often one GitTarget writes status.remote: at most one + // such write per target per interval, whatever the branch's commit rate. + // + // It is deliberately not the reconcile cadence and not the refresh interval. Those two say how + // often a target LOOKS at its world; this says how often it is allowed to write one stanza of + // its status, which is a cost paid by every watcher of the type. A target holding something + // newer than what it published requeues on the remainder of this interval, so the number is a + // real ceiling on the lag rather than a side effect of the 5-minute tick. + // + // One minute: short enough that "where is my branch" is a current answer for a human reading + // kubectl, long enough that ten folders sharing a busy branch cost ten status writes a minute + // instead of ten per commit. + RemotePublicationInterval = 1 * time.Minute // DefaultGitRefreshInterval is how often an IDLE branch's remote state is re-proved, and the // quantizer status.remote's clock is written against. A target that is publishing renews its // observation on every push and never waits for this. diff --git a/internal/controller/gitprovider_branches.go b/internal/controller/gitprovider_branches.go new file mode 100644 index 000000000..f608e4f87 --- /dev/null +++ b/internal/controller/gitprovider_branches.go @@ -0,0 +1,75 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "context" + "sort" + + "sigs.k8s.io/controller-runtime/pkg/client" + + configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +// publishBranchInventory writes status.branches: the branches this repository's GitTargets are +// configured to write, and how many reference each. +// +// It is a question about CONFIGURATION, so it is answered from the API and not from the data +// plane. A blocked or suspended GitTarget still says what the repository is for; a branch worker +// that happens to be running says only what has been reached so far, and a repository nothing has +// published to yet has no worker at all. That is the whole point of the field: "Ready=True with no +// branches" is configured-and-unused, which looks nothing like broken, and telling the two apart +// used to take a GitTarget listing. +// +// A GitProvider is referenced only from its own namespace, so the list is namespace-scoped, and +// it is read from the cache on the provider's own steady tick. There is deliberately no watch on +// GitTarget: this reconcile also proves the credential against the remote, so an edge per +// GitTarget edit would spend a network round trip and advance status.lastVerifiedAt to answer a +// question about configuration. The inventory lags a GitTarget edit by up to one steady interval, +// which is the same freshness every other configuration projection here has. +// +// A List that fails leaves what is published standing rather than emptying it: an inventory that +// collapses to nothing whenever the cache hiccups would report "configured and unused" about a +// repository with ten GitTargets on it. +func (r *GitProviderReconciler) publishBranchInventory( + ctx context.Context, + gitProvider *configbutleraiv1alpha3.GitProvider, +) error { + var targets configbutleraiv1alpha3.GitTargetList + if err := r.List(ctx, &targets, client.InNamespace(gitProvider.Namespace)); err != nil { + return err + } + gitProvider.Status.Branches = branchInventory(gitProvider.Name, targets.Items) + return nil +} + +// branchInventory counts the live GitTargets that name provider, by branch, sorted by name. +// +// Sorting is not cosmetic. The status patch is computed against what is published, so an order +// that followed the list's arrival order would rewrite the field — and wake every watcher of the +// type — on a change that moved nothing. +func branchInventory( + provider string, + targets []configbutleraiv1alpha3.GitTarget, +) []configbutleraiv1alpha3.GitProviderBranchStatus { + counts := map[string]int32{} + for i := range targets { + target := &targets[i] + // A target being deleted is not what the repository is for any more: its worker is + // retired by the sweep the delete drives, and counting it would leave a branch in the + // inventory that nothing writes. + if !target.DeletionTimestamp.IsZero() || target.Spec.GitProviderRef.Name != provider { + continue + } + counts[target.Spec.Branch]++ + } + if len(counts) == 0 { + return nil + } + branches := make([]configbutleraiv1alpha3.GitProviderBranchStatus, 0, len(counts)) + for name, count := range counts { + branches = append(branches, configbutleraiv1alpha3.GitProviderBranchStatus{Name: name, GitTargets: count}) + } + sort.Slice(branches, func(i, j int) bool { return branches[i].Name < branches[j].Name }) + return branches +} diff --git a/internal/controller/gitprovider_branches_test.go b/internal/controller/gitprovider_branches_test.go new file mode 100644 index 000000000..6b7e1c3c1 --- /dev/null +++ b/internal/controller/gitprovider_branches_test.go @@ -0,0 +1,115 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/fluxcd/pkg/apis/meta" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +func inventoryTarget(name, provider, branch string) *configbutleraiv1alpha3.GitTarget { + return &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "shop"}, + Spec: configbutleraiv1alpha3.GitTargetSpec{ + GitProviderRef: meta.LocalObjectReference{Name: provider}, + Branch: branch, + Path: name, + }, + } +} + +// TestBranchInventory_CountsTheGitTargetsThatConfigureEachBranch is the whole field: what is this +// repository being used for, answered from the configuration and sorted so a status patch is sent +// only when the answer moves. +func TestBranchInventory_CountsTheGitTargetsThatConfigureEachBranch(t *testing.T) { + suspended := inventoryTarget("archive", "repo1", "main") + suspended.Spec.Suspend = true + deleting := inventoryTarget("leaving", "repo1", "retired") + deleting.DeletionTimestamp = &metav1.Time{Time: time.Now()} + + got := branchInventory("repo1", []configbutleraiv1alpha3.GitTarget{ + *inventoryTarget("release", "repo1", "rel"), + *inventoryTarget("apps", "repo1", "main"), + *inventoryTarget("infra", "repo1", "main"), + *suspended, + *deleting, + *inventoryTarget("other", "repo2", "main"), + }) + + assert.Equal(t, []configbutleraiv1alpha3.GitProviderBranchStatus{ + {Name: "main", GitTargets: 3}, + {Name: "rel", GitTargets: 1}, + }, got, + "every branch a live GitTarget of THIS provider configures, suspended ones included, sorted by name") +} + +// TestBranchInventory_AConfiguredAndUnusedRepositoryReportsNoBranches is the state the field +// exists to distinguish. It is not an error, and it must not look like one. +func TestBranchInventory_AConfiguredAndUnusedRepositoryReportsNoBranches(t *testing.T) { + assert.Nil(t, branchInventory("repo1", nil)) + assert.Nil(t, branchInventory("repo1", []configbutleraiv1alpha3.GitTarget{ + *inventoryTarget("other", "repo2", "main"), + }), "a GitTarget on another provider says nothing about this one") +} + +// TestPublishBranchInventory_KeepsTheLastAnswerWhenTheTargetsCannotBeRead. An inventory that +// collapsed to nothing on a cache hiccup would report "configured and unused" about a repository +// with GitTargets all over it, which is exactly the misreading the field is here to prevent. +func TestPublishBranchInventory_KeepsTheLastAnswerWhenTheTargetsCannotBeRead(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, configbutleraiv1alpha3.AddToScheme(scheme)) + k8sClient := interceptor.NewClient( + fake.NewClientBuilder().WithScheme(scheme).Build(), + interceptor.Funcs{ + List: func(context.Context, client.WithWatch, client.ObjectList, ...client.ListOption) error { + return errors.New("the cache is not ready") + }, + }) + r := &GitProviderReconciler{Client: k8sClient} + provider := &configbutleraiv1alpha3.GitProvider{ + ObjectMeta: metav1.ObjectMeta{Name: "repo1", Namespace: "shop"}, + Status: configbutleraiv1alpha3.GitProviderStatus{ + Branches: []configbutleraiv1alpha3.GitProviderBranchStatus{{Name: "main", GitTargets: 2}}, + }, + } + + require.Error(t, r.publishBranchInventory(context.Background(), provider)) + assert.Equal(t, []configbutleraiv1alpha3.GitProviderBranchStatus{{Name: "main", GitTargets: 2}}, + provider.Status.Branches) +} + +// TestPublishBranchInventory_ReadsOnlyTheProvidersOwnNamespace. A GitProvider is referenced from +// its own namespace, and counting a same-named provider's targets in another tenant's namespace +// would report somebody else's configuration. +func TestPublishBranchInventory_ReadsOnlyTheProvidersOwnNamespace(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, configbutleraiv1alpha3.AddToScheme(scheme)) + elsewhere := inventoryTarget("apps", "repo1", "other-tenant-branch") + elsewhere.Namespace = "warehouse" + k8sClient := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(inventoryTarget("apps", "repo1", "main"), elsewhere).Build() + r := &GitProviderReconciler{Client: k8sClient} + provider := &configbutleraiv1alpha3.GitProvider{ + ObjectMeta: metav1.ObjectMeta{Name: "repo1", Namespace: "shop"}, + } + + require.NoError(t, r.publishBranchInventory(context.Background(), provider)) + assert.Equal(t, []configbutleraiv1alpha3.GitProviderBranchStatus{{Name: "main", GitTargets: 1}}, + provider.Status.Branches) +} diff --git a/internal/controller/gitprovider_controller.go b/internal/controller/gitprovider_controller.go index 3b0ba5953..00b5aa116 100644 --- a/internal/controller/gitprovider_controller.go +++ b/internal/controller/gitprovider_controller.go @@ -61,6 +61,7 @@ type gitProviderLogFirsts struct { // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders/status,verbs=get;update;patch // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders/finalizers,verbs=update +// +kubebuilder:rbac:groups=configbutler.ai,resources=gittargets,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;create;update // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch @@ -111,6 +112,16 @@ func (r *GitProviderReconciler) reconcileGitProvider( "GitProvider is not stalled", ) + // Ahead of every gate, like the GitTarget's own projections and for the same reason: the + // inventory explains what a refused or unused repository is FOR, so publishing it only on the + // happy path would drop it exactly when it is wanted. It must also come AFTER beginStatus, + // which snapshots the status the patch is computed against. A failure to read the GitTargets + // leaves the last inventory standing and is not a reason to fail the reconcile. + if err := r.publishBranchInventory(ctx, gitProvider); err != nil { + log.Error(err, "Could not list the GitTargets for the branch inventory; the last one stands", + "name", gitProvider.Name, "namespace", gitProvider.Namespace) + } + if err := r.validateCommitConfiguration(gitProvider); err != nil { rd.stalled(ReasonCommitConfigInvalid, err.Error()) return r.commitProvider(ctx, st, rd) diff --git a/internal/controller/gitprovider_controller_test.go b/internal/controller/gitprovider_controller_test.go index 8cdd30694..61a0c27ea 100644 --- a/internal/controller/gitprovider_controller_test.go +++ b/internal/controller/gitprovider_controller_test.go @@ -308,6 +308,62 @@ var _ = Describe("GitProvider Controller", func() { } }) + It("publishes the branches its GitTargets configure, even when the remote is unreachable", func() { + // The inventory is a question about configuration, so it is published ahead of every + // gate: an operator looking at a red GitProvider still needs to see what it is FOR. + // + // The inventory is re-derived on the provider's own reconcile, deliberately with no + // watch on GitTarget: this reconcile also proves the credential against the remote, so + // an edge per GitTarget edit would spend a round trip to answer a question about + // configuration. Each step below therefore drives one reconcile, inside Eventually + // because the suite's manager reconciles the same object and either write can lose the + // optimistic lock. + gitProvider = &configbutleraiv1alpha3.GitProvider{ + ObjectMeta: metav1.ObjectMeta{Name: "test-provider-inventory", Namespace: "default"}, + Spec: configbutleraiv1alpha3.GitProviderSpec{ + URL: "https://example.invalid/unreachable.git", + AllowedBranches: []string{"main", "release"}, + }, + } + Expect(k8sClient.Create(ctx, gitProvider)).To(Succeed()) + + key := types.NamespacedName{Name: gitProvider.Name, Namespace: gitProvider.Namespace} + branches := func(g Gomega) []configbutleraiv1alpha3.GitProviderBranchStatus { + _, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: key}) + g.Expect(err).NotTo(HaveOccurred()) + published := &configbutleraiv1alpha3.GitProvider{} + g.Expect(k8sClient.Get(ctx, key, published)).To(Succeed()) + return published.Status.Branches + } + + Consistently(branches, "2s", "500ms").Should(BeEmpty(), + "configured and unused is a state of its own, and it is not an error") + + targets := []*configbutleraiv1alpha3.GitTarget{ + newInventoryGitTarget("inventory-apps", gitProvider.Name, "main"), + newInventoryGitTarget("inventory-infra", gitProvider.Name, "main"), + newInventoryGitTarget("inventory-release", gitProvider.Name, "release"), + } + for _, target := range targets { + Expect(k8sClient.Create(ctx, target)).To(Succeed()) + } + DeferCleanup(func() { + for _, target := range targets { + _ = k8sClient.Delete(ctx, target) + } + }) + + Eventually(branches, "30s", "500ms").Should(Equal([]configbutleraiv1alpha3.GitProviderBranchStatus{ + {Name: "main", GitTargets: 2}, + {Name: "release", GitTargets: 1}, + }), "two folders share the main branch, and they share one branch worker with it") + + Expect(k8sClient.Delete(ctx, targets[2])).To(Succeed()) + Eventually(branches, "30s", "500ms").Should(Equal([]configbutleraiv1alpha3.GitProviderBranchStatus{ + {Name: "main", GitTargets: 2}, + }), "a branch nothing references any more is not what this repository is for") + }) + It("should fail when secret is not found", func() { gitProvider = &configbutleraiv1alpha3.GitProvider{ ObjectMeta: metav1.ObjectMeta{ @@ -499,3 +555,16 @@ func generateTestSSHKey() ([]byte, error) { return privateKeyPEM, nil } + +// newInventoryGitTarget is a minimal, valid GitTarget for the branch-inventory spec: the inventory +// reads the spec alone, so nothing here has to reconcile. +func newInventoryGitTarget(name, provider, branch string) *configbutleraiv1alpha3.GitTarget { + return &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default"}, + Spec: configbutleraiv1alpha3.GitTargetSpec{ + GitProviderRef: meta.LocalObjectReference{Name: provider}, + Branch: branch, + Path: "apps/" + name, + }, + } +} diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index 3dc533135..d54242cbc 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -147,6 +147,10 @@ type GitTargetReconciler struct { // so a standing annotation forces one re-read rather than one per reconcile forever. Its zero // value is usable. reconcileRequests reconcileRequestTracker + + // remotePublications is what each GitTarget last wrote to status.remote: when, and about which + // repository. It is internal by design; see remotePublicationLedger. + remotePublications remotePublicationLedger } // +kubebuilder:rbac:groups=configbutler.ai,resources=gittargets,verbs=get;list;watch;create;update;patch;delete @@ -173,9 +177,15 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( st := beginStatus(r.Client, r.Recorder, &target) gitPathWasRefused := conditionIsFalse(target.Status.Conditions, GitTargetConditionGitPathAccepted) - r.publishGitObservations(st, &target) - providerNS := target.Namespace + // One read of the GitProvider for everything below it; see getGitProvider. + gitProvider, providerErr := r.getGitProvider(ctx, providerNS, target.Spec.GitProviderRef.Name) + if providerErr != nil { + return ctrl.Result{}, providerErr + } + + r.publishGitObservations(st, &target, providerNS, repoIdentityOf(gitProvider)) + // Ahead of every gate, for the reason the layout stanza above is: the join series has to carry // the targets that are NOT working. git_pushes_total names a branch that stopped advancing and // cannot name the GitTargets behind it, and a mapping published only on the happy path would @@ -185,7 +195,7 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( target.Namespace, target.Name, providerNS, target.Spec.GitProviderRef.Name, target.Spec.Branch, target.SourceCluster()) - validated, validationMsg, validationErr := r.evaluateValidatedGate(ctx, st, &target, providerNS) + validated, validationMsg, validationErr := r.evaluateValidatedGate(ctx, st, &target, providerNS, gitProvider) if validationErr != nil { return ctrl.Result{}, validationErr } @@ -223,7 +233,7 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // Ensure the branch worker exists and register the GitTarget's event stream before declaring // streams. The watch data plane can route live events as soon as it reaches Streaming, so the // destination worker must already be wired. - wired, wiringMessage := r.evaluateWorkerWiringGate(&target, providerNS, log) + wired, wiringMessage := r.evaluateWorkerWiringGate(ctx, &target, providerNS, repoIdentityOf(gitProvider), log) if !wired { return r.stall(ctx, st, blockedGate{ reason: GitTargetReadyReasonWorkerUnavailable, @@ -256,31 +266,11 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( st.setValue(GitTargetConditionGitPathAccepted, observed.axes.GitPath) st.setValue(GitTargetConditionRenderMatchesLive, observed.axes.Render) - // Source-cluster reachability (runtime), GitProvider readiness (destination-side) and - // ClusterProvider readiness (source-config side) are published as conditions of their own AND - // contributed to the trio below. Publishing is all this does: nothing here writes Ready. - sourceReach := r.observeSourceReachable(&target) - provider := r.gitProviderReadiness(ctx, &target, providerNS) - clusterProvider := clusterProviderReadiness(sourceProvider) - st.setValue(GitTargetConditionSourceClusterReachable, sourceReach) - st.setValue(GitTargetConditionGitProviderReady, provider) - st.setValue(GitTargetConditionClusterProviderReady, clusterProvider) + refs := r.publishReferenceReadiness(st, &target, providerNS, gitProvider, sourceProvider) rd := newGitTargetReadiness() - // A suspended target is healthy, not faulty: not writing is the configured outcome, and no - // condition may go False for one — that is what trains operators to ignore the conditions - // that mean the mirror is genuinely broken. status.retention is the precedent. So Ready stays - // True and only its reason changes, which is what makes the state legible without making it - // look like a fault. Every real gate below still applies: a suspended target with a broken - // provider is still not Ready. - if target.Spec.Suspend { - rd.convergesAs(conditionValue{ - Status: metav1.ConditionTrue, - Reason: GitTargetReasonSuspended, - Message: "GitTarget is suspended: it scans and publishes what it resolved, and writes nothing", - }) - } - gitTargetReadinessGates(rd, observed, provider, clusterProvider, sourceReach) + convergeAsSuspended(rd, &target) + gitTargetReadinessGates(rd, observed, refs.gitProvider, refs.clusterProvider, refs.sourceCluster) st.applyReadiness(rd) if err := st.commit(ctx); err != nil { @@ -306,6 +296,25 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{RequeueAfter: requeue}, nil } +// convergeAsSuspended gives a suspended GitTarget the converged outcome, under its own reason. +// +// A suspended target is healthy, not faulty: not writing is the configured outcome, and no +// condition may go False for one — that is what trains operators to ignore the conditions that +// mean the mirror is genuinely broken. status.retention is the precedent. So Ready stays True and +// only its reason changes, which is what makes the state legible without making it look like a +// fault. Every real gate still applies afterwards: a suspended target with a broken provider is +// still not Ready. +func convergeAsSuspended(rd *readiness, target *configbutleraiv1alpha3.GitTarget) { + if !target.Spec.Suspend { + return + } + rd.convergesAs(conditionValue{ + Status: metav1.ConditionTrue, + Reason: GitTargetReasonSuspended, + Message: "GitTarget is suspended: it scans and publishes what it resolved, and writes nothing", + }) +} + // gitTargetRequeue picks the periodic cadence. Only a converged GitTarget earns the steady interval. // // A STALLED target gets the fast loop too, which is deliberate and specific to this kind: its @@ -392,11 +401,9 @@ func (r *GitTargetReconciler) evaluateValidatedGate( st *reconcileStatus, target *configbutleraiv1alpha3.GitTarget, providerNS string, + provider *configbutleraiv1alpha3.GitProvider, ) (bool, string, error) { - validated, message, reason, err := r.validateProviderAndBranch(ctx, target, providerNS) - if err != nil { - return false, "", err - } + validated, message, reason := validateProviderAndBranch(target, providerNS, provider) if !validated { st.set(GitTargetConditionValidated, metav1.ConditionFalse, reason, message) return false, fmt.Sprintf("Validated gate failed: %s", reason), nil @@ -502,15 +509,17 @@ func (r *GitTargetReconciler) evaluateEncryptionGate( // internal plumbing rather than a status condition of its own: rare failures fold into Ready // with reason WorkerUnavailable. A nil EventRouter (test/standalone) is trivially wired. func (r *GitTargetReconciler) evaluateWorkerWiringGate( + ctx context.Context, target *configbutleraiv1alpha3.GitTarget, providerNS string, + repo git.RepoIdentity, log logr.Logger, ) (bool, string) { if r.EventRouter == nil { return true, "" } - if _, err := r.ensureEventStream(target, providerNS, log); err != nil { + if _, err := r.ensureEventStream(ctx, target, providerNS, repo, log); err != nil { return false, fmt.Sprintf("Failed to wire branch worker/event stream for %s/%s: %v", target.Namespace, target.Name, err) } @@ -706,6 +715,36 @@ func renderAxis(renderFidelity watch.RenderFidelityStatus) conditionValue { return value } +// referenceReadiness is what the three objects a GitTarget REFERENCES say about themselves: +// destination-side (the GitProvider), source-config side (the ClusterProvider), and the runtime +// reachability of the source cluster. +type referenceReadiness struct { + gitProvider conditionValue + clusterProvider conditionValue + sourceCluster conditionValue +} + +// publishReferenceReadiness writes those three as conditions of their own and returns them, so the +// readiness trio can also take them into account. Publishing is all this does: nothing here +// writes Ready. +func (r *GitTargetReconciler) publishReferenceReadiness( + st *reconcileStatus, + target *configbutleraiv1alpha3.GitTarget, + providerNS string, + gitProvider *configbutleraiv1alpha3.GitProvider, + sourceProvider *configbutleraiv1alpha3.ClusterProvider, +) referenceReadiness { + refs := referenceReadiness{ + gitProvider: gitProviderReadiness(target, providerNS, gitProvider), + clusterProvider: clusterProviderReadiness(sourceProvider), + sourceCluster: r.observeSourceReachable(target), + } + st.setValue(GitTargetConditionSourceClusterReachable, refs.sourceCluster) + st.setValue(GitTargetConditionGitProviderReady, refs.gitProvider) + st.setValue(GitTargetConditionClusterProviderReady, refs.clusterProvider) + return refs +} + // gitTargetReadinessGates contributes every GitTarget gate to the accumulator that owns the trio. // // THIS FUNCTION IS THE PRECEDENCE. Read it top to bottom: a stall always beats a progressing gate @@ -796,65 +835,88 @@ func (r *GitTargetReconciler) requestRemoteRefresh( // It runs ahead of every gate, so a target held unready still shows both. That ordering is the // point rather than a convenience: these stanzas exist to explain a refused or surprising write, // and a projection that ran only on the happy path would be missing exactly when it is wanted. + func (r *GitTargetReconciler) publishGitObservations( st *reconcileStatus, target *configbutleraiv1alpha3.GitTarget, + providerNS string, + repo git.RepoIdentity, ) { layout, scanned := r.observeLayout(target) publishLayout(st, target, layout, scanned) - remote, remoteSeen := r.observeRemote(target) - publishRemote(target, remote, remoteSeen, r.GitRefreshInterval) + remote, remoteSeen := r.observeRemote(target, providerNS) + r.publishRemote(target, remote, remoteSeen, repo, time.Now()) } +// ensureEventStream wires this GitTarget to the branch worker for the repository its GitProvider +// names NOW, and wires the route live events take to reach it. +// +// EnsureWorker is called on EVERY reconcile rather than only when no worker is in the slot: it is +// the one place that compares the slot's repository against repo, and a lookup that found a worker +// says nothing about WHICH repository that worker is about. It is a map read and a lock when +// nothing has changed. func (r *GitTargetReconciler) ensureEventStream( + ctx context.Context, target *configbutleraiv1alpha3.GitTarget, providerNS string, + repo git.RepoIdentity, log logr.Logger, ) (*reconcile.GitTargetEventStream, error) { if r.WorkerManager == nil { return nil, errors.New("worker manager is not configured") } - worker, exists := r.WorkerManager.GetWorkerForTarget( + err := r.WorkerManager.EnsureWorker( + context.Background(), target.Spec.GitProviderRef.Name, providerNS, target.Spec.Branch, + repo, ) - if !exists { - if err := r.WorkerManager.EnsureWorker( - context.Background(), - target.Spec.GitProviderRef.Name, + if err != nil { + return nil, fmt.Errorf( + "failed to ensure branch worker for provider=%s/%s branch=%s: %w", providerNS, + target.Spec.GitProviderRef.Name, target.Spec.Branch, - ); err != nil { - return nil, fmt.Errorf( - "failed to ensure branch worker for provider=%s/%s branch=%s: %w", - providerNS, - target.Spec.GitProviderRef.Name, - target.Spec.Branch, - err, - ) - } + err, + ) + } + // Asked on EVERY reconcile, not only on the one that did the replacing. The recovery is held + // by the worker manager until it has been delivered, so a delivery that failed is retried by + // whichever GitTarget on the branch reconciles next. + branch := git.BranchKey{ + RepoNamespace: providerNS, + RepoName: target.Spec.GitProviderRef.Name, + Branch: target.Spec.Branch, + } + if r.WorkerManager.ReplacementPending(branch) && + r.recoverBranchAfterReplacement(ctx, branch, log) { + r.WorkerManager.AcknowledgeReplacement(branch) + } - var ensured bool - worker, ensured = r.WorkerManager.GetWorkerForTarget( - target.Spec.GitProviderRef.Name, + worker, ensured := r.WorkerManager.GetWorkerForTarget( + target.Spec.GitProviderRef.Name, + providerNS, + target.Spec.Branch, + ) + if !ensured { + return nil, fmt.Errorf( + "branch worker not found for provider=%s/%s branch=%s", providerNS, + target.Spec.GitProviderRef.Name, target.Spec.Branch, ) - if !ensured { - return nil, fmt.Errorf( - "branch worker not found for provider=%s/%s branch=%s", - providerNS, - target.Spec.GitProviderRef.Name, - target.Spec.Branch, - ) - } } gitDest := types.NewResourceReference(target.Name, target.Namespace) - if existingStream := r.EventRouter.GetGitTargetEventStream(gitDest); existingStream != nil { + // A stream holds its worker directly, so one registered against a REPLACED worker would keep + // handing live events to a stopped goroutine's queue — accepted, never written, and silently + // dropped once it filled. The stream is thin and holds no state of its own, so re-pointing it + // is a fresh registration. + if existingStream := r.EventRouter.GetGitTargetEventStream(gitDest); existingStream != nil && + existingStream.ServesWorker(worker) { return existingStream, nil } @@ -863,19 +925,18 @@ func (r *GitTargetReconciler) ensureEventStream( return stream, nil } -func (r *GitTargetReconciler) validateProviderAndBranch( - ctx context.Context, +// validateProviderAndBranch answers the two questions the GitProvider decides: is there one, and +// does it admit this branch. It takes the provider the reconcile already read rather than reading +// its own, so the branch worker cannot be wired for a different object than the one validated +// here; a nil provider is the not-found case. +func validateProviderAndBranch( target *configbutleraiv1alpha3.GitTarget, providerNS string, -) (bool, string, string, error) { - var gp configbutleraiv1alpha3.GitProvider - gpKey := k8stypes.NamespacedName{Name: target.Spec.GitProviderRef.Name, Namespace: providerNS} - if err := r.Get(ctx, gpKey, &gp); err != nil { - if apierrors.IsNotFound(err) { - msg := fmt.Sprintf("Referenced GitProvider '%s/%s' not found", providerNS, target.Spec.GitProviderRef.Name) - return false, msg, GitTargetReasonProviderNotFound, nil - } - return false, "", "", err + gp *configbutleraiv1alpha3.GitProvider, +) (bool, string, string) { + if gp == nil { + msg := fmt.Sprintf("Referenced GitProvider '%s/%s' not found", providerNS, target.Spec.GitProviderRef.Name) + return false, msg, GitTargetReasonProviderNotFound } branchAllowed := false @@ -897,10 +958,101 @@ func (r *GitTargetReconciler) validateProviderAndBranch( providerNS, target.Spec.GitProviderRef.Name, ) - return false, msg, GitTargetReasonBranchNotAllowed, nil + return false, msg, GitTargetReasonBranchNotAllowed } - return true, "", "", nil + return true, "", "" +} + +// recoverBranchAfterReplacement re-establishes every GitTarget on a branch whose worker was just +// replaced, because the replacement leaves that branch's state describing a repository that is no +// longer the destination. +// +// The new worker starts with no clone, no base trust and an empty queue: whatever the old one held +// was computed against the old repository and is deliberately dropped. Nothing else asks for it +// back. The declaration is level-triggered and its inputs did not change, so the next pass is a +// no-op, the streams keep running, and an IDLE target would leave the new repository empty for as +// long as nothing happened to it. Forcing a recheck re-anchors the streams, and a stream that +// starts replays, which is what drives the mark-and-sweep that rebuilds the folder. +// +// It runs for every target on the branch and not only the one whose reconcile noticed. A worker +// serves them all, and the siblings have no reason of their own to reconcile. +// +// The render-fidelity verdicts go the same way and for the same reason: they say a folder matches +// live, about a folder in the other repository. They are INVALIDATED rather than forgotten — +// forgetting a target leaves it unregistered, which the gate reads as writable, so the writes +// would be admitted against the new repository before anything had looked at it. +// +// It reports whether the recovery was DELIVERED. A list that fails delivers nothing, and the +// request it was serving stays pending on the worker manager for the next reconcile to pick up: +// the alternative, spending the notification on an attempt that reached nobody, left the new +// repository with no target ever asked to populate it. +func (r *GitTargetReconciler) recoverBranchAfterReplacement( + ctx context.Context, + branch git.BranchKey, + log logr.Logger, +) bool { + if r.EventRouter == nil || r.EventRouter.WatchManager == nil { + return false + } + var targets configbutleraiv1alpha3.GitTargetList + if err := r.List(ctx, &targets, client.InNamespace(branch.RepoNamespace)); err != nil { + log.Error(err, "Could not re-establish the branch after its worker was replaced; it stays pending", + "provider", branch.RepoNamespace+"/"+branch.RepoName, "branch", branch.Branch) + return false + } + gate := r.WorkerManager.RenderFidelityGate() + recovered := 0 + for i := range targets.Items { + affected := &targets.Items[i] + if affected.Spec.GitProviderRef.Name != branch.RepoName || affected.Spec.Branch != branch.Branch { + continue + } + if !affected.DeletionTimestamp.IsZero() { + continue + } + ref := types.NewResourceReference(affected.Name, affected.Namespace).WithUID(string(affected.UID)) + gate.Invalidate(ref) + r.EventRouter.WatchManager.RequestRecheckForGitTarget(ref) + recovered++ + } + log.Info("Branch worker replaced; re-establishing every GitTarget on the branch", + "provider", branch.RepoNamespace+"/"+branch.RepoName, "branch", branch.Branch, "gitTargets", recovered) + return true +} + +// getGitProvider reads the GitTarget's GitProvider ONCE for the whole reconcile. +// +// That one object answers the Validated gate's questions, projects GitProviderReady, and names the +// repository the branch worker has to be about. Reading it three times let those three disagree +// about which repository this target points at, which is the disagreement the worker's repository +// identity exists to prevent. +// +// A provider that is not there comes back (nil, nil): its absence is a configuration state the +// Validated gate reports as ProviderNotFound, not a reconcile failure to retry with a backoff. +func (r *GitTargetReconciler) getGitProvider( + ctx context.Context, + providerNS, providerName string, +) (*configbutleraiv1alpha3.GitProvider, error) { + var gp configbutleraiv1alpha3.GitProvider + key := k8stypes.NamespacedName{Name: providerName, Namespace: providerNS} + if err := r.Get(ctx, key, &gp); err != nil { + if apierrors.IsNotFound(err) { + return nil, nil //nolint:nilnil // a GitProvider that is not there is a state, not an error + } + return nil, err + } + return &gp, nil +} + +// repoIdentityOf is which repository a GitProvider names, in the form the data plane keys a branch +// worker by. A provider that could not be read names nothing, and the zero identity says exactly +// that rather than standing in for some repository. +func repoIdentityOf(provider *configbutleraiv1alpha3.GitProvider) git.RepoIdentity { + if provider == nil { + return git.RepoIdentity{} + } + return git.RepoIdentity{ProviderUID: provider.UID, URL: provider.Spec.URL} } func (r *GitTargetReconciler) checkForConflicts( diff --git a/internal/controller/gittarget_placement_validation_test.go b/internal/controller/gittarget_placement_validation_test.go index 5397213b0..6eccc5b9d 100644 --- a/internal/controller/gittarget_placement_validation_test.go +++ b/internal/controller/gittarget_placement_validation_test.go @@ -281,7 +281,7 @@ func TestEvaluateValidatedGate_InvalidPlacementPolicy(t *testing.T) { reconciler := &GitTargetReconciler{Client: client} st := beginStatus(client, nil, target) - validated, msg, err := reconciler.evaluateValidatedGate(context.Background(), st, target, ns) + validated, msg, err := reconciler.evaluateValidatedGate(context.Background(), st, target, ns, provider) require.NoError(t, err) assert.False(t, validated) diff --git a/internal/controller/gittarget_remote.go b/internal/controller/gittarget_remote.go index 7e9c77791..697ccef9f 100644 --- a/internal/controller/gittarget_remote.go +++ b/internal/controller/gittarget_remote.go @@ -3,6 +3,7 @@ package controller import ( + "sync" "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -12,50 +13,117 @@ import ( "github.com/ConfigButler/gitops-reverser/internal/types" ) -// observeRemote reads the observation the data plane last made of this GitTarget's branch. +// remotePublicationLedger remembers, per GitTarget, what its last status.remote WRITE was: when it +// happened, and which repository the revision it published was proved against. +// +// Both halves are deliberately internal. The write TIME is what a rate limit has to measure +// against — the observation's own timestamp is not, because an observation made just before a +// publication and a fresh one made just after are a second apart while the two writes are not. +// The REPOSITORY is what makes invalidation follow this target's published state rather than the +// latest shared observation: one branch worker serves every GitTarget on a branch, so a sibling +// can already have proved the replacement repository while this target still publishes a revision +// from the one it has left, and comparing the shared observation against the provider would find +// them in agreement and leave the wrong revision standing. +// +// Losing it costs nothing. An empty ledger publishes the next observation immediately and then +// resumes limiting, which is the right behaviour after a restart: what is published is older than +// the process. +type remotePublicationLedger struct { + mu sync.Mutex + entries map[string]remotePublication +} + +type remotePublication struct { + at time.Time + repo git.RepoIdentity +} + +func (l *remotePublicationLedger) last(ref types.ResourceReference) (remotePublication, bool) { + l.mu.Lock() + defer l.mu.Unlock() + entry, had := l.entries[ref.Key()] + return entry, had +} + +func (l *remotePublicationLedger) record(ref types.ResourceReference, at time.Time, repo git.RepoIdentity) { + l.mu.Lock() + defer l.mu.Unlock() + if l.entries == nil { + l.entries = map[string]remotePublication{} + } + l.entries[ref.Key()] = remotePublication{at: at, repo: repo} +} + +func (l *remotePublicationLedger) forget(ref types.ResourceReference) { + l.mu.Lock() + defer l.mu.Unlock() + delete(l.entries, ref.Key()) +} + +// observeRemote reads what the data plane last proved about this GitTarget's BRANCH. +// +// The observation is shared: one branch worker serves every GitTarget on its (provider, branch), +// and what it proved — by a push, or by a fetch — is one fact about the branch, available to all +// of them at the moment it was proved, carrying the time it was proved. Sharing it costs nothing +// and re-proving it per target would cost a connection each. // // Absent means nothing has looked yet, which is genuinely different from a look that found no // branch: the first leaves status.remote unset, the second publishes an observation with no // revision. func (r *GitTargetReconciler) observeRemote( target *configbutleraiv1alpha3.GitTarget, + providerNS string, ) (git.RemoteObservation, bool) { - if r.EventRouter == nil || r.EventRouter.WatchManager == nil { + if r.WorkerManager == nil { return git.RemoteObservation{}, false } - return r.EventRouter.WatchManager.RemoteForGitTarget( - types.NewResourceReference(target.Name, target.Namespace)) + return r.WorkerManager.RemoteForBranch(git.BranchKey{ + RepoNamespace: providerNS, + RepoName: target.Spec.GitProviderRef.Name, + Branch: target.Spec.Branch, + }) } -// publishRemote writes status.remote, subject to the rule that keeps its write rate honest. +// publishRemote writes status.remote, sampled onto this reconcile and rate-limited. +// +// DELIVERY IS NOT PUBLICATION. An observation reaches every GitTarget on the branch the instant it +// is proved, in memory, for nothing. A status write is the opposite kind of thing: fanning one +// push out to ten targets sharing a branch turns it into ten etcd writes, each invalidating the +// cached copy every watcher of the type holds. So this publishes what the target holds AT THE +// MOMENT IT RECONCILES, and nothing in the data plane wakes a target because a branch moved. // -// A converged GitTarget writes nothing on its steady tick, because the status patch no-ops on an -// empty diff. A clock in status ends that unless something governs it, and republishing a -// timestamp nobody reads a decision from — once per target per tick, forever — is the trap -// sameLayout was written to avoid. +// The cadence is therefore the target's own: a converged one samples every RequeueSteadyInterval, +// a busy one much sooner, and nobody is scheduled a wake-up for a status field. On top of that +// sits one floor, RemotePublicationInterval, so that a target reconciling in a tight loop still +// writes this stanza at most once a minute. The floor is measured from the last WRITE, kept in the +// ledger above, not from the observation's timestamp. // -// So: write whenever the REVISION changes, including one we pushed ourselves, and otherwise only -// when the published timestamp is older than one refresh interval. The revision has to move with -// the fact it dates; a field showing a ten-minute-old revision on a target somebody is actively -// editing is a slower copy of the answer, not a freshness field. The push case is also the cheap -// one: beside a packfile upload and an accepted ref update, one status patch against the local -// API server is noise, and PushCooldown floors it at one push per 5s per branch worker. -func publishRemote( +// Two things ignore the floor, because they are correctness rather than throughput: the FIRST +// observation, which is the difference between "nothing has looked" and an answer, and any +// correction that involves the repository changing under the target. +func (r *GitTargetReconciler) publishRemote( target *configbutleraiv1alpha3.GitTarget, observed git.RemoteObservation, seen bool, - quantum time.Duration, + repo git.RepoIdentity, + now time.Time, ) { - if !seen { + ref := types.NewResourceReference(target.Name, target.Namespace) + published, everPublished := r.remotePublications.last(ref) + // What this target PUBLISHED was proved against a repository it no longer points at: its + // GitProvider was recreated against a different URL. + staleRepository := everPublished && differentRepository(published.repo, repo) + + if withdrawRemote(staleRepository, seen, observed, repo) { + // Nothing proved about the repository in use now, so there is nothing to replace what is + // published with, and it has to go rather than go stale. + target.Status.Remote = nil + r.remotePublications.forget(ref) return } - // A withdrawal is not an observation: the data plane is saying that what is published names a - // revision in a repository this target no longer points at. It has to be REMOVED rather than - // replaced, because there is nothing to replace it with until a look at the new repository - // succeeds — and an unreachable new remote would otherwise leave the old one's revision - // standing indefinitely. - if observed.Withdrawn { - target.Status.Remote = nil + if !seen { + // Nothing has looked at this branch since the process started. What is published is the + // last thing anyone proved about it, and there is nothing better to replace it with. return } next := &configbutleraiv1alpha3.GitTargetRemoteStatus{ @@ -63,32 +131,57 @@ func publishRemote( LastVerifiedAt: &metav1.Time{Time: observed.At}, VerifiedBy: string(observed.By), } - if !remoteStatusIsNews(target.Status.Remote, next, quantum) { + if !remoteStatusIsNews(target.Status.Remote, next) { + return + } + // The floor, skipped for the two corrections above and for the first publication. + if everPublished && !staleRepository && now.Sub(published.at) < RemotePublicationInterval { return } target.Status.Remote = next + r.remotePublications.record(ref, now, observed.Repo) } -// remoteStatusIsNews decides whether an observation is worth a status write. +// withdrawRemote decides whether what is published has to be taken back rather than replaced. +// +// Two questions, and they are asked of different evidence. The ledger knows which repository THIS +// target published, which is what a sibling's observation cannot answer: one worker serves every +// GitTarget on a branch, so a sibling can already have proved the replacement while this target +// still publishes a revision from the repository it has left. The observation answers for the case +// the ledger cannot — a restart, or a target that has not published under this process — because +// it names the repository it was proved against. // -// metav1.Time is second-granular on the wire, so the age is compared against what was PUBLISHED -// rather than against an in-memory value: a sub-second difference that the API server would round -// away must not count as movement, or the quantizer would leak one write per tick. -func remoteStatusIsNews( - published, next *configbutleraiv1alpha3.GitTargetRemoteStatus, - quantum time.Duration, -) bool { - if published == nil { +// Both comparisons need two known halves: an unreadable GitProvider names no repository, and a +// worker with no identity of its own records none. Neither is evidence of a mismatch. +func withdrawRemote(staleRepository, seen bool, observed git.RemoteObservation, repo git.RepoIdentity) bool { + if staleRepository && (!seen || differentRepository(observed.Repo, repo)) { return true } - if published.Revision != next.Revision || published.VerifiedBy != next.VerifiedBy { + return seen && differentRepository(observed.Repo, repo) +} + +// differentRepository reports a mismatch only when both identities are known. +func differentRepository(a, b git.RepoIdentity) bool { + return !a.IsZero() && !b.IsZero() && a != b +} + +// remoteStatusIsNews decides whether an observation says anything the published one does not. +// +// A CHANGED REVISION is news, and so is the same revision proved again later — that is what makes +// lastVerifiedAt a freshness field rather than decoration. What is deliberately NOT news is +// verifiedBy alone: a revision that changes hands from a Push to a Fetch is the same answer proved +// a second way, and the watch plane has always ignored it for that reason. The two layers agree. +// +// News is not permission to write. It is the first of two tests; publishRemote applies the write +// floor to whatever passes here. +func remoteStatusIsNews(published, next *configbutleraiv1alpha3.GitTargetRemoteStatus) bool { + if published == nil || published.LastVerifiedAt == nil || next.LastVerifiedAt == nil { return true } - if published.LastVerifiedAt == nil || next.LastVerifiedAt == nil { + if published.Revision != next.Revision { return true } - if quantum <= 0 { - quantum = DefaultGitRefreshInterval - } - return next.LastVerifiedAt.Time.Sub(published.LastVerifiedAt.Time) >= quantum + // metav1.Time is second-granular on the wire, so this compares against what was PUBLISHED: a + // sub-second difference the API server would round away is not movement. + return next.LastVerifiedAt.Time.After(published.LastVerifiedAt.Time) } diff --git a/internal/controller/gittarget_remote_test.go b/internal/controller/gittarget_remote_test.go index 6873978a3..9fec52068 100644 --- a/internal/controller/gittarget_remote_test.go +++ b/internal/controller/gittarget_remote_test.go @@ -17,28 +17,42 @@ import ( "github.com/ConfigButler/gitops-reverser/internal/types" ) +// The two repositories a repoint moves between: a recreated GitProvider mints a new UID, and here +// it names a different URL as well. +var ( + firstRepo = git.RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"} + secondRepo = git.RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"} +) + func remoteTestTarget() *configbutleraiv1alpha3.GitTarget { return &configbutleraiv1alpha3.GitTarget{ ObjectMeta: metav1.ObjectMeta{Name: "checkout", Namespace: "shop", Generation: 4}, } } -// TestPublishRemote_AbsentUntilSomethingLooks keeps the two "no revision" cases apart. Nothing -// has looked, so there is no stanza at all — which is not the same as a look that found no -// branch. +func observation(revision string, at time.Time, by git.ObservationSource, repo git.RepoIdentity) git.RemoteObservation { + return git.RemoteObservation{Revision: revision, At: at, By: by, Repo: repo} +} + +// TestPublishRemote_AbsentUntilSomethingLooks keeps the two "no revision" cases apart. Nothing has +// looked, so there is no stanza at all — which is not the same as a look that found no branch. func TestPublishRemote_AbsentUntilSomethingLooks(t *testing.T) { + r := &GitTargetReconciler{} target := remoteTestTarget() - publishRemote(target, git.RemoteObservation{}, false, time.Minute) + + r.publishRemote(target, git.RemoteObservation{}, false, git.RepoIdentity{}, time.Now()) + assert.Nil(t, target.Status.Remote) } // TestPublishRemote_AnObservationWithNoRevisionIsStillPublished is the other half: the branch is // not on the remote, and saying so is the answer rather than the absence of one. func TestPublishRemote_AnObservationWithNoRevisionIsStillPublished(t *testing.T) { + r := &GitTargetReconciler{} target := remoteTestTarget() at := time.Now() - publishRemote(target, git.RemoteObservation{At: at, By: git.ObservedByFetch}, true, time.Minute) + r.publishRemote(target, observation("", at, git.ObservedByFetch, git.RepoIdentity{}), true, git.RepoIdentity{}, at) require.NotNil(t, target.Status.Remote) assert.Empty(t, target.Status.Remote.Revision) @@ -46,132 +60,244 @@ func TestPublishRemote_AnObservationWithNoRevisionIsStillPublished(t *testing.T) assert.Equal(t, at.Unix(), target.Status.Remote.LastVerifiedAt.Unix()) } -// TestPublishRemote_ANewRevisionAlwaysWrites is the rule the field exists for. A revision we -// pushed ourselves is still news: the push is the event that changes where the branch is, and a -// field lagging it by an interval would be a slower copy of the answer rather than a fresh one. -func TestPublishRemote_ANewRevisionAlwaysWrites(t *testing.T) { +// TestPublishRemote_TheFloorIsMeasuredFromTheLastWrite is the rate limit, and the distinction is +// the whole point of keeping a ledger. +// +// Measuring against the PUBLISHED OBSERVATION's timestamp does not limit anything: an observation +// made at 12:00:02 can be written at 12:01:00, and a revision proved at 12:01:02 then looks a full +// minute newer than what is published and is written two seconds after it. +func TestPublishRemote_TheFloorIsMeasuredFromTheLastWrite(t *testing.T) { + r := &GitTargetReconciler{} + target := remoteTestTarget() + start := time.Now() + + // Proved at +2s, written at +60s: a reconcile that happened to be late. + r.publishRemote(target, observation("aaaa", start.Add(2*time.Second), git.ObservedByPush, git.RepoIdentity{}), + true, git.RepoIdentity{}, start.Add(time.Minute)) + require.Equal(t, "aaaa", target.Status.Remote.Revision) + + // Proved at +62s: newer than the published observation by a minute, two seconds after the write. + r.publishRemote(target, observation("bbbb", start.Add(62*time.Second), git.ObservedByPush, git.RepoIdentity{}), + true, git.RepoIdentity{}, start.Add(62*time.Second)) + + assert.Equal(t, "aaaa", target.Status.Remote.Revision, + "two status writes two seconds apart is exactly what the floor exists to prevent") + + // A full interval after the WRITE, it publishes. + r.publishRemote(target, observation("bbbb", start.Add(62*time.Second), git.ObservedByPush, git.RepoIdentity{}), + true, git.RepoIdentity{}, start.Add(2*time.Minute)) + assert.Equal(t, "bbbb", target.Status.Remote.Revision) +} + +// TestPublishRemote_TheFirstObservationIsImmediate. The floor is on the RATE of writes, and the +// first one has no rate: holding it back would leave a target that has just published showing no +// branch at all, which reads as a broken data plane rather than as a sampled field. +func TestPublishRemote_TheFirstObservationIsImmediate(t *testing.T) { + r := &GitTargetReconciler{} target := remoteTestTarget() - first := time.Now() - publishRemote(target, git.RemoteObservation{ - Revision: "aaaa", At: first, By: git.ObservedByPush, - }, true, time.Hour) + at := time.Now() - publishRemote(target, git.RemoteObservation{ - Revision: "bbbb", At: first.Add(time.Second), By: git.ObservedByPush, - }, true, time.Hour) + r.publishRemote( + target, + observation("aaaa", at, git.ObservedByPush, git.RepoIdentity{}), + true, + git.RepoIdentity{}, + at, + ) require.NotNil(t, target.Status.Remote) - assert.Equal(t, "bbbb", target.Status.Remote.Revision, - "a second push a second later moves the revision, whatever the quantizer says") + assert.Equal(t, "aaaa", target.Status.Remote.Revision) } -// TestPublishRemote_AnUnchangedRevisionIsQuantized is the trap sameLayout was written to avoid, -// tested from the other side: a converged target must not write status once per tick merely to -// advance a clock. -func TestPublishRemote_AnUnchangedRevisionIsQuantized(t *testing.T) { +// TestPublishRemote_AConvergedTargetWritesNothing is the trap sameLayout was written to avoid: a +// target whose branch has not moved must not write status once per tick merely to advance a clock, +// however long it has been since the last one. +func TestPublishRemote_AConvergedTargetWritesNothing(t *testing.T) { + r := &GitTargetReconciler{} target := remoteTestTarget() - first := time.Now() - publishRemote(target, git.RemoteObservation{ - Revision: "aaaa", At: first, By: git.ObservedByFetch, - }, true, 10*time.Minute) + at := time.Now() + r.publishRemote( + target, + observation("aaaa", at, git.ObservedByFetch, git.RepoIdentity{}), + true, + git.RepoIdentity{}, + at, + ) published := target.Status.Remote - // One steady tick later, on the same revision. Nothing to say. - publishRemote(target, git.RemoteObservation{ - Revision: "aaaa", At: first.Add(5 * time.Minute), By: git.ObservedByFetch, - }, true, 10*time.Minute) - assert.Same(t, published, target.Status.Remote, - "an unchanged revision inside the interval leaves the published value untouched") - - // A full interval later, the clock is worth a write: an operator reading "verified 40 - // minutes ago" on a target that is being refreshed would be reading a broken refresher. - publishRemote(target, git.RemoteObservation{ - Revision: "aaaa", At: first.Add(10 * time.Minute), By: git.ObservedByFetch, - }, true, 10*time.Minute) - assert.NotSame(t, published, target.Status.Remote) - assert.Equal(t, first.Add(10*time.Minute).Unix(), target.Status.Remote.LastVerifiedAt.Unix()) -} - -// TestPublishRemote_SourceChangeIsNews covers the field that answers "is this revision our own -// work". A branch that moved under us is read off exactly this transition. -func TestPublishRemote_SourceChangeIsNews(t *testing.T) { + r.publishRemote(target, observation("aaaa", at, git.ObservedByFetch, git.RepoIdentity{}), + true, git.RepoIdentity{}, at.Add(time.Hour)) + + assert.Same(t, published, target.Status.Remote) +} + +// TestPublishRemote_AReProvedRevisionRefreshesTheClock. The same revision proved again later IS +// news — that is what makes lastVerifiedAt a freshness field — and like every other kind of news +// it waits for the floor. +func TestPublishRemote_AReProvedRevisionRefreshesTheClock(t *testing.T) { + r := &GitTargetReconciler{} target := remoteTestTarget() at := time.Now() - publishRemote(target, git.RemoteObservation{ - Revision: "aaaa", At: at, By: git.ObservedByPush, - }, true, time.Hour) + r.publishRemote( + target, + observation("aaaa", at, git.ObservedByFetch, git.RepoIdentity{}), + true, + git.RepoIdentity{}, + at, + ) + + r.publishRemote(target, observation("aaaa", at.Add(30*time.Second), git.ObservedByFetch, git.RepoIdentity{}), + true, git.RepoIdentity{}, at.Add(30*time.Second)) + assert.Equal(t, at.Unix(), target.Status.Remote.LastVerifiedAt.Unix(), "inside the floor") + + r.publishRemote(target, observation("aaaa", at.Add(90*time.Second), git.ObservedByFetch, git.RepoIdentity{}), + true, git.RepoIdentity{}, at.Add(90*time.Second)) + assert.Equal(t, at.Add(90*time.Second).Unix(), target.Status.Remote.LastVerifiedAt.Unix(), + "an operator reading 'verified 40 minutes ago' on a target being refreshed would be "+ + "reading a broken refresher") +} - publishRemote(target, git.RemoteObservation{ - Revision: "aaaa", At: at.Add(time.Second), By: git.ObservedByFetch, - }, true, time.Hour) +// TestRemoteStatusIsNews_TheSameAnswerProvedASecondWayIsNot. A revision that changes hands from a +// Push to a Fetch is the same answer proved again; the watch plane has always ignored it for that +// reason, and this layer now agrees. +func TestRemoteStatusIsNews_TheSameAnswerProvedASecondWayIsNot(t *testing.T) { + at := metav1.NewTime(time.Now()) + published := &configbutleraiv1alpha3.GitTargetRemoteStatus{ + Revision: "aaaa", LastVerifiedAt: &at, VerifiedBy: "Push", + } - assert.Equal(t, "Fetch", target.Status.Remote.VerifiedBy) + assert.False(t, remoteStatusIsNews(published, &configbutleraiv1alpha3.GitTargetRemoteStatus{ + Revision: "aaaa", LastVerifiedAt: &at, VerifiedBy: "Fetch", + })) +} + +// TestRemoteStatusIsNews_AMissingTimestampIsAlwaysNews. Neither side can be compared without one, +// and the safe direction is to write: a stanza with no clock is the one an operator cannot read an +// age off at all. +func TestRemoteStatusIsNews_AMissingTimestampIsAlwaysNews(t *testing.T) { + at := metav1.NewTime(time.Now()) + withClock := &configbutleraiv1alpha3.GitTargetRemoteStatus{ + Revision: "aaaa", LastVerifiedAt: &at, VerifiedBy: "Fetch", + } + without := &configbutleraiv1alpha3.GitTargetRemoteStatus{Revision: "aaaa", VerifiedBy: "Fetch"} + + assert.True(t, remoteStatusIsNews(without, withClock)) + assert.True(t, remoteStatusIsNews(withClock, without)) } -// TestPublishRemote_AWithdrawalRemovesTheStanza. A withdrawal says the published revision names a -// commit in a repository this GitTarget no longer points at — its GitProvider was recreated -// against a different URL. There is nothing to replace it with until a look at the new repository -// succeeds, and if that repository is unreachable there never will be, so the stanza goes rather -// than going stale. -func TestPublishRemote_AWithdrawalRemovesTheStanza(t *testing.T) { +// TestPublishRemote_ARevisionFromAnotherRepositoryIsRemoved. The published revision names a commit +// in a repository this GitTarget no longer points at, and there is nothing to replace it with +// until a look at the new one succeeds. If that repository is unreachable there never will be, so +// the stanza goes rather than going stale. +func TestPublishRemote_ARevisionFromAnotherRepositoryIsRemoved(t *testing.T) { + r := &GitTargetReconciler{} target := remoteTestTarget() - publishRemote(target, git.RemoteObservation{ - Revision: "aaaa", At: time.Now(), By: git.ObservedByPush, - }, true, time.Hour) + at := time.Now() + r.publishRemote(target, observation("aaaa", at, git.ObservedByPush, firstRepo), true, firstRepo, at) require.NotNil(t, target.Status.Remote) - publishRemote(target, git.WithdrawnObservation(), true, time.Hour) + r.publishRemote(target, observation("aaaa", at, git.ObservedByPush, firstRepo), true, secondRepo, at) assert.Nil(t, target.Status.Remote, "a revision from a repository this target no longer uses must not be left standing") } -// TestPublishRemote_AWithdrawalIsNotQuantized. The quantizer exists to stop a converged target -// writing status once a tick to advance a clock. A withdrawal carries no clock and is a -// correction, so it must never be held back by it. -func TestPublishRemote_AWithdrawalIsNotQuantized(t *testing.T) { +// TestPublishRemote_ASiblingsObservationCannotHideAStaleRepository is the case a comparison +// against the shared observation alone gets wrong. +// +// One worker serves every GitTarget on a branch, so a sibling can already have proved the +// REPLACEMENT repository. The shared observation then agrees with the GitProvider while this +// target is still publishing a revision from the repository it has left, and the floor would hold +// the correction back on top of that. Invalidation follows what THIS target published. +func TestPublishRemote_ASiblingsObservationCannotHideAStaleRepository(t *testing.T) { + r := &GitTargetReconciler{} target := remoteTestTarget() at := time.Now() - publishRemote(target, git.RemoteObservation{ - Revision: "aaaa", At: at, By: git.ObservedByFetch, - }, true, 10*time.Minute) + r.publishRemote(target, observation("aaaa", at, git.ObservedByPush, firstRepo), true, firstRepo, at) + require.Equal(t, "aaaa", target.Status.Remote.Revision) - publishRemote(target, git.WithdrawnObservation(), true, 10*time.Minute) + // A sibling on the same branch has just proved the new repository; one second later, this + // target reconciles. + r.publishRemote(target, observation("bbbb", at.Add(time.Second), git.ObservedByFetch, secondRepo), + true, secondRepo, at.Add(time.Second)) - assert.Nil(t, target.Status.Remote) + require.NotNil(t, target.Status.Remote) + assert.Equal(t, "bbbb", target.Status.Remote.Revision, + "the repository changed under this target, so the correction does not wait for the floor") } -// TestRemoteStatusIsNews_FallsBackToTheDefaultInterval. The quantum comes from the configured -// refresh interval, and the refresher can be turned off — which must not turn the quantizer into -// "write every tick". A zero falls back to the default rather than to no bound at all. -func TestRemoteStatusIsNews_FallsBackToTheDefaultInterval(t *testing.T) { - at := metav1.NewTime(time.Now()) - published := &configbutleraiv1alpha3.GitTargetRemoteStatus{ - Revision: "aaaa", LastVerifiedAt: &at, VerifiedBy: "Fetch", +// TestPublishRemote_AnUnknownIdentityProvesNothing. The comparison needs both halves. A +// GitProvider that could not be read names no repository, and a worker with no identity of its own +// — the CLI, and tests that never reach a remote — records none. Treating either as a mismatch +// would delete a perfectly good revision every time a provider read blipped. +func TestPublishRemote_AnUnknownIdentityProvesNothing(t *testing.T) { + for _, tc := range []struct { + name string + observed git.RepoIdentity + current git.RepoIdentity + }{ + {"the provider could not be read", firstRepo, git.RepoIdentity{}}, + {"the worker has no identity", git.RepoIdentity{}, firstRepo}, + } { + t.Run(tc.name, func(t *testing.T) { + r := &GitTargetReconciler{} + target := remoteTestTarget() + at := time.Now() + + r.publishRemote(target, observation("aaaa", at, git.ObservedByPush, tc.observed), true, tc.current, at) + + require.NotNil(t, target.Status.Remote) + assert.Equal(t, "aaaa", target.Status.Remote.Revision) + }) } - within := metav1.NewTime(at.Add(DefaultGitRefreshInterval / 2)) - beyond := metav1.NewTime(at.Add(DefaultGitRefreshInterval)) +} - assert.False(t, remoteStatusIsNews(published, &configbutleraiv1alpha3.GitTargetRemoteStatus{ - Revision: "aaaa", LastVerifiedAt: &within, VerifiedBy: "Fetch", - }, 0)) - assert.True(t, remoteStatusIsNews(published, &configbutleraiv1alpha3.GitTargetRemoteStatus{ - Revision: "aaaa", LastVerifiedAt: &beyond, VerifiedBy: "Fetch", - }, 0)) +// TestPublishRemote_TheNewRepositoryRepopulatesTheStanza is the other side of the removal: the +// first look at the repository the target points at NOW publishes again, with no latch to clear +// and no floor to wait for, because the ledger was dropped with the revision it described. +func TestPublishRemote_TheNewRepositoryRepopulatesTheStanza(t *testing.T) { + r := &GitTargetReconciler{} + target := remoteTestTarget() + at := time.Now() + r.publishRemote(target, observation("aaaa", at, git.ObservedByPush, firstRepo), true, secondRepo, at) + require.Nil(t, target.Status.Remote) + + r.publishRemote(target, observation("bbbb", at.Add(time.Second), git.ObservedByFetch, secondRepo), + true, secondRepo, at.Add(time.Second)) + + require.NotNil(t, target.Status.Remote) + assert.Equal(t, "bbbb", target.Status.Remote.Revision) } -// TestRemoteStatusIsNews_AMissingTimestampIsAlwaysNews. Neither side can be compared without one, -// and the safe direction is to write: a stanza with no clock is the one an operator cannot read -// an age off at all. -func TestRemoteStatusIsNews_AMissingTimestampIsAlwaysNews(t *testing.T) { +// TestPublishRemote_ARestartPublishesAgainstTheObservationAlone. The ledger is in memory, so after +// a restart the only evidence about what is published is the observation itself, which names the +// repository it was proved against. +func TestPublishRemote_ARestartPublishesAgainstTheObservationAlone(t *testing.T) { + target := remoteTestTarget() at := metav1.NewTime(time.Now()) - withClock := &configbutleraiv1alpha3.GitTargetRemoteStatus{ - Revision: "aaaa", LastVerifiedAt: &at, VerifiedBy: "Fetch", + target.Status.Remote = &configbutleraiv1alpha3.GitTargetRemoteStatus{ + Revision: "aaaa", LastVerifiedAt: &at, VerifiedBy: "Push", } - without := &configbutleraiv1alpha3.GitTargetRemoteStatus{Revision: "aaaa", VerifiedBy: "Fetch"} + fresh := &GitTargetReconciler{} - assert.True(t, remoteStatusIsNews(without, withClock, time.Hour)) - assert.True(t, remoteStatusIsNews(withClock, without, time.Hour)) + fresh.publishRemote(target, observation("aaaa", at.Time, git.ObservedByPush, firstRepo), true, secondRepo, at.Time) + + assert.Nil(t, target.Status.Remote, "the observation is about a repository this target has left") +} + +// TestObserveRemote_ReadsTheBranchItsGitTargetWrites. The observation is shared by every GitTarget +// on the branch, so it is looked up by (provider namespace, provider, branch) and not by target. +func TestObserveRemote_ReadsTheBranchItsGitTargetWrites(t *testing.T) { + target := remoteTestTarget() + target.Spec.GitProviderRef.Name = "repo1" + target.Spec.Branch = "main" + + _, seen := (&GitTargetReconciler{}).observeRemote(target, "shop") + assert.False(t, seen, "a reconcile with no worker manager has nothing to read") + + workers := git.NewWorkerManager(nil, logr.Discard(), git.BranchWorkerLimits{}, types.SensitiveResourcePolicy{}) + _, seen = (&GitTargetReconciler{WorkerManager: workers}).observeRemote(target, "shop") + assert.False(t, seen, "and neither has one whose branch nothing has looked at") } // TestRequestRemoteRefresh_DoesNothingWithoutADataPlane covers the two ways the trigger is simply @@ -196,24 +322,21 @@ func TestRequestRemoteRefresh_DoesNothingWithoutADataPlane(t *testing.T) { }, "and a target whose branch has no worker yet has nothing to refresh either") } -// TestPublishRemote_TheWithdrawalReachesTheAPIAsADelete. Clearing a Go pointer is only half of +// TestPublishRemote_TheRemovalReachesTheAPIAsADelete. Clearing a Go pointer is only half of // removing a field: the status write is a JSON merge patch computed against the object as read, so -// the fix works only if a field that was there and is now nil is emitted as an explicit null. A -// patch that simply omitted it would leave the old repository's revision in etcd forever. -func TestPublishRemote_TheWithdrawalReachesTheAPIAsADelete(t *testing.T) { +// the fix works only if a field that was there and is now nil is emitted as an explicit null. +func TestPublishRemote_TheRemovalReachesTheAPIAsADelete(t *testing.T) { + r := &GitTargetReconciler{} target := remoteTestTarget() - // A condition, so the patch is computed against a status that outlives the stanza being - // removed: the whole point is that `remote` alone is deleted. target.Status.Conditions = []metav1.Condition{{ Type: "Ready", Status: metav1.ConditionTrue, Reason: "Ready", LastTransitionTime: metav1.NewTime(time.Now()), ObservedGeneration: 4, }} - publishRemote(target, git.RemoteObservation{ - Revision: "aaaa", At: time.Now(), By: git.ObservedByPush, - }, true, time.Hour) + at := time.Now() + r.publishRemote(target, observation("aaaa", at, git.ObservedByPush, firstRepo), true, firstRepo, at) before := target.DeepCopy() - publishRemote(target, git.WithdrawnObservation(), true, time.Hour) + r.publishRemote(target, observation("aaaa", at, git.ObservedByPush, firstRepo), true, secondRepo, at) data, err := client.MergeFrom(before).Data(target) require.NoError(t, err) diff --git a/internal/controller/gittarget_source_cluster.go b/internal/controller/gittarget_source_cluster.go index 26633ec2a..67601786a 100644 --- a/internal/controller/gittarget_source_cluster.go +++ b/internal/controller/gittarget_source_cluster.go @@ -152,15 +152,17 @@ func describeKubeConfigKey(specKey string) string { return specKey } -// gitProviderReadiness reads the referenced GitProvider's Ready condition and projects it. It -// runs only after the Validated gate has confirmed the provider exists. It returns Unknown — -// which does NOT downgrade Ready — when the provider's readiness cannot be observed (a transient -// read error, or a provider that has not reported a Ready condition yet), so a not-yet-reconciled -// provider never blocks its GitTarget; only an EXPLICIT Ready=False downgrades. -func (r *GitTargetReconciler) gitProviderReadiness( - ctx context.Context, +// gitProviderReadiness projects the referenced GitProvider's Ready condition. It takes the +// provider the reconcile already read — the same object the Validated gate and the branch worker's +// identity came from, so the three cannot describe different objects — and runs only after that +// gate has confirmed it exists. It returns Unknown — which does NOT downgrade Ready — when +// readiness cannot be observed (no provider in hand, or one that has not reported a Ready +// condition yet), so a not-yet-reconciled provider never blocks its GitTarget; only an EXPLICIT +// Ready=False downgrades. +func gitProviderReadiness( target *configbutleraiv1alpha3.GitTarget, providerNS string, + gp *configbutleraiv1alpha3.GitProvider, ) conditionValue { key := k8stypes.NamespacedName{Name: target.Spec.GitProviderRef.Name, Namespace: providerNS} notReady := func(status metav1.ConditionStatus, format string, args ...any) conditionValue { @@ -171,9 +173,8 @@ func (r *GitTargetReconciler) gitProviderReadiness( } } - var gp configbutleraiv1alpha3.GitProvider - if err := r.Get(ctx, key, &gp); err != nil { - return notReady(metav1.ConditionUnknown, "referenced GitProvider %s readiness not observed: %v", key, err) + if gp == nil { + return notReady(metav1.ConditionUnknown, "referenced GitProvider %s readiness not observed", key) } c := findCondition(gp.Status.Conditions, ConditionTypeReady) switch { diff --git a/internal/controller/gittarget_source_cluster_test.go b/internal/controller/gittarget_source_cluster_test.go index cfead478e..1431d9d0f 100644 --- a/internal/controller/gittarget_source_cluster_test.go +++ b/internal/controller/gittarget_source_cluster_test.go @@ -477,18 +477,13 @@ func TestGitProviderReadiness_AllScenarios(t *testing.T) { } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - builder := fake.NewClientBuilder().WithScheme(scScheme(t)) - if tc.gp != nil { - builder = builder.WithObjects(tc.gp) - } - r := &GitTargetReconciler{Client: builder.Build()} target := &configbutleraiv1alpha3.GitTarget{ ObjectMeta: metav1.ObjectMeta{Name: "gt", Namespace: "team-a"}, Spec: configbutleraiv1alpha3.GitTargetSpec{ GitProviderRef: meta.LocalObjectReference{Name: "prov"}, }, } - got := r.gitProviderReadiness(context.Background(), target, "team-a") + got := gitProviderReadiness(target, "team-a", tc.gp) assert.Equal(t, tc.want, got.Status) assert.NotEmpty(t, got.Message) }) diff --git a/internal/controller/gittarget_worker_cleanup_test.go b/internal/controller/gittarget_worker_cleanup_test.go index 1c36ed12d..a354e6b86 100644 --- a/internal/controller/gittarget_worker_cleanup_test.go +++ b/internal/controller/gittarget_worker_cleanup_test.go @@ -44,7 +44,10 @@ func TestCleanupDeletedGitTarget_StopsAWorkerNothingNeedsAnyMore(t *testing.T) { go func() { _ = workers.Start(ctx) }() time.Sleep(100 * time.Millisecond) - require.NoError(t, workers.EnsureWorker(ctx, "repo1", "shop", "main")) + func() { + err := workers.EnsureWorker(ctx, "repo1", "shop", "main", git.RepoIdentity{URL: "file:///repo1.git"}) + require.NoError(t, err) + }() _, exists := workers.GetWorkerForTarget("repo1", "shop", "main") require.True(t, exists) @@ -77,7 +80,10 @@ func TestCleanupDeletedGitTarget_LeavesAWorkerAnotherGitTargetStillUses(t *testi defer cancel() go func() { _ = workers.Start(ctx) }() time.Sleep(100 * time.Millisecond) - require.NoError(t, workers.EnsureWorker(ctx, "repo1", "shop", "main")) + func() { + err := workers.EnsureWorker(ctx, "repo1", "shop", "main", git.RepoIdentity{URL: "file:///repo1.git"}) + require.NoError(t, err) + }() r := &GitTargetReconciler{Client: k8sClient, WorkerManager: workers} _ = r.cleanupDeletedGitTarget(ctx, @@ -118,7 +124,10 @@ func TestCleanupDeletedGitTarget_RetriesWhenTheSweepCannotRead(t *testing.T) { defer cancel() go func() { _ = workers.Start(ctx) }() time.Sleep(100 * time.Millisecond) - require.NoError(t, workers.EnsureWorker(ctx, "repo1", "shop", "main")) + func() { + err := workers.EnsureWorker(ctx, "repo1", "shop", "main", git.RepoIdentity{URL: "file:///repo1.git"}) + require.NoError(t, err) + }() r := &GitTargetReconciler{Client: k8sClient, WorkerManager: workers} err := r.cleanupDeletedGitTarget(ctx, @@ -148,7 +157,10 @@ func TestHandleFetchError_RequeuesAFailedCleanup(t *testing.T) { defer cancel() go func() { _ = workers.Start(ctx) }() time.Sleep(100 * time.Millisecond) - require.NoError(t, workers.EnsureWorker(ctx, "repo1", "shop", "main")) + func() { + err := workers.EnsureWorker(ctx, "repo1", "shop", "main", git.RepoIdentity{URL: "file:///repo1.git"}) + require.NoError(t, err) + }() r := &GitTargetReconciler{Client: k8sClient, WorkerManager: workers} _, err := r.handleFetchError(ctx, apierrors.NewNotFound( diff --git a/internal/controller/gittarget_worker_repoint_test.go b/internal/controller/gittarget_worker_repoint_test.go new file mode 100644 index 000000000..a0144f6db --- /dev/null +++ b/internal/controller/gittarget_worker_repoint_test.go @@ -0,0 +1,217 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/fluxcd/pkg/apis/meta" + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + k8stypes "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/git" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" + "github.com/ConfigButler/gitops-reverser/internal/watch" +) + +// TestEnsureEventStream_RepointsTheStreamAtTheReplacementWorker. +// +// A GitTargetEventStream holds its branch worker directly, and the reconcile used to return the +// registered stream without looking at which worker that was. A repointed GitProvider replaces the +// worker, so a stream left alone would go on handing live events to a stopped goroutine's queue: +// accepted, never written, and silently dropped once the queue filled. +func TestEnsureEventStream_RepointsTheStreamAtTheReplacementWorker(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, configbutleraiv1alpha3.AddToScheme(scheme)) + k8sClient := fake.NewClientBuilder().WithScheme(scheme).Build() + + workers := git.NewWorkerManager(k8sClient, logr.Discard(), git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + go func() { _ = workers.Start(ctx) }() + time.Sleep(100 * time.Millisecond) + + router := watch.NewEventRouter(workers, nil, k8sClient, logr.Discard()) + r := &GitTargetReconciler{Client: k8sClient, WorkerManager: workers, EventRouter: router} + + target := &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "apps", Namespace: "shop"}, + Spec: configbutleraiv1alpha3.GitTargetSpec{ + GitProviderRef: meta.LocalObjectReference{Name: "repo1"}, + Branch: "main", + Path: "apps", + }, + } + before := git.RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"} + after := git.RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"} + + first, err := r.ensureEventStream(ctx, target, "shop", before, logr.Discard()) + require.NoError(t, err) + + steady, err := r.ensureEventStream(ctx, target, "shop", before, logr.Discard()) + require.NoError(t, err) + assert.Same(t, first, steady, "a steady tick must not re-register anything") + + repointed, err := r.ensureEventStream(ctx, target, "shop", after, logr.Discard()) + require.NoError(t, err) + + assert.NotSame(t, first, repointed, "the route has to reach the worker for the repository named NOW") + worker, ok := workers.GetWorkerForTarget("repo1", "shop", "main") + require.True(t, ok) + assert.True(t, repointed.ServesWorker(worker)) + assert.Same(t, repointed, router.GetGitTargetEventStream(types.NewResourceReference("apps", "shop")), + "and it is the registered stream, not a copy the reconcile kept to itself") +} + +// TestRecoverBranchAfterReplacement_ReachesEverySiblingOnTheBranch. +// +// A replacement throws away the old worker's queue and its retained writes, which is only safe +// because the folder is rebuilt from the cluster. Nothing asks for that rebuild on its own: the +// declaration is level-triggered, its inputs did not change, so the next pass is a no-op and an +// idle target would leave the new repository empty. +// +// One worker serves every GitTarget on its (provider, branch), and the siblings have no reason of +// their own to reconcile, so recovering only the target whose reconcile noticed leaves the rest +// pointing at a repository nothing will repopulate. Targets on another branch, and on another +// provider, must be left alone: forcing a replay is a full cluster snapshot per target. +func TestRecoverBranchAfterReplacement_ReachesEverySiblingOnTheBranch(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, configbutleraiv1alpha3.AddToScheme(scheme)) + + target := func(name, provider, branch string) *configbutleraiv1alpha3.GitTarget { + return &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "shop", UID: k8stypes.UID("uid-" + name)}, + Spec: configbutleraiv1alpha3.GitTargetSpec{ + GitProviderRef: meta.LocalObjectReference{Name: provider}, + Branch: branch, + Path: name, + }, + } + } + k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects( + target("apps", "repo1", "main"), + target("infra", "repo1", "main"), // sibling: same worker, must be recovered + target("release", "repo1", "rel"), // same provider, another branch: untouched + target("other", "repo2", "main"), // another provider: untouched + ).Build() + + manager := &watch.Manager{} + // Only a declared target can be re-anchored; an undeclared one replays on its first pass + // anyway, so the recovery has nothing to ask of it. + for _, name := range []string{"apps", "infra", "release", "other"} { + manager.DeclareForGitTarget( + types.NewResourceReference(name, "shop").WithUID("uid-"+name), "default", "", "") + } + workers := git.NewWorkerManager(k8sClient, logr.Discard(), git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}) + r := &GitTargetReconciler{ + Client: k8sClient, + WorkerManager: workers, + EventRouter: watch.NewEventRouter(workers, manager, k8sClient, logr.Discard()), + } + // A verdict proved against the repository that is about to stop being the destination. + appsRef := types.NewResourceReference("apps", "shop") + workers.RenderFidelityGate().Fail(appsRef, manifestanalyzer.RenderDivergence{}) + require.False(t, workers.RenderFidelityGate().AllowsWrites(appsRef)) + + branch := git.BranchKey{RepoNamespace: "shop", RepoName: "repo1", Branch: "main"} + require.True(t, r.recoverBranchAfterReplacement(context.Background(), branch, logr.Discard()), + "the targets were listed and told, so the recovery is delivered") + + assert.ElementsMatch(t, []string{"shop/apps", "shop/infra"}, manager.ForcedRecheckTargetsForTest(), + "every GitTarget the replaced worker served, and nothing else") + assert.Equal(t, git.RenderFidelityUnknown, workers.RenderFidelityGate().Status(appsRef).State, + "a verdict about the other repository must not survive into this one") + assert.False(t, workers.RenderFidelityGate().AllowsWrites(appsRef), + "and nothing may be written to the new one until something has measured it") +} + +// TestEnsureEventStream_ARecoveryThatCouldNotBeDeliveredIsRetried. +// +// The recovery is delivered by LISTING the GitTargets on the branch, and a list can fail. When the +// replacement was a one-shot return value, that failure spent it: every later call reported no +// replacement, so neither an ordinary reconcile nor the orphan sweep ever forced the missing +// replay, and an idle target left the new repository empty indefinitely. +// +// The pending recovery now lives on the worker manager, so the next reconcile of any GitTarget on +// the branch picks it up — which is what this asserts, with the first delivery failing and the +// second succeeding. +func TestEnsureEventStream_ARecoveryThatCouldNotBeDeliveredIsRetried(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, configbutleraiv1alpha3.AddToScheme(scheme)) + + target := &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "apps", Namespace: "shop", UID: k8stypes.UID("uid-apps")}, + Spec: configbutleraiv1alpha3.GitTargetSpec{ + GitProviderRef: meta.LocalObjectReference{Name: "repo1"}, + Branch: "main", + Path: "apps", + }, + } + listFails := true + k8sClient := interceptor.NewClient( + fake.NewClientBuilder().WithScheme(scheme).WithObjects(target).Build(), + interceptor.Funcs{ + List: func(ctx context.Context, c client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + if listFails { + return errors.New("the API server is unreachable") + } + return c.List(ctx, list, opts...) + }, + }) + + workers := git.NewWorkerManager(k8sClient, logr.Discard(), git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + go func() { _ = workers.Start(ctx) }() + time.Sleep(100 * time.Millisecond) + + manager := &watch.Manager{} + manager.DeclareForGitTarget( + types.NewResourceReference("apps", "shop").WithUID("uid-apps"), "default", "", "") + r := &GitTargetReconciler{ + Client: k8sClient, + WorkerManager: workers, + EventRouter: watch.NewEventRouter(workers, manager, k8sClient, logr.Discard()), + } + branch := git.BranchKey{RepoNamespace: "shop", RepoName: "repo1", Branch: "main"} + before := git.RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"} + after := git.RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"} + + _, err := r.ensureEventStream(ctx, target, "shop", before, logr.Discard()) + require.NoError(t, err) + + // The repoint. Its recovery cannot be delivered, and the reconcile does not fail for it. + _, err = r.ensureEventStream(ctx, target, "shop", after, logr.Discard()) + require.NoError(t, err) + require.Empty(t, manager.ForcedRecheckTargetsForTest(), "precondition: nothing was told") + assert.True(t, workers.ReplacementPending(branch), + "a recovery that reached nobody is not finished") + + // The next reconcile — this target's own, or a sibling's — delivers it. + listFails = false + _, err = r.ensureEventStream(ctx, target, "shop", after, logr.Discard()) + require.NoError(t, err) + + assert.Equal(t, []string{"shop/apps"}, manager.ForcedRecheckTargetsForTest(), + "the replay the replacement owes is forced by a later pass, not lost with the first") + assert.False(t, workers.ReplacementPending(branch), "and it is not asked for again") +} diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index 72eeef2d9..88bce9753 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -64,6 +64,15 @@ type BranchWorker struct { GitProviderNamespace string Branch string + // repo is the repository this worker is ABOUT: its clone, its base trust, its observations + // and its retained writes all describe that one repository and no other. + // + // It is immutable for the same reason the three fields above are. The worker does not follow + // its GitProvider to a new repository — meeting one is not a state change to be undone field + // by field, it is a different subject — so the WorkerManager replaces the worker instead. + // See RepoIdentity and WorkerManager.EnsureWorker. + repo RepoIdentity + // Dependencies Client client.Client Log logr.Logger @@ -109,9 +118,10 @@ type BranchWorker struct { // alongside pathRefusal; a nil reporter only drops the projection. layoutReporter LayoutReporter - // remoteReporter publishes each confirmed observation of the branch's remote state, - // projected as status.remote. Set by the WorkerManager before Start, alongside pathRefusal; - // a nil reporter only drops the projection. + // remoteReporter delivers each confirmed observation of the branch's remote state to the + // worker manager, which keeps it for every GitTarget on the branch. Set by the WorkerManager + // before Start, alongside pathRefusal; nil in the CLI and in tests, where an observation is + // simply not shared. remoteReporter RemoteReporter // Event processing @@ -122,21 +132,32 @@ type BranchWorker struct { started bool mu sync.Mutex + // stoppingState closes the queue to new work from the moment Stop is called. + // + // Shutdown drains whatever is still on the FIFO without processing it, so an item accepted + // after that point is silently thrown away, while Enqueue's "true" has already told a watch + // loop it may advance its durable cursor past the event. That cost nothing while Stop was + // only ever called at process exit. A worker REPLACED because its GitProvider names a + // different repository is stopped with the process still running, and the sibling GitTargets + // on its branch go on holding streams that point at it until their own next reconcile, which + // is minutes of accepted-and-discarded events. + // + // Refusing them turns a silent loss into the drop path every producer already handles: the + // watch leaves its cursor where it is and redelivers to the replacement. Items already ON the + // queue when Stop is called are still drained, which is the behaviour shutdown always had. + // + // It is written and read under pendingResyncsMu, the lock every enqueue already holds across + // its send. Checking it outside that lock is not enough: an enqueue could pass the check, + // block on the lock, and send after Stop had finished draining, into a queue with no loop left + // to read it — and still answer "accepted". + stoppingState bool + // lastObservation is what this worker last PROVED about the target branch on the remote, and // when. See RemoteObservation: it replaces a fetch-only trio (branchExists, lastCommitSHA, // lastFetchTime) that went arbitrarily stale on exactly the busiest targets, because a push // never wrote to it. lastObservation atomic.Pointer[RemoteObservation] - // observationWithdrawnState records that the observation was dropped because the GitProvider - // now names a DIFFERENT repository, and that whatever was published from it has to be taken - // back. Dropping the record alone is not enough: the revision an operator reads lives in the - // watch plane's projection and in status.remote, and if the new repository is unreachable - // nothing would ever replace it. It is cleared by the next observation that is actually - // proved, and it stays set until then so every target on this branch withdraws on its own - // tick, not only the one that happened to notice the change. - observationWithdrawnState atomic.Bool - // repoMu serializes repository/worktree operations within this worker. repoMu sync.Mutex @@ -185,17 +206,6 @@ type BranchWorker struct { // See docs/design/push-notification-and-reconcile-trigger.md §1.5. replayRequiredState atomic.Bool - // trustedRemote is the repository the flags above are ABOUT: the remote URL the worker last - // planned a cycle against. - // - // Trust is a claim concerning one checkout, and each remote gets its own on-disk clone - // (repoPathForRemote), so a claim carried across a change of destination is not merely stale — - // it points at a directory that may not exist. Workers are keyed by (GitProvider namespace, - // GitProvider name, branch) and never by URL, while spec.url is immutable and repointed by - // deleting and recreating the GitProvider, so a live worker really does meet a new repository - // without anything restarting it. See noteRemoteIdentity. - trustedRemote atomic.Pointer[string] - // branchBufferMaxBytes caps the retained in-memory event data; tripped on // event arrival, an immediate finalize bypasses the commit window. branchBufferMaxBytes int64 @@ -301,6 +311,7 @@ func NewBranchWorker( log logr.Logger, providerName, providerNamespace string, branch string, + repo RepoIdentity, writer *contentWriter, limits BranchWorkerLimits, ) *BranchWorker { @@ -315,6 +326,7 @@ func NewBranchWorker( GitProviderRef: providerName, GitProviderNamespace: providerNamespace, Branch: branch, + repo: repo, Client: client, Log: log.WithValues( "provider", providerName, @@ -388,6 +400,17 @@ func (w *BranchWorker) repoPathForRemote(remoteURL string) string { return filepath.Join(w.repoRootPath(), repoCacheKey(remoteURL)) } +// repoPath is the checkout of the repository this worker is ABOUT. +// +// The URL comes from the worker's own identity and never from the GitProvider as it reads NOW. The +// provider is still read on every cycle — for credentials, commit identity and signing — and it +// can name a different repository by then, because it is repointed by being recreated while the +// worker is keyed by (provider, branch). Following it there is what produced the outage in #382: +// the cycle planned against trust earned on the old repository and every write failed at +// `open repository: repository does not exist`. A worker that cannot be pointed anywhere cannot +// have that happen to it; the WorkerManager replaces it instead. +func (w *BranchWorker) repoPath() string { return w.repoPathForRemote(w.repo.URL) } + // removeLocalState deletes everything this worker kept on disk: the clone of every remote it // worked with, under its own (provider namespace, provider, branch) directory. // @@ -446,6 +469,12 @@ func (w *BranchWorker) Stop() { } w.mu.Unlock() + // Before the cancel, and under the lock every enqueue holds across its send, so admission + // closes at one instant: an enqueue either completed before this point or is refused. + w.pendingResyncsMu.Lock() + w.stoppingState = true + w.pendingResyncsMu.Unlock() + w.Log.Info("Stopping branch worker") w.cancelFunc() w.wg.Wait() @@ -478,6 +507,7 @@ func (w *BranchWorker) EnqueueAttach(req *AttachCommitRequest) { if req == nil { return } + // Increment before the send so inflightItems can never lag the loop's // receive; roll back if the queue is full and the item is dropped. w.inflightItems.Add(1) @@ -486,6 +516,13 @@ func (w *BranchWorker) EnqueueAttach(req *AttachCommitRequest) { // rare, so blocking a coalesce on them costs no storm protection. The mark // and the send are ONE critical section: see enqueueRequest. w.pendingResyncsMu.Lock() + if w.stoppingLocked() { + w.pendingResyncsMu.Unlock() + w.inflightItems.Add(-1) + w.recordQueueDrop(queueDropAttach) + w.Log.V(1).Info("Worker is stopping, CommitRequest attach refused (the controller re-sends)") + return + } w.markResyncTailForTargetLocked(req.GitTargetNamespace, req.GitTargetName) select { case w.eventQueue <- WorkItem{Attach: req}: @@ -523,6 +560,16 @@ func (w *BranchWorker) EnqueueResync(request *ResyncRequest) bool { key := resyncKeyFor(request) w.pendingResyncsMu.Lock() + if w.stoppingLocked() { + w.pendingResyncsMu.Unlock() + w.recordQueueDrop(queueDropResync) + w.Log.V(1).Info("Worker is stopping, resync request refused", + "gitTarget", request.GitTargetNamespace+"/"+request.GitTargetName) + // Answered rather than dropped in silence: the caller is waiting on this channel, and the + // shutdown drain does not answer what it throws away. + request.reply(ResyncResult{Err: ErrFinalizeQueueFull}) + return false + } if w.pendingResyncs == nil { w.pendingResyncs = make(map[resyncKey]*pendingResync) } @@ -677,6 +724,15 @@ func (w *BranchWorker) enqueueRequest(request *WriteRequest) bool { // non-blocking, so holding the lock across them cannot deadlock. Marking a // write the full queue then drops only forgoes a coalesce, the safe direction. w.pendingResyncsMu.Lock() + if w.stoppingLocked() { + w.pendingResyncsMu.Unlock() + w.inflightItems.Add(-1) + w.recordQueueDrop(queueDropWrite) + w.Log.V(1).Info("Worker is stopping, request refused; the producer keeps it to redeliver", + "events", len(request.Events), + "gitTarget", request.GitTargetName) + return false + } w.markResyncTailForWriteLocked(request) select { case w.eventQueue <- item: @@ -822,12 +878,9 @@ func (w *BranchWorker) prepareBootstrapRepository( return "", fmt.Errorf("failed to get auth: %w", err) } - repoPath := w.repoPathForRemote(provider.Spec.URL) - // This fetch establishes a checkout for whatever the provider names now, so record which - // repository that is — see noteRemoteIdentity. - w.noteRemoteIdentity(provider.Spec.URL) + repoPath := w.repoPath() w.recordFetch(fetchReasonBootstrap) - pullReport, err := PrepareBranch(ctx, provider.Spec.URL, repoPath, w.Branch, auth) + pullReport, err := PrepareBranch(ctx, w.repo.URL, repoPath, w.Branch, auth) if err != nil { return "", fmt.Errorf("failed to prepare repository: %w", err) } @@ -1664,44 +1717,13 @@ func (w *BranchWorker) markReplayRequired() { } } +// stoppingLocked reports that Stop has begun, so nothing new may enter the queue. The caller must +// hold pendingResyncsMu and must still be holding it when it sends: see stoppingState. +func (w *BranchWorker) stoppingLocked() bool { return w.stoppingState } + // markReplayComplete is called only when a rebuild has replayed every retained write. func (w *BranchWorker) markReplayComplete() { w.replayRequiredState.Store(false) } -// noteRemoteIdentity binds the base trust to the repository it was gained against, and drops it -// when the GitProvider now names a different one. -// -// `spec.url` is immutable and repointed by deleting the GitProvider and creating it again (see -// GitProviderSpec). That does not restart the worker: workers are keyed by (GitProvider namespace, -// GitProvider name, branch), and the GitTarget that owns this one is untouched. So the same worker -// meets the new repository still holding the trust its last push to the OLD one established. -// -// Without this, the head-of-cycle guard believes that trust, skips the PrepareBranch that would -// have initialised the new clone — each remote has its own, keyed by URL — and every write from -// then on fails at `open repository: repository does not exist`, dropping live windows for as long -// as the worker lives. -// -// It runs before the guard rather than inside it, so a worker holding retained writes (which skips -// the guard entirely, because a reset would destroy the local commits those writes produced) still -// records the change instead of carrying the old trust into the cycle after the retained work -// clears. -func (w *BranchWorker) noteRemoteIdentity(remoteURL string) { - key := repoCacheKey(remoteURL) - previous := w.trustedRemote.Swap(&key) - if previous == nil || *previous == key { - return - } - w.Log.Info("GitProvider now names a different repository; the checkout for it must be established", - "branch", w.Branch) - w.invalidateBase("remote repository changed") - // The observation goes with the trust, and for a sharper reason: it is a statement about a - // REPOSITORY, and this is a different one. Left in place it would be reported as this - // GitTarget's remote state, and a fresh enough one would also suppress the look that corrects - // it. Dropping it makes the next refresh unconditional; the withdrawal takes back what the - // old one already published. - w.lastObservation.Store(nil) - w.observationWithdrawnState.Store(true) -} - // ensureBaseForCycle performs the head-of-cycle fetch, which is now conditional. // // The push session reads the remote's ref advertisement on a connection the cycle was making @@ -1717,10 +1739,6 @@ func (w *BranchWorker) ensureBaseForCycle( repoPath string, hasPendingCommits bool, ) error { - // Before the guard, not inside it: a repointed GitProvider must invalidate even on a cycle - // this guard is going to skip. - w.noteRemoteIdentity(provider.Spec.URL) - if hasPendingCommits || (w.baseTrusted() && !w.worktreeDirty()) { return nil } @@ -1742,7 +1760,7 @@ func (w *BranchWorker) ensureBaseForCycle( } w.recordFetch(reason) - pullReport, err := PrepareBranch(w.ctx, provider.Spec.URL, repoPath, w.Branch, auth) + pullReport, err := PrepareBranch(w.ctx, w.repo.URL, repoPath, w.Branch, auth) if err != nil { return fmt.Errorf("prepare repository: %w", err) } @@ -1769,7 +1787,7 @@ func (w *BranchWorker) commitPendingWrites(pendingWrites []PendingWrite, hasPend return fmt.Errorf("get GitProvider: %w", err) } - repoPath := w.repoPathForRemote(provider.Spec.URL) + repoPath := w.repoPath() if err := w.ensureBaseForCycle(provider, repoPath, hasPendingCommits); err != nil { return err } @@ -1853,7 +1871,7 @@ func (w *BranchWorker) runPushCycle(pendingWrites []PendingWrite) error { return fmt.Errorf("resolve auth: %w", err) } - repoPath := w.repoPathForRemote(provider.Spec.URL) + repoPath := w.repoPath() repo, err := gogit.PlainOpen(repoPath) if err != nil { return fmt.Errorf("open repository: %w", err) @@ -1888,10 +1906,7 @@ func (w *BranchWorker) runPushCycle(pendingWrites []PendingWrite) error { if !outcome.Head.IsZero() { revision = outcome.Head.String() } - observed := w.recordRemoteObservation(revision, ObservedByPush) - // Reported against the targets these writes were for, at no round trip: the - // connection has already been made. - w.reportRemoteObservation(pendingWriteTargets(pendingWrites), observed) + w.recordRemoteObservation(revision, ObservedByPush) w.Log.V(1).Info("Remote observed by push", "branch", w.Branch, "outcome", string(outcome.Kind), "head", revision) w.pushCycleRootBranch = "" @@ -1899,7 +1914,7 @@ func (w *BranchWorker) runPushCycle(pendingWrites []PendingWrite) error { w.firsts.push.Do(func() { w.Log.Info("First push to remote completed", "branch", w.Branch, - "url", provider.Spec.URL, + "url", w.repo.URL, "commits", len(pendingWrites)) }) return nil @@ -2051,7 +2066,7 @@ func (w *BranchWorker) refreshRemoteAndRebuildPendingWrites( return fmt.Errorf("resolve auth: %w", err) } - repoPath := w.repoPathForRemote(provider.Spec.URL) + repoPath := w.repoPath() repo, err := gogit.PlainOpen(repoPath) if err != nil { return fmt.Errorf("open repository: %w", err) @@ -2422,6 +2437,24 @@ func (w *BranchWorker) getGitProvider(ctx context.Context) (*configv1alpha3.GitP return nil, fmt.Errorf("failed to fetch GitProvider: %w", err) } + // The worker writes to ITS repository and reads this object for the credentials, commit + // identity and signing to write it with. Those two must describe the same repository, or the + // worker would present a credential issued for one remote to another: a GitProvider recreated + // against a different URL is normally replaced at the next reconcile, but a replacement that + // cannot pass the Validated gate never arrives, and the old worker would otherwise go on + // mirroring into a repository the operator has abandoned, using the new one's credential. + // + // So a mismatch is fatal to the cycle rather than something to work around. The worker stops + // acting and says why; the reconcile that replaces it is what resolves it. A worker with no + // identity of its own (the CLI, and tests that never reach a remote) has nothing to compare. + if !w.repo.IsZero() { + if found := (RepoIdentity{ProviderUID: provider.UID, URL: provider.Spec.URL}); found != w.repo { + return nil, fmt.Errorf( + "GitProvider %s/%s now names %s; this worker is for %s and is waiting to be replaced", + w.GitProviderNamespace, w.GitProviderRef, found, w.repo) + } + } + return &provider, nil } @@ -2472,19 +2505,20 @@ func (w *BranchWorker) LastRemoteObservation() (RemoteObservation, bool) { // accepted — because both prove the same kind of fact. An error path must NOT call it: a push // that died mid-upload or an advertisement that never arrived observed nothing, and recording a // guess there is how a stale revision reaches status. -func (w *BranchWorker) recordRemoteObservation(revision string, by ObservationSource) RemoteObservation { - observed := RemoteObservation{Revision: revision, At: time.Now(), By: by} +func (w *BranchWorker) recordRemoteObservation(revision string, by ObservationSource) { + // Stamped with the repository it is about, so that whoever publishes it can tell whether it + // still describes the repository the GitTarget points at. See RemoteObservation.Repo. + observed := RemoteObservation{Revision: revision, At: time.Now(), By: by, Repo: w.repo} w.lastObservation.Store(&observed) - // Something has now been proved about the repository the worker points at, so there is - // nothing left to take back. - w.observationWithdrawnState.Store(false) - return observed + // Delivered here, at the one point where an observation is made, rather than by each caller + // against whichever GitTargets it happened to be holding. The fact is about the BRANCH, so it + // goes to the branch, and every target on it reads the same answer with the time it was + // actually proved. + if w.remoteReporter != nil { + w.remoteReporter(observed) + } } -// observationWithdrawn reports that the last observation was about a repository this worker no -// longer points at, so anything published from it must be withdrawn rather than left standing. -func (w *BranchWorker) observationWithdrawn() bool { return w.observationWithdrawnState.Load() } - // syncWithRemote fetches latest changes from remote and resets onto them. It is the // no-retained-writes half of a refresh; resync_flush.go and invalidateAndRefresh are its callers. // @@ -2505,15 +2539,14 @@ func (w *BranchWorker) syncWithRemote(ctx context.Context, reason string) error return fmt.Errorf("failed to get auth: %w", err) } - repoPath := w.repoPathForRemote(provider.Spec.URL) + repoPath := w.repoPath() // Somebody stated that the remote moved: see refreshRemoteAndRebuildPendingWrites. - w.noteRemoteIdentity(provider.Spec.URL) w.invalidateBase(reason) // PrepareBranch handles both initial and update cases w.recordFetch(reason) - report, err := PrepareBranch(ctx, provider.Spec.URL, repoPath, w.Branch, auth) + report, err := PrepareBranch(ctx, w.repo.URL, repoPath, w.Branch, auth) if err != nil { return fmt.Errorf("failed to sync with remote: %w", err) } @@ -2537,7 +2570,7 @@ func (w *BranchWorker) ensureRepositoryInitialized(ctx context.Context) error { return fmt.Errorf("failed to get GitProvider: %w", err) } - repoPath := w.repoPathForRemote(provider.Spec.URL) + repoPath := w.repoPath() if w.shouldReadRetainedLocalRepository(repoPath) { return nil } @@ -2548,7 +2581,7 @@ func (w *BranchWorker) ensureRepositoryInitialized(ctx context.Context) error { } // Use new PrepareBranch abstraction - pullReport, err := PrepareBranch(ctx, provider.Spec.URL, repoPath, w.Branch, auth) + pullReport, err := PrepareBranch(ctx, w.repo.URL, repoPath, w.Branch, auth) if err != nil { return fmt.Errorf("failed to prepare repository: %w", err) } @@ -2628,7 +2661,7 @@ func buildBootstrapOptions(encryptionConfig *ResolvedEncryptionConfig) pathBoots // calls it. func (w *BranchWorker) SeedRemoteObservationForTest(revision string, at time.Time, by string) { w.baseTrustedState.Store(true) - observed := RemoteObservation{Revision: revision, At: at, By: ObservationSource(by)} + observed := RemoteObservation{Revision: revision, At: at, By: ObservationSource(by), Repo: w.repo} w.lastObservation.Store(&observed) } diff --git a/internal/git/branch_worker_credread_test.go b/internal/git/branch_worker_credread_test.go index bbdd19335..2c3effbd3 100644 --- a/internal/git/branch_worker_credread_test.go +++ b/internal/git/branch_worker_credread_test.go @@ -73,7 +73,16 @@ func TestCommitPendingWrites_ResolvesCredentialsOncePerPushCycle(t *testing.T) { }). Build() - worker := NewBranchWorker(countingClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + countingClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: "file://" + remotePath}, + nil, + BranchWorkerLimits{}, + ) worker.ctx = context.Background() // First commit of the cycle: fetches the remote tip, so it resolves credentials exactly once. diff --git a/internal/git/branch_worker_loop_test.go b/internal/git/branch_worker_loop_test.go index a12f51ca4..f7259b5de 100644 --- a/internal/git/branch_worker_loop_test.go +++ b/internal/git/branch_worker_loop_test.go @@ -46,7 +46,7 @@ func TestCommitWindowFor_DefaultsAndParsing(t *testing.T) { target("quarter", &metav1.Duration{Duration: 250 * time.Millisecond}), target("zero", &metav1.Duration{Duration: 0}), ).Build() - w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{}) + w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", RepoIdentity{}, nil, BranchWorkerLimits{}) ctx := t.Context() for _, tc := range []struct { @@ -162,13 +162,31 @@ func TestNewBranchWorker_DefaultsBufferCap(t *testing.T) { require.NoError(t, configv1alpha3.AddToScheme(scheme)) c := fake.NewClientBuilder().WithScheme(scheme).Build() - w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{}) + w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", RepoIdentity{}, nil, BranchWorkerLimits{}) assert.Equal(t, DefaultBranchBufferMaxBytes, w.branchBufferMaxBytes) - w = NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{MaxBufferBytes: 4096}) + w = NewBranchWorker( + c, + logr.Discard(), + "p", + "ns", + "main", + RepoIdentity{}, + nil, + BranchWorkerLimits{MaxBufferBytes: 4096}, + ) assert.Equal(t, int64(4096), w.branchBufferMaxBytes) - w = NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{MaxBufferBytes: -7}) + w = NewBranchWorker( + c, + logr.Discard(), + "p", + "ns", + "main", + RepoIdentity{}, + nil, + BranchWorkerLimits{MaxBufferBytes: -7}, + ) assert.Equal(t, DefaultBranchBufferMaxBytes, w.branchBufferMaxBytes, "non-positive override falls back to default") } @@ -182,13 +200,13 @@ func TestNewBranchWorker_QueueDepthReachesTheChannel(t *testing.T) { require.NoError(t, configv1alpha3.AddToScheme(scheme)) c := fake.NewClientBuilder().WithScheme(scheme).Build() - w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{}) + w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", RepoIdentity{}, nil, BranchWorkerLimits{}) assert.Equal(t, DefaultBranchWorkerQueueDepth, cap(w.eventQueue)) - w = NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{QueueDepth: 7}) + w = NewBranchWorker(c, logr.Discard(), "p", "ns", "main", RepoIdentity{}, nil, BranchWorkerLimits{QueueDepth: 7}) assert.Equal(t, 7, cap(w.eventQueue)) - w = NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{QueueDepth: -1}) + w = NewBranchWorker(c, logr.Discard(), "p", "ns", "main", RepoIdentity{}, nil, BranchWorkerLimits{QueueDepth: -1}) assert.Equal(t, DefaultBranchWorkerQueueDepth, cap(w.eventQueue), "non-positive override falls back to default") } @@ -201,11 +219,20 @@ func TestNewBranchWorker_LimitsAreIndependent(t *testing.T) { require.NoError(t, configv1alpha3.AddToScheme(scheme)) c := fake.NewClientBuilder().WithScheme(scheme).Build() - w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{QueueDepth: 3}) + w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", RepoIdentity{}, nil, BranchWorkerLimits{QueueDepth: 3}) assert.Equal(t, 3, cap(w.eventQueue)) assert.Equal(t, DefaultBranchBufferMaxBytes, w.branchBufferMaxBytes) - w = NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{MaxBufferBytes: 4096}) + w = NewBranchWorker( + c, + logr.Discard(), + "p", + "ns", + "main", + RepoIdentity{}, + nil, + BranchWorkerLimits{MaxBufferBytes: 4096}, + ) assert.Equal(t, DefaultBranchWorkerQueueDepth, cap(w.eventQueue)) assert.Equal(t, int64(4096), w.branchBufferMaxBytes) } @@ -219,7 +246,16 @@ func TestBranchWorker_QueueDepthBoundsAcceptedWrites(t *testing.T) { c := fake.NewClientBuilder().WithScheme(scheme).Build() const depth = 5 - w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{QueueDepth: depth}) + w := NewBranchWorker( + c, + logr.Discard(), + "p", + "ns", + "main", + RepoIdentity{}, + nil, + BranchWorkerLimits{QueueDepth: depth}, + ) // The worker is never started, so nothing drains: the queue fills to exactly its depth. for i := range depth { diff --git a/internal/git/branch_worker_queue_depth_test.go b/internal/git/branch_worker_queue_depth_test.go index a931088b9..583940cf1 100644 --- a/internal/git/branch_worker_queue_depth_test.go +++ b/internal/git/branch_worker_queue_depth_test.go @@ -12,7 +12,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" ) @@ -128,7 +130,7 @@ func TestEnsureWorker_WaitsForTheWorkerItReplacesToStop(t *testing.T) { ensured := make(chan struct{}) go func() { - _ = manager.EnsureWorker(context.Background(), "test-provider", "test-ns", "main") + _ = manager.EnsureWorker(context.Background(), "test-provider", "test-ns", "main", RepoIdentity{}) close(ensured) }() @@ -173,7 +175,10 @@ func TestReconcileWorkers_DoesNotStopAWorkerEnsureWorkerJustHandedOut(t *testing afterOrphanSelection = func() { go func() { defer close(ensured) - _ = manager.EnsureWorker(context.Background(), "test-provider", "test-ns", "main") + // The SAME repository the retiring worker is about, deliberately: that is what makes + // an unguarded EnsureWorker take its "already there" path, which is the regression + // this test is here to catch. + _ = manager.EnsureWorker(context.Background(), "test-provider", "test-ns", "main", retiring.repo) }() // Long enough for an unguarded EnsureWorker to finish inside the window; a guarded one is // blocked on lifecycleMu and finishes after the sweep instead. @@ -200,3 +205,68 @@ func TestReconcileWorkers_DoesNotStopAWorkerEnsureWorkerJustHandedOut(t *testing require.True(t, exists, "the target that asked for a worker must have one, not a slot the sweep emptied") assert.NotSame(t, retiring, got, "and it must be a fresh one, not the worker the sweep retired") } + +// A sweep must not stop a worker created for a GitTarget that its own snapshot is too old to +// contain. +// +// The selection window above is only half of it. ReconcileWorkers decides from the API, so a +// target created AFTER that list is taken is absent from it — and the worker EnsureWorker creates +// for that target moments later matches no needed key, so the sweep stops it, taking its queue +// and its checkout with it. The target is then wired to a dead worker until its next reconcile. +// +// The interceptor stands in the window a list outside the lock would open: it runs while the sweep +// is reading the targets, and a guarded sweep leaves the EnsureWorker below blocked on lifecycleMu +// until the removal is done. +func TestReconcileWorkers_DoesNotStopAWorkerCreatedAfterItsSnapshot(t *testing.T) { + withTemporaryWorkerStateRoot(t) + key := BranchKey{RepoNamespace: "test-ns", RepoName: "newcomer-provider", Branch: "main"} + scheme := runtime.NewScheme() + require.NoError(t, configv1alpha3.AddToScheme(scheme)) + manager := &WorkerManager{ + Log: logr.Discard(), + ctx: context.Background(), + workers: map[BranchKey]*BranchWorker{}, + } + + ensured := make(chan struct{}) + var once sync.Once + manager.Client = interceptor.NewClient( + fake.NewClientBuilder().WithScheme(scheme).Build(), + interceptor.Funcs{ + List: func(ctx context.Context, c client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + if err := c.List(ctx, list, opts...); err != nil { + return err + } + // A GitTarget is created right after this snapshot was taken, and its reconcile + // asks for the branch worker it needs. + once.Do(func() { + go func() { + defer close(ensured) + _ = manager.EnsureWorker( + context.Background(), "newcomer-provider", "test-ns", "main", RepoIdentity{}) + }() + time.Sleep(200 * time.Millisecond) + }) + return nil + }, + }) + + require.NoError(t, manager.ReconcileWorkers(context.Background())) + + select { + case <-ensured: + case <-time.After(5 * time.Second): + t.Fatal("EnsureWorker never returned") + } + + manager.mu.RLock() + created, exists := manager.workers[key] + manager.mu.RUnlock() + t.Cleanup(func() { + if exists { + created.Stop() + } + }) + assert.True(t, exists, + "a worker created after the sweep's snapshot is not an orphan; it is younger than the question") +} diff --git a/internal/git/branch_worker_split_test.go b/internal/git/branch_worker_split_test.go index ea7fcafad..6439370a7 100644 --- a/internal/git/branch_worker_split_test.go +++ b/internal/git/branch_worker_split_test.go @@ -160,7 +160,16 @@ func setupCommitPushSplitWorker(t *testing.T) (*BranchWorker, *git.Repository, s provider.Namespace = "default" require.NoError(t, k8sClient.Create(ctx, provider)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) worker.ctx = ctx return worker, serverRepo, remoteURL } diff --git a/internal/git/branch_worker_test.go b/internal/git/branch_worker_test.go index 38c1de804..301fbd7bc 100644 --- a/internal/git/branch_worker_test.go +++ b/internal/git/branch_worker_test.go @@ -60,7 +60,16 @@ func TestBranchWorker_EmptyRepository(t *testing.T) { _ = configv1alpha3.AddToScheme(scheme) client := fake.NewClientBuilder().WithScheme(scheme).Build() logger := logr.Discard() - worker := NewBranchWorker(client, logger, "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + client, + logger, + "test-repo", + "default", + "main", + RepoIdentity{URL: "file://" + repoPath}, + nil, + BranchWorkerLimits{}, + ) // Create a GitProvider in the fake client pointing to our empty repo repoConfig := &configv1alpha3.GitProvider{ @@ -94,7 +103,16 @@ func TestBranchWorker_IdentityFields(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).Build() log := logr.Discard() - worker := NewBranchWorker(client, log, "my-repo", "my-namespace", "develop", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + client, + log, + "my-repo", + "my-namespace", + "develop", + RepoIdentity{}, + nil, + BranchWorkerLimits{}, + ) if worker.GitProviderRef != "my-repo" { t.Errorf("Expected GitProviderRef 'my-repo', got %q", worker.GitProviderRef) @@ -129,7 +147,16 @@ func TestBranchWorker_EnsurePathBootstrapped_EmptyPathCreatesTemplate(t *testing require.NoError(t, k8sClient.Create(ctx, provider)) createTargetWithEncryption(ctx, t, k8sClient, "bootstrap-target", "default", "test-repo", "main", "clusters/prod") - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) require.NoError(t, worker.EnsurePathBootstrapped("clusters/prod", "bootstrap-target", "default")) require.NoError(t, worker.EnsurePathBootstrapped("clusters/prod", "bootstrap-target", "default")) @@ -178,7 +205,16 @@ func TestBranchWorker_EnsurePathBootstrapped_NonEmptyPathBootstrapsMissingFiles( require.NoError(t, k8sClient.Create(ctx, provider)) createTargetWithEncryption(ctx, t, k8sClient, "bootstrap-target", "default", "test-repo", "main", "clusters/prod") - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) require.NoError(t, worker.EnsurePathBootstrapped("clusters/prod", "bootstrap-target", "default")) require.NoError(t, worker.EnsurePathBootstrapped("clusters/prod", "bootstrap-target", "default")) @@ -217,7 +253,16 @@ func TestBranchWorker_EnsurePathBootstrapped_NoEncryptionSkipsSOPSConfig(t *test require.NoError(t, k8sClient.Create(ctx, provider)) createTargetWithoutEncryption(ctx, t, k8sClient, "bootstrap-target", "default", "test-repo", "main", "clusters/dev") - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) require.NoError(t, worker.EnsurePathBootstrapped("clusters/dev", "bootstrap-target", "default")) repoPath := worker.repoPathForRemote(remoteURL) @@ -260,7 +305,16 @@ func TestBranchWorker_EnsurePathBootstrapped_ExistingFileNotOverwritten(t *testi require.NoError(t, k8sClient.Create(ctx, provider)) createTargetWithEncryption(ctx, t, k8sClient, "bootstrap-target", "default", "test-repo", "main", "clusters/prod") - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) require.NoError(t, worker.EnsurePathBootstrapped("clusters/prod", "bootstrap-target", "default")) repoPath := worker.repoPathForRemote(remoteURL) @@ -293,7 +347,16 @@ func TestBranchWorker_EnsurePathBootstrapped_EnableEncryptionLaterAddsSOPSConfig require.NoError(t, k8sClient.Create(ctx, provider)) createTargetWithoutEncryption(ctx, t, k8sClient, "bootstrap-target", "default", "test-repo", "main", "clusters/dev") - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) require.NoError(t, worker.EnsurePathBootstrapped("clusters/dev", "bootstrap-target", "default")) repoPath := worker.repoPathForRemote(remoteURL) @@ -348,7 +411,16 @@ func TestBranchWorker_EnsurePathBootstrapped_InvalidEncryptionSecretSkipsSOPSCon }, ) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) require.NoError(t, worker.EnsurePathBootstrapped("clusters/dev", "bootstrap-target", "default")) repoPath := worker.repoPathForRemote(remoteURL) @@ -393,7 +465,16 @@ func TestBranchWorker_EnsurePathBootstrapped_MissingSOPSKeySkipsSOPSConfig(t *te }, ) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) require.NoError(t, worker.EnsurePathBootstrapped("clusters/dev", "bootstrap-target", "default")) repoPath := worker.repoPathForRemote(remoteURL) @@ -465,7 +546,16 @@ func TestBranchWorker_EnsurePathBootstrapped_RendersAllResolvedRecipients(t *tes } require.NoError(t, k8sClient.Create(ctx, target)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) require.NoError(t, worker.EnsurePathBootstrapped("clusters/dev", "bootstrap-target", "default")) repoPath := worker.repoPathForRemote(remoteURL) @@ -512,7 +602,16 @@ func TestBranchWorker_CommitAndPushRequest_PreparesRepositoryBeforeFirstWrite(t provider.Namespace = "default" require.NoError(t, k8sClient.Create(ctx, provider)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) worker.ctx = ctx request := &WriteRequest{ @@ -607,7 +706,16 @@ func TestBranchWorker_CommitAndPushRequest_NewBranchStartsFromLatestMain(t *test provider.Namespace = "default" require.NoError(t, k8sClient.Create(ctx, provider)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "feature", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "feature", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) worker.ctx = ctx // Pre-create a stale local checkout while remote main is still at commit A. @@ -746,7 +854,16 @@ func TestBranchWorker_CommitAndPushRequest_UsesProviderCommitterAndTargetMessage } require.NoError(t, k8sClient.Create(ctx, target)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) worker.ctx = ctx request := &WriteRequest{ @@ -836,7 +953,16 @@ func TestBranchWorker_CommitAndPushRequest_UsesBatchTemplateForAtomicRequest(t * } require.NoError(t, k8sClient.Create(ctx, target)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) worker.ctx = ctx request := &WriteRequest{ @@ -959,7 +1085,16 @@ func TestBranchWorker_CommitAndPushRequest_SignsCommitWhenConfigured(t *testing. } require.NoError(t, k8sClient.Create(ctx, provider)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) worker.ctx = ctx request := &WriteRequest{ @@ -1056,7 +1191,16 @@ func TestBranchWorker_CommitAndPushRequest_SkipsWriteWhenSigningSecretIsInvalid( } require.NoError(t, k8sClient.Create(ctx, provider)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) worker.ctx = ctx request := &WriteRequest{ diff --git a/internal/git/commit_request_attach_test.go b/internal/git/commit_request_attach_test.go index c8dea0985..7e3030f18 100644 --- a/internal/git/commit_request_attach_test.go +++ b/internal/git/commit_request_attach_test.go @@ -417,7 +417,16 @@ func TestAttach_FinalizeFailureResolvesFailed(t *testing.T) { provider.Namespace = "default" require.NoError(t, k8sClient.Create(ctx, provider)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + "test-repo", + "default", + "main", + RepoIdentity{URL: "file:///nonexistent/gitops-reverser-repo.git"}, + nil, + BranchWorkerLimits{}, + ) worker.ctx = ctx createPlainGitTarget(t, worker, "team-a", "team-a") diff --git a/internal/git/git_roundtrip_ledger_test.go b/internal/git/git_roundtrip_ledger_test.go index 0395d8c9c..658768a28 100644 --- a/internal/git/git_roundtrip_ledger_test.go +++ b/internal/git/git_roundtrip_ledger_test.go @@ -99,7 +99,16 @@ func newLedgerFixture(t *testing.T, slug string, seeded bool) *ledgerFixture { provider.Namespace = "default" require.NoError(t, k8sClient.Create(ctx, provider)) - worker := NewBranchWorker(k8sClient, logr.Discard(), providerName, "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + providerName, + "default", + "main", + RepoIdentity{URL: sim.RepoURL}, + nil, + BranchWorkerLimits{}, + ) worker.ctx = ctx t.Cleanup(func() { _ = os.RemoveAll(worker.repoRootPath()) }) diff --git a/internal/git/helpers_test.go b/internal/git/helpers_test.go index 53e27de31..b34e76b70 100644 --- a/internal/git/helpers_test.go +++ b/internal/git/helpers_test.go @@ -216,7 +216,16 @@ func newTestBranchWorker( }) k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() - worker := NewBranchWorker(k8sClient, logr.Discard(), providerName, "default", branch, nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + k8sClient, + logr.Discard(), + providerName, + "default", + branch, + RepoIdentity{URL: remoteURL}, + nil, + BranchWorkerLimits{}, + ) worker.ctx = context.Background() return worker, nil } diff --git a/internal/git/pending_writes_test.go b/internal/git/pending_writes_test.go index b038c525a..fc41c2bcc 100644 --- a/internal/git/pending_writes_test.go +++ b/internal/git/pending_writes_test.go @@ -78,7 +78,16 @@ func TestPendingWrites_ResolvesEncryptionOncePerUniqueTarget(t *testing.T) { Build() counting := &countingClient{Client: baseClient} - worker := NewBranchWorker(counting, logr.Discard(), providerName, "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + counting, + logr.Discard(), + providerName, + "default", + "main", + RepoIdentity{URL: "file:///tmp/test-repo.git"}, + nil, + BranchWorkerLimits{}, + ) worker.ctx = context.Background() events := []Event{ diff --git a/internal/git/provider_repoint_test.go b/internal/git/provider_repoint_test.go index f12d110fd..52645bb75 100644 --- a/internal/git/provider_repoint_test.go +++ b/internal/git/provider_repoint_test.go @@ -6,12 +6,21 @@ package git // destination: spec.url is immutable, so an operator deletes the object and creates it again with // the new URL (see GitProviderSpec). // -// Nothing about that restarts the branch worker. Workers are keyed by +// Nothing about that reaches a live branch worker. Workers are keyed by // (GitProvider namespace, GitProvider name, branch) and the GitTarget that owns the worker is -// untouched, so the SAME worker meets the new repository — carrying the base trust it earned -// against the old one. Each remote gets its own on-disk clone, so that trust is not merely stale, -// it is about a checkout that does not exist at the new path: the cycle skips the fetch that would -// have created it and fails on every write with "repository does not exist". +// untouched, so the worker survives the repoint. It does not follow the provider to the new +// repository: it is FOR the old one, and the WorkerManager replaces it at the GitTarget's next +// reconcile (see worker_repo_identity_test.go). +// +// Following the provider's URL on every cycle is what produced #382: the cycle planned against +// trust earned on the old repository, skipped the fetch that would have established the new +// checkout, and every write from then on failed at "repository does not exist". +// +// Until the replacement arrives the worker does NOTHING, and that is deliberate rather than +// incidental. It would otherwise be writing to a repository the operator has just pointed away +// from, with credentials read from the object that now names a different one: the replacement is +// the only thing that resolves it, and a replacement blocked by a failing Validated gate never +// comes. So the mismatch fails the cycle loudly instead. import ( "os/exec" @@ -26,59 +35,52 @@ import ( configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" ) -// TestBaseTrust_RepointedProviderDoesNotInheritTheOldRepositorysTrust publishes to one remote, -// repoints the provider at another, and requires the next publication to land on the new one. -func TestBaseTrust_RepointedProviderDoesNotInheritTheOldRepositorysTrust(t *testing.T) { +// TestRepoint_TheWorkerStopsRatherThanWriteWithTheNewProvidersCredentials. A repointed +// GitProvider is noticed at the GitTarget's next reconcile, and until then the worker is still +// live and still reads that provider on every cycle for credentials, commit identity and signing. +// What it must not do is combine the two: its own repository with the replacement's credential. +func TestRepoint_TheWorkerStopsRatherThanWriteWithTheNewProvidersCredentials(t *testing.T) { f := newLedgerFixture(t, "repoint-provider", true) f.publish("before-the-repoint") require.True(t, f.worker.baseTrusted(), "a successful push leaves the base trusted") - second := f.repointProvider(t) + // Planned while the provider still agreed, so the refusal below is the write path's and not + // merely the planner's: both read the GitProvider, and both have to refuse. + events := []Event{configMapEvent("after-the-repoint", "alice", "team-a")} + write, err := f.worker.buildGroupedPendingWrite(f.worker.ctx, events) + require.NoError(t, err) - // The write that follows must reach the new repository. Before trust was bound to the - // repository it was gained against, this failed with "open repository: repository does not - // exist" and kept failing, because nothing ever initialised the new checkout. - f.commit(false, "after-the-repoint") - f.push() + elsewhere := f.repointProvider(t) - assert.Equal(t, "after-the-repoint.yaml", firstConfigMapFileName(t, second), - "the write went to the repository the provider now names") + require.Error(t, f.worker.commitPendingWrites([]PendingWrite{*write}, false), + "the GitProvider names a repository this worker is not for") + _, err = f.worker.buildGroupedPendingWrite(f.worker.ctx, events) + require.Error(t, err, "and nothing new is planned against it either") + assert.Contains(t, err.Error(), "waiting to be replaced") + + assert.NotContains(t, configMapFileNames(t, f.repoDir), "after-the-repoint.yaml", + "nothing more goes into the repository the operator has pointed away from") + assert.Empty(t, configMapFileNames(t, elsewhere), + "and nothing reaches the one the provider now names: a different worker serves that") } -// TestBaseTrust_RepointIsNoticedEvenWhileWritesAreRetained is the other half. A worker holding -// retained work skips the head-of-cycle guard entirely (a reset would destroy the local commits -// those writes produced), so the identity check must not live behind that guard: the trust has to -// be dropped on the cycle that skips it, or it would survive into the cycle that follows the -// retained work and take that one straight past its fetch as well. -// -// The retained work itself cannot be rescued, and that is not this change's business: its local -// commits are in the old clone, and the cycle fails at the open. That failure is identical before -// and after base trust existed. -func TestBaseTrust_RepointIsNoticedEvenWhileWritesAreRetained(t *testing.T) { +// TestRepoint_WhatWasAlreadyWrittenIsUntouched is the other half of stopping. The worker refuses +// the cycle; it does not undo, re-target or lose what it had already pushed, and its own checkout +// is still the one it was always about. +func TestRepoint_WhatWasAlreadyWrittenIsUntouched(t *testing.T) { f := newLedgerFixture(t, "repoint-with-retained", true) f.publish("before-the-repoint") - f.commit(false, "retained") - require.True(t, f.worker.baseTrusted()) + require.Contains(t, configMapFileNames(t, f.repoDir), "before-the-repoint.yaml") - second := f.repointProvider(t) + elsewhere := f.repointProvider(t) - // A cycle with work in hand: the guard is skipped, so only the identity check can drop trust. - joining := []Event{configMapEvent("joins-the-cycle", "alice", "team-a")} - retained, err := f.worker.buildGroupedPendingWrite(f.worker.ctx, joining) - require.NoError(t, err) - require.Error(t, f.worker.commitPendingWrites([]PendingWrite{*retained}, true), - "the local commits behind the retained writes are in the repository the provider no longer names") - - assert.False(t, f.worker.baseTrusted(), - "the provider names a different repository than the one this trust was gained against") - - // Once nothing is retained, that dropped trust is what makes the next cycle establish the new - // checkout rather than plan against a base it never had. - f.pending = nil - f.publish("after-the-retained-work") - assert.Equal(t, "after-the-retained-work.yaml", firstConfigMapFileName(t, second)) + assert.Contains(t, configMapFileNames(t, f.repoDir), "before-the-repoint.yaml", + "a repoint is not a retraction of what is already in the old repository") + assert.Empty(t, configMapFileNames(t, elsewhere)) + assert.Equal(t, f.sim.RepoURL, f.worker.repo.URL, + "and the worker still knows which repository it is for") } // repointProvider deletes the fixture's GitProvider and creates it again pointing at a second @@ -109,27 +111,29 @@ func (f *ledgerFixture) repointProvider(t *testing.T) string { return repoDir } -// firstConfigMapFileName reads the name of the single ConfigMap document the worker wrote into the -// given bare repository, which is how each test says "the write landed HERE". -func firstConfigMapFileName(t *testing.T, bareRepoDir string) string { +// configMapFileNames lists the ConfigMap documents on main in a bare repository, which is how each +// test says "the write landed HERE, and nowhere else". +func configMapFileNames(t *testing.T, bareRepoDir string) []string { t.Helper() repo, err := gogit.PlainOpen(bareRepoDir) require.NoError(t, err) ref, err := repo.Reference("refs/heads/main", true) - require.NoError(t, err) + if err != nil { + // No branch at all is the honest answer for a repository nothing ever wrote to. + return nil + } commit, err := repo.CommitObject(ref.Hash()) require.NoError(t, err) tree, err := commit.Tree() require.NoError(t, err) - var found string + var found []string require.NoError(t, tree.Files().ForEach(func(file *object.File) error { if filepath.Ext(file.Name) == ".yaml" && filepath.Base(filepath.Dir(file.Name)) == "configmaps" { - found = filepath.Base(file.Name) + found = append(found, filepath.Base(file.Name)) } return nil })) - require.NotEmpty(t, found, "no ConfigMap document in the repository the provider now names") return found } diff --git a/internal/git/prune_replay_test.go b/internal/git/prune_replay_test.go index 0b5c31bf6..ba8652ac6 100644 --- a/internal/git/prune_replay_test.go +++ b/internal/git/prune_replay_test.go @@ -75,7 +75,16 @@ func replayWorker(t *testing.T, objects []client.Object, fns *interceptor.Funcs) if fns != nil { builder = builder.WithInterceptorFuncs(*fns) } - worker := NewBranchWorker(builder.Build(), logr.Discard(), "provider", "default", "main", nil, BranchWorkerLimits{}) + worker := NewBranchWorker( + builder.Build(), + logr.Discard(), + "provider", + "default", + "main", + RepoIdentity{}, + nil, + BranchWorkerLimits{}, + ) worker.ctx = context.Background() return worker } diff --git a/internal/git/refresh.go b/internal/git/refresh.go index 8b162a0d6..a69651498 100644 --- a/internal/git/refresh.go +++ b/internal/git/refresh.go @@ -48,6 +48,15 @@ func (w *BranchWorker) EnqueueRefresh(req *RefreshRequest) { return } w.inflightItems.Add(1) + // Under the same lock as every other admission, for the reason on stoppingState: a check + // outside it can pass and then send into a queue Stop has already finished draining. + w.pendingResyncsMu.Lock() + defer w.pendingResyncsMu.Unlock() + if w.stoppingLocked() { + w.inflightItems.Add(-1) + w.recordQueueDrop(queueDropRefresh) + return + } select { case w.eventQueue <- WorkItem{Refresh: req}: default: @@ -67,32 +76,23 @@ func (w *BranchWorker) EnqueueRefresh(req *RefreshRequest) { func (l *branchWorkerEventLoop) handleRefreshRequest(req *RefreshRequest) { w := l.w - // 1. The GitProvider first, because everything below is a statement about a REPOSITORY and - // this worker may have been handed a different one. spec.url is immutable and repointed by - // recreating the GitProvider, which the worker — keyed by (provider, branch) — survives, so - // noteRemoteIdentity is what notices; it drops the checkout's trust and the observation. + // 1. The GitProvider first, for the credentials the look below needs. WHICH repository is + // looked at is not its decision: that is the worker's own identity, fixed when it was created. provider, err := w.getGitProvider(w.ctx) if err != nil { w.Log.V(1).Info("Skipping refresh: the GitProvider cannot be read", "branch", w.Branch, "gitTarget", req.Target.String(), "error", err.Error()) return } - w.noteRemoteIdentity(provider.Spec.URL) - // 2. Report what is already known BEFORE deciding whether to do any work, and on every exit - // below. A push reports only against the targets whose writes it carried, so a target that is - // not writing learns where its branch is exclusively from its own tick. + // 2. What is already known about the branch needs no re-delivery: it was delivered when it was + // proved, to the branch rather than to a target, and every GitTarget on this branch has been + // reading it since. It is read here only to decide whether there is anything to ask the remote. // - // Once the repository has changed under us there is a withdrawal to report instead. Nothing - // else would correct the published revision if the new remote is unreachable, and that - // revision names a commit in a repository this target no longer points at. + // It is also why status.remote must not be read as evidence that THIS target did anything: the + // observation it publishes can have been proved by a sibling's push, on a tick that goes on to + // do no work at all, two lines below. observed, known := w.LastRemoteObservation() - switch { - case known: - w.reportRemoteObservation([]itypes.ResourceReference{req.Target}, observed) - case w.observationWithdrawn(): - w.reportRemoteObservation([]itypes.ResourceReference{req.Target}, WithdrawnObservation()) - } // 3. Not idle, so not the target this exists for. A reset here would destroy retained // commits, and the worktree may hold a partial write — which is also why this is the one exit @@ -113,12 +113,12 @@ func (l *branchWorkerEventLoop) handleRefreshRequest(req *RefreshRequest) { // fetch that moved this checkout may have been earned by another target's refresh, which // rescanned its own folder and knew nothing about this one. if known && req.MaxAge > 0 && observed.Age(time.Now()) < req.MaxAge && - !w.checkoutMayLagObservation(provider.Spec.URL, observed.Revision) { + !w.checkoutMayLagObservation(observed.Revision) { w.rescanLayoutForTarget(w.ctx, req) return } - if err := l.refreshFromRemote(req, provider); err != nil { + if err := l.refreshFromRemote(provider); err != nil { // A refresh that failed proves nothing, so nothing is recorded: the published // lastVerifiedAt stops advancing, which is precisely how a refresher that stopped // working reports itself. @@ -147,11 +147,11 @@ func (l *branchWorkerEventLoop) handleRefreshRequest(req *RefreshRequest) { // untrusted base falls through to where the checkout actually is: no checkout cannot lag anything // (the rescan reads nothing, and the first publication clones), and a HEAD already at the // observed revision is not behind it. -func (w *BranchWorker) checkoutMayLagObservation(remoteURL, revision string) bool { +func (w *BranchWorker) checkoutMayLagObservation(revision string) bool { if w.baseTrusted() { return false } - repo, err := gogit.PlainOpen(w.repoPathForRemote(remoteURL)) + repo, err := gogit.PlainOpen(w.repoPath()) if errors.Is(err, gogit.ErrRepositoryNotExists) { return false } @@ -176,10 +176,7 @@ func (w *BranchWorker) checkoutMayLagObservation(remoteURL, revision string) boo // unconditionally — which is right for the write path, because it is about to reset either way. // On an idle target the answer is almost always "the branch has not moved", and the advertisement // alone settles it, so the common case here costs ONE connection. -func (l *branchWorkerEventLoop) refreshFromRemote( - req *RefreshRequest, - provider *configv1alpha3.GitProvider, -) error { +func (l *branchWorkerEventLoop) refreshFromRemote(provider *configv1alpha3.GitProvider) error { w := l.w ctx := w.ctx @@ -194,7 +191,7 @@ func (l *branchWorkerEventLoop) refreshFromRemote( // declared but never published to, and one whose GitProvider was recreated against a // different repository. advertised, err := advertiseRemoteBranch( - provider.Spec.URL, plumbing.NewBranchReferenceName(w.Branch), auth) + w.repo.URL, plumbing.NewBranchReferenceName(w.Branch), auth) if err != nil { return fmt.Errorf("read the remote advertisement: %w", err) } @@ -202,8 +199,7 @@ func (l *branchWorkerEventLoop) refreshFromRemote( if !advertised.IsZero() { revision = advertised.String() } - observed := w.recordRemoteObservation(revision, ObservedByFetch) - w.reportRemoteObservation([]itypes.ResourceReference{req.Target}, observed) + w.recordRemoteObservation(revision, ObservedByFetch) // The remote does not carry this branch. That IS the answer, and there is nothing to fetch: a // fetch would fall back to the default branch and teach us nothing about a branch nobody has @@ -217,7 +213,7 @@ func (l *branchWorkerEventLoop) refreshFromRemote( // Everything below compares the advertisement against the local checkout, so a missing one is // simply nothing to compare: the branch's position is already recorded and reported, and the // first publication will clone. - repo, err := gogit.PlainOpen(w.repoPathForRemote(provider.Spec.URL)) + repo, err := gogit.PlainOpen(w.repoPath()) if errors.Is(err, gogit.ErrRepositoryNotExists) { w.Log.V(1).Info("Refresh observed the remote; there is no checkout to reset yet", "branch", w.Branch, "revision", revision) @@ -242,9 +238,6 @@ func (l *branchWorkerEventLoop) refreshFromRemote( if err := w.syncWithRemote(ctx, fetchReasonRefresh); err != nil { return err } - if synced, ok := w.LastRemoteObservation(); ok { - w.reportRemoteObservation([]itypes.ResourceReference{req.Target}, synced) - } return nil } @@ -282,12 +275,7 @@ func (w *BranchWorker) rescanLayoutForTarget(ctx context.Context, req *RefreshRe w.repoMu.Lock() defer w.repoMu.Unlock() - provider, err := w.getGitProvider(ctx) - if err != nil { - w.Log.V(1).Info("Refresh could not re-read the folder", "error", err.Error()) - return - } - repo, err := gogit.PlainOpen(w.repoPathForRemote(provider.Spec.URL)) + repo, err := gogit.PlainOpen(w.repoPath()) if err != nil { w.Log.V(1).Info("Refresh could not re-read the folder", "error", err.Error()) return diff --git a/internal/git/refresh_test.go b/internal/git/refresh_test.go index 0eb95cd1b..95d2b2502 100644 --- a/internal/git/refresh_test.go +++ b/internal/git/refresh_test.go @@ -50,7 +50,7 @@ func newRefreshHarnessOn(t *testing.T, slug string, seeded bool) *refreshHarness path: "team-a", } h.loop = newBranchWorkerEventLoop(h.worker, time.Hour) - h.worker.remoteReporter = func(_ itypes.ResourceReference, observed RemoteObservation) { + h.worker.remoteReporter = func(observed RemoteObservation) { h.reported = append(h.reported, observed) } h.worker.scanAcceptance = func(_ itypes.ResourceReference, refused *manifestanalyzer.AcceptanceRefusedError) { @@ -102,11 +102,16 @@ func TestRefresh_AFreshObservationCostsNothing(t *testing.T) { h := newRefreshHarness(t, "refresh-fresh") h.publish("written-by-a-push") + require.NotEmpty(t, h.reported) + assert.Equal(t, ObservedByPush, h.reported[len(h.reported)-1].By, + "the push proved where the branch is, and delivered it to the branch as it did so") + + h.reported = nil connections := h.refresh(time.Hour) assert.Zero(t, connections, "a push inside the age is the refresh; nothing may be spent on top of it") - require.Len(t, h.reported, 1, "the target still has to be told what we know") - assert.Equal(t, ObservedByPush, h.reported[0].By) + assert.Empty(t, h.reported, + "and nothing is re-delivered: the fact reached every target on the branch when it was proved") } // TestRefresh_AnUnmovedBranchCostsOneConnection is the refresher's common case, and the reason it @@ -162,9 +167,9 @@ func TestRefresh_SkipsABranchMidCycle(t *testing.T) { connections := h.refresh(time.Nanosecond) assert.Zero(t, connections, "a worker mid-cycle is not the target the refresher exists for") - assert.Len(t, h.reported, 1, - "but what is already known is still reported: on a shared branch this tick is the only "+ - "way a target that is not writing hears anything") + assert.Empty(t, h.reported, + "and it proves nothing, so it delivers nothing: what is already known about the branch "+ + "was delivered when it was proved, and every target on the branch has had it since") assert.Len(t, h.loop.pendingWrites, 1, "and the retained write is still there") } @@ -305,43 +310,39 @@ func TestRefresh_AFailedFetchDoesNotBuyQuietUntilTheObservationAges(t *testing.T "an untrusted base must send the tick to the remote, whatever the observation's age says") } -// TestRemoteObservation_DroppedWhenTheProviderNamesANewRepository. A worker is keyed by -// (provider, branch) while spec.url is immutable and repointed by recreating the GitProvider, so -// one worker can meet a second repository without anything restarting it. An observation is a -// statement about a REPOSITORY; carrying it across would report the old one's revision as this -// GitTarget's remote state, and a fresh enough one would suppress the look that corrects it. -func TestRemoteObservation_DroppedWhenTheProviderNamesANewRepository(t *testing.T) { - f := newLedgerFixture(t, "observation-repoint", true) +// TestRemoteObservation_CarriesTheRepositoryItWasProvedAgainst. A revision means nothing without +// the repository it is in, and the layer that publishes it — the GitTarget reconcile — holds no +// worker. Stamping the identity on the observation is what lets that layer compare it against the +// repository the GitProvider names NOW, and take back a revision from one the target has left. +func TestRemoteObservation_CarriesTheRepositoryItWasProvedAgainst(t *testing.T) { + f := newLedgerFixture(t, "observation-identity", true) f.publish("prime") - before, ok := f.worker.LastRemoteObservation() + observed, ok := f.worker.LastRemoteObservation() require.True(t, ok) - require.NotEmpty(t, before.Revision) - - f.worker.noteRemoteIdentity("https://example.invalid/another/repo.git") - - _, known := f.worker.LastRemoteObservation() - assert.False(t, known, "what was proved about the old repository says nothing about this one") - assert.False(t, f.worker.baseTrusted(), "and the checkout for it is not established either") + require.NotEmpty(t, observed.Revision) + assert.Equal(t, f.worker.repo, observed.Repo, + "a push proves where the branch is on THIS worker's repository, and says which one that is") + assert.Equal(t, f.sim.RepoURL, observed.Repo.URL) } -// TestRefresh_DoesNotReportThePreviousRepositoryAfterARepoint is the whole hazard of a worker that -// outlives the repository it was built for. +// TestRefresh_AsksNothingWhileItsGitProviderNamesAnotherRepository is the read-path twin of +// TestRepoint_TheWorkerStopsRatherThanWriteWithTheNewProvidersCredentials. The GitProvider is +// repointed under a live worker, and the refresh reads it for the credentials its look would use. +// It must not go looking with them: pointed at its own remote they are the wrong credentials for +// that host, and pointed at the new remote the answer would describe a repository this worker's +// checkout knows nothing about. // -// spec.url is immutable, so pointing a GitTarget somewhere else means deleting and recreating the -// GitProvider — and the worker, keyed by (provider name, branch), survives that untouched. Its -// cached observation is then a fact about a repository this target no longer uses, and the refresh -// path is where it would be published: it reports what is known before doing any work. -func TestRefresh_DoesNotReportThePreviousRepositoryAfterARepoint(t *testing.T) { +// So the tick costs nothing and reports nothing, and what was already published is taken back by +// the reconcile from the identity the stored observation carries: see publishRemote. +func TestRefresh_AsksNothingWhileItsGitProviderNamesAnotherRepository(t *testing.T) { first := newRefreshHarness(t, "repoint-first") second := newLedgerFixture(t, "repoint-second", true) first.publish("written-to-the-first-repository") - oldRevision := revParseMain(t, first.repoDir) - require.NotEmpty(t, oldRevision) + require.NotEmpty(t, revParseMain(t, first.repoDir)) - // The GitProvider is recreated pointing at a different repository. Nothing restarts the - // worker; the only thing that changes is what its provider says. + // The GitProvider now names a different repository. Nothing restarts the worker. var provider configv1alpha3.GitProvider require.NoError(t, first.worker.Client.Get(first.worker.ctx, client.ObjectKey{Name: first.worker.GitProviderRef, Namespace: "default"}, &provider)) @@ -349,14 +350,25 @@ func TestRefresh_DoesNotReportThePreviousRepositoryAfterARepoint(t *testing.T) { require.NoError(t, first.worker.Client.Update(first.worker.ctx, &provider)) first.reported = nil - first.refresh(time.Hour) // an age that would reuse the cached observation, if anything did - - require.NotEmpty(t, first.reported, "the target still has to be told something") - last := first.reported[len(first.reported)-1] - assert.NotEqual(t, oldRevision, last.Revision, - "a revision from the repository this target no longer points at must never be published") - assert.Equal(t, revParseMain(t, second.repoDir), last.Revision, - "what is published is where the branch is on the repository it points at NOW") + connections := first.refresh(time.Nanosecond) // an age that would otherwise force the look + + assert.Zero(t, connections, "a worker waiting to be replaced asks no remote anything") + assert.Empty(t, first.reported, + "and publishes nothing: the reconcile removes the old revision from the identity it carries") +} + +// TestRemoteObservation_KeepsCarryingItsRepositoryAcrossAPush. The stamp is what lets the layer +// that publishes an observation decide whether it still describes the repository the GitTarget +// points at, so it has to survive the producer that writes it most often. +func TestRemoteObservation_KeepsCarryingItsRepositoryAcrossAPush(t *testing.T) { + f := newLedgerFixture(t, "observation-identity-push", true) + f.publish("first") + f.publish("second") + + observed, ok := f.worker.LastRemoteObservation() + require.True(t, ok) + assert.Equal(t, ObservedByPush, observed.By) + assert.Equal(t, f.worker.repo, observed.Repo) } // TestRefresh_AnAbsentCheckoutHonoursTheInterval. A GitTarget that has been declared but has not @@ -388,66 +400,6 @@ func TestRefresh_AnAbsentBranchHonoursTheInterval(t *testing.T) { "a branch the remote does not carry still gets the interval it was configured with") } -// TestRefresh_WithdrawsThePublishedObservationAfterAFailedRepoint is the hazard a dropped -// observation does not on its own close. -// -// Dropping it stops the OLD repository's revision being reported again, which is what the worker -// owns. It says nothing about the copy already published — in the watch plane's projection and in -// status.remote — and if the repository this target now points at cannot be read, nothing ever -// replaces it: the operator goes on reading a revision that lives in a repository the target no -// longer uses. So the refresh withdraws it explicitly, and keeps withdrawing until something is -// actually proved, because every GitTarget on this branch has to hear it on its own tick. -func TestRefresh_WithdrawsThePublishedObservationAfterAFailedRepoint(t *testing.T) { - h := newRefreshHarness(t, "repoint-unreachable") - h.publish("written-to-the-first-repository") - require.NotEmpty(t, revParseMain(t, h.repoDir)) - - var provider configv1alpha3.GitProvider - require.NoError(t, h.worker.Client.Get(h.worker.ctx, - client.ObjectKey{Name: h.worker.GitProviderRef, Namespace: "default"}, &provider)) - // Recreated pointing at a repository nothing answers for. - provider.Spec.URL = "http://127.0.0.1:1/unreachable.git" - require.NoError(t, h.worker.Client.Update(h.worker.ctx, &provider)) - - h.reported = nil - h.refresh(time.Hour) // an age that would reuse the cached observation, if anything did - - require.Len(t, h.reported, 1, "the target has to be told, and the failed look proves nothing") - assert.True(t, h.reported[0].Withdrawn, - "what is published names a revision in a repository this target no longer points at") - assert.Empty(t, h.reported[0].Revision, "a withdrawal carries nothing to publish") - - h.reported = nil - h.refresh(time.Hour) - require.Len(t, h.reported, 1) - assert.True(t, h.reported[0].Withdrawn, - "it stands until something is proved: a sibling target's first tick must hear it too") -} - -// TestRefresh_StopsWithdrawingOnceSomethingIsProved is the other side of that latch. A withdrawal -// is a correction, not a state to live in: the first successful look at the new repository -// replaces it with a real observation. -func TestRefresh_StopsWithdrawingOnceSomethingIsProved(t *testing.T) { - first := newRefreshHarness(t, "repoint-recovers-first") - second := newLedgerFixture(t, "repoint-recovers-second", true) - first.publish("written-to-the-first-repository") - - var provider configv1alpha3.GitProvider - require.NoError(t, first.worker.Client.Get(first.worker.ctx, - client.ObjectKey{Name: first.worker.GitProviderRef, Namespace: "default"}, &provider)) - provider.Spec.URL = second.sim.RepoURL - require.NoError(t, first.worker.Client.Update(first.worker.ctx, &provider)) - - first.reported = nil - first.refresh(time.Hour) - - require.NotEmpty(t, first.reported) - last := first.reported[len(first.reported)-1] - assert.False(t, last.Withdrawn, "the new repository answered, so there is something to publish") - assert.Equal(t, revParseMain(t, second.repoDir), last.Revision) - assert.False(t, first.worker.observationWithdrawn(), "and nothing is left to take back") -} - // TestRefresh_PublishesARefusalSomebodyElsePushed is the gap this closes. A folder is broken by // another writer's push, which produces NO Kubernetes event, so nothing noticed until the next // live edit arrived — and that edit was refused and its events dropped. A read finds it on a tick diff --git a/internal/git/remote_observation.go b/internal/git/remote_observation.go index 86f5f5c2e..9788e13fd 100644 --- a/internal/git/remote_observation.go +++ b/internal/git/remote_observation.go @@ -38,16 +38,19 @@ type RemoteObservation struct { At time.Time // By is what proved it. By ObservationSource - // Withdrawn marks the one report that is not an observation: "forget what was published". - // A worker sends it when its GitProvider now names a different repository, because the - // revision already published then describes a repository the target no longer points at, and - // if the new one is unreachable nothing else would correct it. It carries no revision and - // dates nothing — the projection turns it into an ABSENT status.remote, not a value. - Withdrawn bool + // Repo is the repository the observation is ABOUT: the identity of the worker that proved it. + // + // A revision means nothing without it. Workers are keyed by (provider, branch) while a + // GitProvider can be repointed at another repository, so the layer that publishes this can be + // holding a revision from a repository its GitTarget no longer uses — and if the new one is + // unreachable, nothing else would ever correct it. Carrying the identity lets that be DERIVED + // at the moment of publication, by comparing it against what the GitProvider names now, + // instead of latched by whoever happened to notice the change. + // + // It is zero for a worker that has no identity of its own: the CLI, and tests that never reach + // a remote. A comparison against an unknown identity proves nothing, so it is not made. + Repo RepoIdentity } -// WithdrawnObservation is the report that takes back whatever was published about a branch. -func WithdrawnObservation() RemoteObservation { return RemoteObservation{Withdrawn: true} } - // Age is how long ago the observation was made, against now. func (o RemoteObservation) Age(now time.Time) time.Duration { return now.Sub(o.At) } diff --git a/internal/git/remote_report.go b/internal/git/remote_report.go index 28fda6244..539355bc4 100644 --- a/internal/git/remote_report.go +++ b/internal/git/remote_report.go @@ -2,60 +2,18 @@ package git -import ( - itypes "github.com/ConfigButler/gitops-reverser/internal/types" -) - -// RemoteReporter publishes a confirmed observation of a branch's remote state to the layer that -// owns GitTarget status. It is the twin of LayoutReporter, for the same structural reason: the -// observation is made on a branch-worker goroutine with no result channel back to the controller, -// so without a hook it would be learned and dropped. +// RemoteReporter delivers a confirmed observation of a branch's remote state to the layer that +// keeps it for the whole branch. Every worker gets one, bound to its own BranchKey, because the +// observation is made on a branch-worker goroutine with no result channel back to the controller +// and would otherwise be learned and dropped. // -// Where this could have grown a registry, it does not. A worker serves EVERY GitTarget on its -// (provider, branch) while the observation is about the branch, and it keeps no list of them: it -// reports only against a target already in hand — the ones a push's writes named, or the one a -// refresh names — and every target reaches the second case on its own tick. -type RemoteReporter func(target itypes.ResourceReference, observed RemoteObservation) - -// reportRemoteObservation publishes one observation against every GitTarget named by the writes -// that produced it. +// DELIVERY IS NOT PUBLICATION, and this is the delivery half. "Where is branch B" is ONE fact, +// proved once — by a push or by a fetch — and shared by every GitTarget on that branch at no cost: +// it is a map write, and distributing a fact is not re-proving it. What each GitTarget then WRITES +// to its status is a separate, bounded decision, made on its own reconcile. See publishRemote. // -// An unattributable write (either half of the reference empty — the CLI, and tests) publishes -// nothing: the projection is keyed by "namespace/name", so an empty half would file the report -// under a key no GitTarget reads. -func (w *BranchWorker) reportRemoteObservation(targets []itypes.ResourceReference, observed RemoteObservation) { - if w.remoteReporter == nil { - return - } - for _, target := range targets { - if target.Name == "" || target.Namespace == "" { - continue - } - w.remoteReporter(target, observed) - } -} - -// pendingWriteTargets is the distinct set of GitTargets a batch of pending writes was written -// for, in a stable order. -func pendingWriteTargets(writes []PendingWrite) []itypes.ResourceReference { - seen := map[string]struct{}{} - targets := make([]itypes.ResourceReference, 0, len(writes)) - add := func(name, namespace string) { - if name == "" || namespace == "" { - return - } - ref := itypes.NewResourceReference(name, namespace) - if _, had := seen[ref.Key()]; had { - return - } - seen[ref.Key()] = struct{}{} - targets = append(targets, ref) - } - for _, write := range writes { - add(write.GitTargetName, write.GitTargetNamespace) - for key := range write.Targets { - add(key.Name, key.Namespace) - } - } - return targets -} +// This is why the worker keeps no list of targets. It used to report against whichever targets +// were in hand — the ones a push's writes named, or the one a refresh was serving — so a fact +// about the branch reached some of its targets and not others, and the layer above had to keep a +// copy per target of a value that was the same for all of them. +type RemoteReporter func(observed RemoteObservation) diff --git a/internal/git/remote_report_test.go b/internal/git/remote_report_test.go index 80b6b8397..9b79fe097 100644 --- a/internal/git/remote_report_test.go +++ b/internal/git/remote_report_test.go @@ -14,48 +14,60 @@ import ( itypes "github.com/ConfigButler/gitops-reverser/internal/types" ) -// TestReportRemoteObservation_SkipsAnUnattributableTarget. The projection is keyed by -// "namespace/name", so a reference with either half empty — the CLI, and tests — would file the -// report under a key no GitTarget ever reads. -func TestReportRemoteObservation_SkipsAnUnattributableTarget(t *testing.T) { - var reported []itypes.ResourceReference - w := &BranchWorker{Log: logr.Discard()} - w.remoteReporter = func(target itypes.ResourceReference, _ RemoteObservation) { - reported = append(reported, target) - } +// TestRecordRemoteObservation_DeliversOneFactToTheWholeBranch. A worker serves every GitTarget on +// its (provider, branch), and "where is branch B" is one fact for all of them. It used to be +// delivered against whichever targets the caller happened to hold — the ones a push's writes +// named, or the one a refresh was serving — so some targets on a branch had it and some did not, +// and the layer above kept a copy per target of a value that was identical for all of them. +func TestRecordRemoteObservation_DeliversOneFactToTheWholeBranch(t *testing.T) { + m := NewWorkerManager(nil, logr.Discard(), BranchWorkerLimits{}, itypes.SensitiveResourcePolicy{}) + key := BranchKey{RepoNamespace: "shop", RepoName: "repo1", Branch: "main"} + repo := RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"} + w := &BranchWorker{Log: logr.Discard(), repo: repo} + w.remoteReporter = func(observed RemoteObservation) { m.recordRemoteObservation(key, observed) } + + _, known := m.RemoteForBranch(key) + require.False(t, known, "nothing has looked yet, which is not the same as a branch with no commits") - w.reportRemoteObservation([]itypes.ResourceReference{ - itypes.NewResourceReference("", "shop"), - itypes.NewResourceReference("checkout", ""), - itypes.NewResourceReference("checkout", "shop"), - }, RemoteObservation{Revision: "aaaa", At: time.Now(), By: ObservedByPush}) + w.recordRemoteObservation("aaaa", ObservedByPush) - require.Len(t, reported, 1, "only a fully named target can be filed against") - assert.Equal(t, "shop/checkout", reported[0].String()) + observed, known := m.RemoteForBranch(key) + require.True(t, known) + assert.Equal(t, "aaaa", observed.Revision) + assert.Equal(t, ObservedByPush, observed.By) + assert.Equal(t, repo, observed.Repo, "a revision means nothing without the repository it is in") + assert.WithinDuration(t, time.Now(), observed.At, time.Minute) } -// TestReportRemoteObservation_IsANoOpWithoutAReporter covers the CLI, which runs the same write -// path with no status surface behind it. -func TestReportRemoteObservation_IsANoOpWithoutAReporter(t *testing.T) { +// TestRecordRemoteObservation_IsANoOpWithoutAReporter covers the CLI, which runs the same write +// path with nothing keeping observations behind it. +func TestRecordRemoteObservation_IsANoOpWithoutAReporter(t *testing.T) { w := &BranchWorker{Log: logr.Discard()} - assert.NotPanics(t, func() { - w.reportRemoteObservation( - []itypes.ResourceReference{itypes.NewResourceReference("checkout", "shop")}, - RemoteObservation{}) - }) + assert.NotPanics(t, func() { w.recordRemoteObservation("aaaa", ObservedByPush) }) } -// TestSetRemoteReporter_ReachesEveryWorkerTheManagerCreates. The hook is installed once at -// startup, before any worker exists, and a worker created later has to carry it: a worker built -// without one silently drops status.remote for every target on its branch. -func TestSetRemoteReporter_ReachesEveryWorkerTheManagerCreates(t *testing.T) { - m := NewWorkerManager(nil, logr.Discard(), BranchWorkerLimits{}, itypes.SensitiveResourcePolicy{}) - called := false - m.SetRemoteReporter(func(itypes.ResourceReference, RemoteObservation) { called = true }) - - require.NotNil(t, m.remoteReporter) - m.remoteReporter(itypes.NewResourceReference("checkout", "shop"), RemoteObservation{}) - assert.True(t, called) +// TestRemoteForBranch_SurvivesTheWorkerItWasProvedBy is why the observation is kept by the manager +// and not by the worker. A worker replaced because its GitProvider names another repository takes +// its own memory with it; the layer that publishes needs the OLD observation, with the repository +// stamped on it, to see that what it has published describes a repository this GitTarget has left. +func TestRemoteForBranch_SurvivesTheWorkerItWasProvedBy(t *testing.T) { + m, ctx := startedManager(t) + key := BranchKey{RepoNamespace: "gitops-system", RepoName: "repo1", Branch: "main"} + before := RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"} + after := RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"} + + require.NoError(t, m.EnsureWorker(ctx, "repo1", "gitops-system", "main", before)) + worker, ok := m.GetWorkerForTarget("repo1", "gitops-system", "main") + require.True(t, ok) + worker.recordRemoteObservation("aaaa", ObservedByPush) + + require.NoError(t, m.EnsureWorker(ctx, "repo1", "gitops-system", "main", after)) + + observed, known := m.RemoteForBranch(key) + require.True(t, known, + "the replacement has proved nothing yet, and the old answer is still what is published") + assert.Equal(t, before, observed.Repo, + "stamped with the repository it was proved against, which is how the publisher takes it back") } // TestUpdateBranchMetadataFromPullReport_RecordsAnAbsentBranchAsNoRevision. A fetch that found no diff --git a/internal/git/render_fidelity_gate.go b/internal/git/render_fidelity_gate.go index 0e5c82f16..3d2222a00 100644 --- a/internal/git/render_fidelity_gate.go +++ b/internal/git/render_fidelity_gate.go @@ -63,6 +63,10 @@ type renderFidelityTargetState struct { // against is the SCOPE's revision. revision uint64 scopes map[types.CellKey]renderFidelityScopeResult + // awaitingFreshMeasurement says that everything this target has proved describes a repository + // it no longer writes to, so nothing it holds may admit a write. Only a COMPLETE fresh + // measurement clears it, which is the rule writeDivergence already lives by. + awaitingFreshMeasurement bool // writeDivergence is a divergence found by a live write rather than by a scope's replay, // so it belongs to no scope and no scope's replay can clear it. Only a plan that restarts // EVERY scope does — a forced recheck, or a target starting from nothing — which is the @@ -145,6 +149,14 @@ func (g *RenderFidelityGate) Reconcile( if len(scopes) > 0 && fresh == len(scopes) { state.writeDivergence = nil } + // The invalidation is cleared by a completed plan, INCLUDING an empty one. A divergence needs + // a measurement to repair it, which an empty plan is not; an invalidation needs a plan that + // has been applied against the new repository, and "this target selects nothing" is one. A + // target with no scopes is converged by every other rule here, and holding it Rechecking for + // ever would keep it on the fast reconcile loop waiting for a report nothing will ever make. + if fresh == len(scopes) { + state.awaitingFreshMeasurement = false + } g.targets[target.Key()] = state return reduceRenderFidelity(state), revisions } @@ -248,6 +260,39 @@ func (g *RenderFidelityGate) AllowsWrites(target types.ResourceReference) bool { return g.Status(target).State == RenderFidelityTrue } +// Invalidate says that everything this target has proved was proved about a repository it no +// longer writes to, and that nothing may be written until it has been measured again. +// +// It is what a branch worker's replacement needs, and Forget is not. Forgetting a target makes it +// UNREGISTERED, and an unregistered target is treated as writable — so the gate opened the moment +// the repository changed, before anything had looked at the new one. It also reset the revision +// counter, so the next plan issued revision 1 again and a result still in flight from the old +// repository, carrying that same number, was accepted as proof about the new one. +// +// So the state is kept and turned back to pending instead: every scope is given a fresh revision, +// which no running stream holds, and the target is held Unknown — including the vacuous case of a +// target with no scopes at all, where an empty plan would otherwise reduce to True — until a pass +// restarts every scope and measures the new repository. +func (g *RenderFidelityGate) Invalidate(target types.ResourceReference) { + g.mu.Lock() + defer g.mu.Unlock() + if g.targets == nil { + g.targets = map[string]renderFidelityTargetState{} + } + state := g.targets[target.Key()] + pending := make(map[types.CellKey]renderFidelityScopeResult, len(state.scopes)) + for scope := range state.scopes { + state.revision++ + pending[scope] = renderFidelityScopeResult{revision: state.revision} + } + state.scopes = pending + // A divergence found in the old repository says nothing about the new one, and the fresh + // measurement this is waiting for is exactly what would have cleared it. + state.writeDivergence = nil + state.awaitingFreshMeasurement = true + g.targets[target.Key()] = state +} + // Forget removes a deleted GitTarget's state. func (g *RenderFidelityGate) Forget(target types.ResourceReference) { g.mu.Lock() @@ -256,6 +301,14 @@ func (g *RenderFidelityGate) Forget(target types.ResourceReference) { } func reduceRenderFidelity(state renderFidelityTargetState) RenderFidelityStatus { + if state.awaitingFreshMeasurement { + return RenderFidelityStatus{ + Revision: state.revision, State: RenderFidelityUnknown, Reason: "Rechecking", + Message: "Waiting for a complete fresh measurement: what this target proved was proved " + + "about a repository it no longer writes to", + ScopeCount: len(state.scopes), CleanScopes: countCleanScopes(state), + } + } if state.writeDivergence != nil { return renderFidelityDivergedStatus(state, *state.writeDivergence, countCleanScopes(state)) } diff --git a/internal/git/render_fidelity_gate_test.go b/internal/git/render_fidelity_gate_test.go index 09e7e30f8..d825e127d 100644 --- a/internal/git/render_fidelity_gate_test.go +++ b/internal/git/render_fidelity_gate_test.go @@ -353,3 +353,114 @@ func TestRenderFidelityGate_PendingMessageNamesARefusedReport(t *testing.T) { require.True(t, applied) assert.Equal(t, RenderFidelityTrue, status.State) } + +// TestRenderFidelityGate_InvalidateClosesWritesUntilAFreshMeasurement is the replacement's half of +// the gate contract. Forgetting the target made it unregistered, and an unregistered target is +// treated as writable, so the gate opened the instant the repository changed — before anything had +// looked at the new one. +func TestRenderFidelityGate_InvalidateClosesWritesUntilAFreshMeasurement(t *testing.T) { + gate := NewRenderFidelityGate() + target := types.NewResourceReference("apps", "default") + scope := fidelityScope("apps", "deployments") + + _, revisions := restartAll(gate, target, scope) + _, applied := gate.RecordScopeClean(target, revisions[scope], scope) + require.True(t, applied) + require.True(t, gate.AllowsWrites(target), "precondition: the target is proved clean") + + gate.Invalidate(target) + + assert.Equal(t, RenderFidelityUnknown, gate.Status(target).State) + assert.False(t, gate.AllowsWrites(target), + "what this target proved was proved about a repository it no longer writes to") + + // A pass that restarts every scope is the fresh measurement, and only its result reopens. + _, after := restartAll(gate, target, scope) + assert.False(t, gate.AllowsWrites(target), "the fresh scope still owes a report") + _, applied = gate.RecordScopeClean(target, after[scope], scope) + require.True(t, applied) + assert.True(t, gate.AllowsWrites(target)) +} + +// TestRenderFidelityGate_InvalidateAdvancesEveryScopeRevision closes the other half: a result +// computed against the OLD repository can still be in flight, and a reset counter would hand it +// the very number the new plan is issuing. +func TestRenderFidelityGate_InvalidateAdvancesEveryScopeRevision(t *testing.T) { + gate := NewRenderFidelityGate() + target := types.NewResourceReference("apps", "default") + scope := fidelityScope("apps", "deployments") + + _, before := restartAll(gate, target, scope) + + gate.Invalidate(target) + _, after := restartAll(gate, target, scope) + assert.Greater(t, after[scope], before[scope], + "a revision the old repository's streams hold must never be issued again") + + _, applied := gate.RecordScopeClean(target, before[scope], scope) + assert.False(t, applied, "a clean result about the old repository proves nothing about this one") + assert.False(t, gate.AllowsWrites(target)) +} + +// TestRenderFidelityGate_InvalidateHoldsATargetWithNoScopes is the vacuous case, and it is the one +// an unregistered target got wrong: with no scopes to report, an empty plan reduces to True, so a +// target whose rules select nothing would have been writable against a repository nothing has +// looked at. Atomic requests and CommitRequests do not come from a watch. +func TestRenderFidelityGate_InvalidateHoldsATargetWithNoScopes(t *testing.T) { + gate := NewRenderFidelityGate() + target := types.NewResourceReference("apps", "default") + + gate.Invalidate(target) + + assert.Equal(t, RenderFidelityUnknown, gate.Status(target).State) + assert.False(t, gate.AllowsWrites(target)) + assert.Equal(t, "Rechecking", gate.Status(target).Reason) +} + +// TestRenderFidelityGate_InvalidateDropsAWriteDivergence keeps the two recovery routes consistent: +// a divergence found in the old repository says nothing about the new one, and the complete fresh +// measurement this waits for is exactly what would have cleared it anyway. +func TestRenderFidelityGate_InvalidateDropsAWriteDivergence(t *testing.T) { + gate := NewRenderFidelityGate() + target := types.NewResourceReference("apps", "default") + scope := fidelityScope("apps", "deployments") + + _, revisions := restartAll(gate, target, scope) + _, applied := gate.RecordScopeClean(target, revisions[scope], scope) + require.True(t, applied) + require.Equal(t, RenderFidelityFalse, + gate.Fail(target, manifestanalyzer.RenderDivergence{Token: "x", Field: "spec.replicas"}).State) + + gate.Invalidate(target) + assert.Equal(t, RenderFidelityUnknown, gate.Status(target).State, + "the old repository's divergence is neither carried over nor reported as clean") + + _, after := restartAll(gate, target, scope) + _, applied = gate.RecordScopeClean(target, after[scope], scope) + require.True(t, applied) + assert.True(t, gate.AllowsWrites(target)) +} + +// TestRenderFidelityGate_AnEmptyPlanCompletesAnInvalidation. A target whose rules select nothing +// is converged by every other rule in this file, and an invalidation it could never clear would +// hold it Rechecking for ever — on the fast reconcile loop, waiting for a report no stream will +// make. An empty plan is not a measurement, so it still does not clear a write DIVERGENCE; it is +// an applied plan, which is what an invalidation is waiting for. +func TestRenderFidelityGate_AnEmptyPlanCompletesAnInvalidation(t *testing.T) { + gate := NewRenderFidelityGate() + target := types.NewResourceReference("apps", "default") + scope := fidelityScope("apps", "deployments") + + _, revisions := restartAll(gate, target, scope) + _, applied := gate.RecordScopeClean(target, revisions[scope], scope) + require.True(t, applied) + + gate.Invalidate(target) + require.False(t, gate.AllowsWrites(target)) + + // The target's rules now select nothing, and that plan lands. + status, _ := gate.Reconcile(target, nil, nil) + + assert.Equal(t, RenderFidelityTrue, status.State) + assert.True(t, gate.AllowsWrites(target), "nothing to mirror is converged, before and after a replacement") +} diff --git a/internal/git/resync_flush.go b/internal/git/resync_flush.go index d7f8f68ae..b4e1d4400 100644 --- a/internal/git/resync_flush.go +++ b/internal/git/resync_flush.go @@ -112,17 +112,6 @@ func (l *branchWorkerEventLoop) prepareBaseForResync(req *ResyncRequest) error { return err } - // A resync ALWAYS fetches, and that fetch proves where the branch is exactly as a refresh's - // does. Without this the worker knew more than its own status said: a resync could discover a - // commit somebody else pushed and leave status.remote naming the revision before it — - // indefinitely, on an install that has turned the periodic refresh off. The target is in hand - // here, which is the only thing reporting needs. - if observed, known := l.w.LastRemoteObservation(); known { - l.w.reportRemoteObservation( - []itypes.ResourceReference{ - itypes.NewResourceReference(req.GitTargetName, req.GitTargetNamespace), - }, observed) - } return nil } diff --git a/internal/git/resync_flush_test.go b/internal/git/resync_flush_test.go index b26417a1d..126d9b8c0 100644 --- a/internal/git/resync_flush_test.go +++ b/internal/git/resync_flush_test.go @@ -440,7 +440,7 @@ func TestResync_DropsOneDocFromMultiDocKeepsSiblings(t *testing.T) { // A resync ALWAYS fetches — applyResync may find nothing to change and so never open a push, which // is why it cannot rely on the advertisement a publication would have read — and that fetch proves // where the branch is exactly as a refresh's does. Before this, the observation was recorded in -// worker memory and reported to nobody, so a resync could discover somebody else's commit and +// worker memory and delivered to nobody, so a resync could discover somebody else's commit and // leave status.remote naming the revision before it. On an install that has turned the periodic // refresh off, that is indefinite. func TestResync_ReportsWhatItsFetchProved(t *testing.T) { @@ -448,9 +448,7 @@ func TestResync_ReportsWhatItsFetchProved(t *testing.T) { createPlainGitTarget(t, worker, "team-a", "team-a") var reported []RemoteObservation - var reportedFor []types.ResourceReference - worker.remoteReporter = func(target types.ResourceReference, observed RemoteObservation) { - reportedFor = append(reportedFor, target) + worker.remoteReporter = func(observed RemoteObservation) { reported = append(reported, observed) } @@ -468,7 +466,6 @@ func TestResync_ReportsWhatItsFetchProved(t *testing.T) { }) require.NotEmpty(t, reported, "the resync's own fetch must reach the status surface") - assert.Equal(t, ObservedByFetch, reported[len(reported)-1].By) - assert.Equal(t, "team-a", reportedFor[len(reportedFor)-1].Name, - "reported against the target the resync was for, which is the one in hand") + assert.Equal(t, ObservedByFetch, reported[len(reported)-1].By, + "delivered to the branch, which is what every GitTarget on it reads") } diff --git a/internal/git/types.go b/internal/git/types.go index 72196920b..e14f6b273 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -5,6 +5,7 @@ package git import ( "errors" "fmt" + "net/url" "sort" "strings" @@ -12,6 +13,7 @@ import ( "github.com/go-git/go-git/v6/plumbing" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" + k8stypes "k8s.io/apimachinery/pkg/types" v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" @@ -161,6 +163,48 @@ func (k BranchKey) String() string { return fmt.Sprintf("%s/%s/%s", k.RepoNamespace, k.RepoName, k.Branch) } +// RepoIdentity is WHICH REPOSITORY a branch worker's clone, base trust and observations are +// about. It is what BranchKey does not say: every field of that key comes off the GitTarget, and +// none of them mentions the repository. +// +// The UID carries the identity because spec.url is immutable, so within the lifetime of one +// GitProvider object the repository cannot change: a new object is the only way to reach a new +// one, and that mints a new UID. The URL rides along as a safety net for an immutability rule +// that was bypassed — a CRD reinstalled without the CEL rule, or a write straight to etcd. It is +// compared EXACTLY, with no normalization, because a re-spelled URL can only arrive with a +// recreate, which the UID has already caught. +// +// It is deliberately NOT part of BranchKey. That key is derivable from the GitTarget alone, which +// is what lets the event router resolve a worker with a map read and no GitProvider in hand; a key +// naming the repository would put a provider read on the per-event path, and would let two workers +// for one (provider, branch) report under one set of metric labels while both were live. +type RepoIdentity struct { + // ProviderUID is the GitProvider object's metadata.uid. + ProviderUID k8stypes.UID + // URL is the GitProvider's spec.url, exactly as it is written there. + URL string +} + +// IsZero reports that nothing is known about the repository. The CLI and tests that never touch a +// remote build workers this way, and a comparison against an unknown identity proves nothing. +func (r RepoIdentity) IsZero() bool { return r.ProviderUID == "" && r.URL == "" } + +// String is for logs: enough to tell two repositories apart without printing a credential. +// +// spec.url is validated for length and nothing else, so it can carry userinfo +// (https://user:token@host/repo.git), and a replacement logs both identities at default +// verbosity. The userinfo is stripped rather than the whole URL withheld, because telling two +// repositories apart is the entire job of this line. A URL that will not parse is printed as +// written: it reached no remote, so it holds no credential a remote accepted. +func (r RepoIdentity) String() string { + shown := r.URL + if parsed, err := url.Parse(r.URL); err == nil && parsed.User != nil { + parsed.User = nil + shown = parsed.String() + } + return fmt.Sprintf("%s (uid %s)", shown, r.ProviderUID) +} + // UserInfo contains relevant user information for commit messages. type UserInfo struct { Username string diff --git a/internal/git/worker_manager.go b/internal/git/worker_manager.go index bc6b816c1..566b6af72 100644 --- a/internal/git/worker_manager.go +++ b/internal/git/worker_manager.go @@ -6,6 +6,7 @@ import ( "context" "fmt" "sync" + "time" "github.com/go-logr/logr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -123,11 +124,6 @@ type WorkerManager struct { // CLI and in tests that do not assert on status.placement. layoutReporter LayoutReporter - // remoteReporter publishes each confirmed observation of a branch's remote state to the - // GitTarget status surface. Set once at startup (SetRemoteReporter) before any worker is - // created; nil in the CLI and in tests that do not assert on status.remote. - remoteReporter RemoteReporter - // scanAcceptance publishes a read-only folder scan's verdict to the GitTarget status surface. // Set once at startup (SetScanAcceptanceReporter) before any worker is created; nil in the CLI // and in tests that do not assert on GitPathAccepted. @@ -136,6 +132,26 @@ type WorkerManager struct { // renderFidelityGate is shared by every worker and the watch manager. It is created with the // manager so a target's state survives workers being recreated for the same branch. renderFidelityGate *RenderFidelityGate + + // remotes is what each branch was last PROVED to be at, keyed by branch and guarded by mu. + // + // It lives here rather than on the worker because it has to survive one: a worker replaced + // because its GitProvider names another repository takes its own observation with it, and the + // layer that publishes needs the old one to see that what it has published describes a + // repository this GitTarget has left. It is dropped only when the branch itself is retired. + remotes map[BranchKey]RemoteObservation + + // replacements are the branches whose worker was replaced and whose GitTargets have not been + // re-established since. Guarded by mu. + // + // A replacement is not finished when EnsureWorker returns: the new worker has no clone, no + // base trust and an empty queue, so every GitTarget on that branch has to be told to + // re-anchor its streams. That delivery can fail — it lists the targets, and a list can fail — + // and a one-shot "I replaced something" return value was then spent: later calls reported no + // replacement, so nothing ever asked again and an idle target could leave the new repository + // empty indefinitely. Recorded here instead, a pending recovery survives every failure until + // something acknowledges it. + replacements map[BranchKey]struct{} } // NewWorkerManager creates a new worker manager. limits bounds every worker this manager @@ -152,6 +168,8 @@ func NewWorkerManager( limits: limits.withDefaults(), sensitiveResources: sensitiveResources, workers: make(map[BranchKey]*BranchWorker), + remotes: make(map[BranchKey]RemoteObservation), + replacements: make(map[BranchKey]struct{}), renderFidelityGate: NewRenderFidelityGate(), } } @@ -225,44 +243,95 @@ func (m *WorkerManager) SetScanAcceptanceReporter(reporter ScanAcceptanceReporte m.scanAcceptance = reporter } -// SetRemoteReporter injects the hook every worker calls after it proves where its branch is on -// the remote, so status.remote reflects the last confirmed look rather than being learned and -// dropped. Like SetLayoutReporter, it is called once at startup before any worker is created. -func (m *WorkerManager) SetRemoteReporter(reporter RemoteReporter) { +// recordRemoteObservation is the delivery point for one branch's confirmed observation. Every +// worker is given it bound to its own key; it is called on the worker's loop goroutine. +func (m *WorkerManager) recordRemoteObservation(key BranchKey, observed RemoteObservation) { m.mu.Lock() defer m.mu.Unlock() - m.remoteReporter = reporter + if m.remotes == nil { + m.remotes = map[BranchKey]RemoteObservation{} + } + m.remotes[key] = observed +} + +// RemoteForBranch is what this branch was last proved to be at, and whether anything has looked at +// all. It opens no connection and reads no repository. +// +// It answers for the BRANCH, which is the whole of the delivery half: two GitTargets writing +// different folders of one branch share a worker, so they share its observation and the moment it +// was proved. Absent means nothing has looked yet, which is different from a present observation +// with no revision: that one means the branch is not on the remote. +func (m *WorkerManager) RemoteForBranch(key BranchKey) (RemoteObservation, bool) { + m.mu.RLock() + defer m.mu.RUnlock() + observed, known := m.remotes[key] + return observed, known } -// EnsureWorker ensures a worker exists for the given (provider, branch). +// EnsureWorker ensures a worker exists for the given (provider, branch), and that it is the +// worker for the repository the GitProvider names NOW. +// +// repo is the caller's answer to "which repository is this"; the reconcile has already read the +// GitProvider one gate earlier, so it costs no API call. A slot holding a worker for a DIFFERENT +// repository is not corrected field by field — the clone, the base trust, the observation and the +// retained writes are all statements about the old one — so that worker is stopped, its checkout +// reclaimed, and a fresh one takes the slot. +// +// A replacement is not complete when it returns: the new worker has no clone and no queue, so +// every GitTarget on that branch has to re-establish its folder against the new repository. That +// is recorded as a PENDING RECOVERY rather than returned, because arranging it is the caller's job +// and the caller can fail. See ReplacementPending; the delivery is the GitTarget reconcile's +// worker wiring gate. +// // Worker creation/start is protected by the manager lock. func (m *WorkerManager) EnsureWorker( _ context.Context, providerName, providerNamespace string, branch string, + repo RepoIdentity, ) error { // Held across the whole check-and-create so a replacement cannot start while the worker it // replaces is still stopping; they would share a clone. m.lifecycleMu.Lock() defer m.lifecycleMu.Unlock() - m.mu.Lock() - defer m.mu.Unlock() - key := BranchKey{ RepoNamespace: providerNamespace, RepoName: providerName, Branch: branch, } - if _, exists := m.workers[key]; !exists { - m.Log.Info("Creating new branch worker", "key", key.String()) + m.mu.RLock() + existing, exists := m.workers[key] + m.mu.RUnlock() + + if exists { + if existing.repo == repo { + return nil + } + // Both identities, at default verbosity: a comparison that is not stable across steady + // reconciles is a restart loop, and this line is what makes one visible. + m.Log.Info("GitProvider now names a different repository; replacing the branch worker", + "key", key.String(), + "was", existing.repo.String(), + "now", repo.String()) + // lifecycleMu is already held, so the detach-and-stop must not take it again. + m.removeWorkersLocked([]BranchKey{key}, "the GitProvider names a different repository") + m.noteReplacement(key) + } + + m.mu.Lock() + defer m.mu.Unlock() + + if _, live := m.workers[key]; !live { + m.Log.Info("Creating new branch worker", "key", key.String(), "repository", repo.String()) worker := NewBranchWorker( m.Client, m.Log.WithName("branch-worker"), providerName, providerNamespace, branch, + repo, newContentWriter(m.sensitiveResources), m.limits, ) @@ -275,7 +344,7 @@ func (m *WorkerManager) EnsureWorker( worker.credentialPolicy = m.credentialPolicy worker.pathRefusal = m.pathRefusal worker.layoutReporter = m.layoutReporter - worker.remoteReporter = m.remoteReporter + worker.remoteReporter = func(observed RemoteObservation) { m.recordRemoteObservation(key, observed) } worker.scanAcceptance = m.scanAcceptance worker.renderFidelityGate = m.renderFidelityGate @@ -289,6 +358,40 @@ func (m *WorkerManager) EnsureWorker( return nil } +// noteReplacement records that a branch's GitTargets have to be re-established. It is called with +// lifecycleMu held and takes mu itself, which is the lock order everything here uses. +func (m *WorkerManager) noteReplacement(key BranchKey) { + m.mu.Lock() + defer m.mu.Unlock() + if m.replacements == nil { + m.replacements = map[BranchKey]struct{}{} + } + m.replacements[key] = struct{}{} +} + +// ReplacementPending reports whether this branch's GitTargets still have to be re-established +// after their worker was replaced. +// +// It answers for the BRANCH rather than for the caller, so whichever GitTarget reconciles next +// picks the recovery up — including the siblings, which have no reason of their own to notice +// that the repository changed. +func (m *WorkerManager) ReplacementPending(key BranchKey) bool { + m.mu.RLock() + defer m.mu.RUnlock() + _, pending := m.replacements[key] + return pending +} + +// AcknowledgeReplacement clears a pending recovery, and must be called only once the targets on +// the branch have actually been told. Anything less leaves the request standing, which is the +// whole point of keeping it here: a failed delivery is retried by the next reconcile of any +// GitTarget on the branch, and by the periodic sweep that follows them. +func (m *WorkerManager) AcknowledgeReplacement(key BranchKey) { + m.mu.Lock() + defer m.mu.Unlock() + delete(m.replacements, key) +} + // removeWorkers detaches the named workers and stops them. // // The lock discipline is the whole reason this is one function rather than a loop at each call @@ -304,8 +407,9 @@ func (m *WorkerManager) removeWorkers(keys []BranchKey, reason string) { m.removeWorkersLocked(keys, reason) } -// removeWorkersLocked is removeWorkers for a caller that already holds lifecycleMu, so a sweep can -// hold it across choosing the workers to retire and retiring them. +// removeWorkersLocked is removeWorkers for a caller that already holds lifecycleMu: a sweep holding +// it across choosing the workers to retire and retiring them, and EnsureWorker holding it across +// its whole check-and-create, where a second acquisition would deadlock. func (m *WorkerManager) removeWorkersLocked(keys []BranchKey, reason string) { if len(keys) == 0 { return @@ -376,7 +480,20 @@ var afterOrphanSelection func() // It fails safe. A List that errors stops nothing: the alternative — treating "I could not read // the targets" as "there are no targets" — would take down every live worker in the process. func (m *WorkerManager) ReconcileWorkers(ctx context.Context) error { - // The List and the Stops stay outside m.mu, for the reason in removeWorkers. + // lifecycleMu is held across the whole decision — the List, the selection and the removal — + // and the three must not be separated. + // + // EnsureWorker takes this lock, finds the key present and returns "already there" without + // creating anything, so a selection made outside it would let this sweep stop a worker a + // GitTarget had just been told it has. The LIST is inside for the same reason read the other + // way round: a target created after the snapshot is taken is absent from it, and its worker — + // created under this lock moments later — then looks like an orphan and is stopped with its + // queue and its checkout. The client is cached, so the read costs no round trip. + // + // m.mu is still taken only for the map itself, and the Stops still run outside it, for the + // reason in removeWorkers. + m.lifecycleMu.Lock() + defer m.lifecycleMu.Unlock() // Get all GitTargets var targetList configv1alpha3.GitTargetList @@ -403,14 +520,6 @@ func (m *WorkerManager) ReconcileWorkers(ctx context.Context) error { neededWorkers[key] = true } - // lifecycleMu is held across the SELECTION as well as the removal, and the two must not be - // separated. EnsureWorker takes this lock, finds the key present and returns "already there" - // without creating anything; if it ran between a selection made outside the lock and the - // removal, this sweep would then stop the worker that GitTarget had just been told it has, - // leaving it with no worker until its next reconcile. - m.lifecycleMu.Lock() - defer m.lifecycleMu.Unlock() - m.mu.RLock() orphans := make([]BranchKey, 0, len(m.workers)) for key := range m.workers { @@ -427,6 +536,14 @@ func (m *WorkerManager) ReconcileWorkers(ctx context.Context) error { } m.removeWorkersLocked(orphans, "no GitTarget needs this worker any more") + // A branch no GitTarget needs has nothing left to re-establish and nothing left to report: a + // recovery or an observation held for it would outlive every object it was about. + m.mu.Lock() + for _, key := range orphans { + delete(m.replacements, key) + delete(m.remotes, key) + } + m.mu.Unlock() m.mu.RLock() remaining := len(m.workers) @@ -451,7 +568,7 @@ func (m *WorkerManager) Start(ctx context.Context) error { telemetry.SetGaugeSource(telemetry.GaugeGitQueueDepth, m.queueDepthSamples) m.Log.Info("WorkerManager started") - <-ctx.Done() + m.sweepPeriodically(ctx) // Clear the source before the workers go, so the callback cannot outlive them. telemetry.SetGaugeSource(telemetry.GaugeGitQueueDepth, nil) @@ -473,6 +590,40 @@ func (m *WorkerManager) Start(ctx context.Context) error { return nil } +// WorkerSweepInterval is the floor under the orphan sweep. It is a FLOOR and not the mechanism: a +// deleted GitTarget's reconcile sweeps immediately, and this catches what that path cannot see. +const WorkerSweepInterval = time.Minute + +// sweepPeriodically runs the orphan sweep until the context ends. +// +// The delete-triggered sweep is prompt but not complete, because it runs only where a reconcile +// READ NotFound. Delete a GitTarget and recreate it under the same name on another branch before +// that reconcile runs, and the controller sees the successor, wires its worker, and never learns +// that the predecessor existed: the old branch's worker, its goroutine and its clone are then held +// until the process restarts. Kubernetes reconciliation cannot be built on observing every +// intermediate state, so the sweep needs a trigger that does not depend on seeing the delete. +// +// It is a separate goroutine rather than a step in the GitTarget reconcile on purpose: the sweep +// takes lifecycleMu and holds it across Stop, and GitTarget reconciles are serialized, so a slow +// stop on the reconcile path would stall every target's reconcile behind it. +func (m *WorkerManager) sweepPeriodically(ctx context.Context) { + ticker := time.NewTicker(WorkerSweepInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + if err := m.ReconcileWorkers(ctx); err != nil { + // Failing safe: a List that errors stops nothing, so the only cost is that the + // sweep is late. + m.Log.V(1).Info("Periodic worker sweep could not read the GitTargets", + "error", err.Error()) + } + } + } +} + // queueDepthSamples is the git_queue_depth source: one sample per live worker, read when // Prometheus scrapes. // diff --git a/internal/git/worker_manager_test.go b/internal/git/worker_manager_test.go index da3f760f3..2ba09b120 100644 --- a/internal/git/worker_manager_test.go +++ b/internal/git/worker_manager_test.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/runtime" + k8stypes "k8s.io/apimachinery/pkg/types" clientgoscheme "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -42,7 +43,8 @@ func TestWorkerManager_SetMapperInjectsIntoWorkers(t *testing.T) { go func() { _ = manager.Start(ctx) }() time.Sleep(100 * time.Millisecond) // allow Start to set m.ctx - require.NoError(t, manager.EnsureWorker(ctx, "repo1", testProviderNamespace, "main")) + repo := createProviderWithLocalRepo(ctx, t, client, "repo1") + mustEnsureWorker(ctx, t, manager, "repo1", testProviderNamespace, "main", repo) worker, exists := manager.GetWorkerForTarget("repo1", testProviderNamespace, "main") require.True(t, exists) require.NotNil(t, worker) @@ -55,6 +57,24 @@ const ( testTargetNamespace = "default" ) +// mustEnsureWorker wires a worker and fails the test if it could not. It then does what a +// GitTarget reconcile does with the branch: take any pending replacement recovery and acknowledge +// it, reporting whether there was one. +func mustEnsureWorker( + ctx context.Context, + t *testing.T, + m *WorkerManager, + providerName, providerNamespace, branch string, + repo RepoIdentity, +) bool { + t.Helper() + require.NoError(t, m.EnsureWorker(ctx, providerName, providerNamespace, branch, repo)) + key := BranchKey{RepoNamespace: providerNamespace, RepoName: providerName, Branch: branch} + pending := m.ReplacementPending(key) + m.AcknowledgeReplacement(key) + return pending +} + func setupScheme() *runtime.Scheme { scheme := runtime.NewScheme() _ = clientgoscheme.AddToScheme(scheme) @@ -62,12 +82,14 @@ func setupScheme() *runtime.Scheme { return scheme } +// createProviderWithLocalRepo creates a GitProvider over a local bare repository and returns the +// identity a reconcile would hand EnsureWorker for it. func createProviderWithLocalRepo( ctx context.Context, t *testing.T, k8sClient client.Client, name string, -) { +) RepoIdentity { t.Helper() remotePath := filepath.Join(t.TempDir(), name+".git") @@ -80,7 +102,9 @@ func createProviderWithLocalRepo( } provider.Name = name provider.Namespace = testProviderNamespace + provider.UID = k8stypes.UID("uid-" + name) require.NoError(t, k8sClient.Create(ctx, provider)) + return RepoIdentity{ProviderUID: provider.UID, URL: provider.Spec.URL} } func createTargetForRegister( @@ -116,11 +140,11 @@ func TestEnsureWorker_CreatesAWorkerWithTheIdentityItWasAskedFor(t *testing.T) { _ = manager.Start(ctx) }() time.Sleep(100 * time.Millisecond) // Allow manager to start - createProviderWithLocalRepo(ctx, t, client, "repo1") + repo := createProviderWithLocalRepo(ctx, t, client, "repo1") createTargetForRegister(ctx, t, client, "target1", "repo1", "main", "clusters/prod") // Register first target - err := manager.EnsureWorker(ctx, "repo1", "gitops-system", "main") + err := manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", repo) if err != nil { t.Fatalf("Failed to register target: %v", err) } @@ -167,20 +191,13 @@ func TestWorkerManagerMultipleTargetsSameBranch(t *testing.T) { _ = manager.Start(ctx) }() time.Sleep(100 * time.Millisecond) - createProviderWithLocalRepo(ctx, t, client, "shared-repo") + repo := createProviderWithLocalRepo(ctx, t, client, "shared-repo") createTargetForRegister(ctx, t, client, "target-apps", "shared-repo", "main", "apps/") createTargetForRegister(ctx, t, client, "target-infra", "shared-repo", "main", "infra/") // Register two targets for same repo+branch, different paths - err := manager.EnsureWorker(ctx, "shared-repo", "gitops-system", "main") - if err != nil { - t.Fatalf("Failed to register target-apps: %v", err) - } - - err = manager.EnsureWorker(ctx, "shared-repo", "gitops-system", "main") - if err != nil { - t.Fatalf("Failed to register target-infra: %v", err) - } + mustEnsureWorker(ctx, t, manager, "shared-repo", "gitops-system", "main", repo) + mustEnsureWorker(ctx, t, manager, "shared-repo", "gitops-system", "main", repo) // Verify only one worker exists manager.mu.RLock() @@ -215,17 +232,17 @@ func TestWorkerManagerDifferentBranches(t *testing.T) { _ = manager.Start(ctx) }() time.Sleep(100 * time.Millisecond) - createProviderWithLocalRepo(ctx, t, client, "repo1") + repo := createProviderWithLocalRepo(ctx, t, client, "repo1") createTargetForRegister(ctx, t, client, "target-main", "repo1", "main", "base/") createTargetForRegister(ctx, t, client, "target-dev", "repo1", "develop", "base/") // Register targets for same repo, different branches - err := manager.EnsureWorker(ctx, "repo1", "gitops-system", "main") + err := manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", repo) if err != nil { t.Fatalf("Failed to register target-main: %v", err) } - err = manager.EnsureWorker(ctx, "repo1", "gitops-system", "develop") + err = manager.EnsureWorker(ctx, "repo1", "gitops-system", "develop", repo) if err != nil { t.Fatalf("Failed to register target-dev: %v", err) } @@ -275,11 +292,11 @@ func TestReconcileWorkers_KeepsAWorkerASiblingStillNeeds(t *testing.T) { go func() { _ = manager.Start(ctx) }() time.Sleep(100 * time.Millisecond) - createProviderWithLocalRepo(ctx, t, k8sClient, "repo1") + repo := createProviderWithLocalRepo(ctx, t, k8sClient, "repo1") createTargetForRegister(ctx, t, k8sClient, "target1", "repo1", "main", "apps/") createTargetForRegister(ctx, t, k8sClient, "target2", "repo1", "main", "infra/") // The provider lives beside its targets, which is the namespace the sweep derives. - require.NoError(t, manager.EnsureWorker(ctx, "repo1", testTargetNamespace, "main")) + mustEnsureWorker(ctx, t, manager, "repo1", testTargetNamespace, "main", repo) // One of the two goes. require.NoError(t, k8sClient.Delete(ctx, targetForRegister("target1"))) @@ -314,7 +331,8 @@ func TestReconcileWorkers_StopsNothingWhenTheTargetsCannotBeRead(t *testing.T) { defer cancel() go func() { _ = manager.Start(ctx) }() time.Sleep(100 * time.Millisecond) - require.NoError(t, manager.EnsureWorker(ctx, "repo1", testTargetNamespace, "main")) + mustEnsureWorker(ctx, t, manager, "repo1", testTargetNamespace, "main", + RepoIdentity{URL: "file:///unread.git"}) err := manager.ReconcileWorkers(ctx) @@ -337,14 +355,14 @@ func TestWorkerManagerConcurrentRegistration(t *testing.T) { _ = manager.Start(ctx) }() time.Sleep(100 * time.Millisecond) - createProviderWithLocalRepo(ctx, t, client, "repo1") + repo := createProviderWithLocalRepo(ctx, t, client, "repo1") createTargetForRegister(ctx, t, client, "target", "repo1", "main", "base/") // Concurrently register multiple targets done := make(chan bool, 10) for i := range 10 { go func(index int) { - err := manager.EnsureWorker(ctx, "repo1", "gitops-system", "main") + err := manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", repo) if err != nil { t.Errorf("Failed to ensure worker %d: %v", index, err) } @@ -417,9 +435,9 @@ func TestReconcileWorkers_RemovesTheRetiredWorkersCheckout(t *testing.T) { go func() { _ = manager.Start(ctx) }() time.Sleep(100 * time.Millisecond) - createProviderWithLocalRepo(ctx, t, k8sClient, "repo1") + repo := createProviderWithLocalRepo(ctx, t, k8sClient, "repo1") createTargetForRegister(ctx, t, k8sClient, "target1", "repo1", "main", "apps/") - require.NoError(t, manager.EnsureWorker(ctx, "repo1", testTargetNamespace, "main")) + mustEnsureWorker(ctx, t, manager, "repo1", testTargetNamespace, "main", repo) worker, exists := manager.GetWorkerForTarget("repo1", testTargetNamespace, "main") require.True(t, exists) diff --git a/internal/git/worker_repo_identity_test.go b/internal/git/worker_repo_identity_test.go new file mode 100644 index 000000000..3721dd0b5 --- /dev/null +++ b/internal/git/worker_repo_identity_test.go @@ -0,0 +1,406 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +// A branch worker is keyed by (GitProvider namespace, GitProvider name, branch), and none of those +// fields says which REPOSITORY it is about. spec.url is immutable and repointed by deleting the +// GitProvider and creating it again, which the key survives untouched, so one live worker could +// meet a second repository holding everything it learned about the first: its clone, its base +// trust, its observation and its retained writes. +// +// These tests are about the replacement that makes that impossible. Meeting a different repository +// is not a state change to be undone field by field; it is a different subject, so the worker for +// the old one is stopped and a fresh one takes the slot. + +import ( + "context" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// startedManager is a WorkerManager with its Runnable context published, which is what EnsureWorker +// hands to every worker it starts. +func startedManager(t *testing.T) (*WorkerManager, context.Context) { + t.Helper() + + // Replacing a worker DELETES its on-disk state, so every test here points that state at its + // own directory rather than the shared root a developer's operator also writes under. + withTemporaryWorkerStateRoot(t) + + k8sClient := fake.NewClientBuilder().WithScheme(setupScheme()).Build() + manager := NewWorkerManager(k8sClient, logr.Discard(), BranchWorkerLimits{}, types.SensitiveResourcePolicy{}) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + t.Cleanup(cancel) + go func() { _ = manager.Start(ctx) }() + require.Eventually(t, func() bool { + manager.mu.RLock() + defer manager.mu.RUnlock() + return manager.ctx != nil + }, 5*time.Second, 10*time.Millisecond, "the manager never published its context") + return manager, ctx +} + +// TestEnsureWorker_KeepsTheWorkerWhenTheRepositoryIsUnchanged is the rule every other test here +// depends on. The comparison runs on EVERY reconcile of every GitTarget on the branch, so an +// identity that is not stable across steady ticks is not a correctness detail: it is a worker +// restart loop, and one that would throw away a commit window each time round. +func TestEnsureWorker_KeepsTheWorkerWhenTheRepositoryIsUnchanged(t *testing.T) { + manager, ctx := startedManager(t) + repo := RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/repo.git"} + + mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", repo) + first, ok := manager.GetWorkerForTarget("repo1", "gitops-system", "main") + require.True(t, ok) + + for range 3 { + mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", repo) + } + + second, ok := manager.GetWorkerForTarget("repo1", "gitops-system", "main") + require.True(t, ok) + assert.Same(t, first, second, "a steady tick must leave the worker exactly where it was") + assert.NoError(t, first.ctx.Err(), "and must not have stopped it") +} + +// TestEnsureWorker_ReplacesTheWorkerWhenTheProviderIsRecreated is the ordinary repoint: the +// operator deletes the GitProvider and creates it again against another repository, which mints a +// new UID. +func TestEnsureWorker_ReplacesTheWorkerWhenTheProviderIsRecreated(t *testing.T) { + manager, ctx := startedManager(t) + before := RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"} + after := RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"} + + mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", before) + old, ok := manager.GetWorkerForTarget("repo1", "gitops-system", "main") + require.True(t, ok) + + mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", after) + + replacement, ok := manager.GetWorkerForTarget("repo1", "gitops-system", "main") + require.True(t, ok) + assert.NotSame(t, old, replacement, "the slot holds the worker for the repository named NOW") + assert.Equal(t, after, replacement.repo) + assert.Error(t, old.ctx.Err(), "the worker for the old repository must have been stopped") +} + +// TestEnsureWorker_ReplacesOnARecreateThatKeepsTheURL. The UID is the identity, and it changes on +// every recreate — including one that spells the same URL. Replacing there costs a re-clone of +// state that was arguably still good; not replacing would mean trusting that a repository somebody +// recreated the provider for is the same one, which is the belief this whole mechanism removes. +func TestEnsureWorker_ReplacesOnARecreateThatKeepsTheURL(t *testing.T) { + manager, ctx := startedManager(t) + const url = "https://example.invalid/repo.git" + + mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", + RepoIdentity{ProviderUID: "uid-1", URL: url}) + old, ok := manager.GetWorkerForTarget("repo1", "gitops-system", "main") + require.True(t, ok) + + mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", + RepoIdentity{ProviderUID: "uid-2", URL: url}) + + replacement, ok := manager.GetWorkerForTarget("repo1", "gitops-system", "main") + require.True(t, ok) + assert.NotSame(t, old, replacement, "a recreated GitProvider is a new object, so it is a new subject") +} + +// TestEnsureWorker_ReplacesWhenTheURLMovedUnderTheSameObject is the safety-net half of the pair. +// spec.url is immutable, so a URL that changes without a new UID can only come from a rule that was +// bypassed — a CRD reinstalled without the CEL validation, or a write straight to etcd. The UID +// cannot see it, which is exactly why the URL is compared beside it. +func TestEnsureWorker_ReplacesWhenTheURLMovedUnderTheSameObject(t *testing.T) { + manager, ctx := startedManager(t) + + mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", + RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"}) + old, ok := manager.GetWorkerForTarget("repo1", "gitops-system", "main") + require.True(t, ok) + + mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", + RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/second.git"}) + + replacement, ok := manager.GetWorkerForTarget("repo1", "gitops-system", "main") + require.True(t, ok) + assert.NotSame(t, old, replacement, "a differing URL is a different repository, whatever the UID says") +} + +// TestEnsureWorker_ReplacementTakesTheOldCheckoutWithIt. The clone is the half of the worker that +// survives a restart, and it is keyed by remote URL under the worker's own directory: leaving it +// behind would accumulate one tree per repository the operator ever repointed away from. +func TestEnsureWorker_ReplacementTakesTheOldCheckoutWithIt(t *testing.T) { + manager, ctx := startedManager(t) + const oldURL = "https://example.invalid/first.git" + + mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", + RepoIdentity{ProviderUID: "uid-1", URL: oldURL}) + old, ok := manager.GetWorkerForTarget("repo1", "gitops-system", "main") + require.True(t, ok) + + // Stand in for the clone a first publication would have left behind. + checkout := old.repoPathForRemote(oldURL) + require.NoError(t, os.MkdirAll(checkout, 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(checkout, "HEAD"), []byte("ref: refs/heads/main\n"), 0o600)) + + mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", + RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"}) + + _, err := os.Stat(checkout) + assert.True(t, os.IsNotExist(err), "the replaced worker's checkout must go with it") +} + +// TestEnsureWorker_ReplacementDropsWhatTheOldWorkerWasHolding states the one behaviour change +// deliberately: queued events do not follow the worker into the new repository. +// +// They were computed against the old tree, any local commits behind retained writes are in the old +// clone, and the resync rebuilds the whole folder from the cluster. Replaying them would be writing +// one repository's content into another on the strength of a queue position. +func TestEnsureWorker_ReplacementDropsWhatTheOldWorkerWasHolding(t *testing.T) { + manager, ctx := startedManager(t) + + mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", + RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"}) + old, ok := manager.GetWorkerForTarget("repo1", "gitops-system", "main") + require.True(t, ok) + + // Enqueued without a loop that can reach a remote, so they are still in hand at replacement. + old.inflightItems.Add(2) + old.eventQueue <- WorkItem{} + old.eventQueue <- WorkItem{} + + mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", + RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"}) + + replacement, ok := manager.GetWorkerForTarget("repo1", "gitops-system", "main") + require.True(t, ok) + assert.Zero(t, replacement.queueDepth(), + "the new repository starts empty; nothing computed against the old tree follows it") +} + +// TestEnsureWorker_TheSlotReportsOneQueueDepthAcrossAReplacement. git_queue_depth is labelled +// (provider namespace, provider name, branch) — the worker's key, which a replacement does not +// change — so two workers live at once for one slot would publish two series under one label set +// and silently add them together. +func TestEnsureWorker_TheSlotReportsOneQueueDepthAcrossAReplacement(t *testing.T) { + manager, ctx := startedManager(t) + + mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", + RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"}) + require.Len(t, manager.queueDepthSamples(), 1) + + mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", + RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"}) + + assert.Len(t, manager.queueDepthSamples(), 1, + "one slot is one series, whatever happened to the worker behind it") +} + +// TestRepoIdentityString_DoesNotPrintACredential. `spec.url` is validated for length and nothing +// else, so it accepts userinfo, and a replacement logs both identities at default verbosity. The +// line has to stay useful — telling two repositories apart is its whole job — so the userinfo is +// stripped rather than the URL withheld. +func TestRepoIdentityString_DoesNotPrintACredential(t *testing.T) { + for _, tc := range []struct { + name string + url string + want string + }{ + { + "user and token", + "https://git:ghp_supersecret@example.invalid/repo.git", + "https://example.invalid/repo.git (uid uid-1)", + }, + {"user only", "https://git@example.invalid/repo.git", "https://example.invalid/repo.git (uid uid-1)"}, + {"no userinfo", "https://example.invalid/repo.git", "https://example.invalid/repo.git (uid uid-1)"}, + {"ssh scp form, which url.Parse does not read as a URL", "git@example.invalid:org/repo.git", + "git@example.invalid:org/repo.git (uid uid-1)"}, + } { + t.Run(tc.name, func(t *testing.T) { + got := RepoIdentity{ProviderUID: "uid-1", URL: tc.url}.String() + assert.Equal(t, tc.want, got) + assert.NotContains(t, got, "ghp_supersecret") + }) + } +} + +// TestEnqueue_RefusesOnceTheWorkerIsStopping is the cursor-safety contract across a replacement. +// +// A worker is stopped while the process keeps running, and the sibling GitTargets on its branch go +// on holding streams that point at it until their own next reconcile. Shutdown DRAINS the queue +// without processing it, so an event accepted in that window is thrown away — while the `true` +// return has already told the watch loop it may advance its durable cursor past it. Refusing turns +// that silent loss into the drop path every producer already handles. +func TestEnqueue_RefusesOnceTheWorkerIsStopping(t *testing.T) { + withTemporaryWorkerStateRoot(t) + worker := newMetricsTestWorker() + // The loop reads its GitProvider on every pass; with none present it finds nothing to do, + // which is all this test needs from it. + worker.Client = fake.NewClientBuilder().WithScheme(setupScheme()).Build() + require.NoError(t, worker.Start(context.Background())) + + require.True(t, worker.Enqueue(Event{Operation: "UPDATE"}), + "a running worker accepts the event and owns it from then on") + + worker.Stop() + + assert.False(t, worker.Enqueue(Event{Operation: "UPDATE"}), + "an event the shutdown drain would discard must not be reported as accepted") + assert.False(t, worker.EnqueueResync(&ResyncRequest{}), + "and a resync must be answered rather than left waiting on a reply that never comes") +} + +// TestEnqueue_AdmissionClosesAtOneInstant is the half a check-then-send misses. +// +// Reading a stopping flag OUTSIDE the lock the send is made under leaves an enqueue able to pass +// the check, block on the lock, and deliver into a queue Stop has already finished draining: no +// loop is left to read it, and the caller was told "accepted". So admission and the stop +// transition are taken under one lock, and the invariant is absolute rather than probabilistic: +// once Stop has returned, nothing may be on the queue and nothing may be accepted. +func TestEnqueue_AdmissionClosesAtOneInstant(t *testing.T) { + withTemporaryWorkerStateRoot(t) + worker := newMetricsTestWorker() + worker.Client = fake.NewClientBuilder().WithScheme(setupScheme()).Build() + require.NoError(t, worker.Start(context.Background())) + + // Producers pushing continuously, so the stop lands in the middle of one. + var wg sync.WaitGroup + done := make(chan struct{}) + for range 8 { + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-done: + return + default: + } + worker.Enqueue(Event{Operation: "UPDATE"}) + } + }() + } + time.Sleep(20 * time.Millisecond) // let them get going + + worker.Stop() + + // Still producing, and every one of them has to be refused from here on. With the check taken + // outside the send's lock, a producer that was already past it lands an item here and answers + // "accepted" for an event nothing will ever read. + for range 500 { + require.False(t, worker.Enqueue(Event{Operation: "UPDATE"}), + "the queue accepted an event after Stop returned; nothing is left to process it") + } + + close(done) + wg.Wait() +} + +// TestEnsureWorker_HoldsTheReplacementUntilItIsAcknowledged. Dropping the old worker's queue and +// retained writes is only safe if something rebuilds the folder in the new repository, and nothing +// asks for that on its own: the GitTarget's declaration is level-triggered and its inputs did not +// change. So the caller has to be TOLD, and the steady case must not tell it, or every reconcile +// would force a replay of every target on the branch. +// +// mustEnsureWorker does what the reconcile does — take the pending recovery and acknowledge it — +// so each assertion below reads "was there a recovery to deliver on this pass". +func TestEnsureWorker_HoldsTheReplacementUntilItIsAcknowledged(t *testing.T) { + manager, ctx := startedManager(t) + before := RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"} + after := RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"} + + assert.False(t, mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", before), + "creating the first worker replaces nothing") + assert.False(t, mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", before), + "and a steady tick must not claim a replacement it did not make") + + assert.True(t, mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", after), + "the repository changed, so the branch has to be re-established against the new one") + assert.False(t, mustEnsureWorker(ctx, t, manager, "repo1", "gitops-system", "main", after), + "reported once, for the reconcile that delivered it") +} + +// TestEnsureWorker_AnUndeliveredRecoveryIsStillPendingOnTheNextPass is the hazard of spending the +// notification on the pass that made the replacement. +// +// Delivering it can fail — it lists the GitTargets on the branch, and a list can fail — and once +// the notification was spent, every later call said there was nothing to recover. Neither an +// ordinary reconcile nor the orphan sweep forced the missing replay, so an idle target left the +// new repository empty indefinitely. The recovery is now held until something acknowledges it. +func TestEnsureWorker_AnUndeliveredRecoveryIsStillPendingOnTheNextPass(t *testing.T) { + manager, ctx := startedManager(t) + key := BranchKey{RepoNamespace: "gitops-system", RepoName: "repo1", Branch: "main"} + before := RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"} + after := RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"} + + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", before)) + require.False(t, manager.ReplacementPending(key)) + + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", after)) + require.True(t, manager.ReplacementPending(key)) + + // The delivery fails, so nothing acknowledges it. Two more reconciles of this target, and of + // a sibling on the same branch, still find the recovery waiting. + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", after)) + assert.True(t, manager.ReplacementPending(key), + "a recovery nobody delivered must survive the calls that come after it") + + manager.AcknowledgeReplacement(key) + assert.False(t, manager.ReplacementPending(key)) + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", after)) + assert.False(t, manager.ReplacementPending(key), + "and a delivered one must not be asked for again on every steady tick") +} + +// TestReconcileWorkers_DropsARecoveryForABranchNothingNeeds keeps the pending set bounded by live +// configuration: a branch whose last GitTarget is gone has no folder left to re-establish. +func TestReconcileWorkers_DropsARecoveryForABranchNothingNeeds(t *testing.T) { + manager, ctx := startedManager(t) + key := BranchKey{RepoNamespace: "gitops-system", RepoName: "repo1", Branch: "main"} + before := RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"} + after := RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"} + + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", before)) + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", after)) + require.True(t, manager.ReplacementPending(key)) + + // No GitTarget names this branch, so the sweep retires its worker. + require.NoError(t, manager.ReconcileWorkers(ctx)) + + assert.False(t, manager.ReplacementPending(key), + "a recovery held for a branch nothing needs would outlive every object it was about") +} + +// TestSweep_RetiresAWorkerNoDeleteWasEverObservedFor. The delete-triggered sweep runs where a +// reconcile READ NotFound. Delete a GitTarget and recreate it under the same name on another +// branch before that reconcile runs, and the controller sees only the successor: it wires the new +// branch's worker and never learns the predecessor existed, so the old branch's worker, goroutine +// and clone are held until the process restarts. +// +// Kubernetes reconciliation cannot be built on observing every intermediate state, so the sweep +// needs a trigger that does not depend on seeing the delete. +func TestSweep_RetiresAWorkerNoDeleteWasEverObserved(t *testing.T) { + manager, ctx := startedManager(t) + repo := RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/repo.git"} + + // The state a rapid delete-and-recreate leaves: a worker for the branch the predecessor named, + // and a live GitTarget of the same name on a different one. + mustEnsureWorker(ctx, t, manager, "repo1", testTargetNamespace, "main", repo) + createTargetForRegister(ctx, t, manager.Client, "target1", "repo1", "release", "apps/") + mustEnsureWorker(ctx, t, manager, "repo1", testTargetNamespace, "release", repo) + + require.NoError(t, manager.ReconcileWorkers(ctx)) + + _, stranded := manager.GetWorkerForTarget("repo1", testTargetNamespace, "main") + assert.False(t, stranded, "no GitTarget names this branch any more, and no delete was ever seen") + _, live := manager.GetWorkerForTarget("repo1", testTargetNamespace, "release") + assert.True(t, live, "and the successor's worker is untouched") +} diff --git a/internal/reconcile/git_target_event_stream.go b/internal/reconcile/git_target_event_stream.go index 49c1dfdfb..956fcde66 100644 --- a/internal/reconcile/git_target_event_stream.go +++ b/internal/reconcile/git_target_event_stream.go @@ -73,6 +73,15 @@ func (s *GitTargetEventStream) OnWatchEvent(event git.Event) error { return nil } +// ServesWorker reports whether this stream still forwards to the given branch worker. +// +// It exists because a worker can be REPLACED in its slot — its GitProvider now names a different +// repository — and a stream still pointing at the old one would enqueue live events onto a stopped +// worker's queue, where nothing would ever write them. +func (s *GitTargetEventStream) ServesWorker(worker EventEnqueuer) bool { + return s.branchWorker == worker +} + // String returns a string representation for debugging. func (s *GitTargetEventStream) String() string { return fmt.Sprintf("GitTargetEventStream(gitTarget=%s/%s)", s.gitTargetNamespace, s.gitTargetName) diff --git a/internal/reconcile/git_target_event_stream_test.go b/internal/reconcile/git_target_event_stream_test.go index c63dacf88..f2be7d951 100644 --- a/internal/reconcile/git_target_event_stream_test.go +++ b/internal/reconcile/git_target_event_stream_test.go @@ -87,6 +87,19 @@ var _ = Describe("GitTargetEventStream", func() { Expect(stream.OnWatchEvent(createTestEvent("configmap", "full-queue", "UPDATE"))).NotTo(Succeed()) Expect(mockWorker.events).To(BeEmpty()) }) + + // A branch worker is REPLACED in its slot when its GitProvider names a different repository, + // and the stream holds its worker directly. One left pointing at the retired worker would go + // on enqueueing live events onto a stopped goroutine's queue, where they are accepted, never + // written, and silently dropped once it fills. The reconcile asks this before reusing a + // registered stream, so it is the whole guard against that. + It("reports which worker it still forwards to", func() { + Expect(stream.ServesWorker(mockWorker)).To(BeTrue()) + + replacement := &mockBranchWorker{events: make([]git.Event, 0)} + Expect(stream.ServesWorker(replacement)).To(BeFalse(), + "a stream that survived a worker replacement must not claim to serve the new one") + }) }) // mockBranchWorker implements the EventEnqueuer interface for testing. When full is set diff --git a/internal/watch/declare_force_test.go b/internal/watch/declare_force_test.go new file mode 100644 index 000000000..d5dc43960 --- /dev/null +++ b/internal/watch/declare_force_test.go @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// TestDeclareForce_ARequestArrivingMidPassSurvivesIt is the hazard a boolean could not express. +// +// A pass plans against a COPY of the declare record, so a recovery request raised while it runs +// cannot be the one it is about to satisfy: the streams it installs were chosen before that +// request existed. Acknowledging "a force was pending" rather than "this request was consumed" +// therefore reported a recovery that never happened — and because the dirty sequence schedules +// the next pass without one, the target kept its existing streams, nothing replayed, and a branch +// whose worker had just been replaced could stay unpopulated. +func TestDeclareForce_ARequestArrivingMidPassSurvivesIt(t *testing.T) { + m := &Manager{} + ref := types.NewResourceReference("target", "tenant") + m.declareIntentFor(ref, "default", "", v1alpha3.PruneOnEvent, false) + + m.RequestRecheckForGitTarget(ref) + planned := *m.triggers().declares[ref.Key()] + require.True(t, planned.forcePending(), "precondition: the pass plans a forced recheck") + + // The branch worker is replaced again while that pass is in flight. + m.RequestRecheckForGitTarget(ref) + m.clearDeclareForce(ref, planned.forceRequests) + + assert.True(t, m.triggers().declares[ref.Key()].forcePending(), + "the second request was not satisfied by a pass planned before it arrived") + + // The next pass consumes it, and only then is the target settled. + second := *m.triggers().declares[ref.Key()] + m.clearDeclareForce(ref, second.forceRequests) + assert.False(t, m.triggers().declares[ref.Key()].forcePending(), + "a pass that planned against the request does satisfy it") + assert.Empty(t, m.ForcedRecheckTargetsForTest()) +} + +// TestDeclareForce_AFailedPassLeavesTheRequestStanding is the other half of the same rule: only a +// pass that reached the data plane acknowledges anything, so a failure cannot consume a recovery. +func TestDeclareForce_AFailedPassLeavesTheRequestStanding(t *testing.T) { + m := &Manager{} + ref := types.NewResourceReference("target", "tenant") + m.declareIntentFor(ref, "default", "", v1alpha3.PruneOnEvent, true) + + require.Equal(t, []string{ref.Key()}, m.ForcedRecheckTargetsForTest()) + + // A pass fails: applyTargetPlan returns before it acknowledges anything. + assert.True(t, m.triggers().declares[ref.Key()].forcePending()) + + // The level-triggered declare repeats, and the request is still there to be consumed. + m.declareIntentFor(ref, "default", "", v1alpha3.PruneOnEvent, false) + assert.True(t, m.triggers().declares[ref.Key()].forcePending(), + "a re-declare that asks for nothing new must not drop what is outstanding") +} + +// TestDeclareForce_AnUndeclaredTargetNeedsNoRecovery keeps RequestRecheckForGitTarget from +// inventing an intent: a target that has not declared starts its streams on its first pass, and a +// stream that starts replays. +func TestDeclareForce_AnUndeclaredTargetNeedsNoRecovery(t *testing.T) { + m := &Manager{} + m.RequestRecheckForGitTarget(types.NewResourceReference("never-declared", "tenant")) + + assert.Empty(t, m.ForcedRecheckTargetsForTest()) + assert.Empty(t, m.triggers().declares) +} diff --git a/internal/watch/event_router_test.go b/internal/watch/event_router_test.go index 9c15e1c12..d612233af 100644 --- a/internal/watch/event_router_test.go +++ b/internal/watch/event_router_test.go @@ -363,7 +363,11 @@ func TestServiceCommitRequest_RegisteredWorkerResolvesNoOpenWindow(t *testing.T) go func() { _ = workerManager.Start(ctx) }() time.Sleep(100 * time.Millisecond) // allow the manager to record its context - require.NoError(t, workerManager.EnsureWorker(ctx, "team-a-provider", "team-a", "main")) + ensureErr := workerManager.EnsureWorker( + ctx, "team-a-provider", "team-a", "main", + git.RepoIdentity{URL: "file:///tmp/does-not-need-to-exist"}, + ) + require.NoError(t, ensureErr) router := NewEventRouter(workerManager, nil, client, logr.Discard()) diff --git a/internal/watch/materialization.go b/internal/watch/materialization.go index f65cbedf7..caa4c17fd 100644 --- a/internal/watch/materialization.go +++ b/internal/watch/materialization.go @@ -3,6 +3,8 @@ package watch import ( + "time" + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/types" ) @@ -36,6 +38,51 @@ func (m *Manager) DeclareForGitTarget( m.declareIntentFor(gitDest, clusterID, auditRoute, pruneMode, force) } +// RequestRecheckForGitTarget forces this target's next pass to re-anchor its streams, which is +// what makes each one replay and drive the mark-and-sweep that rebuilds its folder. +// +// It exists for the branch worker replacement. The declaration is level-triggered and its inputs +// do not change when the repository underneath it does, so without this the pass is a no-op: the +// streams keep running, nothing replays, and an idle target would leave the new repository empty +// indefinitely. The force flag is the same one a refused Git path raises, and it is sticky on the +// intent, so it survives until a pass actually consumes it. +// +// A target that has not declared yet needs nothing: its first pass starts its streams, and a +// stream that starts replays. +func (m *Manager) RequestRecheckForGitTarget(gitDest types.ResourceReference) { + t := m.triggers() + t.mu.Lock() + prior, declared := t.declares[gitDest.Key()] + if declared { + prior.forceRequests++ + t.markDirtyLocked(gitDest, TriggerReasonWorkerReplaced, time.Now()) + } + t.mu.Unlock() + if !declared { + return + } + t.signal() + // The controller owns status, and what it reads back about this target (its render fidelity, + // its remote revision) was proved against the repository that is no longer there. + m.enqueueGitTargetReconcile(gitDest) +} + +// ForcedRecheckTargetsForTest lists the declared GitTargets whose next pass will re-anchor their +// streams. It exists so a test in another package can assert which targets a recovery reached; +// nothing in production calls it. +func (m *Manager) ForcedRecheckTargetsForTest() []string { + t := m.triggers() + t.mu.Lock() + defer t.mu.Unlock() + forced := make([]string, 0, len(t.declares)) + for key, intent := range t.declares { + if intent.forcePending() { + forced = append(forced, key) + } + } + return forced +} + // ForgetGitTargetDeclaration drops in-memory watch state for a deleted GitTarget, and tears down // its source cluster's context when it was the last GitTarget mirroring from it. // @@ -58,13 +105,11 @@ func (m *Manager) tearDownGitTarget(gitDest types.ResourceReference) { m.mutateWatchPlane(func(s *watchPlaneState) bool { _, hadPass := s.passes[gitDest.Key()] _, hadLayout := s.layouts[gitDest.Key()] - _, hadRemote := s.remotes[gitDest.Key()] - if !hadPass && !hadLayout && !hadRemote { + if !hadPass && !hadLayout { return false } delete(s.passes, gitDest.Key()) delete(s.layouts, gitDest.Key()) - delete(s.remotes, gitDest.Key()) return true }) } diff --git a/internal/watch/owner.go b/internal/watch/owner.go index cfc41c61b..a65aaa227 100644 --- a/internal/watch/owner.go +++ b/internal/watch/owner.go @@ -67,6 +67,10 @@ const ( TriggerReasonSharedRefresh = "shared_refresh" // TriggerReasonPeriodic is the 30s floor, the one path that still marks everything. TriggerReasonPeriodic = "periodic" + // TriggerReasonWorkerReplaced is a branch worker replaced because its GitProvider now names a + // different repository. The new worker has no clone and no queue, so every GitTarget on that + // branch has to re-establish its folder there. + TriggerReasonWorkerReplaced = "worker_replaced" ) // declareIntent is what only the GitTarget controller knows about a target: its UID and the three @@ -78,10 +82,21 @@ type declareIntent struct { clusterID string auditRoute string pruneMode v1alpha3.PruneMode - // force is sticky: a declare that asks for a fresh recheck keeps asking until a pass actually - // succeeds, so a failed attempt cannot consume it. - force bool -} + // forceRequests counts the recovery requests this target has been given, and forceConsumed is + // the last one a pass actually put in place. A request is outstanding while they differ. + // + // It is a COUNTER rather than a flag because a pass plans against a copy of this record: a + // request arriving while that pass runs is not the request the pass is about to satisfy, and a + // flag cleared on success erased it. The dirty sequence then scheduled a second pass that saw + // nothing to force, so the target kept its existing streams, nothing replayed, and a branch + // whose repository had just been replaced could stay unpopulated indefinitely. + forceRequests uint64 + forceConsumed uint64 +} + +// forcePending reports whether a recovery request is outstanding: one has been asked for that no +// pass has yet put in place. +func (d declareIntent) forcePending() bool { return d.forceRequests != d.forceConsumed } // dirtyTarget is one GitTarget the owner owes a pass. type dirtyTarget struct { @@ -246,8 +261,9 @@ func (m *Manager) signalSharedRefresh() { t.signal() } -// declareIntentFor records the capture-on-Declare values and marks the target dirty. The force -// flag is sticky across attempts: it survives until a pass actually succeeds. +// declareIntentFor records the capture-on-Declare values and marks the target dirty. A recovery +// request is sticky across attempts: it survives until a pass actually puts the watches it asks +// for in place. func (m *Manager) declareIntentFor( gitDest types.ResourceReference, clusterID string, @@ -262,7 +278,6 @@ func (m *Manager) declareIntentFor( clusterID: clusterID, auditRoute: auditRoute, pruneMode: pruneMode, - force: force, } // A GitTarget reconcile is LEVEL-triggered: the controller re-declares on its steady requeue // and on every event it watches, with the same values every time. Marking the target dirty for @@ -274,7 +289,10 @@ func (m *Manager) declareIntentFor( prior := t.declares[gitDest.Key()] landed := m.watchPlane().passes[gitDest.Key()].Landed if prior != nil { - next.force = next.force || prior.force + next.forceRequests, next.forceConsumed = prior.forceRequests, prior.forceConsumed + } + if force { + next.forceRequests++ } t.declares[gitDest.Key()] = &next if prior == nil || *prior != next || !landed { @@ -705,7 +723,7 @@ func (m *Manager) runTargetPass(ctx context.Context, log logr.Logger, entry *dir // trigger names a target; it does not carry the rule that caused it, and a plan built from // whichever rule controller happened to fire first would not be the configuration as it stands. func (m *Manager) applyTargetPlan(ctx context.Context, intent declareIntent) error { - force := intent.force || m.pruneModeRequiresReplay(intent.ref, intent.pruneMode) + force := intent.forcePending() || m.pruneModeRequiresReplay(intent.ref, intent.pruneMode) m.rememberDeclareCapture(intent.ref, intent.clusterID, intent.auditRoute) // A pass never dials anything, not even for a cluster nothing has observed yet. Discovery is @@ -723,7 +741,7 @@ func (m *Manager) applyTargetPlan(ctx context.Context, intent declareIntent) err // Only once the watches are actually in place: a failed pass must leave the pending force // standing for the next one rather than consuming it. m.rememberGitTargetPruneMode(intent.ref, intent.pruneMode) - m.clearDeclareForce(intent.ref) + m.clearDeclareForce(intent.ref, intent.forceRequests) return nil } diff --git a/internal/watch/owner_observability.go b/internal/watch/owner_observability.go index 2307092aa..2c617b37c 100644 --- a/internal/watch/owner_observability.go +++ b/internal/watch/owner_observability.go @@ -162,14 +162,24 @@ func (m *Manager) logOwnerHeartbeat(log logr.Logger) { log.V(1).Info("watch plane owner heartbeat", "dirtyTargets", count, "oldestDirtyAge", oldest.String()) } -// clearDeclareForce consumes a satisfied force-recheck request. It runs only after a pass has -// actually put the watches in place, so a failed attempt leaves the request standing. -func (m *Manager) clearDeclareForce(ref types.ResourceReference) { +// clearDeclareForce acknowledges the force-recheck request a pass actually consumed, named by the +// request counter the pass planned against. It runs only after that pass has put the watches in +// place, so a failed attempt leaves the request standing. +// +// Acknowledging BY NUMBER is what makes a request that arrived mid-pass survive it. The pass was +// planned from a copy taken before that request existed, and the streams it installed cannot be +// the ones the new request is asking for; clearing unconditionally therefore reported a recovery +// that never happened. Anything newer than `consumed` stays outstanding for the next pass. +func (m *Manager) clearDeclareForce(ref types.ResourceReference, consumed uint64) { t := m.triggers() t.mu.Lock() defer t.mu.Unlock() - if intent := t.declares[ref.Key()]; intent != nil && matchesUID(intent.ref, ref) { - intent.force = false + intent := t.declares[ref.Key()] + if intent == nil || !matchesUID(intent.ref, ref) { + return + } + if consumed > intent.forceConsumed { + intent.forceConsumed = consumed } } diff --git a/internal/watch/remote_observation.go b/internal/watch/remote_observation.go deleted file mode 100644 index 95ad4c07b..000000000 --- a/internal/watch/remote_observation.go +++ /dev/null @@ -1,59 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 - -package watch - -import ( - "github.com/ConfigButler/gitops-reverser/internal/git" - "github.com/ConfigButler/gitops-reverser/internal/types" -) - -// ReportRemoteObserved records where a GitTarget's branch was last proved to be. It is installed -// on the WorkerManager (git.RemoteReporter) at startup, and it is the only writer of the remotes -// projection. -// -// Every confirmed look calls it — a successful push as well as a refresh. The transition test -// lives here, as it does for layouts, but it is drawn differently: a NEW REVISION is always news, -// including one we pushed ourselves, because status.remote.revision answers "where is my branch" -// and the push is the event that changes the answer. Renewing only the clock is not news, and is -// left to the target's own steady tick. -// -// That enqueue puts a per-push series back onto gitPathEvents, which 14eeef46 deliberately -// cleared of high-volume traffic. PushCooldown floors it at one push per 5s per branch worker, -// well under what the buffer absorbs — and a dropped enqueue is benign here in a way it is not -// elsewhere on that channel: a dropped acceptance transition leaves a WRONG condition standing, -// a dropped observation only a TIMESTAMPED one that the next tick corrects. -func (m *Manager) ReportRemoteObserved(gitDest types.ResourceReference, observed git.RemoteObservation) { - newAnswer := false - m.mutateWatchPlane(func(s *watchPlaneState) bool { - prior, had := s.remotes[gitDest.Key()] - // A byte-identical report changes nothing an outside reader could see, and it is the - // steady state: a refresh re-reports what it already knew on every tick, for every target - // on the branch. Publishing it would clone the whole watch-plane snapshot to store the - // value that is already there, which is the trap both sibling reporters avoid. - if had && prior == observed { - return false - } - // What wakes the controller is a change in the ANSWER to "where is my branch", which the - // revision alone does not carry. A withdrawal and a branch the remote does not have both - // have no revision and mean opposite things — one removes the stanza, the other publishes - // it — so a move between them is news in both directions, and comparing revisions alone - // left the wrong one standing until some other reconcile happened by. - // - // `By` is deliberately not compared: a revision that changes hands from a push to a fetch - // is the same answer, and the target's own steady tick republishes it. - newAnswer = !had || prior.Revision != observed.Revision || prior.Withdrawn != observed.Withdrawn - s.remotes[gitDest.Key()] = observed - return true - }) - if newAnswer { - m.enqueueGitTargetReconcile(gitDest) - } -} - -// RemoteForGitTarget returns the most recent observation of a GitTarget's branch, and whether -// anything has looked at all. Absent leaves status.remote unset, which is different from a -// present observation with no revision: that one means the branch is not on the remote. -func (m *Manager) RemoteForGitTarget(gitDest types.ResourceReference) (git.RemoteObservation, bool) { - observed, ok := m.watchPlane().remotes[gitDest.Key()] - return observed, ok -} diff --git a/internal/watch/remote_observation_test.go b/internal/watch/remote_observation_test.go deleted file mode 100644 index 18cc520e8..000000000 --- a/internal/watch/remote_observation_test.go +++ /dev/null @@ -1,163 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 - -package watch - -import ( - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/ConfigButler/gitops-reverser/internal/git" - "github.com/ConfigButler/gitops-reverser/internal/types" -) - -// TestReportRemoteObserved_EnqueuesOnANewRevisionOnly draws the transition test where -// status.remote needs it, which is not where layouts need it. A moved branch is what an operator -// is waiting to see, so it must not wait out the steady requeue; renewing only the clock is not -// news, and enqueueing for it would put a reconcile per refresh per target on a channel that was -// deliberately cleared of high-volume traffic. -func TestReportRemoteObserved_EnqueuesOnANewRevisionOnly(t *testing.T) { - m := &Manager{} - events := m.GitPathEvents() - gitDest := types.NewResourceReference("checkout", "shop") - at := time.Now() - - m.ReportRemoteObserved(gitDest, git.RemoteObservation{Revision: "aaaa", At: at, By: git.ObservedByPush}) - require.Len(t, events, 1, "the first observation is a change: nothing was known before") - - m.ReportRemoteObserved(gitDest, git.RemoteObservation{ - Revision: "aaaa", At: at.Add(10 * time.Minute), By: git.ObservedByFetch, - }) - assert.Len(t, events, 1, "a refresh that finds the branch where it was is not news") - - m.ReportRemoteObserved(gitDest, git.RemoteObservation{ - Revision: "bbbb", At: at.Add(11 * time.Minute), By: git.ObservedByFetch, - }) - assert.Len(t, events, 2, "a branch that moved is exactly what the field is read for") -} - -// TestReportRemoteObserved_KeepsTheClockEvenWithoutEnqueueing is the other half of the rule -// above: the projection always carries the latest look, so the target's own steady tick -// republishes a renewed clock without anything having to wake it. -func TestReportRemoteObserved_KeepsTheClockEvenWithoutEnqueueing(t *testing.T) { - m := &Manager{} - gitDest := types.NewResourceReference("checkout", "shop") - at := time.Now() - - m.ReportRemoteObserved(gitDest, git.RemoteObservation{Revision: "aaaa", At: at, By: git.ObservedByPush}) - m.ReportRemoteObserved(gitDest, git.RemoteObservation{ - Revision: "aaaa", At: at.Add(10 * time.Minute), By: git.ObservedByFetch, - }) - - observed, ok := m.RemoteForGitTarget(gitDest) - require.True(t, ok) - assert.Equal(t, at.Add(10*time.Minute), observed.At) - assert.Equal(t, git.ObservedByFetch, observed.By) -} - -// TestRemoteForGitTarget_AbsentUntilSomethingLooks keeps "nothing has looked" distinguishable -// from "we looked and the branch is not there", which the controller publishes differently. -func TestRemoteForGitTarget_AbsentUntilSomethingLooks(t *testing.T) { - m := &Manager{} - _, ok := m.RemoteForGitTarget(types.NewResourceReference("checkout", "shop")) - assert.False(t, ok) -} - -// TestReportRemoteObserved_AnIdenticalReportPublishesNothing. Every refresh tick re-reports what -// it already knew, for every target on the branch, and every push reports too. Storing a value -// that is already there would clone the whole watch-plane snapshot to change nothing — the trap -// both sibling reporters short-circuit on. -func TestReportRemoteObserved_AnIdenticalReportPublishesNothing(t *testing.T) { - m := &Manager{} - gitDest := types.NewResourceReference("checkout", "shop") - observed := git.RemoteObservation{Revision: "aaaa", At: time.Now(), By: git.ObservedByPush} - - m.ReportRemoteObserved(gitDest, observed) - published := m.watchPlane() - - m.ReportRemoteObserved(gitDest, observed) - - assert.Same(t, published, m.watchPlane(), - "a byte-identical report changes nothing a reader could see, so nothing is republished") -} - -// TestReportRemoteObserved_AWithdrawalReplacesTheRevisionAndWakesTheTarget is the correction path. -// The data plane has met a different repository, so the revision in the projection describes one -// the target no longer points at; it has to be replaced AND the target woken, because the -// controller is the only thing that can take it out of status. -func TestReportRemoteObserved_AWithdrawalReplacesTheRevisionAndWakesTheTarget(t *testing.T) { - m := &Manager{} - events := m.GitPathEvents() - gitDest := types.NewResourceReference("checkout", "shop") - - m.ReportRemoteObserved(gitDest, git.RemoteObservation{ - Revision: "aaaa", At: time.Now(), By: git.ObservedByPush, - }) - require.Len(t, events, 1) - - m.ReportRemoteObserved(gitDest, git.WithdrawnObservation()) - - observed, ok := m.RemoteForGitTarget(gitDest) - require.True(t, ok) - assert.True(t, observed.Withdrawn) - assert.Empty(t, observed.Revision, "the old repository's revision is gone from the projection") - assert.Len(t, events, 2, "and the target has to reconcile to take it out of status") - - m.ReportRemoteObserved(gitDest, git.WithdrawnObservation()) - assert.Len(t, events, 2, "a withdrawal that is already standing is not news again") -} - -// TestTearDownGitTarget_DropsTheRemoteObservation. The projection is keyed by namespace/name, and -// a GitTarget deleted and recreated under the same name is a different target that may point at a -// different branch entirely. Leaving the observation behind would hand the new one its -// predecessor's revision before anything had looked on its behalf. -func TestTearDownGitTarget_DropsTheRemoteObservation(t *testing.T) { - m := &Manager{} - gitDest := types.NewResourceReference("checkout", "shop") - m.ReportRemoteObserved(gitDest, git.RemoteObservation{ - Revision: "aaaa", At: time.Now(), By: git.ObservedByPush, - }) - _, seen := m.RemoteForGitTarget(gitDest) - require.True(t, seen) - - m.tearDownGitTarget(gitDest) - - _, seen = m.RemoteForGitTarget(gitDest) - assert.False(t, seen, "a deleted GitTarget leaves nothing behind for its successor to inherit") -} - -// TestReportRemoteObserved_WakesOnEveryChangeOfAnswer. Both "the remote does not carry this -// branch" and "what was published is withdrawn" have no revision, and they mean opposite things: -// one publishes a stanza saying the branch is not there, the other removes the stanza entirely. -// Comparing revisions alone left whichever was published standing until some unrelated reconcile -// happened by. -func TestReportRemoteObserved_WakesOnEveryChangeOfAnswer(t *testing.T) { - absent := git.RemoteObservation{At: time.Now(), By: git.ObservedByFetch} - - t.Run("absent branch to withdrawn", func(t *testing.T) { - m := &Manager{} - events := m.GitPathEvents() - gitDest := types.NewResourceReference("checkout", "shop") - - m.ReportRemoteObserved(gitDest, absent) - require.Len(t, events, 1) - - m.ReportRemoteObserved(gitDest, git.WithdrawnObservation()) - assert.Len(t, events, 2, "the stanza has to come out of status, and only a reconcile does that") - }) - - t.Run("withdrawn to absent branch", func(t *testing.T) { - m := &Manager{} - events := m.GitPathEvents() - gitDest := types.NewResourceReference("checkout", "shop") - - m.ReportRemoteObserved(gitDest, git.WithdrawnObservation()) - require.Len(t, events, 1) - - m.ReportRemoteObserved(gitDest, absent) - assert.Len(t, events, 2, - "the new repository answered — it does not carry the branch — and that is publishable") - }) -} diff --git a/internal/watch/watch_plane_state.go b/internal/watch/watch_plane_state.go index c24aed767..b6e38ae09 100644 --- a/internal/watch/watch_plane_state.go +++ b/internal/watch/watch_plane_state.go @@ -48,10 +48,6 @@ type watchPlaneState struct { // status.placement and the LayoutResolved condition. It is a report from a scan, not from a // write, so it is present for a target that has never written and for a suspended one. layouts map[string]git.LayoutReport - // remotes is each GitTarget's most recently observed remote branch state, published as - // status.remote. Like layouts it is a report from a look at Git rather than from a write, so - // it is present for a target that has never written and for a suspended one. - remotes map[string]git.RemoteObservation } // targetPassStatus is how one GitTarget's most recent plan pass ended. @@ -108,7 +104,6 @@ func newWatchPlaneState() *watchPlaneState { pruneModes: map[string]v1alpha3.PruneMode{}, passes: map[string]targetPassStatus{}, layouts: map[string]git.LayoutReport{}, - remotes: map[string]git.RemoteObservation{}, } } @@ -127,7 +122,6 @@ func (s *watchPlaneState) clone() *watchPlaneState { pruneModes: copyMap(s.pruneModes), passes: copyMap(s.passes), layouts: copyMap(s.layouts), - remotes: copyMap(s.remotes), } for key, cells := range s.streams { out.streams[key] = copyMap(cells)