Skip to content

[Store] Make NoF storage gauges projection-only - #6

Open
Hubert-Zhu wants to merge 1 commit into
mainfrom
hzhu/store-drop-nof-gauge-dual-write
Open

Hubert-Zhu wants to merge 1 commit into
mainfrom
hzhu/store-drop-nof-gauge-dual-write

Conversation

@Hubert-Zhu

@Hubert-Zhu Hubert-Zhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Description

Since the storage metrics projection landed in
#3385, the NoF gauges
have had two writers with incompatible semantics. project_storage_usage()
sets absolute values from the authoritative NoFSegmentManager snapshot,
while allocate, deallocate, MountSegment and CommitUnmountSegment
still applied relative increments. The two agree only as long as every
increment is paired correctly, so a single missed decrement makes the
exported value jump the next time the projection runs, and every NoF
allocation pays for gauge bookkeeping it no longer needs.

This drops the incremental writers and lets the projection own the NoF
gauges, following the direction in
RFC #3158.

The read side is unchanged. The same gauge names and per-segment labels are
still exported, now refreshed from NoFSegmentManager::GetUsageSnapshot().
Two things make this behavior-preserving for consumers:

  • MasterAdminServer::RefreshStorageMetrics() runs before every externally
    visible read (/metrics scrape, metrics summary, and the periodic
    reporting loop), so no consumer can observe a stale gauge.
  • project_storage_usage() already drops per-segment labels for segments
    absent from the snapshot via projected_nof_segments_, which is what
    remove_nof_segment_metrics() used to do on unmount.

The gauge names are untouched, and the only consumer outside this module is
monitoring/grafana/dashboards/mooncake.json, which queries
master_nof_allocated_bytes and master_total_nof_capacity_bytes and never
the per-segment variants. Nothing under docs/ references either name, so
there is no documentation to update.

The NoF unmount API carried a metrics_dec_capacity out-parameter whose only
purpose was handing the segment size from PrepareUnmountSegment to
CommitUnmountSegment for that decrement, so it goes away with the decrement
it fed. The DRAM path has a separate copy of that parameter and keeps it
until its own writers are removed.

This also fixes a latent leak. MasterMetricManager is a process-lifetime
singleton that outlives MasterService, which is reconstructed per HA
leadership term. ~MasterService releases the capacity of still-mounted DRAM
segments via SegmentManager::releaseCapacityMetrics(), but there has never
been a NoFSegmentManager equivalent, so NoF segments that were mounted at
demotion left their inc_total_nof_capacity() behind and the next term
mounted on top of it. Absolute projection makes that unreachable by
construction: each refresh recomputes the gauge from the segments that are
actually mounted.

Scoped to the NoF tier deliberately. NoF and DRAM have independent gauges,
managers and allocator paths, so removing one tier's writers is atomic and
leaves no half-migrated state. The DRAM tier is larger and carries three
design decisions of its own (standby accounting, the CxlRegionDriver
capacity-gauge-as-refcount, and the ApplySnapshotState metric rebuild); it
will follow separately.

Module

  • Mooncake Store (mooncake-store)

Type of Change

  • Refactor

How Has This Been Tested?

NoFUsageSnapshotSurvivesMetricsReset no longer has to re-inflate the gauges
before teardown, because teardown no longer decrements them; it now zeroes
them the way a standby transition does, by projecting an empty snapshot.

Added NoFGaugesMoveOnlyWhenProjected, which pins the new contract:
allocating from a mounted NoF segment moves domain state while the previously
projected gauge values stay put, and only the next projection brings them
back in sync.

The same test also covers the per-segment label lifecycle that
remove_nof_segment_metrics() used to own, asserting the label appears in
serialize_metrics() once the mounted segment is projected and is gone once
the unmounted one is. That path previously had no end-to-end coverage:
ProjectStorageUsageRemovesAbsentSegmentLabels exercises the projection with
synthetic snapshots only, and no NoF test asserted on
StorageUsageSnapshot::segments at all.

Test commands:

bash scripts/run_ci_test.sh

Test results:

  • Unit tests pass
  • Integration tests pass (if applicable)
  • Manual testing done (describe below)

Not yet verified locally: the development machine has no cmake,
clang-format or pre-commit available, so this branch has not been compiled,
run or formatted locally. Every added line was checked by hand against the
80-column limit, and the repository was swept for remaining references to the
removed APIs, but clang-format's exact wrapping is unverified.

Checklist

  • I have performed a self-review of my own code
  • I have formatted my code using ./scripts/code_format.sh
  • I have run pre-commit on the files changed in this PR and all hooks pass
  • I have updated the documentation (if applicable)
  • I have added tests to prove my changes are effective
  • For changes >500 LOC: I have filed an RFC issue

AI Assistance Disclosure

  • AI tools were used (specify below)

Cursor was used to enumerate the legacy write call sites, confirm the
projection already covers per-segment label cleanup and the
refresh-before-read paths, and apply the removals.

Made with Cursor

Since the storage metrics projection landed, the NoF gauges have had two
writers with incompatible semantics: project_storage_usage() sets absolute
values from the authoritative NoFSegmentManager snapshot, while allocate,
deallocate, mount and unmount still applied relative increments. The two
agree only as long as every increment is paired correctly, so a single
missed decrement makes the exported value jump whenever the projection next
runs, and every NoF allocation pays for gauge bookkeeping it no longer
needs.

Drop the incremental writers and let the projection own the NoF gauges. The
read side is unchanged: the same gauge names and per-segment labels are
still exported, now refreshed from NoFSegmentManager::GetUsageSnapshot().
RefreshStorageMetrics() runs before every externally visible read, and
project_storage_usage() already retires labels for segments absent from the
snapshot, which is what remove_nof_segment_metrics() did on unmount.

This also fixes a latent leak. MasterMetricManager is a process-lifetime
singleton that outlives MasterService, which is reconstructed per HA
leadership term. ~MasterService releases the capacity of still-mounted DRAM
segments via SegmentManager::releaseCapacityMetrics(), but there has never
been a NoFSegmentManager equivalent, so NoF segments mounted at demotion
left their inc_total_nof_capacity() behind and the next term mounted on top
of it. Absolute projection makes that unreachable by construction.

The unmount API carried a metrics_dec_capacity out-parameter that existed
only to hand the segment size from Prepare to Commit for the decrement, so
it goes away with the decrement it fed. The DRAM path has a separate copy of
that parameter and keeps it until its own writers are removed.

The decoupling guard test no longer has to re-inflate the gauges before
teardown, because teardown no longer decrements them. Add a test pinning the
new contract: allocating moves domain state while the previously projected
values stay put, and the per-segment label appears and is retired purely as
a function of what the projection sees. That label path previously had no
end-to-end coverage.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Hubert-Zhu
Hubert-Zhu force-pushed the hzhu/store-drop-nof-gauge-dual-write branch from e307fc4 to bd8c11b Compare September 4, 2026 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant