You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "AlinsRan (via GitHub)" <gi...@apache.org> on 2026/07/20 07:38:26 UTC
[PR] fix(etcd): advance the watch revision only on in-stream progress [apisix]
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(etcd): do not advance the watch revision on a timeout, and make the recovery reload cheap [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on PR #13721:
URL: https://github.com/apache/apisix/pull/13721#issuecomment-5186831063
I opened https://github.com/apache/apisix/issues/13777 to track the remaining etcd read pressure. It proposes using `WatchProgressRequest` on the same stream, so an idle watcher advances only after etcd confirms event delivery, without bringing back the event-loss window fixed here. A snapshot reload remains the correctness fallback for an actual `compacted` response.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(etcd): do not advance the watch revision on a timeout, and make the recovery reload cheap [apisix]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan merged PR #13721:
URL: https://github.com/apache/apisix/pull/13721
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(etcd): advance the watch revision only on in-stream progress [apisix]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on PR #13721:
URL: https://github.com/apache/apisix/pull/13721#issuecomment-5028160867
Two things a review surfaced that belong in the description of this PR.
**A client-driven alternative exists and should be recorded as evaluated-but-deferred.** `WatchRequest.progress_request` / `clientv3.RequestProgress` gives the same in-stream guarantee (etcd replies with a progress response only when the watcher is already synced) without needing any server-side flag — so it would keep the #12514 benefit *and* the #13067 correctness on a stock etcd config. It is not free: lua-resty-etcd's HTTP/JSON `request_chunk` writes a fixed body then only reads, so it would need a chunked request body; the gRPC path is already bidirectional and would be a smaller change. Worth naming in the description so "change the etcd flag" doesn't read as the only option.
**One doc nit on the trade-off section**: on etcd >= 3.6 the flag is renamed `--watch-progress-notify-interval` (the `--experimental-` prefix is deprecated), so the FAQ should mention both spellings, plus the 100ms lower bound.
Neither blocks the direction — trading a silent-staleness optimization for the correct-but-costly compaction reload is the right call. Just make the deferred option explicit.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org