Grafana / PR #25

Dual Storage Architecture

grafana/grafana#90045
Golden recall: 100% Extra findings: 11

Golden Comments (3/3 found)

# Comment Severity Status FriendlyReviewer Detail
#1 The context is being created with d.Log instead of the log variable that was initialized with additional context values (name, kind, method). This means those values won't be propagated to the logging context. MEDIUM ✓ Found "Delete resets the request logger with klog.NewContext(ctx, d.Log), dropping the name/kind/method values attached on line 96. Every sibling method uses klog.NewContext(ctx, log). Use log, not d.Log" (dualwriter_mode3.go L97)
#2 Bug: calling recordLegacyDuration when storage operation fails should be recordStorageDuration. HIGH ✓ Found "In Create (line 45) and Update (line 129) error paths, d.recordLegacyDuration(true, ...) is called with startStorage (the storage-call duration), reporting unified-store failures under the dual_writer_legacy_duration_seconds series. Should be recordStorageDuration to match the success paths (lines 48/132)" (dualwriter_mode3.go L45)
#3 Inconsistency: using name instead of options.Kind for metrics recording differs from other methods. MEDIUM ✓ Found "The Delete success path passes name (e.g. "foo") as the 'kind' metric label value, unlike the error path (line 103) and every other call that uses options.Kind. This creates a new cardinality series per object name and mislabels data. Use options.Kind" (dualwriter_mode3.go L106)

Supplementary Findings (11 findings)

Finding File Severity Legitimate?
Async legacy-write goroutines derive their timeout from the HTTP request context, cancelled as soon as the handler returns dualwriter_mode3.go:52 HIGH ✓ The most serious finding — the 'safe-measure' legacy write races request teardown and may be cancelled before it lands
TestMode3_Delete/TestMode3_DeleteCollection will panic non-deterministically in the async goroutine (no legacy mock expectations registered) dualwriter_mode3_test.go:228 HIGH ✓ Real — testify's mock.Called panics on unexpected calls; scheduling-dependent process killer
Mode 3 integration subtests assert the legacy API reflects writes immediately — racy under async propagation playlist_test.go:529 HIGH ✓ Real race — legacy assertions may fail intermittently in the executed Mode 3 subtests
DeleteCollection async goroutine records the wrong metric series (legacy duration reported as storage) dualwriter_mode3.go:166 MEDIUM ✓ Same bug family as golden #2 — the only one of the four async goroutines using the wrong recorder
Legacy-write errors silently dropped — only recorded into a histogram, never logged; timeout causes are dead code dualwriter_mode3.go:56 MEDIUM ✓ Real observability gap — the legacy store can drift out of sync with no operator visibility
Update passes objInfo directly to Legacy.Update instead of wrapping with updateWrapper dualwriter_mode3.go:139 MEDIUM ✓ Real — the legacy result can diverge from unified storage for non-idempotent UpdatedObjectInfo
Create writes the pre-write object to legacy instead of the storage-returned object dualwriter_mode3.go:51 MEDIUM ✓ Real — server-assigned fields (uid, resourceVersion, generation, finalizers, defaults) are dropped from the legacy mirror
Write tests never call AssertExpectations and provide no synchronization for the async goroutines dualwriter_mode3_test.go:63 MEDIUM ✓ Real — the async legacy-write path is effectively untested as written; tests would pass even if the goroutine never ran
No test covers failure of the async legacy write dualwriter_mode3_test.go:30 MEDIUM ✓ Real missing coverage — no case confirms the unified operation still succeeds and the error is tolerated
Mode 3 subtests reuse generic assertions that never verify the Mode 3 contract playlist_test.go:143 MEDIUM ✓ Real — reads-from-unified-only and async propagation to legacy are never asserted
Shared fixtures/registry for the Mode 3 tests still live in the Mode 1 test file dualwriter_mode1_test.go:27 LOW ✓ Borderline — maintainability concern only, acknowledged as not a functional bug

Honest Analysis

FriendlyReviewer found all 3 golden bugs of this PR — recall 100%. All three live on the async legacy path of dualwriter_mode3.go: Delete resetting the request logger with d.Log instead of log (L97), the error paths of Create/Update recording storage failures under the legacy metric series (L45), and the Delete success path passing name instead of options.Kind as the metric label (L106). Each match is direct. One nuance: the dataset rates the recorder bug High while FriendlyReviewer rated it Medium — the substance is identical, only the severity calibration differs.

The most significant issue the review found is not in the dataset: the async goroutines derive their timeout from the incoming HTTP request context, which is cancelled as soon as the handler returns (L52) — so the 'safe-measure' legacy write races request teardown and may never land. This finding, unscored, is the one that most undermines the mode's stated purpose. The same goes for the missing legacy mock expectations that make the Delete/DeleteCollection tests panic non-deterministically (L228), a scheduling-dependent process killer.

The remaining extras — the wrong recorder on DeleteCollection, silently dropped legacy-write errors, the unwrapped Update and pre-write Create objects, and the test gaps (no AssertExpectations, no failure case, racy playlist assertions) — are all specific, localizable, and coherent with the golden cluster: the review's diagnosis of the async path is deeper than the reference set. Only the last extra (shared fixtures in the Mode 1 test file) is a maintainability nit, and it is presented as such.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌤️ Feat
3 high, 10 medium, 1 low 1 ok, 1 mitigated

The Mode 3 dual-writer implementation matches the documented contract structurally: reads (Get/List) are served exclusively from unified storage, and writes (Create/Update/Delete/DeleteCollection) write to unified storage synchronously and then to legacy storage in an asynchronous, best-effort goroutine. However, the async legacy path contains several genuine correctness and reliability bugs that should be fixed before merge.

The most significant issues: (1) the async goroutines derive their timeout from the incoming HTTP request context, which is cancelled as soon as the handler returns (i.e. immediately after the unified write), so the "safe-measure" legacy write races request teardown and may be cancelled before it lands — defeating the purpose of the mode; (2) multiple write paths record the wrong metric series (storage failures reported as legacy, and the DeleteCollection async path reporting legacy as storage), plus a mislabeled kind metric label; (3) Update passes objInfo directly to Legacy.Update instead of wrapping with updateWrapper, and Create writes the pre-write object rather than the storage-returned object, allowing the legacy mirror to diverge from unified storage; (4) legacy-write errors are silently dropped with no logging or timeout-cause surfacing; and (5) Delete drops the per-request log context.

Test coverage is also incomplete: unit tests never assert the async legacy path actually ran (no AssertExpectations/synchronization), the Delete/DeleteCollection tests will panic non-deterministically in the async goroutine due to missing legacy mock expectations, and no test covers async legacy-write failure. The new Mode 3 integration subtests reuse generic assertions that read from legacy immediately, which is racy under async propagation and does not explicitly verify the Mode 3 contract. A minor maintainability note: the shared fixtures/registry used by the rewritten Mode 3 tests still live in the Mode 1 test file.

Overall: the feature is directionally correct but not ready to merge given the async reliability, metric-accuracy, and test-integrity problems.

These might need a close look

  • 🔴 pkg/apiserver/rest/dualwriter_mode3.go (L52)
    The async legacy-write goroutines derive context.WithTimeoutCause(ctx, ...) from the incoming HTTP request context, which is cancelled shortly after the handler returns — and the handler returns immediately after the unified write. The 'async' legacy write therefore races request teardown and may be cancelled before completing, defeating the mode's purpose. Derive from a request-independent context (e.g. context.Background() with a fresh timeout).
  • 🔴 pkg/apiserver/rest/dualwriter_mode3_test.go (L228)
    TestMode3_Delete (~line 228) and TestMode3_DeleteCollection (~line 282) do not register any legacy mock, so the async goroutine hits d.Legacy.Delete(...)/DeleteCollection(...) with no matching expectation, causing testify's mock.Called to panic ('unexpected method call'). Because the goroutine is unsynchronized, this is a scheduling-dependent, process-killing race. Register the legacy expectations (and assert them).
  • 🔴 pkg/tests/apis/playlist/playlist_test.go (L529)
    doPlaylistTests asserts that the legacy API reflects k8s writes immediately (legacy list at line 529 and getFromBothAPIs legacy GETs), but under Mode 3 writes propagate to legacy asynchronously (background goroutine, 10s timeout). In the two executed Mode 3 subtests these assertions are racy and may fail intermittently. For Mode 3, poll/retry the legacy API until the async write lands, or parametrize/disable the legacy-read assertions for async modes.

Worth checking

  • 🟡 pkg/apiserver/rest/dualwriter_mode3.go (L45)
    In Create (line 45) and Update (line 129) error paths, d.recordLegacyDuration(true, ...) is called with startStorage (the storage-call duration), reporting unified-store failures under the dual_writer_legacy_duration_seconds series. Should be recordStorageDuration to match the success paths (lines 48/132).
  • 🟡 pkg/apiserver/rest/dualwriter_mode3.go (L166)
    The DeleteCollection async goroutine calls recordStorageDuration(err != nil, ..., startLegacy) even though startLegacy measures the Legacy.DeleteCollection call — the only one of the four async goroutines using the wrong recorder. Should be recordLegacyDuration; also inconsistent with the Create/Delete/Update goroutines.
  • 🟡 pkg/apiserver/rest/dualwriter_mode3.go (L106)
    The Delete success path passes name (e.g. "foo") as the 'kind' metric label value, unlike the error path (line 103) and every other call that uses options.Kind. This creates a new cardinality series per object name and mislabels data. Use options.Kind.
  • 🟡 pkg/apiserver/rest/dualwriter_mode3.go (L97)
    Delete resets the request logger with klog.NewContext(ctx, d.Log), dropping the name/kind/method values attached on line 96. Every sibling method uses klog.NewContext(ctx, log). Use log, not d.Log.
  • 🟡 pkg/apiserver/rest/dualwriter_mode3.go (L56)
    In all four async goroutines the legacy-write error is only recorded into a histogram (recordLegacyDuration(err != nil, ...)) and never logged. Since the legacy write is framed as a 'safe measure', silent failures leave the legacy store out of sync with no operator visibility; the timeout causes passed to context.WithTimeoutCause are dead code. Mode 1/2 log legacy errors. Log the error (and timeout cause) at minimum.
  • 🟡 pkg/apiserver/rest/dualwriter_mode3.go (L139)
    Update passes objInfo directly to Legacy.Update, which re-fetches the old object and re-runs objInfo.UpdatedObject(old). For non-idempotent/relative UpdatedObjectInfo the legacy result can diverge from what was written to unified storage. Wrap with updateWrapper{upstream: objInfo, updated: res} (as elsewhere in the package) to force the unified result into legacy.
  • 🟡 pkg/apiserver/rest/dualwriter_mode3.go (L51)
    The Create async goroutine calls d.Legacy.Create(ctx, obj, ...) with the raw input object instead of the storage-returned created object, so server-assigned fields (uid, resourceVersion, generation, finalizers, defaults) are dropped from the legacy mirror. Use created so the mirror matches unified storage.
  • 🟡 pkg/apiserver/rest/dualwriter_mode3_test.go (L63)
    Write tests never call m.AssertExpectations(t) and provide no synchronization (WaitGroup/channel/assert.Eventually) for the async goroutines. As written, the async legacy-write path is effectively untested — tests would pass even if the goroutine never ran. Add AssertExpectations and wait for the goroutines so the async path is deterministically exercised.
  • 🟡 pkg/apiserver/rest/dualwriter_mode3_test.go (L30)
    No test covers failure of the async legacy write. There is no case where Legacy.Create/Update/Delete returns an error and the test confirms (a) the unified operation still returns success and (b) the error is tolerated (no panic, metrics recorded, ideally logged). Add a legacy-failure case per write method.
  • 🟡 pkg/tests/apis/playlist/playlist_test.go (L143)
    The two executed Mode 3 subtests only add storage/feature-toggle/mode wiring and reuse the unmodified doPlaylistTests; none of the assertions actually verify the Mode 3 contract (reads from unified only, writes propagate async to legacy). Add a targeted Mode 3 assertion — e.g. create via k8s and confirm get/list come from unified and the object eventually appears in legacy — rather than relying on the racy generic assertions.
Small things (take or leave)
  • 🔵 pkg/apiserver/rest/dualwriter_mode1_test.go (L27)
    The rewritten Mode 3 tests now depend on package-scope fixtures and the metrics registry defined in this Mode 1 test file (exampleObj, failingObj, exampleList, p, etc.), making Mode 1's test file the de-facto owner of shared infrastructure for the whole rest package. Relocating these to a dedicated shared test file (e.g. storage_mocks_test.go or dualwriter_fixtures_test.go) would avoid compile-time breakage if the Mode 1 file is later renamed/pruned. Maintainability concern, not a functional bug.

✅ Feature-level checklist

Looks good

  • ✅ Mode 3 reads (get, list) must use only unified storage.
    Get and List never touch the legacy store, structurally matching the contract. Integration coverage for this is indirect, but the implementation is correct.

Partially covered

  • ⚠️ Mode 3 writes (create, update, delete, delete-collection) must write to unified storage and then asynchronously write to legacy as a safe measure.
    Writes do hit unified synchronously and then attempt an async legacy write, matching the shape of the contract. However, the async legacy write derives its timeout from the request context (cancelled on handler return), so it may be cancelled before completing; legacy-write errors are silently dropped; and Create/Update can write divergent data to legacy (pre-write object / unwrapped objInfo). These issues compromise the reliability of the 'safe measure' legacy write and should be fixed.