From bd8c11bc3c9d52d7d11472efdd2b922f1a6e1396 Mon Sep 17 00:00:00 2001 From: Hubert-Zhu Date: Thu, 3 Sep 2026 21:57:44 -0400 Subject: [PATCH] [Store] Make NoF storage gauges projection-only 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 --- .../include/master_metric_manager.h | 13 +-- mooncake-store/include/segment.h | 6 +- mooncake-store/src/allocator.cpp | 21 ----- mooncake-store/src/master_metric_manager.cpp | 38 --------- mooncake-store/src/master_service.cpp | 15 ++-- mooncake-store/src/segment.cpp | 15 +--- mooncake-store/tests/segment_test.cpp | 80 ++++++++++++++++--- 7 files changed, 82 insertions(+), 106 deletions(-) diff --git a/mooncake-store/include/master_metric_manager.h b/mooncake-store/include/master_metric_manager.h index 60e909fb52..e7d470f540 100644 --- a/mooncake-store/include/master_metric_manager.h +++ b/mooncake-store/include/master_metric_manager.h @@ -50,14 +50,6 @@ class MasterMetricManager { void inc_valid_get_nums(int64_t val = 1); void inc_total_get_nums(int64_t val = 1); - // NoF segment Metrics - void inc_allocated_nof_size(const std::string& segment, int64_t val = 1); - void dec_allocated_nof_size(const std::string& segment, int64_t val = 1); - void reset_allocated_nof_size(); - void inc_total_nof_capacity(const std::string& segment, int64_t val = 1); - void dec_total_nof_capacity(const std::string& segment, int64_t val = 1); - void reset_total_nof_capacity(); - /** * @brief Refresh storage gauges from authoritative allocator snapshots. * @@ -102,13 +94,12 @@ class MasterMetricManager { // followed by client expiry / reaper cleanup). void remove_segment_metrics(const std::string& segment); - // NoF segment Metrics + // NoF segment Metrics. Written only by project_storage_usage(); business + // code reads NoFSegmentManager::GetUsage() instead. int64_t get_allocated_nof_size(); int64_t get_total_nof_capacity(); int64_t get_segment_allocated_nof_size(const std::string& segment); int64_t get_segment_total_nof_capacity(const std::string& segment); - // Remove all per-segment NoF metric labels for the given segment. - void remove_nof_segment_metrics(const std::string& segment); // File Storage Metrics void inc_allocated_file_size(int64_t val = 1); diff --git a/mooncake-store/include/segment.h b/mooncake-store/include/segment.h index 11e9a7e472..0d27cd126b 100644 --- a/mooncake-store/include/segment.h +++ b/mooncake-store/include/segment.h @@ -264,15 +264,13 @@ class ScopedNoFSegmentAccess { /** * @brief Prepare to unmount a segment by deleting its allocator */ - ErrorCode PrepareUnmountSegment(const UUID& segment_id, - size_t& metrics_dec_capacity); + ErrorCode PrepareUnmountSegment(const UUID& segment_id); /** * @brief Deleting the segment to complete the unmounting operation */ ErrorCode CommitUnmountSegment(const UUID& segment_id, - const UUID& client_id, - const size_t& metrics_dec_capacity); + const UUID& client_id); /** * @brief Get all the segments of a client diff --git a/mooncake-store/src/allocator.cpp b/mooncake-store/src/allocator.cpp index 23405e8607..87c9411881 100644 --- a/mooncake-store/src/allocator.cpp +++ b/mooncake-store/src/allocator.cpp @@ -219,9 +219,6 @@ CachelibBufferAllocator::~CachelibBufferAllocator() { if (replica_type_ == ReplicaType::MEMORY) { MasterMetricManager::instance().dec_allocated_mem_size(segment_name_, size()); - } else if (replica_type_ == ReplicaType::NOF_SSD) { - MasterMetricManager::instance().dec_allocated_nof_size(segment_name_, - size()); } }; @@ -251,9 +248,6 @@ std::unique_ptr CachelibBufferAllocator::allocate( if (replica_type_ == ReplicaType::MEMORY) { MasterMetricManager::instance().inc_allocated_mem_size(segment_name_, size); - } else if (replica_type_ == ReplicaType::NOF_SSD) { - MasterMetricManager::instance().inc_allocated_nof_size(segment_name_, - size); } return std::make_unique(shared_from_this(), buffer, size); } @@ -271,9 +265,6 @@ void CachelibBufferAllocator::deallocate(AllocatedBuffer* handle) { if (replica_type_ == ReplicaType::MEMORY) { MasterMetricManager::instance().dec_allocated_mem_size( segment_name_, freed_size); - } else if (replica_type_ == ReplicaType::NOF_SSD) { - MasterMetricManager::instance().dec_allocated_nof_size( - segment_name_, freed_size); } VLOG(1) << "deallocation_succeeded address=" << handle->buffer_ptr_ << " size=" << freed_size << " segment=" << segment_name_; @@ -290,9 +281,6 @@ std::unique_ptr CachelibBufferAllocator::adoptImportedBuffer( if (replica_type_ == ReplicaType::MEMORY) { MasterMetricManager::instance().inc_allocated_mem_size( segment_name_, allocation.requested_size); - } else if (replica_type_ == ReplicaType::NOF_SSD) { - MasterMetricManager::instance().inc_allocated_nof_size( - segment_name_, allocation.requested_size); } return std::make_unique( shared_from_this(), @@ -388,9 +376,6 @@ OffsetBufferAllocator::~OffsetBufferAllocator() { if (replica_type_ == ReplicaType::MEMORY) { MasterMetricManager::instance().dec_allocated_mem_size(segment_name_, size()); - } else if (replica_type_ == ReplicaType::NOF_SSD) { - MasterMetricManager::instance().dec_allocated_nof_size(segment_name_, - size()); } }; @@ -432,9 +417,6 @@ std::unique_ptr OffsetBufferAllocator::allocate(size_t size) { if (replica_type_ == ReplicaType::MEMORY) { MasterMetricManager::instance().inc_allocated_mem_size(segment_name_, size); - } else if (replica_type_ == ReplicaType::NOF_SSD) { - MasterMetricManager::instance().inc_allocated_nof_size(segment_name_, - size); } return allocated_buffer; } @@ -449,9 +431,6 @@ void OffsetBufferAllocator::deallocate(AllocatedBuffer* handle) { if (replica_type_ == ReplicaType::MEMORY) { MasterMetricManager::instance().dec_allocated_mem_size( segment_name_, freed_size); - } else if (replica_type_ == ReplicaType::NOF_SSD) { - MasterMetricManager::instance().dec_allocated_nof_size( - segment_name_, freed_size); } VLOG(1) << "deallocation_succeeded address=" << handle->data() << " size=" << freed_size << " segment=" << segment_name_; diff --git a/mooncake-store/src/master_metric_manager.cpp b/mooncake-store/src/master_metric_manager.cpp index 4339a74fe6..b45732f5f4 100644 --- a/mooncake-store/src/master_metric_manager.cpp +++ b/mooncake-store/src/master_metric_manager.cpp @@ -718,38 +718,6 @@ void MasterMetricManager::remove_segment_metrics(const std::string& segment) { } // NoF segment Metrics -void MasterMetricManager::inc_allocated_nof_size(const std::string& segment, - int64_t val) { - nof_allocated_size_.inc(val); - if (!segment.empty()) nof_allocated_size_per_segment_.inc({segment}, val); -} - -void MasterMetricManager::dec_allocated_nof_size(const std::string& segment, - int64_t val) { - nof_allocated_size_.dec(val); - if (!segment.empty()) nof_allocated_size_per_segment_.dec({segment}, val); -} - -void MasterMetricManager::reset_allocated_nof_size() { - nof_allocated_size_.reset(); -} - -void MasterMetricManager::inc_total_nof_capacity(const std::string& segment, - int64_t val) { - nof_total_capacity_.inc(val); - if (!segment.empty()) nof_total_capacity_per_segment_.inc({segment}, val); -} - -void MasterMetricManager::dec_total_nof_capacity(const std::string& segment, - int64_t val) { - nof_total_capacity_.dec(val); - if (!segment.empty()) nof_total_capacity_per_segment_.dec({segment}, val); -} - -void MasterMetricManager::reset_total_nof_capacity() { - nof_total_capacity_.reset(); -} - int64_t MasterMetricManager::get_allocated_nof_size() { return nof_allocated_size_.value(); } @@ -808,12 +776,6 @@ int64_t MasterMetricManager::get_segment_total_nof_capacity( return nof_total_capacity_per_segment_.value({segment}); } -void MasterMetricManager::remove_nof_segment_metrics( - const std::string& segment) { - nof_allocated_size_per_segment_.remove_label_value({{"segment", segment}}); - nof_total_capacity_per_segment_.remove_label_value({{"segment", segment}}); -} - // File Storage Metrics void MasterMetricManager::inc_allocated_file_size(int64_t val) { file_allocated_size_.inc(val); diff --git a/mooncake-store/src/master_service.cpp b/mooncake-store/src/master_service.cpp index c2b27127dc..b7426cbdbb 100644 --- a/mooncake-store/src/master_service.cpp +++ b/mooncake-store/src/master_service.cpp @@ -2857,8 +2857,6 @@ auto MasterService::UnmountNoFSegment(const UUID& segment_id, << ", error=nof_pool_disabled"; return tl::make_unexpected(ErrorCode::UNAVAILABLE_IN_CURRENT_MODE); #else - size_t metrics_dec_capacity = 0; // to update the metrics - std::shared_lock client_lock(client_mutex_); std::shared_lock shared_lock(snapshot_mutex_); auto alive_clients = ok_client_; @@ -2868,8 +2866,7 @@ auto MasterService::UnmountNoFSegment(const UUID& segment_id, { ScopedNoFSegmentAccess segment_access = nof_segment_manager_.getNoFSegmentAccess(); - ErrorCode err = segment_access.PrepareUnmountSegment( - segment_id, metrics_dec_capacity); + ErrorCode err = segment_access.PrepareUnmountSegment(segment_id); if (err == ErrorCode::SEGMENT_NOT_FOUND) { // Return OK because this is an idempotent operation return {}; @@ -2886,8 +2883,7 @@ auto MasterService::UnmountNoFSegment(const UUID& segment_id, // 3. Commit the unmount operation ScopedNoFSegmentAccess segment_access = nof_segment_manager_.getNoFSegmentAccess(); - auto err = segment_access.CommitUnmountSegment(segment_id, client_id, - metrics_dec_capacity); + auto err = segment_access.CommitUnmountSegment(segment_id, client_id); if (err != ErrorCode::OK) { return tl::make_unexpected(err); } @@ -11282,15 +11278,14 @@ bool MasterService::ProbeNoFSegment(const std::string& te_endpoint, bool MasterService::TryUnmountNoFSegmentByHeartbeat( const MountedNoFSegmentSnapshot& snapshot, const std::string& error_reason) { - size_t metrics_dec_capacity = 0; std::shared_lock client_lock(client_mutex_); std::shared_lock snapshot_lock(snapshot_mutex_); auto alive_clients = ok_client_; client_lock.unlock(); { auto nof_segment_access = nof_segment_manager_.getNoFSegmentAccess(); - ErrorCode err = nof_segment_access.PrepareUnmountSegment( - snapshot.segment_id, metrics_dec_capacity); + ErrorCode err = + nof_segment_access.PrepareUnmountSegment(snapshot.segment_id); if (err == ErrorCode::SEGMENT_NOT_FOUND || err == ErrorCode::UNAVAILABLE_IN_CURRENT_STATUS) { std::lock_guard lock(nof_heartbeat_mutex_); @@ -11315,7 +11310,7 @@ bool MasterService::TryUnmountNoFSegmentByHeartbeat( { auto nof_segment_access = nof_segment_manager_.getNoFSegmentAccess(); ErrorCode err = nof_segment_access.CommitUnmountSegment( - snapshot.segment_id, snapshot.client_id, metrics_dec_capacity); + snapshot.segment_id, snapshot.client_id); if (err != ErrorCode::OK && err != ErrorCode::SEGMENT_NOT_FOUND) { LOG(ERROR) << "segment_id=" << snapshot.segment_id << ", segment_name=" << snapshot.segment.name diff --git a/mooncake-store/src/segment.cpp b/mooncake-store/src/segment.cpp index 0ecfefdd5a..964e8c9612 100644 --- a/mooncake-store/src/segment.cpp +++ b/mooncake-store/src/segment.cpp @@ -1212,7 +1212,6 @@ ErrorCode ScopedNoFSegmentAccess::MountSegment(const NoFSegment& segment, nof_segment_manager_->mounted_segments_[segment.id] = { segment, client_id, SegmentStatus::OK, std::move(allocator)}; nof_segment_manager_->client_by_name_[segment.name] = client_id; - MasterMetricManager::instance().inc_total_nof_capacity(segment.name, size); return ErrorCode::OK; } @@ -1243,7 +1242,7 @@ ErrorCode ScopedNoFSegmentAccess::ReMountSegment( } ErrorCode ScopedNoFSegmentAccess::PrepareUnmountSegment( - const UUID& segment_id, size_t& metrics_dec_capacity) { + const UUID& segment_id) { auto it = nof_segment_manager_->mounted_segments_.find(segment_id); if (it == nof_segment_manager_->mounted_segments_.end()) { LOG(WARNING) << "NoF segment unmount: segment_id=" << segment_id @@ -1258,7 +1257,6 @@ ErrorCode ScopedNoFSegmentAccess::PrepareUnmountSegment( auto& mounted_segment = it->second; auto& segment = mounted_segment.segment; - metrics_dec_capacity = segment.size; std::shared_ptr allocator = mounted_segment.buf_allocator; @@ -1274,8 +1272,7 @@ ErrorCode ScopedNoFSegmentAccess::PrepareUnmountSegment( } ErrorCode ScopedNoFSegmentAccess::CommitUnmountSegment( - const UUID& segment_id, const UUID& client_id, - const size_t& metrics_dec_capacity) { + const UUID& segment_id, const UUID& client_id) { bool found_in_client_segments = false; auto client_it = nof_segment_manager_->client_segments_.find(client_id); if (client_it != nof_segment_manager_->client_segments_.end()) { @@ -1295,17 +1292,13 @@ ErrorCode ScopedNoFSegmentAccess::CommitUnmountSegment( << ", error=segment_not_found_in_client_segments"; } - std::string segment_name; auto segment_it = nof_segment_manager_->mounted_segments_.find(segment_id); if (segment_it != nof_segment_manager_->mounted_segments_.end()) { - segment_name = segment_it->second.segment.name; - nof_segment_manager_->client_by_name_.erase(segment_name); + nof_segment_manager_->client_by_name_.erase( + segment_it->second.segment.name); } nof_segment_manager_->mounted_segments_.erase(segment_id); - MasterMetricManager::instance().dec_total_nof_capacity( - segment_name, metrics_dec_capacity); - MasterMetricManager::instance().remove_nof_segment_metrics(segment_name); return ErrorCode::OK; } diff --git a/mooncake-store/tests/segment_test.cpp b/mooncake-store/tests/segment_test.cpp index 096fabb83c..0b676ab095 100644 --- a/mooncake-store/tests/segment_test.cpp +++ b/mooncake-store/tests/segment_test.cpp @@ -461,9 +461,9 @@ TEST_F(SegmentTest, NoFUsageSnapshotSurvivesMetricsReset) { EXPECT_EQ(usage.capacity_bytes, kSegmentSize); EXPECT_DOUBLE_EQ(usage.used_ratio(), 0.25); + // Zero the gauges by projecting an empty snapshot, as a standby does. auto& metrics = MasterMetricManager::instance(); - metrics.reset_allocated_nof_size(); - metrics.reset_total_nof_capacity(); + metrics.project_storage_usage({}); EXPECT_EQ(metrics.get_allocated_nof_size(), 0); EXPECT_EQ(metrics.get_total_nof_capacity(), 0); @@ -476,19 +476,12 @@ TEST_F(SegmentTest, NoFUsageSnapshotSurvivesMetricsReset) { EXPECT_EQ(usage.capacity_bytes, kSegmentSize); EXPECT_DOUBLE_EQ(usage.used_ratio(), 0.25); - // Restore global gauges before teardown because allocator and segment - // cleanup still emit their matching decrements. - metrics.inc_allocated_nof_size("", kAllocationSize); - metrics.inc_total_nof_capacity("", kSegmentSize); buffer.reset(); { auto segment_access = segment_manager.getNoFSegmentAccess(); - size_t metrics_dec_capacity = 0; - ASSERT_EQ(segment_access.PrepareUnmountSegment(segment.id, - metrics_dec_capacity), + ASSERT_EQ(segment_access.PrepareUnmountSegment(segment.id), ErrorCode::OK); - ASSERT_EQ(segment_access.CommitUnmountSegment(segment.id, client_id, - metrics_dec_capacity), + ASSERT_EQ(segment_access.CommitUnmountSegment(segment.id, client_id), ErrorCode::OK); } allocator.reset(); @@ -496,6 +489,71 @@ TEST_F(SegmentTest, NoFUsageSnapshotSurvivesMetricsReset) { EXPECT_EQ(segment_manager.GetUsage().capacity_bytes, 0u); } +// The NoF gauges are a projection: only project_storage_usage() writes them. +TEST_F(SegmentTest, NoFGaugesMoveOnlyWhenProjected) { + NoFSegmentManager segment_manager(BufferAllocatorType::OFFSET); + constexpr size_t kSegmentSize = 16 * 1024 * 1024; + constexpr size_t kAllocationSize = 4 * 1024 * 1024; + constexpr int64_t kProjectedUsed = 111; + constexpr int64_t kProjectedCapacity = 222; + + auto& metrics = MasterMetricManager::instance(); + TieredStorageUsageSnapshot projected; + projected.nof.used_bytes = static_cast(kProjectedUsed); + projected.nof.capacity_bytes = static_cast(kProjectedCapacity); + metrics.project_storage_usage(projected); + ASSERT_EQ(metrics.get_allocated_nof_size(), kProjectedUsed); + ASSERT_EQ(metrics.get_total_nof_capacity(), kProjectedCapacity); + + NoFSegment segment; + segment.id = generate_uuid(); + segment.name = "nof_projection_only_segment"; + segment.size = kSegmentSize; + segment.base = 0x340000000; + segment.te_endpoint = "nof_projection_only_endpoint"; + UUID client_id = generate_uuid(); + { + auto segment_access = segment_manager.getNoFSegmentAccess(); + ASSERT_EQ(segment_access.MountSegment(segment, client_id), + ErrorCode::OK); + } + auto allocator = GetNoFAllocatorForTesting(segment_manager, segment.id); + ASSERT_NE(allocator, nullptr); + auto buffer = allocator->allocate(kAllocationSize); + ASSERT_NE(buffer, nullptr); + + // Domain state moved; the gauges did not. + EXPECT_EQ(segment_manager.GetUsage().used_bytes, kAllocationSize); + EXPECT_EQ(metrics.get_allocated_nof_size(), kProjectedUsed); + EXPECT_EQ(metrics.get_total_nof_capacity(), kProjectedCapacity); + + projected.nof = segment_manager.GetUsageSnapshot(); + metrics.project_storage_usage(projected); + EXPECT_EQ(metrics.get_allocated_nof_size(), + static_cast(kAllocationSize)); + EXPECT_EQ(metrics.get_total_nof_capacity(), + static_cast(kSegmentSize)); + const std::string label = "segment=\"" + segment.name + "\""; + EXPECT_NE(metrics.serialize_metrics().find(label), std::string::npos); + + buffer.reset(); + { + auto segment_access = segment_manager.getNoFSegmentAccess(); + ASSERT_EQ(segment_access.PrepareUnmountSegment(segment.id), + ErrorCode::OK); + ASSERT_EQ(segment_access.CommitUnmountSegment(segment.id, client_id), + ErrorCode::OK); + } + allocator.reset(); + + // The projection, not the unmount, retires the per-segment labels. + projected.nof = segment_manager.GetUsageSnapshot(); + metrics.project_storage_usage(projected); + EXPECT_EQ(metrics.get_allocated_nof_size(), 0); + EXPECT_EQ(metrics.get_total_nof_capacity(), 0); + EXPECT_EQ(metrics.serialize_metrics().find(label), std::string::npos); +} + // MountSegmentDuplicate Tests: // 1. MountSegment with the same segment id. The second mount operation return // SEGMENT_ALREADY_EXISTS.