From 1138eb0d29719f66bb090a44a079f6d47b05ac10 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Mon, 3 Aug 2026 17:32:24 +0800 Subject: [PATCH] perf(etcd): reuse unchanged items on a full reload A full reload is how APISIX recovers from a compacted watch, and today it rebuilds everything unconditionally: every item is re-validated through check_schema, the checker and the filter, and 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 too, so downstream caches keyed on them all miss. None of that is necessary when nothing actually changed, which is the common case for the deployment that suffers from this: a prefix idle enough to fall behind compaction is a prefix whose configuration did not change. Compare each key against the previous snapshot and reuse the item when the modifiedIndex matches. etcd increments mod_revision on every write, so an equal modifiedIndex means equal content. A reload that changes nothing now keeps the existing objects, leaves conf_version alone and rebuilds no routers. This is the same semantics the incremental watch path already has: sync_data re-runs the checker and filter only for the keys that changed, and leaves the other items untouched. Every filter but /plugins' only mutates fields of the item it is given, so an item that was filtered once is already in its filtered state; /plugins is single_item and is left out of the optimisation because its filter calls plugin.load(), which has global effects. Deletions need an explicit check. 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. Fixes #12167 --- apisix/core/config_etcd.lua | 52 ++++++++--- t/core/config_etcd.t | 181 ++++++++++++++++++++++++++++++++++++ 2 files changed, 222 insertions(+), 11 deletions(-) diff --git a/apisix/core/config_etcd.lua b/apisix/core/config_etcd.lua index d7d6f1971a48..20489888fdbf 100644 --- a/apisix/core/config_etcd.lua +++ b/apisix/core/config_etcd.lua @@ -26,6 +26,7 @@ local json = require("apisix.core.json") local etcd_apisix = require("apisix.core.etcd") local core_str = require("apisix.core.string") local new_tab = require("table.new") +local nkeys = require("table.nkeys") local inspect = require("inspect") local process = require("ngx.process") local check_schema = require("apisix.core.schema").check @@ -560,6 +561,8 @@ end local function load_full_data(self, dir_res, headers, prev_values, prev_values_hash) local err local changed = false + -- how many of the previous keys are still present, used to detect deletions + local matched_prev = 0 if self.single_item then self.values = new_tab(1, 0) @@ -621,6 +624,25 @@ local function load_full_data(self, dir_res, headers, prev_values, prev_values_h for _, item in ipairs(values) do local key = short_key(self, item.key) + local prev_item = get_prev_item(prev_values, prev_values_hash, key) + if prev_item then + matched_prev = matched_prev + 1 + end + + -- Nothing changed for this key, so reuse the item we already have + -- instead of rebuilding it. This keeps the object identity stable, + -- which matters because downstream caches are keyed on it, and it + -- leaves `changed` alone so that a reload which changed nothing + -- does not bump conf_version and rebuild every router. + -- Same semantics as the incremental watch path in sync_data, which + -- only re-runs the checker and filter for the keys that changed. + 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 + end + local data_valid = true err = nil if type(item.value) ~= "table" then @@ -660,20 +682,28 @@ local function load_full_data(self, dir_res, headers, prev_values, prev_values_h self.filter(item) end - else - local prev_item = get_prev_item(prev_values, prev_values_hash, key) - if prev_item then - -- keep serving with the last valid configuration instead of - -- silently dropping the whole item on a full reload, see the - -- incremental path in sync_data for the same semantics - log.warn("failed to check item data of [", self.key, "/", key, - "], keep the previous configuration, err: ", err) - insert_tab(self.values, prev_item) - self.values_hash[key] = #self.values - end + elseif prev_item then + -- keep serving with the last valid configuration instead of + -- silently dropping the whole item on a full reload, see the + -- incremental path in sync_data for the same semantics + log.warn("failed to check item data of [", self.key, "/", key, + "], keep the previous configuration, err: ", err) + insert_tab(self.values, prev_item) + self.values_hash[key] = #self.values end self:upgrade_version(item.modifiedIndex) + + ::continue:: + end + + -- Keys present in the previous snapshot but absent now were deleted + -- while we were not watching. Every surviving key can be untouched and + -- still leave us with a changed configuration, so this has to be + -- checked separately or a reload that only deletes would keep serving + -- the removed items. + if prev_values_hash and matched_prev < nkeys(prev_values_hash) then + changed = true end end diff --git a/t/core/config_etcd.t b/t/core/config_etcd.t index a40f425b684c..e25ffbb1c43b 100644 --- a/t/core/config_etcd.t +++ b/t/core/config_etcd.t @@ -862,3 +862,184 @@ GET /t invalid new item loaded: false --- no_error_log keep the previous configuration + + + +=== TEST 19: a full reload that changes nothing reuses the items and does not bump conf_version +--- timeout: 25 +--- yaml_config +deployment: + role: traditional + role_traditional: + config_provider: etcd + etcd: + host: + - "http://127.0.0.1:2379" + prefix: /apisix +--- extra_yaml_config +nginx_config: + worker_processes: 1 +--- config + location /t { + content_by_lua_block { + local core = require("apisix.core") + local etcd = require("resty.etcd") + local etcd_cli, err = etcd.new({ + http_host = "http://127.0.0.1:2379", + }) + if not etcd_cli then + ngx.say("failed to create etcd client: ", err) + return + end + + etcd_cli:set("/apisix/global_rules/1", { + id = "1", + create_time = 1700000000, + update_time = 1700000000, + plugins = {["response-rewrite"] = {headers = {set = {["X-T"] = "a"}}}} + }) + ngx.sleep(2) + + local obj = core.config.fetch_created_obj("/global_rules") + local before_version = obj.conf_version + + -- Two independent probes. A reload always builds a fresh `values` + -- array, so losing this one proves the reload actually ran; the + -- incremental path only mutates elements and would keep it. + obj.values.array_probe = "old" + -- The items inside must survive: reusing them is the whole point. + for _, item in ipairs(obj.values) do + if item and item.value and item.value.id == "1" then + item.reload_probe = "kept" + end + end + + -- Arm the recovery path taken after a `compacted` error, then write + -- a second rule. sync_data is parked in waitdir, so the write is + -- what wakes it: the incremental path adds /2 and bumps + -- conf_version once, and the next sync_data round reaches the + -- need_reload branch. By then /1 and /2 are both in memory at the + -- revisions etcd reports, so the reload has nothing to change and + -- must not bump conf_version a second time. + obj.need_reload = true + etcd_cli:set("/apisix/global_rules/2", { + id = "2", + create_time = 1700000000, + update_time = 1700000000, + plugins = {["response-rewrite"] = {headers = {set = {["X-T2"] = "b"}}}} + }) + ngx.sleep(3) + + local probe_kept = false + for _, item in ipairs(obj.values) do + if item and item.value and item.value.id == "1" then + probe_kept = (item.reload_probe == "kept") + end + end + + ngx.say("reload ran: ", obj.values.array_probe == nil) + ngx.say("item reused: ", probe_kept) + ngx.say("conf_version bumped once, not twice: ", + obj.conf_version == before_version + 1) + + etcd_cli:delete("/apisix/global_rules/1") + etcd_cli:delete("/apisix/global_rules/2") + ngx.sleep(1) + } + } +--- request +GET /t +--- response_body +reload ran: true +item reused: true +conf_version bumped once, not twice: true + + + +=== TEST 20: a full reload that only deletes must still bump conf_version +--- timeout: 25 +--- yaml_config +deployment: + role: traditional + role_traditional: + config_provider: etcd + etcd: + host: + - "http://127.0.0.1:2379" + prefix: /apisix +--- extra_yaml_config +nginx_config: + worker_processes: 1 +--- config + location /t { + content_by_lua_block { + local core = require("apisix.core") + local etcd = require("resty.etcd") + local etcd_cli, err = etcd.new({ + http_host = "http://127.0.0.1:2379", + }) + if not etcd_cli then + ngx.say("failed to create etcd client: ", err) + return + end + + etcd_cli:set("/apisix/global_rules/1", { + id = "1", + create_time = 1700000000, + update_time = 1700000000, + plugins = {["response-rewrite"] = {headers = {set = {["X-T"] = "a"}}}} + }) + ngx.sleep(2) + + local obj = core.config.fetch_created_obj("/global_rules") + obj.values.array_probe = "old" + + -- An item that is live in memory but gone from etcd, so the reload + -- has to drop it. Every surviving key is untouched and therefore + -- reused, so without an explicit deletion check `changed` would + -- stay false, conf_version would not move, and the routers would + -- go on serving the dropped item. + local ghost = { + key = "/apisix/global_rules/ghost", + modifiedIndex = 1, + value = {id = "ghost", plugins = {}}, + } + core.table.insert(obj.values, ghost) + obj.values_hash["ghost"] = #obj.values + + local before_version = obj.conf_version + + -- same wake-up mechanism as TEST 19: /2 arrives incrementally + -- (+1), then the reload drops the ghost (+1) + obj.need_reload = true + etcd_cli:set("/apisix/global_rules/2", { + id = "2", + create_time = 1700000000, + update_time = 1700000000, + plugins = {["response-rewrite"] = {headers = {set = {["X-T2"] = "b"}}}} + }) + ngx.sleep(3) + + local found_ghost = false + for _, item in ipairs(obj.values) do + if item and item.value and item.value.id == "ghost" then + found_ghost = true + end + end + + ngx.say("reload ran: ", obj.values.array_probe == nil) + ngx.say("ghost dropped: ", not found_ghost) + ngx.say("conf_version bumped for the deletion: ", + obj.conf_version == before_version + 2) + + etcd_cli:delete("/apisix/global_rules/1") + etcd_cli:delete("/apisix/global_rules/2") + ngx.sleep(1) + } + } +--- request +GET /t +--- response_body +reload ran: true +ghost dropped: true +conf_version bumped for the deletion: true