AlinsRan opened a new pull request, #13720:
URL: https://github.com/apache/apisix/pull/13720
### Description
Fixes #13087.
`plugin.load()` tears the live plugin tables down before rebuilding them: it
destroys every old plugin, clears `local_plugins` / `local_plugins_hash` **in
place**, then re-requires and re-`init()`s the plugins one by one, with no
`pcall` and no rollback.
If one `init()` throws, the worker is left permanently broken:
- `load_plugin()` inserts into the array *before* calling `init()`, so even
the failing plugin stays in the served array;
- the `local_plugins_hash` rebuild loop never runs, so the hash stays
**empty** and the Admin API rejects *every* plugin with `unknown plugin [...]`;
- which plugins survive depends on the `pairs()` hash order, so it differs
per worker;
- the error is swallowed by the worker-events `pcall`, and the reload
endpoint has already answered `200 done`.
Reproduced on master: after a reload with one plugin whose `init()` throws,
`plugin.plugins` = `[bad-init]`, `plugin.plugins_hash` = `{}`, the previously
working route silently loses its `response-rewrite`, and re-`PUT`ing the exact
same route conf returns `400 unknown plugin [response-rewrite]`. The state is
permanent until the next successful reload.
The same structure is also what makes the second half of the issue possible:
the live tables are cleared and then repopulated across `require()` / `init()`
calls, which can yield, so an in-flight request can traverse a partial plugin
set.
### Solution
`load()` / `load_stream()` become a three-phase transaction:
1. **build** — the new plugin set is built in a local table. `require()` and
the completeness checks all happen here; the tables the request path reads are
untouched, so nothing is observable yet.
2. **switch hooks** — destroy the old instances, then run `init()` /
`workflow_handler()` of the new instances, all under `pcall`. If any hook
fails: destroy the new instances, restore the `package.loaded` snapshot,
re-`init()` the old instances, and return the error. The live tables were never
touched.
3. **commit** — repopulate the live tables in place.
Two deliberate choices that are not the obvious ones:
- **Old instances are destroyed before the new ones are initialized**, not
after. Several in-tree plugins register global resources *by name*:
`server-info`, `log-rotate` and `error-log-logger` call
`timers.register_timer("plugin#<name>", ...)` in `init()` and
`timers.unregister_timer(...)` in `destroy()`. If the new instance registered
first and the old one was destroyed afterwards, the old `destroy()` would
unregister the timer the new instance had just registered — a "successful"
reload that silently loses background tasks. Keeping today's order and
implementing the rollback as "re-`init()` the old instances" avoids that;
`init`/`destroy` are already written to be re-runnable across reloads.
- **The live tables are repopulated in place, not swapped.** `_M.plugins` is
bound to `local_plugins` at module load time, and `apisix/api_router.lua` and
`apisix/control/router.lua` read it via `ipairs(plugin_mod.plugins)`; swapping
the upvalue would leave those readers holding a dead table. In-place
repopulation keeps the table identity, and there is no yield point between the
`clear` and the end of the loop, so a concurrent request cannot observe a
half-built set. `ctx.plugins` holds plugin *object* references, so in-flight
requests finish against the old objects exactly as they do today.
`init()` moves out of `load_plugin()` into the caller, which is what makes
"insert before init" stop mattering.
`require` / priority / version / schema failures keep today's "log and skip"
semantics — that is also the cold-start behaviour, and such a failure does not
corrupt anything (the plugin is simply absent, array and hash stay consistent).
Only `init` / `workflow_handler` failures, which have side effects, abort the
reload. Making the reload stricter about unknown plugin names is a separate
discussion.
### Behaviour change (please read)
This changes the public behaviour of the reload endpoints:
- `PUT /apisix/admin/plugins/reload` and `PUT /v1/plugins/reload` now
perform the load **synchronously on the serving worker first**, and only
broadcast the event to the other workers if it succeeded.
- On failure they return **`500` with the error message instead of `200
done`**, and no event is broadcast, so the other workers are not asked to break
themselves too.
- The success path is unchanged: still `200 done`.
- The endpoints are now slower on the success path, since the request does a
full load (require + init hooks) instead of only posting an event. Reload is a
low-frequency operational call, so this seemed the right trade.
I think the current `200 done` is the worse behaviour: the operator is told
the reload succeeded while the plugin table is corrupted and the Admin API has
started rejecting every plugin. But this will be visible to anyone whose
automation asserts "reload always returns 200", so it deserves discussion
rather than a silent merge.
A maintainer previously said a proposal for this would be welcome — this PR
is that proposal in runnable form. Happy to change direction on any of the
above; opened as a draft for that reason.
The event handlers skip the originating worker (via the worker id the events
frame carries) so the serving worker does not load twice.
### Tests
New `t/admin/plugins-reload-transaction.t` plus a fixture plugin
`t/apisix/plugins/reload-bad-init.lua` whose `init()` always throws (resolved
through the existing `$apisix_home/t/?.lua` package path, so it does not ship
in the release).
- **TEST 1** reloads a plugin list that keeps `response-rewrite` and adds
the throwing plugin, then asserts: the endpoint returns `500` with `failed to
init plugin [reload-bad-init] ... boom`; `plugin.plugins` is still exactly
`[response-rewrite]`; `plugin.plugins_hash` still contains `response-rewrite`;
re-`PUT`ing the identical route conf still returns `200`; and `/hello` still
returns the rewritten body.
- **TEST 2** runs a failing reload followed by a good one and asserts the
good one returns `200 done` and the new plugin set is live — i.e. a failed
reload leaves no sticky state.
Every one of those assertions fails on current master: the endpoint returns
`200`, the array is `[reload-bad-init]` or a random remnant, the hash is empty,
the route `PUT` returns `400 unknown plugin [response-rewrite]` and `/hello`
returns `hello world`. So the test discriminates — reverting the fix turns TEST
1 red on all five assertions.
Existing coverage that should stay green: `t/admin/plugins-reload.t` (the
`load plugin times: 2` / `start to hot reload plugins` counts are preserved by
the skip-self design), `t/control/plugins-reload.t`, `t/plugin/example.t` (the
`plugin.load()` return contract), and the by-name-timer plugins
`t/plugin/log-rotate.t` / `t/plugin/prometheus4.t`.
I have not run the test suite locally; relying on CI here.
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (if not,
please discuss on the [APISIX mailing
list](https://github.com/apache/apisix/tree/master#community) first)
--
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]
