etcd watcher: skip full reload + radixtree rebuild on compacted error when data is unchanged
- Dominant language
- Lua
- Stars
- 17.1k
- Forks
- 2.9k
- Avg merge
- 3d 16h
- Merged PRs (30d)
- 63
Description
### Issue description
When the etcd watcher in `apisix/core/config_etcd.lua` receives a `compacted` error and falls back to a full `readdir`, APISIX always bumps `conf_version` and rebuilds downstream caches (radixtree router, etc.) even when the data hasn't changed at all. In the multi-prefix-on-shared-etcd scenario, this causes a noticeable on-CPU latency spike (10s–100s of ms depending on route count) for every compaction cycle on inactive prefixes — the worker is busy reloading data that's byte-for-byte identical to what it already had.
### How to reproduce
Pretty easy in a multi-cluster setup, but the trigger is also reproducible in a single cluster:
1. Run two APISIX instances against one etcd, separated by `etcd.prefix` (`/apisix-A`, `/apisix-B`).
2. On A, push routes / consumers continuously so its prev_index keeps moving.
3. On B, never write anything after the initial config.
4. Configure etcd `--auto-compaction-mode=periodic --auto-compaction-retention=1m` (just to get a fast loop; production usually 1h).
5. After each compaction, B's worker logs `waitdir [...] err: compacted, will read the configuration again via readdir`, then fully reloads every resource type (routes, services, upstreams, consumers, plugins, plugin_configs, ssl, global_rules, …).
6. The next request hitting B trips a fresh radixtree rebuild via `apisix/http/router/radixtree_uri.lua:34-41` because `conf_version` has changed.
7. Latency spike on B even though nothing about B's config has changed.
### What's actually happening (code refs against current `master` `99f5599`)
`config_etcd.lua:537-645` (`load_full_data`):
- Line 586-587: `self.values = new_tab(...)` — discards the existing values table wholesale.
- Line 589-631: iterates every node, runs `check_schema` + `checker` + `filter`, sets `item.clean_handlers = {}`, inserts into the new table.
- Line 617-628: `changed = true` whenever **any** item passes validation — i.e. it tracks "did we have data?", not "did anything actually change?".
- Line 639-641: `conf_version` always bumps.
`radixtree_uri.lua:34-41` keys on `conf_version`, so any bump forces a full radixtree rebuild on the next request. Same pattern in the host_uri / uri_with_parameter routers and in plugin_config / service caches.
The kicker: when the reload was triggered by a `compacted` error (`config_etcd.lua:702-703` sets `need_reload = true`), the data we just pulled is by construction the same data the worker already held — there are no new modifications, only the watcher's `prev_index` fell behind etcd's `compact_revision`. Range responses also carry `header.revision` (the global cluster revision) which is already used to refresh `prev_index`, so the watcher self-recovers on its own. The values copy / clean_handlers reset / conf_version bump / radixtree rebuild are all wasted work.
### Why this is worth fixing
Two pain points compound:
1. **CPU on-CPU stall**: a full reload of all resource types on a worker can run 10s–100s of ms with thousands of routes; the worker doesn't reach `epoll_wait` during the iteration. We've seen this manifest as gRPC clients with tight deadlines (45ms) hitting `RST_STREAM (CANCEL)` and Nginx logging 499 / HTTP/2 400 because the request was never forwarded.
2. **GC pressure**: discarding the old `values` + clearing `clean_handlers` makes the previous radixtree + thousands of route closures unreachable in one go, feeding LuaJIT GC's atomic phase a big batch of work — another on-CPU pause shortly after the reload.
Both go away if we just notice the data is unchanged.
### Proposed fix
In the `need_reload` path inside `sync_data` (or at the head of `load_full_data`), do a cheap "is this actually new?" check before touching `self.values`:
- Compare `dir_res.nodes` against the current `self.values_hash` by `(key, modifiedIndex)` (or `(key, mod_revision)` on the v3 side). modifiedIndex is monotonic per key, so if every key is present and every modifiedIndex matches, by definition the data hasn't changed.
- If unchanged: just refresh `self.prev_index` from `headers["X-Etcd-Index"]` (which is already `body.header.revision`), reset `need_reload = false`, return. No values copy, no clean_handlers reset, no `conf_version` bump, no radixtree rebuild.
- If changed: existing path.
This is a fast O(N) hash lookup with no allocations beyond the comparison, and it's strictly a no-op for the unchanged case — no behavior change for legitimate config updates because those always carry a higher modifiedIndex.
Happy to send a PR if this direction makes sense. Wanted to surface the analysis first because the fast-path placement (sync_data vs load_full_data vs a wrapper) and the modifiedIndex equality check have some prior-art trade-offs in the etcd v3 path that are worth a quick review.
### Environment
- APISIX master `99f5599aa`, also reproduces on the 2.13.x line which is what the original investigation was on.
- Reproduction needs `--auto-compaction-mode=periodic` (or `revision`) on etcd, plus at least one inactive prefix.
Contributor guide
Research direction
Start with config_etcd.lua:537-645 and the need_reload path around lines 702-703, then inspect radixtree_uri.lua:34-41 and the related cache behavior. Reproduce compaction with separate etcd prefixes and compare the directory response with the existing values_hash. Done means an unchanged compacted reload refreshes the watcher state without replacing values, bumping conf_version, or rebuilding caches, while changed data keeps the existing reload behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- lua
- Domain
- backend, distributed-systems, performance
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100