*: delete per-store metrics when a store is tombstoned - #11127
*: delete per-store metrics when a store is tombstoned#11127bufferflies wants to merge 6 commits into
Conversation
Several per-store Prometheus metrics were never cleaned up on store removal, leaking stale time series until the next PD leader election forced a full Reset(). Most visibly, pd_hotcache_status entries in HotPeerCache.gc() were only removed from the in-memory map, never from the GaugeVec itself. Add DeleteStoreMetrics helpers to pkg/schedule/filter, pkg/schedule/hbstream, and pkg/schedule/schedulers, and wire them into both the classic bury path (server/cluster.BuryStoreLocked / removeStoreStatistics) and the standalone scheduling microservice's store watcher (pkg/mcs/scheduling/server/meta), matching the existing pattern used by statistics.ResetStoreStatistics. Also switch storeStatusGauge cleanup in ResetStoreStatistics from a hand-maintained type-string list (which had already drifted out of sync with what observe()/ObserveHotStat() actually set) to DeletePartialMatch, so it cannot silently drift again. The per-store heartbeat/bucket-report histograms and counters defined directly in package server and pkg/mcs/scheduling/server cannot be reached from the bury path without a cross-package cycle, so they are swept periodically instead (serverMetricsLoop and Cluster.collectMetrics respectively). Close tikv#11126. Signed-off-by: bufferflies <tongjian3@foxmail.com> Signed-off-by: bufferflies <1045931706@qq.com>
ti-chi-bot
Bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds per-store Prometheus metric deletion helpers. Store removal, burial, periodic cleanup, and hot-cache garbage collection now delete metrics for removed stores. Tests verify that hot-cache metrics are removed after garbage collection. ChangesRemoved-store metric cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/statistics/hot_peer_cache_test.go`:
- Around line 841-843: The garbage-collection assertions currently recreate
removed gauge series through WithLabelValues, so they do not verify deletion. In
the assertions near the hot-cache GC test, replace those calls with
hotCacheStatusGauge.DeletePartialMatch using the partial labels for each store
and cache type, and assert that each deletion removes the expected series.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d5d34920-8b53-4652-97e2-635ab9ae28ac
📒 Files selected for processing (13)
pkg/mcs/scheduling/server/cluster.gopkg/mcs/scheduling/server/meta/watcher.gopkg/mcs/scheduling/server/metrics.gopkg/schedule/filter/metrics.gopkg/schedule/hbstream/metric.gopkg/schedule/schedulers/metrics.gopkg/statistics/hot_peer_cache.gopkg/statistics/hot_peer_cache_test.gopkg/statistics/store_collection.goserver/cluster/cluster.goserver/cluster/scheduling_controller.goserver/metrics.goserver/server.go
…tombstoned stores collectMetrics/cleanupRemovedStoreMetrics run on a fixed ticker and, for every store still IsRemoved() this tick, called DeleteStoreMetrics unconditionally. DeletePartialMatch takes an exclusive lock and does a linear scan of the whole metric vector, so a store lingering in the tombstoned-but-not-yet-fully-removed state (up to the tombstone GC interval) triggered that scan on every tick for no reason after the first pass. Track which store IDs have already been cleaned in a per-ticker-owned map (no lock needed, since only that ticker's single goroutine touches it), skip stores already in it, and drop entries once a store is no longer tombstoned (fully removed), so the tracking map itself stays bounded. Signed-off-by: bufferflies <tongjian3@foxmail.com> Signed-off-by: bufferflies <1045931706@qq.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/server.go`:
- Around line 786-794: Update cleanupRemovedStoreMetrics and the
metric-reporting paths so late heartbeat or bucket-report writes for tombstoned
stores cannot recreate deleted series. Either reject those writes while the
store remains in removed, or make cleanup repeatedly delete metrics after each
write; preserve cleanedRemovedStoreMetrics bookkeeping consistently with the
chosen approach.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68b250e4-34a7-4997-b6d0-a662465090cf
📒 Files selected for processing (2)
pkg/mcs/scheduling/server/cluster.goserver/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/mcs/scheduling/server/cluster.go
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (64.07%) is below the target coverage (74.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #11127 +/- ##
========================================
Coverage 79.35% 79.36%
========================================
Files 542 542
Lines 76993 77187 +194
========================================
+ Hits 61097 61256 +159
- Misses 11594 11630 +36
+ Partials 4302 4301 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…den test - pkg/statistics/store_collection.go: fix import group ordering flagged by the statics CI check (gci) -- prometheus/client_golang belongs in the default group before the pingcap group, not merged into it. - server/server.go, pkg/mcs/scheduling/server/cluster.go: drop the cleanedRemovedStoreMetrics dedup tracking added earlier. It was unsafe: HandleRegionHeartbeat (both server/grpc_service.go and pkg/mcs/scheduling/server/grpc_service.go) only checks store == nil, not store.IsRemoved(), before recording regionHeartbeat*/ bucketReport* metrics, unlike HandleStoreHeartbeat which rejects tombstoned stores up front via checkStore(). A late region heartbeat for an already-cleaned, still-tombstoned store could therefore recreate a metric series that the dedup tracking would then never sweep again. Go back to deleting unconditionally every tick; a DeletePartialMatch on labels that no longer exist is a cheap no-op. - pkg/statistics/hot_peer_cache_test.go: assert on hotCacheStatusGauge.DeletePartialMatch's return count instead of WithLabelValues+ToFloat64. The latter recreates a fresh, zero-valued series on every call regardless of whether gc() actually deleted the old one, so it verified less than it looked like; the former proves no series remain. Signed-off-by: bufferflies <tongjian3@foxmail.com> Signed-off-by: bufferflies <1045931706@qq.com>
|
/retest-required |
…ery tick filter.DeleteStoreMetrics and hbstream.DeleteStoreMetrics were only called once, from BuryStoreLocked at tombstone time. That one-shot delete gets undone almost immediately for both packages: - Every scheduler's Schedule() still runs cluster.GetStores() (which includes tombstoned-but-not-fully-removed stores) through StoreStateFilter each cycle. The rejection itself is what increments filterSourceCounter/filterTargetCounter, so as long as the store stays known, the counters get recreated on the very next scheduling cycle. - HeartbeatStreams' keepalive ticker sends to every entry in s.streams as long as storeInformer.GetStore(storeID) != nil, without checking IsRemoved(). Nothing unbinds a store's stream on bury, only on a failed Send, so heartbeatStreamCounter keeps getting rewritten for as long as the tombstoned store's stream stays connected. Neither has any cleanup tied to full removal either (unlike clusterStatusGauge's DeleteClusterStatusMetrics), so once recreated they would never be swept again. Move both into the existing per-tick collectSchedulingMetrics (server/cluster) and collectMetrics (pkg/mcs/scheduling/server) loops, which already iterate every store every cycle for other metrics, mirroring the same unconditional-delete pattern already used there for regionHeartbeat*/bucketReport* metrics. Keep the one-shot calls in BuryStoreLocked for immediacy; the periodic sweep makes them safe against the ongoing rewrites. Found via a systematic post-merge review of PR tikv#11127 after CI went green, using the github-pr-review skill. Signed-off-by: bufferflies <tongjian3@foxmail.com> Signed-off-by: bufferflies <1045931706@qq.com>
|
/retest-required |
1 similar comment
|
/retest-required |
|
/test pull-unit-test-next-gen-2 |
1 similar comment
|
/test pull-unit-test-next-gen-2 |
resetSchedulingMetrics (classic) and resetMetrics (mcs scheduling service) already reset schedulerStatusGauge, hotSpotStatusGauge, and other scheduling-related metrics when the scheduling service/job stops (context cancellation or primary handoff), but filter and heartbeat-stream counters were left out, so a standby replica or a freshly-stopped scheduling job could keep showing stale values from when it was last active. Add ResetFilterMetrics and ResetHeartbeatStreamMetrics, following the existing Reset*Metrics naming convention, and call them from both reset functions for parity with the other metrics already reset there. Signed-off-by: bufferflies <tongjian3@foxmail.com> Signed-off-by: bufferflies <1045931706@qq.com>
| delete(f.regionsOfStore, storeID) | ||
| delete(f.thresholdsOfStore, storeID) | ||
| delete(f.metrics, storeID) | ||
| hotCacheStatusGauge.DeletePartialMatch(prometheus.Labels{"store": storeTag(storeID), "type": f.kind.String()}) |
There was a problem hiding this comment.
This cleanup only runs from HotPeerCache.UpdateStat() via gc(). If an idle store is removed and no later hot-peer update arrives, this branch never executes, so the pd_hotcache_status series for that store remains indefinitely—the main remove-tombstone scenario in #11126 is therefore still uncovered.
There was a problem hiding this comment.
Fixed in 2f87de5: gc() is now also called from HotCache.CollectMetrics()'s existing periodic tick, so cleanup no longer depends on UpdateStat being triggered by unrelated activity.
| // late write would never get swept again for as long as the store stays | ||
| // tombstoned-but-not-yet-removed. DeletePartialMatch on labels that no longer | ||
| // exist is a cheap no-op, so repeating it every tick is safe. | ||
| for _, store := range rc.GetStores() { |
There was a problem hiding this comment.
cleanupRemovedStoreMetrics can only discover tombstoned stores while they remain in RaftCluster.GetStores(). A late heartbeat or bucket write after the last sweep followed by final store-metadata deletion recreates a series that no subsequent sweep can identify, leaving it permanently stale.
There was a problem hiding this comment.
Fixed in 2f87de5: cleanupRemovedStoreMetrics now tracks the set of store IDs seen tombstoned on the previous tick and, when one drops out of GetStores() entirely between ticks, performs one more delete for it.
| evictedSlowStoreStatusGauge.DeletePartialMatch(prometheus.Labels{"store": storeID}) | ||
| evictedStoppingStoreStatusGauge.DeleteLabelValues(storeID) | ||
| slowStoreTriggerLimitGauge.DeletePartialMatch(prometheus.Labels{"store": storeID}) | ||
| storeSlowTrendEvictedStatusGauge.DeletePartialMatch(prometheus.Labels{"store": storeID}) |
There was a problem hiding this comment.
HotPendingSum is also keyed by the store label and is written by the hot-region scheduler, but this helper never deletes it. Consequently, pd_scheduler_hot_pending_sum series survive store tombstoning even when every caller invokes this cleanup.
There was a problem hiding this comment.
Fixed in 2f87de5: DeleteStoreMetrics now also deletes HotPendingSum.
|
|
||
| // DeleteStoreMetrics deletes the per-store heartbeat/bucket-report metrics of a store. | ||
| func DeleteStoreMetrics(storeAddress, id string) { | ||
| labels := prometheus.Labels{"address": storeAddress, "store": id} |
There was a problem hiding this comment.
Deletion matches both the current address and store ID, but PD allows an existing store ID to change address. If the store emitted metrics at address A and later moves to B, tombstone cleanup matches only B and permanently leaves the A-labeled heartbeat and bucket series.
There was a problem hiding this comment.
Fixed in 2f87de5: DeleteStoreMetrics, ResetStoreStatistics, and the mcs-scheduling equivalent now match on the store label alone, dropping the address requirement (and the pkg/mcs/router watcher, which had the same pattern).
| for _, s := range stores { | ||
| statsMap.Observe(s) | ||
| statistics.ObserveHotStat(s, c.hotStat.StoresStats) | ||
| if s.IsRemoved() { |
There was a problem hiding this comment.
ObserveHotStat runs immediately before this branch and can recreate storeStatusGauge from retained rolling stats for a tombstoned store, while this block never resets that vector. The one-shot reset in the meta watcher is therefore undone on the next collection tick, and the series becomes unreachable after final metadata deletion.
There was a problem hiding this comment.
Fixed in 2f87de5: the per-store cleanup block now also calls statistics.ResetStoreStatistics, via a small deleteTombstonedStoreMetrics helper shared with the final-removal catch-up path, so it's reapplied every tick alongside the other metrics.
…moval race Five issues raised in review, all fixed: - pkg/statistics/hot_cache.go: HotPeerCache.gc() was only ever triggered from UpdateStat, so a store removed while the cluster is idle (or while it was the only store still receiving hot-peer updates) would never have its hotCacheStatusGauge series cleaned up -- the main remove-tombstone scenario in tikv#11126. Call gc() from CollectMetrics's existing periodic, activity-independent tick instead; it already self-throttles via topNTTL, so calling it every tick is cheap. - pkg/statistics/store_collection.go, server/metrics.go, pkg/mcs/scheduling/server/metrics.go: ResetStoreStatistics and DeleteStoreMetrics matched on address as well as store ID, but PD allows an existing store ID to change address (e.g. after a TiKV restart with a new IP). Matching both permanently leaked any series recorded under a previous address. Match on the store label alone; it's the stable identifier. Updated all call sites accordingly, including pkg/mcs/router's watcher, which had the same pattern. - pkg/schedule/schedulers/metrics.go: HotPendingSum is keyed by store but wasn't deleted by DeleteStoreMetrics. - server/server.go, pkg/mcs/scheduling/server/cluster.go, server/cluster/scheduling_controller.go: the periodic sweeps can only discover tombstoned stores while they remain in GetStores(). A write landing after the last sweep but before the store's final metadata deletion recreates a series that no later sweep can reach, since the per-tick loop only considers stores GetStores() currently returns. Track the set of store IDs seen tombstoned on the previous tick; when one drops out of GetStores() entirely between ticks (i.e. it was fully removed), do one more delete for it. This now needs only the store ID, not a cached address, since the address match was dropped above. - pkg/mcs/scheduling/server/cluster.go: ObserveHotStat runs just before the per-store cleanup block and can recreate storeStatusGauge from rolling stats that were never retired for a tombstoned store (unlike classic mode, nothing here calls RemoveRollingStoreStats). The block never reset that vector, so the meta watcher's one-shot reset was undone on the very next collection tick. Call ResetStoreStatistics from the same per-tick sweep as the other per-store metrics, via a small deleteTombstonedStoreMetrics helper shared with the final-removal catch-up path above. Signed-off-by: bufferflies <tongjian3@foxmail.com> Signed-off-by: bufferflies <1045931706@qq.com>
ti-chi-bot
Bot
added
size/XL
|
@bufferflies: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| // it tombstoned but before removal completed -- would otherwise be unreachable | ||
| // by any future sweep. One more delete call closes that window; it's a no-op if | ||
| // nothing was actually rewritten. | ||
| for storeID := range s.recentlyTombstonedStores { |
There was a problem hiding this comment.
recentlyTombstonedStores only contains stores observed by an earlier one-minute tick. If a store is buried and remove-tombstone deletes it before that tick, this loop never learns its ID; the existing heartbeat/bucket series (and any late writes) therefore remain in this process indefinitely, so the cleanup still misses a supported manual-removal path.
| if s.IsRemoved() { | ||
| storeID := s.GetID() | ||
| current[storeID] = struct{}{} | ||
| deleteTombstonedStoreMetrics(strconv.FormatUint(storeID, 10)) |
There was a problem hiding this comment.
DeletePartialMatch takes an exclusive lock and scans the entire vector even when nothing matches, so running 12 such calls for every tombstoned store every 10 seconds makes this path O(tombstones × series), and quadratic for recreated per-store filter series. With the reported 1k-store case, the metrics loop repeatedly performs thousands of full scans and contends with heartbeat and scheduler writes.
| // about (heartbeat/bucket metrics, storeStatusGauge/clusterStatusGauge, filter | ||
| // counters, and heartbeat-stream counters) for a tombstoned store. | ||
| func deleteTombstonedStoreMetrics(storeID string) { | ||
| statistics.ResetStoreStatistics(storeID) |
There was a problem hiding this comment.
ResetStoreStatistics also deletes pd_cluster_status, including the store_tombstone_count series that statsMap.Observe intentionally emits just above and the dashboard sums. Because this helper runs after every observation, standalone scheduling permanently hides tombstoned stores from that metric, unlike classic mode where the next collection restores it.
| addr := store.GetAddress() | ||
| storeIDStr := strconv.FormatUint(storeID, 10) | ||
| statistics.ResetStoreStatistics(addr, storeIDStr) | ||
| statistics.ResetStoreStatistics(storeIDStr) |
There was a problem hiding this comment.
pd_hotspot_status is also keyed by address and store, but none of these tombstone helpers delete it. CollectHotSpotMetrics can keep it alive while cached hot peers remain and stops iterating the store after final metadata deletion; an immediate remove-tombstone or an address change therefore leaves this series indefinitely despite the per-store scheduler cleanup.
| delete(f.regionsOfStore, storeID) | ||
| delete(f.thresholdsOfStore, storeID) | ||
| delete(f.metrics, storeID) | ||
| hotCacheStatusGauge.DeletePartialMatch(prometheus.Labels{"store": storeTag(storeID), "type": f.kind.String()}) |
There was a problem hiding this comment.
gc only iterates peersOfStore, but calcHotThresholds creates five pd_hotcache_status children and a thresholdsOfStore entry before any peer is hot. A store that never enters peersOfStore is therefore never visited here, so its threshold series survive final store removal and #11126 remains reproducible for idle or non-hot stores.
| deleteTombstonedStoreMetrics(strconv.FormatUint(storeID, 10)) | ||
| } | ||
| } | ||
| c.recentlyTombstonedStores = current |
There was a problem hiding this comment.
recentlyTombstonedStores drops the ID immediately after the first absent-store sweep, but StoreHeartbeat records an error counter and duration for an unknown store after final metadata deletion. Any later heartbeat recreates these series after the last possible delete, and subsequent ticks can no longer identify the store, so the standalone scheduling service still leaks per-store heartbeat metrics.
| storeID := s.GetID() | ||
| current[storeID] = struct{}{} | ||
| storeIDStr := strconv.FormatUint(storeID, 10) | ||
| filter.DeleteStoreMetrics(storeIDStr) |
There was a problem hiding this comment.
The periodic cleanup here only covers filter and heartbeat-stream metrics. A pre-bury store snapshot can republish storeStatusGauge after ResetStoreStatistics, while evict-stopping and slow-trend cleanup recreate scheduler gauges with Set(0) after schedulers.DeleteStoreMetrics; neither family is swept again, so those per-store series can still persist after final removal.
| statistics.ResetLabelStatsMetrics() | ||
| // reset hot cache metrics | ||
| statistics.ResetHotCacheStatusMetrics() | ||
| filter.ResetFilterMetrics() |
There was a problem hiding this comment.
resetSchedulingMetrics still leaves most per-store scheduler vectors untouched: ResetSchedulerMetrics only resets schedulerStatusGauge, ruleStatusGauge, and regionLabelStatusGauge, while opInfluenceStatus, balanceDirectionCounter, the evicted-store gauges, balanceRangeGauge, and similar vectors remain. After a primary handoff or scheduling-job stop, the inactive replica therefore continues exposing stale store-labeled series even though the newly added filter and heartbeat-stream metrics are reset.
2 participants
What problem does this PR solve?
Issue Number: Close #11126
What is changed and how does it work?
Check List
Tests
Release note
Summary by CodeRabbit
Bug Fixes
Tests