AlinsRan opened a new pull request, #13714:
URL: https://github.com/apache/apisix/pull/13714

   ### Description
   
   Fixes #13537
   
   `PUT /apisix/admin/plugins/reload` returns `done` as soon as `events:post()` 
accepts the event, but that post carries no delivery guarantee. A process that 
is connecting or reconnecting to the events broker at that moment loses the 
event permanently, and nothing ever brings it back in line — `plugin.load()` 
has exactly two call sites and neither reconciles.
   
   The privileged agent is where this hurts. It hosts the timers registered by 
plugins, so a lost reload leaves it running the timers of plugins that were 
just removed. In #13537 that shows up as log-rotate continuing to rotate long 
after it was disabled, with no way out except sending the reload again.
   
   This adds a version counter in the `internal-status` shared dict:
   
   - `post_reload_plugins()` increments it **before** broadcasting, so even a 
completely lost event still leaves a record that a reload was requested
   - every process that registers the reload handler compares the shared 
version against the one it applied, once a second, and calls `reload_plugins()` 
when they differ
   
   The broadcast stays as the sub-second fast path; the counter is the slow 
path that actually guarantees convergence (≤1s). This is the same shape 
`admin/standalone.lua` already uses, for the same underlying reason — its 
comment spells it out.
   
   On the details worth checking:
   
   - **`internal-status` is a safe choice.** It is declared unconditionally for 
the http subsystem in `ngx_tpl.lua` and is already a hard dependency of the 
server-info plugin. The privileged agent is an http process and sees it. The 
key is its own, so there is no collision with server-info.
   - **`ngx.timer.every` rather than `timers.lua`.** The background timer runs 
all registered entries through `thread_spawn`/`thread_wait`, so one slow entry 
would drag reconciliation with it; and `admin.init_worker()` runs before 
`timers.init_worker()`, so a self-contained timer is simpler.
   - **Idempotence.** `plugin.load()` unloads everything and re-inits, and 
log-rotate's `destroy()`/`init()` register and unregister its timer 
symmetrically, so a redundant round costs nothing. Steady-state cost is one 
shdict read per process per second.
   - **No spurious reloads.** Cold start: the dict is empty, both sides are 0. 
HUP reload: the dict survives, and a fresh worker adopts the current version at 
`init_worker`, so it does not reload just for being new.
   - **Concurrent reloads.** The version is sampled *before* `plugin.load()`, 
so a reload accepted while load is running leaves the versions unequal and gets 
applied on the next tick rather than being swallowed.
   - **`plugin.load()` throwing** (say a broken `config.yaml`) aborts the timer 
callback without recording the version, so it retries next tick and keeps 
logging. Deliberately not swallowed — a transient failure should not 
permanently skip a reload.
   
   One API nuance to note: a failed `events:post` still returns 503, which is 
left as is. Since the increment already happened, that 503 now means "broadcast 
failed but the reload will still take effect within a second" rather than 
"nothing happened". Retrying the PUT is idempotent either way.
   
   Two gaps are deliberately left open, both pre-existing: the stream subsystem 
shares neither the shared dict nor the events socket with http 
(`admin/standalone.lua` acknowledges the same limitation), and propagation 
between instances still goes through etcd `/plugins`.
   
   ### Tests
   
   New `t/admin/plugins-reload-reconcile.t`:
   
   - TEST 1 — a reload request bumps the shared version
   - TEST 2 — a process whose applied version is behind reloads within the 
reconciliation interval, reproducing the "broadcast never arrived" state 
directly rather than through the events layer
   - TEST 3 — an unchanged version never triggers a reload
   
   Verified discriminating: without the change TEST 1 reports `bumped=false` 
and TEST 2 sees no reload; TEST 3 passes either way. Ran the file five times to 
confirm it is not timing-flaky.


-- 
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]

Reply via email to