AlinsRan commented on issue #13087:
URL: https://github.com/apache/apisix/issues/13087#issuecomment-5237066163
Feedback on #13720 is that it is too complex to be worth it, on three
grounds: the problem was found by reading code rather than reported in
production; triggering it needs a plugin with invalid code, and invalid plugin
code breaks `reload`/`restart` anyway; and plugin hot-reload is a stopgap that
does not justify a transaction mechanism.
I reproduced the failure on master to check those. Setup: three routes (one
behind `key-auth`, one with no plugins, one with `echo`), then a plugin whose
`init()` throws is added to the list and `PUT /apisix/admin/plugins/reload` is
called.
### Invalid plugin code is not the trigger
`load_plugin()` already wraps `require` in `pcall`, so a plugin with a
syntax error is logged and skipped — verified, the gateway is unaffected. That
protection works.
The trigger is a plugin that **loads fine but whose `init()` fails on this
build or config**. `apisix/plugins/gm.lua:init()` does that by design on a
runtime without Tongsuo (`error("need to build Tongsuo into the
APISIX-Runtime")`), and `ai.lua:init()` does it when
`nginx_config.http.upstream` is absent. No invalid code involved — a supported
plugin plus a mismatched build.
### What the failure actually looks like
master, same scenario three times:
| | before | after the reload |
|---|---|---|
| endpoint answer | — | `200 done` |
| `/auth` without key | 401 | 401 / **502** / **502** |
| `/plain`, no plugins | 502 | 502 |
| `/echoed`, echo plugin | ECHOED | ECHOED / `<html>` |
| Admin `PUT` route with plugins | 201 | **400 `unknown plugin [key-auth]`**
|
| Admin `PUT` route without plugins | 201 | 201 |
Routing is never lost and etcd is untouched. Two things break:
- `local_plugins_hash` is left empty, because the loop that rebuilds it
never runs. The Admin API then rejects every plugin, so the config plane can no
longer write any resource carrying `plugins` — including the change that would
undo this.
- `local_plugins` is left as a partial array, filled in `pairs()` order.
Which plugins survive therefore differs per process: in 2 of 3 runs `key-auth`
was absent and the protected route let unauthenticated requests through.
The endpoint answered `200 done` in every run.
### Compared with reload / restart
| | after the same plugin | visible to the operator |
|---|---|---|
| `apisix reload` (CLI) | every request 500, including no-plugin routes; CLI
still exits 0 | immediately |
| restart | same | immediately |
| `PUT /plugins/reload` | routing fine, most requests fine, auth randomly
absent | **no signal** |
So "reload/restart break too" is accurate but describes a different failure.
Those fail loudly and all-or-nothing, and an operator rolls back within
seconds. This one fails quietly and partially, which is the worse shape of the
two.
For completeness on atomicity: CLI reload is atomic up to `init_by_lua`
(`nginx -t -q` gates it) and cannot be beyond it — `init_worker_by_lua` runs
after the old workers have started leaving, so there is nothing left to roll
back to. The API path is different in kind: it mutates Lua tables inside one
worker, where the previous instances are still in memory and can be restored.
### The decision
I think the objection about complexity is fair — #13720 is ~380 lines of
production code, and a large part of that is not the original problem but the
consequences of making the switch transactional. The cost comes from one
constraint: old instances must be destroyed **before** the new ones are
initialized, because `server-info`, `log-rotate` and `error-log-logger`
register timers by name and initializing first would let the old `destroy()`
unregister the timer the new instance just registered. Destroy-first means a
failure has to resurrect the old instances, and everything else follows from
that.
Two levels are possible, and the difference is roughly an order of magnitude:
1. **Consistent but not atomic** (~50 lines): `pcall` around
`init`/`destroy`, build into a temp table and repopulate array and hash
together, return 500 instead of `200 done`. Kills the empty-hash and
random-subset states. A plugin whose `init()` fails is skipped, so an auth
plugin can still end up silently absent — but the operator gets a 500.
2. **Atomic** (what #13720 does): a failed reload leaves the previous set
completely intact. Verified: reload answers 500, `/auth` still 401, the Admin
API still accepts routes with plugins.
If hot-reload is positioned as a stopgap and the supported path for changing
plugin code is redeploying, option 1 may well be the right amount of
investment. What I would not keep either way is the current combination of a
partial random plugin set and `200 done` — it is neither atomic nor honest, and
option 1 alone removes it.
Happy to cut #13720 down to option 1 if that is the call. @membphis has
asked for the opposite direction on that PR (two P1s, both about strengthening
the guarantee), so it needs a decision from maintainers rather than from me.
Reproduction scripts and raw output available if useful.
--
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]
