Skip to content

perf(etcd): reuse unchanged items on a full reload #13772

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AlinsRan wants to merge 1 commit into
base: master
Choose a base branch
from
+222 −11
Open

perf(etcd): reuse unchanged items on a full reload #13772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 41 additions & 11 deletions apisix/core/config_etcd.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
181 changes: 181 additions & 0 deletions t/core/config_etcd.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading