fix: retry transient network errors fetching control plane version graph (AROSLSRE-2030) - #6828
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new retry helper currently masks context cancellation and discards the underlying retryable error details, reducing diagnosability and potentially changing cancellation semantics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves the reliability of backend control plane version selection by adding retry logic when fetching the Cincinnati update graph, reducing E2E flakes caused by transient network issues.
Changes:
- Adds exponential-backoff retries around the HTTP GET to the Cincinnati graph endpoint.
- Treats transport errors and 5xx responses as retryable, while failing fast on non-200 non-5xx responses.
- Introduces a new
doWithRetryhelper to encapsulate this retry behavior.
File summaries
| File | Description |
|---|---|
| backend/pkg/controllers/controlplaneversion/cincinnati.go | Wraps Cincinnati graph fetch with exponential-backoff retry logic via a new helper function. |
Review details
Suppressed comments (2)
backend/pkg/controllers/controlplaneversion/cincinnati.go:156
- The error handling here can mask context cancellation/timeouts and drops the underlying wait error details by replacing it with a generic message. Other retry helpers in this repo return ctx.Err() when the parent context is canceled and wrap the backoff error for diagnosability.
if err != nil {
if wait.Interrupted(err) {
return nil, fmt.Errorf("%s did not respond successfully after retries", req.URL)
}
backend/pkg/controllers/controlplaneversion/cincinnati.go:135
- Transport-level errors are treated as retryable but the specific error is discarded (returning (false, nil)). If retries are exhausted, callers won’t see the last underlying network/TLS/DNS error, which makes diagnosing persistent failures harder. Track the last retryable error/status and return it when wait.Interrupted(err) is hit (similar to tooling/templatize/pkg/pipeline/arm_retry_policy.go and slot-manager lease proxy retry).
resp, err := client.Do(req.Clone(ctx))
if err != nil {
// Treat any transport-level error (DNS lookup failures, dial
// timeouts, connection resets, etc.) as transient and retry.
return false, nil
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
8bfec2c to
760d12b
Compare
|
Force-pushed a fix for the `ci/prow/lint` failure: `gci` flagged `backend/pkg/controllers/controlplaneversion/cincinnati.go` for import grouping, the `k8s.io/apimachinery/pkg/util/wait` import needs its own group before the `github.com/openshift` group per `.golangci.yml`'s custom-order sections. Reordered the import block accordingly. Verified locally with `golangci-lint run --build-tags='E2Etests' ./backend/...` (0 issues) and `go build ./backend/...` / `go test ./backend/pkg/controllers/controlplaneversion/...` (pass). The `Analyze (go)` CodeQL check failure on the previous push was `The runner has received a shutdown signal`, an unrelated GitHub Actions runner infra hiccup, not caused by this diff. |
There was a problem hiding this comment.
🟡 Changes recommended
The new retry path currently discards the underlying failure details (transport errors / 5xx statuses), reducing debuggability and making exhausted-retry errors non-actionable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
backend/pkg/controllers/controlplaneversion/cincinnati.go:143
- On 5xx responses
doWithRetryretries but doesn’t preserve the status for the final error. Capturing the last seen 5xx (and draining the body) improves debuggability and helps HTTP connection reuse under retry.
if resp.StatusCode >= http.StatusInternalServerError {
// Server-side errors are typically transient; retry.
return false, nil
}
backend/pkg/controllers/controlplaneversion/cincinnati.go:158
- When retries are exhausted, the returned error loses the root cause (last transport error / last 5xx status). Wrapping the final error with the last observed failure makes this actionable in logs and test output.
if err != nil {
if wait.Interrupted(err) {
return nil, fmt.Errorf("%s did not respond successfully after retries", req.URL)
}
return nil, err
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
760d12b to
0505a5d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The retry implementation currently loses useful error context / cancellation semantics in some paths and should be adjusted for debuggability and correct retry behavior before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
backend/pkg/controllers/controlplaneversion/cincinnati.go:162
- The retry timeout/cancellation error handling drops context: when ExponentialBackoffWithContext returns an interruption/timeout, this wraps it into a generic message and loses the underlying error (and can also hide ctx cancellation/deadline). Returning ctx.Err() when set and wrapping the interruption error keeps debugging information without changing retry behavior.
if err != nil {
if wait.Interrupted(err) {
return nil, fmt.Errorf("%s did not respond successfully after retries", req.URL)
}
return nil, err
backend/pkg/controllers/controlplaneversion/cincinnati.go:147
- When retrying on 5xx responses, the response body is closed but not drained; this prevents HTTP connection reuse and can amplify transient failures by forcing new TCP/TLS handshakes on each retry. Drain a bounded amount of the body before retrying.
if resp.StatusCode >= http.StatusInternalServerError {
// Server-side errors are typically transient; retry.
return false, nil
}
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
0505a5d to
895e8f8
Compare
|
Pushed a follow-up commit addressing Copilot's two review comments:
Verified locally: |
895e8f8 to
4cdbcf1
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new retry logic should drain non-200 response bodies for connection reuse and should avoid retrying clearly non-transient transport errors to match the stated intent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The retry/backoff behavior is well-contained, matches the stated failure mode, and is covered by targeted unit tests; remaining feedback is a minor performance optimization.
Review details
Suppressed comments (2)
backend/pkg/controllers/controlplaneversion/cincinnati.go:157
- On 5xx responses you close the response body without draining it. In net/http that typically prevents connection reuse, so repeated retries can open a new TCP/TLS connection per attempt. Consider draining the body before retrying to allow keep-alive reuse.
if resp.StatusCode >= http.StatusInternalServerError {
// Server-side errors are typically transient; retry.
lastErr = fmt.Errorf("%s returned unexpected HTTP status %s", req.URL, resp.Status)
return false, nil
backend/pkg/controllers/controlplaneversion/cincinnati.go:160
- For non-200 non-5xx responses you also return without draining the body, which typically prevents HTTP connection reuse for subsequent calls. Draining before returning the error can reduce connection churn.
if resp.StatusCode != http.StatusOK {
return false, fmt.Errorf("%s returned unexpected HTTP status %s", req.URL, resp.Status)
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
4cdbcf1 to
24a90aa
Compare
|
One more follow-up push addressing two more Copilot review comments:
Verified locally: |
There was a problem hiding this comment.
🟡 Changes recommended
DNS errors are currently treated as transient unconditionally, which can cause retries/backoff on permanent DNS failures (e.g., NXDOMAIN) and unintentionally delay fail-fast behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
backend/pkg/controllers/controlplaneversion/cincinnati.go:216
- For
net.Error, onlyTimeout()is treated as transient. Many transient network errors are reported asTemporary()without being timeouts; includingTemporary()would better match the goal of retrying transient transport failures while still failing fast for non-transient errors.
var netErr net.Error
if errors.As(err, &netErr) {
return netErr.Timeout()
}
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
49c8874 to
982e792
Compare
|
Fixed the NXDOMAIN edge case from Copilot's last comment: |
There was a problem hiding this comment.
🟢 Approval recommended
The retry logic is narrowly scoped to transient conditions, preserves cancellation semantics, and is covered by deterministic unit tests for the key retry and fail-fast paths.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/retest-required |
|
|
|
/retest |
|
/test e2e-parallel |
…aph (AROSLSRE-2030)
982e792 to
dc8fecc
Compare
…OSLSRE-2030) isTransientTransportError checked net.Error before the syscall.ECONNRESET/ ECONNREFUSED/ETIMEDOUT case. Since *net.OpError implements net.Error and its Timeout() returns false for a plain connection reset, that check short-circuited before ever reaching the syscall case, so ECONNRESET wasn't retried. Move the syscall and io.EOF checks ahead of the net.Error check so those errors are retried as intended.
There was a problem hiding this comment.
🟢 Approval recommended
The retry logic is narrowly scoped, preserves fail-fast semantics for non-transient failures, and is covered by deterministic unit tests for the key behaviors.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/test e2e-parallel |
1 similar comment
|
/test e2e-parallel |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hbhushan3, raelga 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 |
|
/test e2e-parallel |
|
/test e2e-parallel |
3 similar comments
|
/test e2e-parallel |
|
/test e2e-parallel |
|
/test e2e-parallel |
AROSLSRE-2030
What
Retries transient failures (DNS/dial timeouts, connection resets, 5xx) with exponential backoff when fetching the control plane version graph from the Cincinnati update service, instead of failing on the first hiccup.
Why
E2E build 2095800805629104128 failed the
Update HCPOpenShiftClustertest with:A single DNS lookup timeout on the test pod's cluster DNS failed the whole run.
SelectControlPlaneVersionmade one unretried HTTP GET, so any transient network blip was fatal.Testing
go build ./backend/...go test ./backend/...A genuine 4xx response from the update service still fails immediately with no retry; existing unit tests for
SelectControlPlaneVersionmock a single 200 response and pass unchanged.Special notes for your reviewer
Backoff is 1s doubling up to a 30s cap over 5 attempts, using
k8s.io/apimachinery/pkg/util/wait(already a dependency elsewhere in the repo).PR Checklist