diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 803fbfd..a7adacc 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -33,6 +33,7 @@ jobs: COVERALLS_REPO_TOKEN: ${{ secrets.GITHUB_TOKEN }} # Only report test coverage for lua 5.1 to avoid doing it twice. if: ${{ matrix.lua == '5.2' }} + - run: lua prometheus_test.lua integration: runs-on: ubuntu-latest timeout-minutes: 10 diff --git a/prometheus_keys.lua b/prometheus_keys.lua index 0c5f102..faeaa66 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -51,6 +51,20 @@ function KeyIndex:remove_expired_keys() self.expire_keys[i] = nil end end + + -- The loop above only drops worker-local references. The expired shared-dict + -- entries themselves (both the __ngx_prom__key_N index slots and the metric + -- value keys, which live in the same dict) are only *logically* dead: every + -- dict API treats them as missing, but their slab pages stay allocated. The + -- passive per-write expiry scan cannot reclaim them either, because it stops + -- at the first non-expired entry at the LRU tail, and a permanent entry (the + -- error metric, or any metric registered without an exptime) inevitably ends + -- up sitting there. Without this call the dict grows without bound under + -- label churn: index slots are never reused, so free_space steps down on + -- every new series and never recovers (apache/apisix#13658). Since expired + -- entries are indistinguishable from absent ones through every dict API, + -- reclaiming them here cannot change any observable behaviour. + self.dict:flush_expired() end -- Loads new keys that might have been added by other workers since last sync. diff --git a/prometheus_test.lua b/prometheus_test.lua index edb2017..b485b0a 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -65,13 +65,32 @@ function SimpleDict:expire(k, exptime) self.dict[k]["expired"] = os.time() + exptime return true -- match ngx.shared.DICT:expire, which returns true on success end +function SimpleDict:flush_expired(n) + -- Mirrors ngx.shared.DICT:flush_expired: scans the whole dict, physically + -- removes only the entries that are already expired and returns how many + -- were removed. The optional argument caps the number removed, not scanned. + if not self.dict then self.dict = {} end + local flushed = 0 + local now = os.time() + for k, v in pairs(self.dict) do + if v["expired"] and v["expired"] < now then + self.dict[k] = nil + flushed = flushed + 1 + if n and n > 0 and flushed >= n then break end + end + end + return flushed +end function SimpleDict:ttl(k) - self:get(k) - if not self.dict[k] then + -- Like ngx.shared.DICT:ttl, an expired entry reads as "not found" but is NOT + -- freed here: only flush_expired (or a write reusing the node) reclaims it. + -- Do not prune, or the physical-reclamation assertions below become vacuous. + local e = self.dict and self.dict[k] + if not e or (e["expired"] and e["expired"] < os.time()) then return nil, "not found" end - if self.dict[k]["expired"] then - return self.dict[k]["expired"] - os.time() + if e["expired"] then + return e["expired"] - os.time() else return 0 end @@ -868,6 +887,58 @@ function TestKeyIndex:testExpiredReAddNoDuplicate() luaunit.assertEquals(keys2[1], "expkey") end +-- remove_expired_keys() must physically reclaim the shared-dict space of +-- expired entries, not just drop the worker-local references to them. +-- Logically expired entries keep occupying slab pages, and the passive +-- per-write expiry scan cannot reach them once a permanent entry sits at the +-- LRU tail, so without an explicit flush the dict grows without bound under +-- label churn (apache/apisix#13658). +function TestKeyIndex:testRemoveExpiredKeysReclaimsSharedDict() + local err = self.key_index:add("expkey", "eviction_err", 1) + luaunit.assertEquals(err, nil) + -- A metric value key: lives in the same dict, and KeyIndex never reads it. + self.dict:set("expkey", 1, 1) + -- A permanent entry, like the error metric or any metric without an exptime. + self.dict:set("permanent", 1) + + sleep(2) + + -- Both entries are logically gone but still physically present: every dict + -- API reports them as missing while they still hold their slab pages. + luaunit.assertEquals(self.dict:ttl("_prefix_key_1"), nil) + luaunit.assertNotNil(self.dict.dict["_prefix_key_1"]) + luaunit.assertNotNil(self.dict.dict["expkey"]) + + self.key_index:remove_expired_keys() + + -- Both the index slot and the metric value must be reclaimed in the same + -- round. Before the fix remove_expired_keys() issued no dict writes at all, + -- so both entries survived and the dict only ever grew. + luaunit.assertNil(self.dict.dict["_prefix_key_1"]) + luaunit.assertNil(self.dict.dict["expkey"]) + -- Entries that have not expired must be left alone. + luaunit.assertNotNil(self.dict.dict["permanent"]) +end + +-- The index slots of a churning metric must not accumulate: each round expires +-- the previous slot, and the reclaim must keep the dict bounded rather than +-- letting every new series step free space down for good. +function TestKeyIndex:testExpiredSlotsDoNotAccumulate() + for _ = 1, 3 do + luaunit.assertEquals(self.key_index:add("churn", "eviction_err", 1), nil) + sleep(2) + self.key_index:remove_expired_keys() + end + + local live = 0 + for k in pairs(self.dict.dict) do + if k:find("_prefix_key_", 1, true) == 1 then live = live + 1 end + end + -- Slot numbers keep advancing (key_count is monotonic), but at most the most + -- recent slot may still be physically present; the earlier ones are gone. + luaunit.assertTrue(live <= 1) +end + -- list() must de-duplicate when self.keys transiently holds the same key in two -- slots (the window before a stale slot is reclaimed): only the slot the index -- points at is canonical. This isolates the list() guard from the add() path. @@ -1136,4 +1207,27 @@ function TestPrometheus:testKeyTimeout2() end +-- Same as TestKeyIndex:testRemoveExpiredKeysReclaimsSharedDict, but driven +-- through the public metric API: once a counter with an exptime goes stale, +-- neither its value key nor its index slot may keep occupying the shared dict. +function TestPrometheus:testExpiredMetricIsReclaimedFromDict() + self.counter_exp:inc(1) + self.p._counter:sync() + local i = self.p.key_index.index["metric_exp"] + luaunit.assertNotNil(i) + luaunit.assertEquals(self.dict:get("metric_exp"), 1) + + sleep(2) + -- Expired, so invisible through the dict API, yet still holding its space. + luaunit.assertNotNil(self.dict.dict["metric_exp"]) + luaunit.assertNotNil(self.dict.dict["__ngx_prom__key_" .. i]) + + self.p.key_index:remove_expired_keys() + + luaunit.assertNil(self.dict.dict["metric_exp"]) + luaunit.assertNil(self.dict.dict["__ngx_prom__key_" .. i]) + -- Metrics registered without an exptime are untouched. + luaunit.assertEquals(self.dict:get("nginx_metric_errors_total"), 0) +end + os.exit(luaunit.run())