fix: read live status before update comparison#9509
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Fixes a Kubernetes provider status-update edge case where comparing against a stale informer-cached object could incorrectly skip a required Gateway status update (e.g., Programmed recovery), by reading the live object from the API server before doing the status equality comparison.
Changes:
- Inject a
statusReaderinto the statusUpdateHandlerand use it toGetthe current object before comparing status updates. - Wire the provider to pass
mgr.GetAPIReader()to avoid stale cache reads during status comparison. - Add a regression test ensuring a stale cached Gateway status does not prevent a required Programmed recovery update.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/provider/kubernetes/status_updater.go | Adds an API-reader-based fetch path for status comparisons to avoid stale cache skipping updates. |
| internal/provider/kubernetes/status_updater_test.go | Adds a regression test that exercises the stale-cache comparison scenario and asserts a status write occurs. |
| internal/provider/kubernetes/kubernetes.go | Wires mgr.GetAPIReader() into the status update handler construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Get the resource directly from the API server when available. Using the | ||
| // informer-backed client here can compare against stale status and skip a | ||
| // required status update. | ||
| if err := u.statusReader.Get(context.Background(), update.NamespacedName, obj); err != nil { |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab81d3505c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Get the resource directly from the API server when available. Using the | ||
| // informer-backed client here can compare against stale status and skip a | ||
| // required status update. | ||
| if err := u.statusReader.Get(context.Background(), update.NamespacedName, obj); err != nil { |
There was a problem hiding this comment.
This change is a user-visible bug fix for stale Gateway status updates, but I checked release-notes/current and there is no fragment for it. release-notes/current/README.md requires bug fixes to add a new file under release-notes/current/bug_fixes/, so this fix would be omitted from the next release notes unless a fragment such as 9432-...md is added.
Useful? React with 👍 / 👎.
469d75c to
fbc7846
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9509 +/- ##
==========================================
- Coverage 75.43% 75.41% -0.03%
==========================================
Files 252 252
Lines 41641 41644 +3
==========================================
- Hits 31411 31404 -7
- Misses 8098 8106 +8
- Partials 2132 2134 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: liuhy <liuhongyu@apache.org>
fbc7846 to
18e7837
Compare
|
/retest |
| if statusReader == nil { | ||
| statusReader = client | ||
| } |
There was a problem hiding this comment.
Looks like this is only needed for the tests to pass? Can we pass a non-nil reader there instead?
| // Get the resource directly from the API server when available. Using the | ||
| // informer-backed client here can compare against stale status and skip a | ||
| // required status update. | ||
| if err := u.statusReader.Get(context.Background(), update.NamespacedName, obj); err != nil { |
There was a problem hiding this comment.
will this increase the likelihood of getting ratelimited by the API server ?
Summary
Test Plan
Fixes #9432