fix(keys): reclaim expired shared-dict entries in remove_expired_keys()#18
fix(keys): reclaim expired shared-dict entries in remove_expired_keys()#18AlinsRan wants to merge 1 commit into
Conversation
remove_expired_keys() only cleared the worker-local keys/index/expire_keys tables and issued no writes to the shared dict, so expired entries -- both the __ngx_prom__key_N index slots and the metric value keys -- stayed physically allocated. Nothing else reclaimed them. Index slots are never reused: when a metric's slot has expired, add() allocates a fresh slot at key_count+1, so under label churn with an exptime the slots grow without bound. And the passive per-write expiry scan cannot help, because it stops at the first non-expired entry at the LRU tail, where a permanent entry (the error metric, or any metric registered without an exptime) inevitably ends up sitting. The result is prometheus-metrics free_space stepping down on every new series and never recovering, until the dict is full and starts force-evicting live metrics. Flush the expired entries once per timer round. Expired entries are indistinguishable from absent ones through every shared-dict API, so reclaiming them cannot change any observable behaviour; only free_space and eviction pressure change. Also run prometheus_test.lua in CI -- it was never executed, only linted. Fixes apache/apisix#13658
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Problem
prometheus-metricsfree_spacedrops in steps and never recovers when metrics are registered with anexpire(apache/apisix#13658). A reporter's dump showed 747970 of 751496 keys were stale__ngx_prom__key_Nindex slots against 28 live ones, 388MB of a 512MB dict, with very few active series.KeyIndex:remove_expired_keys()only cleared the worker-localkeys/index/expire_keystables. It issued no writes to the shared dict, so the expired entries themselves — both the index slots and the metric value keys, which share the dict — stayed physically allocated.Nothing else reclaimed them:
add()takes the not-found branch and allocates a fresh slot atkey_count+1.key_countis monotonic, so under label churn with anexptimethe slot count grows without bound.exptime— inevitably goes cold and ends up sitting there, blocking the scan for good.Reproduced standalone against
0.20250302and0.20260623with identical output: 1000 active series held constant,free_spacedropping one step per burst and never recovering, and a single explicitflush_expired(0)at the end reclaiming 7000 entries and restoringfree_spaceto its initial value. A separate experiment confirmed the LRU-tail blocking: with a permanent entry at the tail, 1000 writes reclaimed nothing; without it, everything was reclaimed.This is also what feeds #12275 — the unbounded growth is what fills the dict in the first place, which is the precondition for the spin fixed in #16.
Fix
Call
self.dict:flush_expired()once perremove_expired_keys()timer round.Expired entries are indistinguishable from absent ones through every shared-dict API (
ngx_http_lua_shdict_lookup()returnsNGX_DONEfor them, and every caller treats that as a miss), so reclaiming them cannot change any observable behaviour —/metricsoutput, metric values andexpiresemantics are unchanged. Onlyfree_spaceand eviction pressure change.Live series are not at risk: an active metric's index slot TTL is refreshed on every operation via the
lookup_or_create()cache-hit path, which callskey_index:add()with the exptime wheneverexptime > 0, and counter value TTLs are refreshed by the counter sync each round. Only series that genuinely saw no writes for the wholeexpirewindow can be flushed, which is exactly the semantics of the feature.Cost is a locked linear scan of the dict per worker per timer round (3600s under the APISIX wiring). At steady state that is microseconds to milliseconds. An instance that has already accumulated the backlog pays one larger scan on the first round after upgrade and then converges — no manual flush or restart needed.
Tests
Three regression tests, all of which fail without the one-line change and pass with it:
TestKeyIndex:testRemoveExpiredKeysReclaimsSharedDict— an expired index slot and an expired value key are both physically gone after a round; a non-expired entry survives.TestKeyIndex:testExpiredSlotsDoNotAccumulate— repeated expire/re-add churn leaves at most one index slot in the dict.TestPrometheus:testExpiredMetricIsReclaimedFromDict— same, driven through the public counter API.Two supporting changes to the test harness:
SimpleDict:flush_expired()added, matching the real semantics (scans everything, removes only expired entries, returns the count; the optional argument caps removals, not scans).SimpleDict:ttl()no longer physically prunes the entry it reads. The realttlreports an expired entry asnot foundbut leaves it allocated; the mock's pruning would have made the reclamation assertions vacuous, sinceremove_expired_keys()callsttlon exactly the slots under test.The assertions read
dict.dictdirectly rather than going throughget(), sinceget()prunes as well — physical presence is the whole point here and cannot be observed through the dict API by design.CI
prometheus_test.luawas never executed by CI, only linted —luaunitwas installed and unused, so the tests added in #14 and #16 have never actually gated anything. Added a step to run it, which is what makes the above regression tests meaningful.Notes
TestPrometheus.testPrintfTable, which asserts#on a table with holes ({nil,2,nil,"foo",nil}) — undefined behaviour that differs between 5.1 and 5.2. It reproduces identically on unmodifiedmainand passes under the Lua 5.2 the CI matrix uses. Unrelated to this change.if self.dict.flush_expiredguard: both consumers (apache/apisix and the API7 EE gateway) pass a realngx.shareddict, and the test mock now implements it.