moonming commented on issue #14149:
URL: https://github.com/apache/apisix/issues/14149#issuecomment-6676783611
I went through this on `master` (b5b50fa7) and can confirm the report. Two
things combine, and both are needed for a worker to get stuck:
1. **Write order.** `update_endpoint_dict` writes `<svc>#version` before
`<svc>`. Each `safe_set` is atomic, but lua-nginx-module's atomicity stops at
the method-call boundary, so a worker can read the new version and then the old
content. This order dates back to the first version of the module (#4880).
2. **The cache never re-reads while the version matches.** `core.lrucache`
without `invalid_stale` hands an expired entry back as long as `ver` still
matches, so the worker keeps the old content until the service changes again.
For a quiet service that is never. I replayed this against the real
`apisix/core/lrucache.lua` with a fake clock: after one torn read, TTL expiry
and the refresh timer both keep returning the old content; only the next real
change clears it. The same code path exists in the tars discovery.
### The fix (#14167)
Rather than reordering the two writes, the PR removes the version key, so
there is no write order left to get wrong and nothing in a worker trusts a
version:
- one key per service in the shared dict, written by a single `safe_set`;
the dict is the only source of truth
- a worker's lrucache entry lives for 1 second and is then re-read
(`invalid_stale = true`); unknown services are negative-cached for the same time
- the multi-cluster merge and the tars discovery get the same change
This is the model the consul discovery already uses since #13066 (`ttl = 1`,
`invalid_stale = true`, no version key). The alternative of keeping the version
probe and only swapping the write order also works (content first makes the
only harmful interleaving impossible), but it keeps two keys and a write-order
rule that has to be maintained forever. A lock was rejected: it would put a
shared-memory lock on every request. For comparison, Envoy, NGINX's `zone`
upstreams, HAProxy, Kong and mlcache all publish version and data as one unit
(an immutable snapshot, one lock scope, one transaction, or one shm key); none
writes a version marker and the payload as two stores a reader can interleave
with.
```mermaid
flowchart TD
subgraph agent["privileged agent: the only writer, events handled in
order"]
E["watch event ADDED or MODIFIED"] --> B["build node list JSON"] -->
W["safe_set ns/svc"]
W -- "no memory" --> F["log error, the old value may already be
gone"]
D["watch event DELETED"] --> X["delete ns/svc"]
R["watch closed, 410, or 8 watch rounds done"] --> L["full list:
rewrite every key, post_list removes keys not seen"]
U["apiserver unreachable"] --> T["retry after 40 s, dict untouched"]
end
W --> S[("ngx.shared.kubernetes, ns/svc = node list JSON, single source
of truth")]
X --> S
L --> S
subgraph worker["every worker, every request"]
Q{"lrucache entry present and not expired?"} -- yes --> H["return
cached nodes"]
Q -- no --> G["dict:get ns/svc"]
G -- missing --> N["negative cache 1 s, no upstream node, 503"]
G -- found --> C["decode, cache for 1 s, return nodes. An expired
entry is dropped, never handed back"]
G -- "bad data" --> Z["log error, 503"]
end
S --> G
```
### Normal cases
| Case | Behaviour |
|---|---|
| Steady state | lrucache hit, no shared dict access on the request path |
| Endpoints change | the agent overwrites `ns/svc`; every worker picks it up
within 1 s |
| Burst of changes (scale-down) | written in order; a worker may skip
intermediate states and see the last one |
| Worker restart or new worker | empty cache, first request reads the dict |
| Endpoints deleted | key deleted; within 1 s workers return 503 for that
service |
| Periodic relist | same content rewritten; `compare_upstream_node` sees
equal nodes, nothing else happens |
| Cache expiry | one dict read plus one `cjson` decode per active service
per worker per second, about 18 µs for 72 nodes |
### Abnormal cases
| Case | Behaviour |
|---|---|
| Reader between any two writes | not possible any more: there is one write
per change |
| Agent crashes mid-change | the dict keeps the last complete write; workers
keep serving it; the restarted agent relists |
| apiserver unreachable or watch broken | dict untouched, workers keep
serving the last known nodes, agent retries every 40 s |
| Shared dict full, `safe_set` fails | the old node is freed before the new
allocation when sizes differ, so the service becomes unknown: 503 plus an error
log until the next write succeeds (same as today) |
| Missed watch event | corrected by the next relist, as today |
| Endpoints with no ready address | empty node list, 503 (same as today; a
last-known-good policy would be a separate change) |
| Hot reload from an older build | leftover `#version` keys are ignored by
readers and removed as dirty keys by the first relist |
| Decode failure on bad data | error log, 503 for that service until the
next write |
| TTL boundaries differ per worker | a change rolls across workers within 1
s rather than at one instant; both states are valid node lists |
What is no longer possible by construction: caching the new version with the
old content, one worker disagreeing with the others for longer than 1 s, and an
inconsistency that only the next real change can clear.
The trade-off is that a change reaches a worker within one second instead of
on the next request. The CI job for the kubernetes discovery runs new
end-to-end cases against a real apiserver for the cases above (backend switch,
scale to zero and back, delete and recreate, unknown service), plus an
apiserver-free test that shows an entry is re-read once it expires.
--
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]
