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: 75c04d67af
ℹ️ 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10408 +/- ##
==========================================
+ Coverage 82.25% 82.38% +0.13%
==========================================
Files 275 275
Lines 43515 43524 +9
==========================================
+ Hits 35792 35856 +64
+ Misses 7722 7667 -55
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2f7af1bf7
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c56408a5c3
ℹ️ 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".
| return | ||
| } | ||
| accepted := meta.FindStatusCondition(stored.Conditions, string(gwapiv1.GatewayClassConditionStatusAccepted)) | ||
| if accepted != nil && accepted.ObservedGeneration != gc.Generation { |
There was a problem hiding this comment.
When a GatewayClass is deleted and recreated under the same name at the same generation (commonly generation 1 for both), this condition is false even though the UIDs differ, so a delayed translation of the deleted object CASes its status over the replacement. If the two generation-1 specs produce different status (for example, an old invalid EnvoyProxy versus a new valid one), this publishes the old Accepted value and the later correction can be skipped by the cache-backed updater, leaving the replacement stale without another generation event. Fresh evidence in this revision is that the final guard removed UID tracking while its recreation test covers only generations 5 and 1; retain or verify object identity for the equal-generation case.
Useful? React with 👍 / 👎.
c56408a to
5c36831
Compare
…atewayClass status The provider stores a GatewayClass status as soon as it reconciles the class, and the Gateway API runner stores the status again from each snapshot it translates. When the runner was still translating a snapshot with the previous generation after a spec change, it put that generation's status back. The status updater then wrote it, and could skip the following write of the newer status because its cache had not yet seen its own write, leaving the GatewayClass at the old observedGeneration. This made the GatewayClassObservedGenerationBump conformance test flaky. Skip storing a translated GatewayClass status when the stored status was already computed for a later generation. Signed-off-by: Alexej Disterhoft <alexej@disterhoft.de>
Signed-off-by: Alexej Disterhoft <alexej@disterhoft.de>
…s in the status guard The guard compared generations only. After a GatewayClass was deleted and created again under the same name, a late translation of the old object stored its higher generation and then kept every translation of the new object from storing its status. The check and the store were also two map operations, so a newer status the provider stored between them could still be replaced. Remember the status the runner stored for each GatewayClass and the UID it was computed for, and do not treat a stored status the runner computed for another object as newer. Store with LoadOrStore and CompareAndSwap, and check again when the provider stored in between. Signed-off-by: Alexej Disterhoft <alexej@disterhoft.de>
The runner translates the generations of a GatewayClass in order, so its own last store can hold a later generation than the one it is translating only when that store was computed for an object deleted and created again under the same name. Comparing the stored status with the runner's last store already identifies that case, so the UID check added nothing. Remember only the last status the runner stored, fold the generation check into storeGatewayClassStatus, and start the compare-and-swap loop with LoadOrStore. Signed-off-by: Alexej Disterhoft <alexej@disterhoft.de>
… runner Only the older-snapshot test needs the whole runner, since it is the one that covers the call site. The recreated-GatewayClass test now calls storeGatewayClassStatus directly, like the race test, which removes the helpers that started the runner, published snapshots with sentinels and checked the stored generation. Signed-off-by: Alexej Disterhoft <alexej@disterhoft.de>
…ation The guard refused only a stored status of a later generation. When a GatewayClass was deleted and created again under the same name, a late translation of the old object still replaced the status the provider had stored for the new one, and the status updater could leave that status on the new object. The guard also kept the last status it stored for every GatewayClass name and never dropped it. The provider stores the status of a GatewayClass before it publishes a snapshot with it, and generations only grow, so a stored status of any other generation is either newer than the translation or belongs to another object. Store the translated status only when the stored Accepted condition has the same observedGeneration, which needs no state in the runner. Run the race test from the provider's status of the translated generation, since the runner now leaves any other generation alone, and test that a status the translator changed at the same generation, such as Accepted=False for an invalid EnvoyProxy, still replaces the provider's. Signed-off-by: Alexej Disterhoft <alexej@disterhoft.de>
5c36831 to
f481847
Compare
What this PR does / why we need it:
TestGatewayAPIConformance/GatewayClassObservedGenerationBumpis flaky: 117 conformance jobs failed on it between 12 and 26 Oct. Each time, the GatewayClass keptAcceptedat the oldobservedGenerationfor the full 60s after its spec changed. Those runs logged no reconcile errors and no failed status writes.Two writers store a GatewayClass status: the provider when it reconciles the class, and the Gateway API runner from each snapshot it translates. The runner can lag a snapshot behind:
This PR keeps the runner from storing a translated GatewayClass status when the stored status was already computed for a later generation of the same GatewayClass:
if result.GatewayClass != nil { - key := utils.NamespacedName(result.GatewayClass) - r.ProviderResources.GatewayClassStatuses.Store(key, &result.GatewayClass.Status) + r.storeGatewayClassStatus(result.GatewayClass) }The provider writes the same map concurrently, hence the compare-and-swap. A status the provider stored is trusted, since the provider only stores the status of a GatewayClass that exists.
The skipped write in the status updater is fixed separately in #10409, since it affects every kind. Either PR on its own closes this flake. #10028 targets a different path, transient errors aborting
Reconcile, and the failing runs had none.Evidence:
TestTranslationOfOlderSnapshotKeepsNewerGatewayClassStatustranslates a snapshot that still holds generation 1 after the generation-2 status is stored, and the stored status goes back toobservedGeneration1.After: it keeps
observedGeneration2.TestTranslationOfRecreatedGatewayClassStoresItsOwnStatusstores a late translation of a deleted GatewayClass at generation 5, then translates its replacement at generation 1. With the guard as first proposed here, the replacement stayed atobservedGeneration5.After: it stores its own status.
TestStoreGatewayClassStatusRacingProviderStoreraces a provider store of generation 2N against the runner storing generation 2N−1, 20,000 rounds per run. With a separate check and store, it failed within the first ~100 rounds.After: 200,000 rounds pass, also under
-race.observedGeneration1. After, 0 of 8 did. Without the forced delays, the race showed up about twice in 400 iterations locally.Merge danger: a two-way door. The blast radius is GatewayClass status. The runner no longer replaces a stored status with one computed for an older generation of the same GatewayClass, and it keeps one small entry per GatewayClass to recognize a recreated one. The provider still stores the status of every reconcile, and the runner still sets
Accepted=Falsefor an invalid EnvoyProxy at the current generation.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 runner package 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/.