AlinsRan opened a new pull request, #13721:
URL: https://github.com/apache/apisix/pull/13721
Fixes #13067
### Blocked on
Depends on api7/lua-resty-etcd#224 being merged and released as `v1.10.7`.
Once it is, this PR bumps `lua-resty-etcd` in `apisix-master-0.rockspec` from
`1.10.6-0` to `1.10.7-0` and comes out of draft. The rockspec is deliberately
left untouched for now, so CI here runs against `1.10.6`.
Why the dependency: with `progress_notify` enabled, etcd can coalesce a
notification right behind an event response in a single read, and
`lua-resty-etcd` currently drops the event in that case. Merging this PR
against `1.10.6` would reintroduce the very class of silent event loss it fixes.
### Problem
#12514 samples the global etcd revision with an out-of-band `readdir` before
each watch, and on a watch timeout moves `watch_ctx.rev` up to that sample:
```lua
if err == "timeout" then
if latest_rev and watch_ctx.rev < latest_rev + 1 then
watch_ctx.rev = latest_rev + 1
end
end
```
A timeout only means "no bytes for `watch_timeout` seconds". It does not
distinguish an idle prefix from a watch stream that established and then died
silently. In the second case etcd has already written the pending events into
the dead stream, so skipping to the sampled revision drops them permanently.
The failure is silent and does not self-heal: `sync_data` only sets
`need_reload` on `compacted` / `restarted`, and after the jump the start
revision is fresh, so compaction never fires either. The worker serves a stale
configuration — deleted routes still routing, new routes and certificates never
applied — until that key is written again or the worker restarts. The only
trace is one info-level log line.
Reproduced on master by putting a TCP proxy between APISIX and etcd that
forwards short requests normally but, for the watch stream, forwards the HTTP
response headers and then silently discards the body without FIN/RST. Writes
made during that window are skipped and never recovered; the route stayed 404
across many healthy watch cycles afterwards while etcd still held it. The
asymmetric shape matters: if the whole link goes dark the sampling `readdir`
fails too, `latest_rev` is `nil` and the guard prevents the jump. Real-world
equivalents are NAT/conntrack reaping long connections while short ones pass,
asymmetric packet loss, and an etcd-side watch stall with `range` still healthy.
### Fix
Create the watch with `progress_notify` and advance the revision only on
responses read from the stream itself.
etcd sends progress notifications only to watchers that are fully synced
(`watchable_store.go`: `progressIfSync` bails out if any watcher is unsynced),
so the revision a notification carries is a server-side guarantee that every
event up to it was already delivered on that same stream. A stream that dies
silently stops producing them, so the revision simply stops advancing — the
safe direction.
The out-of-band `readdir` is removed rather than kept for diagnostics: its
result no longer has any safe use, it costs a range RPC on every watch cycle,
and keeping it around invites reintroducing the jump.
The notification branch must run **before** the "smaller revision" check. On
an idle prefix `watch_ctx.rev` is the last event revision plus one, while a
notification carries the current store revision — one lower. Handled after the
check it would be read as "etcd may be restarted" and force a full resync on
every notification.
### Trade-off, stated up front
etcd sends notifications every 10 minutes by default, while an idle APISIX
watch stream only lives for `watch_timeout` (50s by default). **So under a
default etcd configuration an idle stream will usually not see one, the
revision stops moving while the prefix is idle, and behaviour falls back to
what it was before #12514**: unrelated writes advance the global revision, a
compaction then cancels the watch with `compacted`, and APISIX does one full
`readdir` reload to recover. That is the CPU spike #12167 complained about, at
compaction frequency.
This is deliberate. A bounded, self-healing, observable reload is a better
failure mode than an unbounded, silent, permanent configuration drift. A
gateway that reads etcd once more is a slower gateway; a gateway that loses
configuration is a wrong one.
Operators who want #12514's benefit back can set
`--experimental-watch-progress-notify-interval` below `watch_timeout` (e.g.
`30s`, minimum `100ms`) on the etcd side. Then an idle stream is notified
before it times out, never times out, keeps a fresh revision, and compaction
never cancels it — same outcome as #12514 but with the server-side delivery
guarantee behind it. This is documented in the FAQ and configured for the CI
etcd.
Older etcd versions without the flag degrade to the fixed 10-minute
interval, i.e. the plain "remove the jump" behaviour. `progress_notify` itself
has been in the etcd v3 watch API since its first release, so there is no
compatibility floor.
### Alternatives considered
- **Keep the sampling and verify the `(rev, latest]` range is free of events
for this prefix before jumping.** `readdir` has no `min_mod_revision` filter,
so the library would need changing; verify-then-jump is not atomic, so events
landing after the check are still skipped; and the cost approaches a full
`readdir` anyway. Not worth it over simply not jumping.
- **Client-driven `WatchProgressRequest`** (what the k8s apiserver does —
notification frequency controlled by the client, no dependency on an etcd
flag). Requires writing further request frames onto an established HTTP/1.1
chunked stream; neither `lua-resty-http`'s request/body_reader model nor
`lua-resty-etcd`'s `request_chunk` supports that. Worth doing as a follow-up,
out of scope here.
- **Plain revert of #12514.** Same correctness, but gives up the ability to
avoid compaction storms entirely on a well-configured etcd. `progress_notify`
costs ~10 lines and nothing at all when the server is not asked for it.
### Tests
- `t/core/config_etcd.t` TEST 14 asserted exactly the removed behaviour
(`etcd watch timeout, upgrade revision to` appearing at least twice). Same
topology, inverted assertion: the log must not appear at all. Old test and new
test are mirrors, which pins the change in both directions.
- TEST 16 is new: default `watch_timeout` so the stream stays up, idle for
10s, then asserts the notification log appears and that a route created
afterwards still takes effect — i.e. the notification advanced the revision
without breaking delivery.
- `ci/pod/docker-compose.common.yml` sets
`ETCD_EXPERIMENTAL_WATCH_PROGRESS_NOTIFY_INTERVAL: 3s` so notifications land
inside a stream's lifetime. Its only other effect is that idle watch streams
stop being rebuilt every 50s.
- The blackhole scenario cannot be pinned down in test-nginx — it needs a
selectively silent TCP proxy — so it stays a manual verification procedure,
described above.
Not run locally; relying on CI.
### Backport
Affects 3.14.0 and later. Should be backported to the 3.14 / 3.15 release
branches.
--
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]
