Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion docs/TODO.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,13 @@ should land before the next tag is cut (the last release is v0.55.0). Checked ag

## Before the next release

Nothing open.
- [ ] **Test the failure boundaries of concurrent commit windows** (stage 3 of
[concurrent-author-windows.md](design/concurrent-author-windows.md#stages)). Stage 2 ships with no
flag: a branch worker keeps a window per author whenever their footprints are proven disjoint.
Before it is released, the reference model (`internal/git/window_model_test.go`) must cover both
close orders, conflicting writes, delete and recreate, `attach_next`, zero-duration windows, a
resync, shutdown, failed publication and replay, and buffer pressure, each with several windows
open.

## Applier claims and bi-directional editing

Expand Down
21 changes: 16 additions & 5 deletions docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -1450,11 +1450,22 @@ editing them affects windows that open afterward, never one already running. A
[`CommitRequest`](#the-request-window-specwindow) that attaches to a window replaces both timers
for that window.

One branch worker holds one live window, bound to an author and target. Interleaved authors or
targets split a burst. Atomic writes, a resync, buffer limits, shutdown, an `attach: Next` request,
a push notice from the [receiver](#gitreceiver), and the
[flush](bi-directional.md#flush-then-apply) before an apply can close a window early. Push cooldown
is independent: a zero timer does not promise an immediate remote push. See the
A branch worker holds a window per author of one target, side by side, while it can prove their
writes change different files: each object alone in its own file, at the built-in path or where its
document already lives, with no kustomization governing it and no `placement.byType` or
`placement.default` template for a new one. Anything it cannot prove stays serial, one window at a
time: an object in a file with other documents, anything under a kustomization or with
`useKustomize` on, a new document placed by a template, an encrypted type. Such a write, a write to
a file another author's open window holds, and a write of another target close the windows it
cannot sit beside first, so those still split a burst. **Commits land in the order their windows
close**, not the order their first writes arrived: with Alice's window open since 10:00 and Bob's
since 10:01, Bob's commit can come first. No file ever goes back in time, but `git log` is no
longer one timeline of edits.

Atomic writes, a resync, buffer limits, shutdown, an `attach: Next` request, a push notice from the
[receiver](#gitreceiver), and the [flush](bi-directional.md#flush-then-apply) before an apply close
the open windows early. Push cooldown is independent: a zero timer does not promise an immediate
remote push. See the
[complete applier claim rules](spec/commit-window-refactor.md).

Both values are **Go duration strings with a mandatory unit**, at most `24h`, checked by the API
Expand Down
27 changes: 26 additions & 1 deletion docs/design/concurrent-author-windows.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
# Design: one commit window per author, open at the same time

Status: **proposal, not built.** Raised by gitops-api's review of the source-side save (PR #456):
Status: **stages 1 and 2 built** (`internal/git/window_set.go`, `footprint.go`,
`window_admission.go`; see [what stage 2 built](#what-stage-2-built)). Stage 2 ships without the
startup flag of stage 4, so stage 3 lands before the next release (`docs/TODO.md`). Raised by
gitops-api's review of the source-side save (PR #456):
with saves created straight from a browser, a second person's edit splits the first person's change
into two commits. This page sets out the change that fixes that, the rules it must keep, how to test
it, and the stages to ship it in.
Expand Down Expand Up @@ -300,6 +303,28 @@ window, so they are proven against today's behavior before they are used to judg
measurements. Field ownership and the apply hold are separate changes, each with its own
state-machine review.

### What stage 2 built

The rules above, with these choices where the page left room:

- **A window is proven once it has company.** A window that never sat beside another has no
footprint ("unproven") and its commit is not checked; proving costs a scan of the folder per
revision, and a lone window needs no proof. From the first write admitted beside it, a window's
footprint covers every write it collects.
- **The proof also needs the folder's bootstrap files committed**, the checkout to hold every decided
write, a whole object (not a field patch) and a type that is not encrypted. Anything else is
unknown. The revision is the count of the worker's resets and own commits, with the generation.
- **The check before the commit uses a proof of the commit's own tree.** The windows a write's
admission closes commit before it and can move what it changes (a delete committed first turns
its update into a create), so the decided write is proven again once the checkout holds every
write before it. After a reset or a `GitTarget` change the proof is of another tree, and the
commit is not checked. A refusal fails the write as terminal: the worktree is undone, its save
fails with the paths, the worker logs `SingleWindowFallback` and holds one window at a time until
it restarts, and a resync of the `GitTarget` is asked for.
- **A request's own author's window stands in no one's way.** A save waiting while another author's
window is open records a mismatch only when no window of its own author is open.
- **The window ID** (`window`) is in the open, close and finalized log lines, with the branch.

## What it costs

- **The loop's state.** The single `openWindow` becomes a set with an opening order, the close timer
Expand Down
12 changes: 8 additions & 4 deletions docs/interpreting-metrics.md
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ boundary, the commit, the push. Background:
| `git_queue_refusals_total` | counter | `gitbranch_namespace`, `gitbranch_name`, `provider_namespace`, `provider_name`, `kind` | Work refused at enqueue: the queue was full, intake was paused through an outage, or the worker was stopping. `kind` is `write` / `attach` / `resync` / `refresh`. The producer keeps a refused item and offers it again: the watch delivers a write again from its cursor, the controller re-sends a save, a resync is gathered again, and the next reconcile asks for a refresh. What it can cost, when its watch cursor expires before the write comes back, is the write's own commit and, under `prune.mode: OnEvent`, a deleted object's removal from Git. |
| `git_commit_failures_total` | counter | `gitbranch_namespace`, `gitbranch_name`, `provider_namespace`, `provider_name`, `reason` | A live commit window that died between routing and pushing; a resync's failure is `git_resync_failures_total`. `reason` is `refused` (a Git path a human must fix) / `error`. Every increment is a window's events lost until the next resync. An unreachable remote does not count: a decided window waits for the publication retry instead. |
| `git_writes_dropped_total` | counter | `gitbranch_namespace`, `gitbranch_name`, `provider_namespace`, `provider_name`, `reason` | A live event the branch worker dropped because its `GitTarget`'s render fidelity was not established. `reason` is `render_fidelity_diverged`: a rendered value does not match the live object, and every write is dropped until a fresh replay measures the folder clean. Or it is `render_fidelity_pending`: a collection of the `GitTarget` has not finished its replay, which closes every collection of that target until it does. A live event that meets a pending gate is held, not dropped, and committed with its author once the gate opens; it is counted here only when the hold gives up, 10 minutes after the first one was held or once the branch holds more than its retained-byte budget, and the worker then asks for a fresh snapshot, which commits the current state as the operator (a deletion comes back only under prune mode `Always`). Nothing replays a dropped write, so each one is an edit missing from Git until the object changes again or its collection replays. The worker logs the first drop under each gate status at Info, naming the collections the gate waits for, and each hold, release and give-up at Info. |
| `git_commit_windows_total` | counter | `gittarget_namespace`, `gittarget_name`, `close_reason`, `timer_source` | One per closed commit window, whether or not it then produced a commit. `close_reason` is `idle_timeout` / `max_duration` / `attach_next` / `identity_change` / `buffer_limit` / `resync_before_apply` / `foreign_commit` (a Git host said the branch may have moved, so the window is published now) / `apply_due` (an apply came due for an applier on the branch, so the window's edits are published before the apply is sent) / `shutdown`; `timer_source` is `target` or `commit_request`. See [tuning commit windows](#tuning-commit-windows). |
| `git_commit_windows_total` | counter | `gittarget_namespace`, `gittarget_name`, `close_reason`, `timer_source` | One per closed commit window, whether or not it then produced a commit. `close_reason` is `idle_timeout` / `max_duration` / `attach_next` / `identity_change` / `path_conflict` / `buffer_limit` / `resync_before_apply` / `foreign_commit` (a Git host said the branch may have moved, so the window is published now) / `apply_due` (an apply came due for an applier on the branch, so the window's edits are published before the apply is sent) / `shutdown`; `timer_source` is `target` or `commit_request`. See [tuning commit windows](#tuning-commit-windows). |
| `git_commit_window_duration_seconds` | histogram | `gittarget_namespace`, `gittarget_name` | How long each window collected: from the write that opened it to the start of its finalize. Its count equals `git_commit_windows_total` summed over `close_reason` and `timer_source`. |
| `git_queue_depth` | gauge | `gitbranch_namespace`, `gitbranch_name`, `provider_namespace`, `provider_name` | Pending + in-flight + committed-but-unpushed. Read at scrape time. |
| `git_retained_bytes` | gauge | `gitbranch_namespace`, `gitbranch_name`, `provider_namespace`, `provider_name` | What the worker holds until a push lands, in the units of the retained-byte budget: the open window, the retained writes and waiting saves, each with a 1 KiB per-item charge. Serialized bytes, not heap. |
Expand Down Expand Up @@ -645,9 +645,13 @@ picture toward `max_duration`.
writes kept arriving closer together than `idleTimeout`; raise `maxDuration` if those changes
belong in one commit. It does not prove continuous activity: an `idleTimeout` close to
`maxDuration` ends quiet windows this way too.
- `identity_change` means a write arrived that could not join the open window: a different author,
a different GitTarget on the same branch, or a different attribution state. A high share means
commits are being split by interleaving rather than by time, and no timer changes that.
- `identity_change` means a write arrived that could not join an open window, nor be proven to change
other files than it: a different GitTarget on the same branch, or a different author or
attribution state writing where the worker cannot prove a file of its own (a file with other
documents, a kustomize folder, a placement template). A high share means commits are being split
by interleaving rather than by time, and no timer changes that.
- `path_conflict` means another author's write changes a file the window's writes change, so the
window committed first. Authors editing different files keep a window each.
- `attach_next` is a save with `attach: Next` closing the author's open window before it waits.
- `buffer_limit` means the worker's retained-write budget was reached, and the window was flushed
to stay within it. Writes committed but not yet pushed count toward that budget, so this reads
Expand Down
2 changes: 1 addition & 1 deletion internal/git/applier_claim.go
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ func (l *branchWorkerEventLoop) closeWindowForPushHint() {
if !hinted || l.w.applyRequester == nil {
return
}
for _, window := range l.commitWindows.all() {
for _, window := range l.windows.all() {
if window.pendingCR != nil {
continue
}
Expand Down
10 changes: 5 additions & 5 deletions internal/git/applier_claim_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,12 +370,12 @@ func TestCloseWindowForPushHint_PublishesTheOpenWindowNow(t *testing.T) {
loop.handleQueueItem(WorkItem{Request: &WriteRequest{
Events: []Event{configMapTargetEvent("edited", "alice", "team-a")},
}})
require.NotNil(t, loop.openWindow, "an hour-long window is still open")
require.NotNil(t, loop.windows.only(), "an hour-long window is still open")

loop.handleRefreshRequest(&RefreshRequest{PushNotice: true})
loop.closeWindowForPushHint()

assert.Nil(t, loop.openWindow)
assert.Nil(t, loop.windows.only())
assert.Len(t, loop.pendingWrites, 1, "the window's write is decided, to be published")
}

Expand All @@ -391,14 +391,14 @@ func TestCloseWindowForPushHint_LeavesASavesWindowToTheSave(t *testing.T) {
loop.handleQueueItem(WorkItem{Request: &WriteRequest{
Events: []Event{configMapTargetEvent("edited", "alice", "team-a")},
}})
require.NotNil(t, loop.openWindow, "an hour-long window is still open")
require.NotNil(t, loop.windows.only(), "an hour-long window is still open")
id := commitRequestID{Namespace: "team-a", Name: "save", UID: "uid-save"}
loop.openWindow.pendingCR = &id
loop.windows.only().pendingCR = &id

loop.handleRefreshRequest(&RefreshRequest{PushNotice: true})
loop.closeWindowForPushHint()

assert.NotNil(t, loop.openWindow, "the save's window is still open")
assert.NotNil(t, loop.windows.only(), "the save's window is still open")
assert.Empty(t, loop.pendingWrites)
}

Expand Down
25 changes: 22 additions & 3 deletions internal/git/applier_echoes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,15 @@ type echoFixture struct {
}

func newEchoFixture(t *testing.T, slug string, prune configv1alpha3.PruneMode, docs map[string]string) *echoFixture {
t.Helper()
return newEchoFixtureWith(t, slug, prune, docs, nil)
}

// newEchoFixtureWith is newEchoFixture with files, by path, committed from outside after the documents
// and before the worker clones, so the worker starts on them as on anything it has applied.
func newEchoFixtureWith(
t *testing.T, slug string, prune configv1alpha3.PruneMode, docs, files map[string]string,
) *echoFixture {
t.Helper()
f := newLedgerFixture(t, slug, true)
f.worker.mapper = configMapMapper()
Expand All @@ -47,6 +56,16 @@ func newEchoFixture(t *testing.T, slug string, prune configv1alpha3.PruneMode, d
for name, value := range docs {
e.pushFromOutside(name, value)
}
if len(files) > 0 {
for path, content := range files {
full := filepath.Join(clone, path)
require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o750))
require.NoError(t, os.WriteFile(full, []byte(content), 0o600))
gitIn(t, clone, "add", path)
}
gitIn(t, clone, "commit", "--quiet", "-m", "outside: files")
gitIn(t, clone, "push", "--quiet", "origin", "HEAD:main")
}
require.NoError(t, f.worker.ensureRepositoryInitialized(f.worker.ctx))
e.loop = newBranchWorkerEventLoop(f.worker, 0)
t.Cleanup(e.loop.stopTimers)
Expand Down Expand Up @@ -317,17 +336,17 @@ func TestApplierEchoes_FlushThenApply(t *testing.T) {
holdPushes(f.loop) // the push cooldown runs: a flush does not wait for it

f.writeWindow(echoEvent("x", "x2")) // E0, in the open window when the apply comes due
require.NotNil(t, f.loop.openWindow)
require.NotNil(t, f.loop.windows.only())
restore := failPushes(t, f.worker)
f.echoApplier().repair = true
f.loop.endWake(0)

assert.Nil(t, f.loop.openWindow, "the apply closed the open window")
assert.Nil(t, f.loop.windows.only(), "the apply closed the open window")
assert.Empty(t, requester.taken(), "the writes up to the mark are not published yet")
require.NotNil(t, f.loop.flushEdits, "edits are recorded from the mark")

f.writeWindow(echoEvent("y", "y2")) // E1, decided after the mark
require.NotNil(t, f.loop.openWindow, "it opens a window of its own")
require.NotNil(t, f.loop.windows.only(), "it opens a window of its own")

restore()
f.loop.retry.due = time.Now().Add(-time.Second)
Expand Down
6 changes: 3 additions & 3 deletions internal/git/apply_attempt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ func TestApplier_ARecoveredRefusalIsNotRetried(t *testing.T) {
// Requests carry the uid, and the worker drops what was pending for an incarnation that is gone.
func TestApplier_ARecreatedTargetInheritsNothing(t *testing.T) {
loop, requester := manualLoop(t, applierTestTarget("alpha", true, reapplying))
loop.openWindow = &openWindow{}
loop.windows.setOnly(&openWindow{})
addAlphaUnsentRefusal(loop)

old := &configv1alpha3.GitTarget{}
Expand All @@ -165,7 +165,7 @@ func TestApplier_ARecreatedTargetInheritsNothing(t *testing.T) {
recreated.UID = "uid-alpha-recreated"
require.NoError(t, loop.w.Client.Create(context.Background(), recreated))

loop.openWindow = nil
loop.windows.setOnly(nil)
loop.serviceAppliers()
assert.Empty(t, requester.taken(), "the refusal was the old incarnation's")
assert.Empty(t, loop.unsentRefusals)
Expand Down Expand Up @@ -340,7 +340,7 @@ func TestPushNotice_TheTipAlreadySeenDoesNotCutAWindow(t *testing.T) {
Spec: configv1alpha3.GitProviderSpec{URL: "https://github.com/acme/platform"},
})
loop.w.recordRemoteObservation("ours", ObservedByPush)
loop.openWindow = &openWindow{}
loop.windows.setOnly(&openWindow{})

loop.handleRefreshRequest(&RefreshRequest{PushNotice: true, After: tc.after})

Expand Down
4 changes: 2 additions & 2 deletions internal/git/apply_catchup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,14 @@ func TestCatchUp_AnAPICreateIsAdoptedEvenAtTheTip(t *testing.T) {
owned := createdEvent("alpha", "kubectl-create")
owned.AppliedBy = AppliedBy{Kind: configv1alpha3.ApplierKindKustomization, Namespace: "x", Name: "other"}
loop.handleLiveEvents(&WriteRequest{Events: []Event{owned}})
loop.openWindow = nil
loop.windows.setOnly(nil)
loop.serviceAppliers()
require.Empty(t, requester.taken(), "neither is an API create of the folder's own objects")

loop.handleLiveEvents(&WriteRequest{
Events: []Event{createdEvent("alpha", "kubectl-create")},
})
loop.openWindow = nil
loop.windows.setOnly(nil)
loop.serviceAppliers()
adopt := requester.taken()
require.Len(t, adopt, 1, "adopted, although the applier is at the tip")
Expand Down
2 changes: 1 addition & 1 deletion internal/git/apply_converge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ func TestCatchUpAfter_YieldsToAnEditBeforeItsResume(t *testing.T) {
assert.False(t, sent[0].Current(), "an edit arrived during the fetch")

report(sent[0], ApplyStale)
loop.openWindow = nil
loop.windows.setOnly(nil)
loop.serviceAppliers()
assert.Empty(t, requester.taken(), "not again at once: a fresh interval")
state := loop.appliers[applierRef("alpha")]
Expand Down
2 changes: 1 addition & 1 deletion internal/git/apply_ownership_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func TestOwnership_ALiveWriteOfAnotherAppliersObjectIsNotMirrored(t *testing.T)
event := windowEvent("alpha", "2", "kubectl-edit")
event.AppliedBy = other
loop.handleLiveEvents(&WriteRequest{Events: []Event{event}})
assert.Nil(t, loop.openWindow, "the object another applier owns opened no window")
assert.Nil(t, loop.windows.only(), "the object another applier owns opened no window")
}

// TestOwnership_ASnapshotObjectOfAnotherApplierIsNeitherUpsertedNorSwept: the resync drops the object
Expand Down
2 changes: 1 addition & 1 deletion internal/git/apply_window_carry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func carryKey() string { return objectKey(carryEdit("alice", "", "").Identifier)
func TestApplyWindow_AWithheldApplyIsNotCommittedUnderTheNextEdit(t *testing.T) {
t.Run("in the window that holds the edit", func(t *testing.T) {
f := carryFlushOverAnEdit(t, "carry-same-window")
require.NotNil(t, f.loop.openWindow, "E1 is collecting in alice's window")
require.NotNil(t, f.loop.windows.only(), "E1 is collecting in alice's window")

f.writeWindow(carryApply("a1", "b1"))
require.True(t, f.echoApplier().repair, "the apply over E1 is withheld, and a Repair is pending")
Expand Down
6 changes: 3 additions & 3 deletions internal/git/apply_window_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,9 @@ func TestApplyWindow_OnlyAWithheldWriteIsKeptFromTheCommitWindow(t *testing.T) {
windowEvent("alpha", "2", FluxFieldManager),
}})

require.NotNil(t, loop.openWindow)
require.Len(t, loop.openWindow.pathToEvent, 1)
for _, event := range loop.openWindow.pathToEvent {
require.NotNil(t, loop.windows.only())
require.Len(t, loop.windows.only().pathToEvent, 1)
for _, event := range loop.windows.only().pathToEvent {
replicas, _, _ := unstructured.NestedString(event.Object.Object, "spec", "replicas")
assert.Equal(t, "3", replicas, "Git keeps the API's value")
}
Expand Down
Loading
Loading