AlinsRan opened a new pull request, #13772:
URL: https://github.com/apache/apisix/pull/13772
Fixes #12167
**Makes the `compacted` recovery reload cheap instead of trying to avoid
it.** A reload that changes nothing now reuses the existing items, leaves
`conf_version` alone and rebuilds no routers.
Companion to #13721, which removes the unsafe way of avoiding the reload.
Together they close #12167 and #13067; each stands on its own and they can be
reviewed independently.
## The problem
#12167 reports periodic CPU spikes correlated with this log line:
```
waitdir [/proxygw/services] err: compacted, will read the configuration
again via readdir
```
The reporter's ask is specific — *"APISIX CPU usage fluctuates when
'compacted' errors occur. I want to avoid this problem."* Not the log line: the
CPU.
Recovery from `compacted` is a full reload, and today it rebuilds everything
unconditionally:
- every item is re-validated through `check_schema`, the `checker` and the
`filter`
- `load_full_data` sets `changed` as soon as **any** item is valid, so
`conf_version` always moves and every router rebuilds its radixtree
- the item tables are new objects, so downstream caches keyed on them all
miss
And it happens for **every resource type** — routes, services, upstreams,
consumers, ssls, global_rules, plugin_configs … — in **every worker**, because
`need_reload` is per config instance and `produce_res(nil, "compacted")`
broadcasts to all of them.
None of that work is necessary when nothing actually changed — which is
precisely the case for the deployment that suffers from this. A prefix idle
enough to fall behind compaction is a prefix whose configuration did not change.
## The fix
Compare each key against the previous snapshot and reuse the item when
`modifiedIndex` matches:
```lua
local prev_item = get_prev_item(prev_values, prev_values_hash, key)
if prev_item and prev_item.modifiedIndex == item.modifiedIndex then
insert_tab(self.values, prev_item)
self.values_hash[key] = #self.values
self:upgrade_version(item.modifiedIndex)
goto continue -- note: `changed` is left alone
end
```
etcd increments `mod_revision` on every write, so an equal `modifiedIndex`
means equal content.
The `prev_values` / `get_prev_item` plumbing already exists — it was added
so an item whose new data fails validation can keep serving its last valid
value. This reuses it.
### Why skipping the filter is safe
The incremental watch path already works this way: `sync_data` re-runs the
checker and filter only for the keys that changed, and never touches the other
items. So this is not new semantics, it aligns the reload path with the watch
path.
Checked every filter individually:
| config | what its filter does | only touches its own item |
|---|---|---|
| `/routes` | `has_domain`, `set_plugins_meta_parent`, host lowercasing,
`filter_upstream` | ✅ |
| `/services` | same | ✅ |
| `/upstreams` | `has_domain`, `filter_upstream` | ✅ |
| `/consumers`, `/consumer_groups`, `/global_rules`, `/plugin_configs` |
`set_plugins_meta_parent` | ✅ |
| `/ssls` | sni lowercasing, trailing-dot strip | ✅ |
| `/plugins` | **`plugin.load(item)` — global effects** | ❌ |
The first eight are idempotent and only mutate fields of the item they are
handed, so an item that was filtered once is already in its filtered state.
`/plugins` is the exception, and it is `single_item` — one item, negligible
gain — so the `single_item` branch is left out of the optimisation entirely.
### Deletions need an explicit check
This is the trap. Keys that vanished while we were not watching leave *every
surviving key untouched*, so `changed` would stay `false`, `conf_version` would
not move, and the routers would go on serving the deleted items:
```lua
if prev_values_hash and matched_prev < nkeys(prev_values_hash) then
changed = true
end
```
## Tests
Both are verified to be discriminating — a test that passes either way
proves nothing.
**TEST 19** — a reload with nothing changed. Two independent probes: a tag
on the `values` array (a reload always allocates a fresh one, so losing it
proves the reload really ran) and a tag on the item inside it (which must
survive). Asserts `conf_version` moved once for the incremental write that
wakes the watcher, not twice.
Against unpatched `master`:
```
reload ran: true
-item reused: true
+item reused: false
-conf_version bumped once, not twice: true
+conf_version bumped once, not twice: false
```
**TEST 20** — a reload whose only change is a deletion. Passes on unpatched
`master` (which bumps unconditionally), so it was verified against the variant
that matters: the reuse optimisation *with the deletion check disabled*:
```
reload ran: true
ghost dropped: true
-conf_version bumped for the deletion: true
+conf_version bumped for the deletion: false
```
Full file run locally: the failure set is identical before and after this
change (TEST 3/4/5/9, which need a TLS etcd on :12379 that this machine does
not have), and TEST 16/17/18 — the existing full-reload tests — still pass.
## What this does not do
The `readdir` itself still happens on every `compacted`: without reading the
full snapshot there is no way to know what was missed. The transfer and JSON
parse remain. What goes away is the rebuild on top of it, which is the part
that scales with configuration size and shows up as the spike.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
