Repository navigation
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: cf0f1c3cd2
ℹ️ 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".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10409 +/- ##
=======================================
Coverage 82.19% 82.19%
=======================================
Files 277 277
Lines 44014 44032 +18
=======================================
+ Hits 36178 36193 +15
- Misses 7835 7838 +3
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f136544ad
ℹ️ 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".
|
|
||
| // lastWriteTTL is how long a record of a status write is kept. The record is only needed until the | ||
| // cache has seen the write, which takes far less. | ||
| const lastWriteTTL = time.Minute |
There was a problem hiding this comment.
When the cache remains behind the last write past this TTL and the next sweep (one to two minutes), expireLastWrites removes the only signal that forces an API-server read. In the same S2 → stale-cache S1 → corrective S2 sequence covered by the new test, delaying the correction until after expiry makes cacheMayLag false, so the updater treats cached S2 as current and leaves server-side S1 in place; the deduplicating status store may never emit S2 again. Cleanup therefore needs to distinguish records whose resource version has been observed by the cache, or otherwise retain a bounded guard that survives prolonged cache lag.
Useful? React with 👍 / 👎.
7f13654 to
064d62d
Compare
064d62d to
03a6c97
Compare
…s write The status updater compares the desired status with the object from the manager's cache and skips the write when they are equal. Right after its own write, the cache can still hold the previous version of the object. When updates for an object arrive out of order, as when the Gateway API runner stores a status computed from an older snapshot, the updater wrote the older status and then skipped the newer one, because the cache still showed the newer status from its write before. The object kept the older status, as seen in the flaky GatewayClassObservedGenerationBump conformance test. Remember the resourceVersion of the updater's last write per object. When the cache is behind it and the status looks unchanged, read the object from the API server before deciding. The entry is dropped once the cache has caught up or the object was read from the server. Signed-off-by: Alexej Disterhoft <alexej@disterhoft.de>
Signed-off-by: Alexej Disterhoft <alexej@disterhoft.de>
…onfirmed When the cache looked behind the status updater's last write, the updater dropped its record of that write before checking the API server. If the following write then conflicted, the retry met the same stale cache without the record, took the cached status for the current one, and skipped the write. Keep the record until the server confirms the status is unchanged, the object is gone, or a write succeeds. Signed-off-by: Alexej Disterhoft <alexej@disterhoft.de>
The status updater keyed its last-write records by kind and name. Extension policy kinds are unstructured, and two of them can share a Kind in different API groups, so their objects of the same namespace and name shared a record. An update for one could consume the other's record, and that object's next correction was then compared with its stale cache and skipped. Include the API group in the key. Signed-off-by: Alexej Disterhoft <alexej@disterhoft.de>
A last-write record was only dropped by the next update of the same object. The last update of most objects is a write, and updates for deleted objects never arrive, so the records of every object that ever had its status written stayed for the life of the process. Record when each write happened and drop records older than a minute, at most once a minute. A record is only needed until the cache has seen the write, which takes far less. Signed-off-by: Alexej Disterhoft <alexej@disterhoft.de>
…ater Key the records by UID instead of group, kind and name, since no two objects share a UID. Records are now only added by writes and only dropped when they expire. The early drops, once the cache showed the write or the API server confirmed the status, only spared an API read in rare cases. Sweep expired records from a ticker on the Start goroutine instead of throttling a sweep inside apply, which needs neither a sweep timestamp nor an injected clock. Signed-off-by: Alexej Disterhoft <alexej@disterhoft.de>
03a6c97 to
14a957f
Compare
What this PR does / why we need it:
The status updater decides whether a write is needed by comparing the desired status with the object from the manager's cache, and skips the write when they are equal. Right after its own write, the cache can still hold the previous version of the object. When two updates for one object arrive out of order, the updater writes the older status and then skips the newer one:
This is how
GatewayClassObservedGenerationBumpflakes (117 failed conformance jobs between 12 and 26 Oct): the Gateway API runner stores a GatewayClass status from an older snapshot (fixed in #10408), and the corrective write is skipped. Any kind with more than one writer, or with out-of-order updates, can end up the same way.This PR records the resourceVersion of each of the updater's writes, keyed by UID. When the status looks unchanged but the cache shows another resourceVersion than the recorded write, the updater reads the object from the API server before skipping:
A ticker on the updater's goroutine drops records older than a minute, since a record only matters until the cache has seen the write. The extra read happens only when a write is about to be skipped while the cache may be behind.
Evidence:
TestStatusUpdateIsNotSkippedWhileCacheLagsBehindOwnWrite, the first unit test for the status updater, runs the sequence above against a fake API server with a cache the test holds behind, and the object ends at S1.After: it ends at S2.
TestStatusUpdateRetriedAfterConflictIsNotSkippedWhileCacheLagsmakes the write after the API-server read conflict once; the retry still meets the stale cache and writes S2.TestStatusUpdateRecordsExpirechecks that a sweep a minute after writing 100 GatewayClasses leaves no records.observedGenerationbefore, 0 of 8 after.-race) and the envtest integration tests pass.Merge danger: a two-way door. The blast radius is every status write the provider makes. In the case above the updater reads the object from the API server once before deciding, and it keeps a record of each write for one to two minutes. Otherwise nothing changes.
Which issue(s) this PR fixes:
Fixes #9691
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes. (N/A: no API changes.)make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.) (make lintand the provider unit and integration tests pass locally.gen-checkand the full coverage run are left to CI.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.make gen-checkand committed the result if API/helm charts/modules changed. (N/A: nothing generated changed.)release-notes/current/breaking_changes/.