fix: confirm NotFound against the API server before dropping status writes - #9566
Conversation
…rites Under high watch-event churn the status updater's cache-backed Get can return NotFound for a freshly-created object that the informer cache has not caught up to yet. apply() then silently skipped the write (and even counted it as a success), so the object was left with an empty status until a controller restart rebuilt the cache from a fresh LIST. Confirm the NotFound against an uncached APIReader before giving up. The extra uncached read only happens on a cache miss, so it adds no meaningful API-server load in steady state. Addresses envoyproxy#9536 Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9566 +/- ##
=======================================
Coverage 75.59% 75.60%
=======================================
Files 252 252
Lines 41747 41751 +4
=======================================
+ Hits 31559 31564 +5
+ Misses 8061 8060 -1
Partials 2127 2127 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Addresses envoyproxy#9536 Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77ea9433ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Confirm against the API server before dropping the status write; | ||
| // otherwise the update is silently lost under high cache churn, leaving | ||
| // the object with an empty status until a controller restart (issue #9536). | ||
| if err := u.apiReader.Get(context.Background(), update.NamespacedName, obj); err != nil { |
There was a problem hiding this comment.
When the cache-backed client returns NotFound but the API reader finds the object—the exact production scenario this change fixes—no unit or e2e test verifies that the mutator and status update still run. The existing status tests do not exercise UpdateHandler.apply, so this behavior could regress while the suite remains green; add a deterministic test using separate cache and API readers, along with the genuine-NotFound case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The logic is straightforward and isn't worth the complexity of the test setup.
|
LGTM, thanks! |
…rites (envoyproxy#9566) * fix: confirm NotFound against the API server before dropping status writes Under high watch-event churn the status updater's cache-backed Get can return NotFound for a freshly-created object that the informer cache has not caught up to yet. apply() then silently skipped the write (and even counted it as a success), so the object was left with an empty status until a controller restart rebuilt the cache from a fresh LIST. Confirm the NotFound against an uncached APIReader before giving up. The extra uncached read only happens on a cache miss, so it adds no meaningful API-server load in steady state. Addresses envoyproxy#9536 Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com> * docs: add release note for envoyproxy#9566 Addresses envoyproxy#9536 Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
3 participants
Description
Fixes #9536: under high watch-event churn (e.g. ~300+ HTTPRoutes on a single Gateway with ArgoCD auto-sync), some objects end up with an empty
status({"parents":[]}) forever, even though the data plane is programmed correctly. A controller restart flushes all of them at once.Root cause
The status updater's
apply()reads the object with the manager's cache-backed client before writing:Under churn the informer cache lags behind the API server, so this
Getcan returnNotFoundfor a freshly-created object the cache hasn't caught up to yet.apply()then silently returns, so the status is never written.Fix
When the cache-backed
GetreturnsNotFound, confirm against an uncachedAPIReader(mgr.GetAPIReader()) before giving up. If the object is genuinely gone, skip as before; otherwise proceed with the fresh object and write the status.The extra uncached read happens only on a cache miss — never for stable, already-cached objects — so it is bounded by create/delete churn rather than the status-event rate and adds no meaningful API-server load in steady state.