fix(test-integration): honor Retry-After HTTP headers on GET requests. - #6838
Merged
openshift-merge-bot[bot] merged 2 commits intoSep 9, 2026
Merged
Conversation
openshift-ci
Bot
requested review from
Gerd Oberlechner (geoberle) and
Miguel Soriano (miguelsorianod)
September 4, 2026 19:54
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new Retry-After retry loop has a correctness issue where a large Retry-After value can sleep past the intended overall timeout bound.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR reduces flakiness in test-integration by teaching the HTTP GET step helper to honor Retry-After headers, allowing eventual-consistency paths (like informer/lister caches) time to converge before asserting the final expected response.
Changes:
- Add a bounded retry loop to
httpGetsteps that re-GETs only when the server requests retry viaRetry-After. - Extend the HTTP test accessor GET response to return both decoded body and response headers (
GetResponse), and update affected tests/call sites accordingly.
File summaries
| File | Description |
|---|---|
| test-integration/utils/databasemutationhelpers/step_httpget.go | Implements GET retry behavior driven by Retry-After, plus helper functions for matching/asserting bodies. |
| test-integration/utils/databasemutationhelpers/per_resource_http.go | Wraps frontend SDK GET results into GetResponse (body + optional headers). |
| test-integration/utils/databasemutationhelpers/http_test_accessor.go | Introduces GetResponse and plumbs response headers through the raw HTTP accessor. |
| test-integration/frontend/version_compliance_test.go | Updates GET assertions to unwrap GetResponse.Body. |
| test-integration/frontend/cross_version_roundtrip_test.go | Updates GET assertions to unwrap GetResponse.Body. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…kglass test The breakglass admin integration test was flaky under CPU load. kubernetesApply writes to the fake object tracker synchronously, but the breakglass kubeconfig handler reads the session through an informer lister cache that the watch goroutine updates asynchronously. An immediate read-after-write in the next step could observe the stale (not-ready) session, so step 6 got "Session is not ready" (HTTP 202) instead of the kubeconfig. This is the eventual consistency a real client already handles via the endpoint's 202 + Retry-After contract. Make the httpGet step honor that contract: when the response does not yet match the expectation and carries a Retry-After header, re-issue the GET, waiting the server-specified delay (bounded by an overall safety timeout). Because it retries only when the response does not already match, steps that assert a not-ready/Retry-After body still pass on the first attempt - so this is safe for every httpGet with no new step type or per-step configuration. HTTPTestAccessor.Get now returns a *GetResponse (decoded body + response headers) as any; callers type-assert to read Body/Header, and the SDK-backed accessor leaves Header nil (no Retry-After, so no retry). Test-only change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ben Vesel (bennerv)
force-pushed
the
bvesel/admin-race-condition
branch
from
September 4, 2026 20:10
fb823f4 to
13e7ff1
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new GET retry behavior cannot currently honor Retry-After on non-2xx responses because headers are dropped/ignored on error paths, which undermines the stated intent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
This was referenced Sep 5, 2026
Closed
Collaborator
Author
|
/retest-required |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The PR is missing a required tracking ticket link, and the updated HTTP accessor methods should drain successful response bodies before closing to avoid connection churn in integration tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
test-integration/utils/databasemutationhelpers/http_test_accessor.go:125
- This method closes the response body without draining it; that prevents HTTP connection reuse and can add avoidable overhead in integration tests. Drain the body (discard) before closing.
resp, err := a.doRequest(ctx, http.MethodPost, resourceIDString, content)
if err != nil {
return err
}
return resp.Body.Close()
test-integration/utils/databasemutationhelpers/http_test_accessor.go:133
- This method closes the response body without draining it. To allow net/http connection reuse (and reduce connection churn in the test suite), drain the body to io.Discard before closing.
resp, err := a.doRequest(ctx, http.MethodPatch, resourceIDString, content)
if err != nil {
return err
}
return resp.Body.Close()
test-integration/utils/databasemutationhelpers/http_test_accessor.go:141
- This method closes the response body without draining it, which prevents HTTP keep-alive reuse and can lead to extra connection setup during integration tests. Drain the body (discard) before closing.
resp, err := a.doRequest(ctx, http.MethodDelete, resourceIDString, nil)
if err != nil {
return err
}
return resp.Body.Close()
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+113
to
+117
| resp, err := a.doRequest(ctx, http.MethodPut, resourceIDString, content) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return resp.Body.Close() |
Ben Vesel (bennerv)
force-pushed
the
bvesel/admin-race-condition
branch
from
September 8, 2026 16:48
044e5a4 to
ec57484
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new GET retry loop’s timeout budget does not currently bound a stalled in-flight GET (and timer handling can be tightened), which undermines the “safety bound” intent for test reliability.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+111
to
+120
| // Bounding the retries with a context rather than a manual deadline means an | ||
| // over-large Retry-After can't overshoot retryAfterTimeout: whichever of the | ||
| // two fires first wins the select below. The GET itself keeps the parent | ||
| // ctx, so a slow in-flight request isn't turned into a context error. | ||
| retryCtx, cancel := context.WithTimeout(ctx, retryAfterTimeout) | ||
| defer cancel() | ||
|
|
||
| for { | ||
| resp, err := accessor.Get(ctx, l.key.ResourceID) | ||
|
|
Comment on lines
+144
to
+146
| case <-retryCtx.Done(): | ||
| timer.Stop() | ||
| // Out of budget (or the parent ctx went away). Assert on the last |
…or.Get
The preceding commit needed response headers (Retry-After) on the GET path.
That was bolted on by wrapping the decoded body in a `GetResponse{Body,
Header}` struct behind an `any` return, which forced every caller into an
unchecked type assertion to recover a type the accessor already knew.
Return the raw `*http.Response` instead. doRequest still validates the status
code, consuming the body to build the error on non-2xx, but on success hands
the response back untouched. Callers read `resp.Header` directly and decode
via the new DecodeResponseBody, which reads, decodes (JSON or YAML) and
closes the body. The mutating verbs call it too and discard the value: they
do not want the content, but reading to completion drains the connection for
reuse and surfaces a malformed 2xx body as an error, both of which the old
doRequest gave every verb for free.
Also bounds the httpGet retry loop with a derived context rather than a
manual deadline. Previously each Retry-After delay had to be capped against
the remaining budget so a large server value could not sleep past
retryAfterTimeout; with a context, whichever expires first simply wins the
select. On expiry the loop falls through to the same assert as a definitive
response instead of reporting a context error, so the failure shows the
actual/expected diff that kept it retrying. The GET keeps the parent ctx so a
slow in-flight request is not converted into a context error, and time.After
becomes an explicitly stopped time.NewTimer, since a large Retry-After would
otherwise pin a timer until it fired.
There is no wait.Poll* helper that fits this loop: ConditionWithContextFunc
is func(context.Context) (done bool, err error), giving the condition no way
to dictate the next interval, and every Poll variant takes the interval as a
caller-supplied constant. Using one would mean discarding the server's
Retry-After value.
frontendHTTPTestAccessor is deleted rather than converted. It has been dead
since 5dbeb2e switched the harness to NewVersionedHTTPTestAccessor so tests
could be parameterized by API version; before this commit the only references
to it in the workspace were its own declarations. Converting it is possible —
the generated SDK is azcore-based, so policy.WithCaptureResponse could
surface the raw response — but that is plumbing written for zero callers, and
the alternative was keeping the interface at `any` to accommodate them. Note
that 468f0b0 on sudobrendan/feat-api-version-overlays removes the same file
independently.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ben Vesel (bennerv)
force-pushed
the
bvesel/admin-race-condition
branch
from
September 8, 2026 17:07
ec57484 to
fd2dd2c
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new retry loop can still hang on stalled GET requests (timeout doesn’t bound in-flight requests) and the loop can fail prematurely on nil/empty bodies due to ResourceInstanceEquals using require internally.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
test-integration/utils/databasemutationhelpers/step_httpget.go:119
- The retry loop is bounded by retryCtx, but the HTTP GET itself uses the parent ctx, which has no deadline (steps use t.Context()). A stalled/hung request can therefore exceed retryAfterTimeout and hang the step/test despite the intended safety bound. Consider issuing the request with retryCtx so the overall retry budget also bounds in-flight requests.
retryCtx, cancel := context.WithTimeout(ctx, retryAfterTimeout)
defer cancel()
for {
resp, err := accessor.Get(ctx, l.key.ResourceID)
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+162
to
+166
| if err != nil { | ||
| return false | ||
| } | ||
| _, equals := ResourceInstanceEquals(t, l.expectedResource, body) | ||
| return equals |
Collaborator
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bennerv, geoberle The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Collaborator
Author
|
/test e2e-parallel |
Contributor
Contributor
Collaborator
Author
|
/test e2e-parallel |
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes flaky integration admin tests.
Why
The breakglass admin integration test was flaky under CPU load. kubernetesApply writes to the fake object tracker synchronously, but the handler reads the session through an informer lister cache updated asynchronously by the watch goroutine. An immediate read-after-write in the next step could observe the stale (not-ready) session, so step 6 got "Session is not ready" instead of the kubeconfig. The handler only falls back to the live client on NotFound, and the fake tracker does not bump resourceVersion, so nothing bridged the gap.
Similar to our GAs, let's honor the retry-after header if the session isn't immediately ready to allow time for the lister cache to get updated by the informer.
Testing
Used the script to replicate, no longer happening after the change.
Testing is required for feature completion and tests should be part of the pull
request along with the feature changes.
Describe the testing provided. If you did not add tests, provide a clear
justification.
Special notes for your reviewer
PR Checklist
If E2E tests are included:
demonstrate that the test is able to detect a defect/error and fail with
proper error message and logs which communicates nature of the problem.