docs(specs): correct S22 storage accounting against the current schema

What

Four corrections to the S22 storage accounting spec, found while planning its implementation. Spec-only change; no code and no plan content.

The S22 plan MR (!1296 (merged)) is stacked on this branch and is written against the amended spec, so this should merge first.

A fifth correction, to ADR-007, could not be made here and is open as a handbook MR: gitlab-com/content-sites/handbook!20663 (merged). See ADR-007 below.

1. Maven's counter stub is not where the spec says

The Dependencies table cited internal/format/maven/store.go twice. That file does not exist — only store_integration_test.go does — so both citations sent a reader nowhere. The stub pair is declared on the UploadStore interface in internal/format/maven/upload.go.

Also records that the datastore implementation, MavenRepositoryStore.BumpRepoCounters, carries an extra qrm.DB parameter in second position that the interface method does not. An implementer wiring the real call hits that mismatch immediately.

2. maven_remote_versions exists

It landed with the Maven remote versions schema (S14 Step 3) after this spec was written. The spec's version-type enumerations named four tables and asserted the Maven remote-cache table did not exist yet. There are five, and container_remote_manifests is now the only remote-cache table still missing.

The soft-delete branch list gains maven_remote_versions, which makes container_manifests the sole version-type table without the column.

The normative rule was already correct and self-updating — the sum is "the set of version-type tables across all implemented (format, kind) combinations", not a fixed list — so only the illustrative enumerations and the existence claim were stale. This also means (maven, remote) is a live positive-hit branch for the recomputes rather than a zero-result case.

3. reconciliation_backlog needs a single writer, not the trigger leader

The gauge-versus-histogram reasoning is untouched and still load-bearing: a gauge set once per fire leaves stale series across ex-leaders, corrupting the falling-behind alert. What that reasoning requires is a single writer, not that the writer be the reconciliation-trigger leader specifically.

River's leader elector is unexported and jobsriver.Client exposes no leadership accessor, so binding the collector to trigger leadership cannot be expressed without widening the background-jobs foundation's API. A Redis lease over the cache-purpose client — the coordination shape S27 already prescribes for its queue-stats pollers — satisfies the property directly. Either mechanism is now conformant provided at most one pod emits the series at a time.

The wedged-trigger analysis is reworked accordingly: with the collector on its own lease, its liveness no longer tracks trigger leadership, and the old paragraph would have been incoherent. The acceptance criterion gains a third assertion — with two instances running, exactly one emits — which is what actually pins the property.

4. The three paging alerts stay required; their wiring moves to #354

This is the one hunk worth arguing about, so it is called out rather than buried. The original text said the alert definitions are "in scope for S22, not deferred to a follow-up" — wording that exists precisely to prevent this kind of edit.

It is not a deferral of the requirement. Definition and wiring are separated:

  • The three alert definitions stay here, stay normative, and the text now says they are "the three above" so there is no ambiguity about what S22 owes. Wiring them is transcription, not design.
  • Only wiring and the firing assertion move to the change that lands #354.
  • Added explicitly: this remains "a release gate on the service being alertable rather than an item S22 may drop".

The blocker is outside S22's reach and concrete: this repository contains no alerting whatsoever — no rule files, no PrometheusRule, no rules-testing job — and no gitlab_artifact_registry_* series reach the Mimir tenant from either environment, so there is nothing for a rule to select on and nothing to assert a firing against. #354 tracks the metrics-catalog work and is itself blocked on collection working first.

The acceptance criterion is split so the verifiable half is not lost: metric registration (names, types, bounded label sets) is asserted now; alert wiring is asserted with #354.

ADR-007: handled in the handbook repo

ADR-007 contradicted itself on what the repository-level size_bytes sum joins. Two prose sentences said blob_storage_blobs, while all three of its repository-level example queries join blob_storage_blobs_by_namespace — and the cost paragraph in the same section already called that lookup "a single-partition index-only scan".

docs/adr/ in this repo is generated by the daily sync:adrs job and must not be edited here, so the fix is open upstream as handbook!20663. It corrects both prose mentions, states why the shadow is the right source so the next reader does not revert it, and clarifies the "no additional insurance structures" sentence that read as though the shadow were unused at that level.

That MR is not a blocker for this one or for !1296 (merged): the plan records the contradiction and follows the example queries, which is the reading handbook!20663 confirms. The correction reaches docs/adr/ here automatically once it merges.

Not changed here

  • Acceptance criterion 19's over-statement of the OCI manifest cascadeContainerManifestDeleter never touches container_blobs, and ADR-007 states the manifest-to-layer relationship is deliberately not modeled as a foreign key. That wording travels to the S20-phase1 spec along with criteria 17–21 and 25, so amending it here would be churn.

Related to #515

Edited by Pawel Rozlach

Merge request reports

Loading
Loading