OCPBUGS-98387: Add retry logic to fix Azure self-managed e2e presubmit failures#8983
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-98387, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds retry-based HostedCluster existence validation for transient Kubernetes API errors. Azure public IP creation now retries HTTP 409 conflicts during creation and polling. Tests cover Kubernetes validation outcomes and Azure conflict classification. The end-to-end guest creation command increases the HostedCluster availability wait timeout from 30 to 45 minutes. Sequence Diagram(s)sequenceDiagram
participant ClusterCreate
participant ClusterValidation
participant KubernetesAPI
ClusterCreate->>ClusterValidation: validate HostedCluster existence
ClusterValidation->>KubernetesAPI: Get HostedCluster
KubernetesAPI-->>ClusterValidation: object, NotFound, or transient error
ClusterValidation->>KubernetesAPI: retry transient error with backoff
ClusterValidation-->>ClusterCreate: validation result
sequenceDiagram
participant LoadBalancerSetup
participant AzureSDK
participant AzureAPI
LoadBalancerSetup->>AzureSDK: BeginCreateOrUpdate
AzureSDK->>AzureAPI: create public IP
AzureAPI-->>AzureSDK: operation or HTTP 409 conflict
LoadBalancerSetup->>AzureSDK: PollUntilDone
AzureSDK->>AzureAPI: poll operation
AzureAPI-->>LoadBalancerSetup: result or retryable error
LoadBalancerSetup->>AzureSDK: retry conflict with backoff
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox 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 |
openshift-ci
Bot
added
area/cli
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/infra/azure/networking.go (1)
288-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate Azure error-classification logic.
isAzureRetryableErrorre-implements the sameerrors.As(*azcore.ResponseError)+ status-code switch pattern already present inClassifyAzureError(support/azureutil/errors.go), which also treats 409 as retryable. Consider extracting a shared "is this status code retryable" helper (or reusingClassifyAzureError's classification) instead of maintaining two independent copies of the same Azure status-code logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/infra/azure/networking.go` around lines 288 - 303, Eliminate the duplicated Azure retry-status classification in isAzureRetryableError by reusing the existing classification logic from ClassifyAzureError or extracting a shared helper for retryable HTTP status codes. Preserve the current handling of azcore.ResponseError extraction and ensure status 409 and the existing transient statuses remain retryable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/cluster/core/create_test.go`:
- Around line 1842-1855: The transientErrorClient currently only returns timeout
followed by NotFound, leaving validateClusterExistenceWithClient’s stale-error
path untested. Extend the fake with a configurable terminal error, or add a
separate fake, so it returns one or more transient timeouts followed by an
existing-cluster error; add a unit test asserting the final error from
validateClusterExistenceWithClient contains “already exists”.
In `@cmd/cluster/core/create.go`:
- Around line 813-835: Update validateClusterExistenceWithClient to base the
final error selection on retry.OnError’s returned err rather than stale lastErr:
preserve and return the “hostedcluster already exists” error from the final
attempt, while wrapping only when the retry operation itself ultimately fails
with a transient error. Alternatively, ensure the tracked error is updated on
every callback attempt so lastErr cannot override the final result.
In `@cmd/infra/azure/networking.go`:
- Around line 288-303: Limit isAzureRetryableError to return true only for
http.StatusConflict (409); remove the other status codes because the Azure
client’s default retry policy already handles them. Update the surrounding
BeginCreateOrUpdate/PollUntilDone retry usage if needed so the outer retry
wrapper exclusively handles 409 Conflict.
---
Nitpick comments:
In `@cmd/infra/azure/networking.go`:
- Around line 288-303: Eliminate the duplicated Azure retry-status
classification in isAzureRetryableError by reusing the existing classification
logic from ClassifyAzureError or extracting a shared helper for retryable HTTP
status codes. Preserve the current handling of azcore.ResponseError extraction
and ensure status 409 and the existing transient statuses remain retryable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3848c13f-9374-485b-84f7-c7515be14856
📒 Files selected for processing (5)
cmd/cluster/core/create.gocmd/cluster/core/create_test.gocmd/infra/azure/networking.gocmd/infra/azure/networking_test.gotest/e2e/v2/cmd/create-guests/main.go
|
/jira refresh |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8983 +/- ##
==========================================
+ Coverage 43.67% 43.69% +0.01%
==========================================
Files 771 771
Lines 95840 95895 +55
==========================================
+ Hits 41862 41897 +35
- Misses 51067 51087 +20
Partials 2911 2911
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
openshift-ci-robot
added
jira/valid-bug
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-98387, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-98387, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/infra/azure/networking.go (1)
315-350: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake the retry loop context-aware.
retry.OnErrordoesn’t take acontext.Context, so a canceled request can still sit in the backoff sleep before the SDK seesctx.Done(). Switch this to a context-aware backoff loop and keep returning the last conflict error when retries are exhausted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/infra/azure/networking.go` around lines 315 - 350, Replace retry.OnError in the public IP creation flow with a context-aware retry loop using the existing backoff configuration, checking ctx.Done() before and during each delay so cancellation exits promptly. Preserve the BeginCreateOrUpdate and PollUntilDone retry behavior for isAzureConflictError, and return the final conflict error when all retries are exhausted; update the logic around result assignment accordingly.Source: Path instructions
🧹 Nitpick comments (1)
cmd/infra/azure/networking_test.go (1)
237-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a stable “When ... it should ...” subtest description.
Wrap this assertion in a descriptive
t.Run, e.g.When an Azure response error is wrapped it should be discoverable, to follow the repository’s unit-test convention.As per coding guidelines, “Always use ‘When ... it should ...’ format for describing test cases when creating unit tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/infra/azure/networking_test.go` around lines 237 - 246, Wrap the assertions in TestErrorsAsAzureResponseError with a descriptive t.Run subtest named “When an Azure response error is wrapped it should be discoverable”, keeping the existing error setup and expectations inside the subtest.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/cluster/core/create_test.go`:
- Around line 1823-1831: Update the “forbidden error” test to verify the client
performs exactly one GET, not merely returns the expected error. Add call
tracking to nonTransientErrorClient, expose or inspect its GET count after
execution, and assert it equals one while retaining the existing error
assertion.
---
Outside diff comments:
In `@cmd/infra/azure/networking.go`:
- Around line 315-350: Replace retry.OnError in the public IP creation flow with
a context-aware retry loop using the existing backoff configuration, checking
ctx.Done() before and during each delay so cancellation exits promptly. Preserve
the BeginCreateOrUpdate and PollUntilDone retry behavior for
isAzureConflictError, and return the final conflict error when all retries are
exhausted; update the logic around result assignment accordingly.
---
Nitpick comments:
In `@cmd/infra/azure/networking_test.go`:
- Around line 237-246: Wrap the assertions in TestErrorsAsAzureResponseError
with a descriptive t.Run subtest named “When an Azure response error is wrapped
it should be discoverable”, keeping the existing error setup and expectations
inside the subtest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b10ae2b9-3cc4-4088-b993-d170c1a14097
📒 Files selected for processing (4)
cmd/cluster/core/create.gocmd/cluster/core/create_test.gocmd/infra/azure/networking.gocmd/infra/azure/networking_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cluster/core/create.go
…ter creation - Add validateClusterExistenceWithClient with exponential backoff retry (5 steps, 1s base, 2x factor, ~31s max) for transient API errors (timeout, server timeout, internal error, too many requests, service unavailable) in the shared CLI create path - Add isAzureConflictError and retry wrapper around public IP BeginCreateOrUpdate/PollUntilDone for Azure 409 Conflict errors (5 steps, 5s base, 2x factor, ~155s max); scoped to only 409 since the Azure SDK default retry policy already handles 408/429/500-504 - Add 7 unit tests for validateClusterExistence covering success, already-exists, transient retry+succeed, exhaustion, immediate non-transient fail, service unavailable retry, and timeout-then-exists - Add 9 unit tests for isAzureConflictError covering 409 variants, non-retryable codes, non-Azure errors, wrapped errors, and nil Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
sdminonne
left a comment
sdminonne
left a comment
There was a problem hiding this comment.
@bryan-cox many many thanks for this!
Only nits, and in general I think will improve our pass rate.
I don't think there will be any impact on other platforms.
May you have a look at my comments and follow-up or pushing back?
TY!
| type timeoutThenExistsClient struct { | ||
| crclient.Client | ||
| callsBeforeTerminal int | ||
| existingCluster *hyperv1.HostedCluster |
There was a problem hiding this comment.
Is existingCluster used anywhere?
There was a problem hiding this comment.
Good catch — the field isn't used since the production code only checks err == nil to determine the cluster exists. Removed it.
AI-assisted response via Claude Code
| if err := c.Get(ctx, crclient.ObjectKeyFromObject(cluster), cluster); err == nil { | ||
| return fmt.Errorf("hostedcluster %s already exists", crclient.ObjectKeyFromObject(cluster)) | ||
| } else if !apierrors.IsNotFound(err) { | ||
| return err |
There was a problem hiding this comment.
May we log something before returning err? Guess it may add some info for next failures/flakyness
There was a problem hiding this comment.
Done. Added opts.Log.Error(...) before the return to log the error with namespace/name context for easier debugging of future flakiness.
AI-assisted response via Claude Code
| // Phase 3: Watch for Available condition on all clusters. | ||
| log.Println("Phase 3: Waiting for all clusters to become Available") | ||
| availableErrors := waitForClustersAvailable(ctx, mgmtClient, cfg.namespace, named, 30*time.Minute) | ||
| availableErrors := waitForClustersAvailable(ctx, mgmtClient, cfg.namespace, named, 45*time.Minute) |
There was a problem hiding this comment.
Claude is reporting this timeout now is similar to timeout at main.go:122. Are they related, don't think so, May you clarifiy with a comment evenutally?
There was a problem hiding this comment.
They are related — both are timeouts for cluster readiness but for different conditions. cfg.waitTimeout (line 122, 45m) is used for version rollout at line 352, and this timeout was for the Available condition (previously 30m). Changed this to use cfg.waitTimeout directly instead of a magic number, which makes the relationship explicit and keeps them in sync.
AI-assisted response via Claude Code
…Timeout Increase the HostedCluster Available condition timeout from 30m to 45m to match the overall waitTimeout already used for version rollout. Azure VM-to-node registration regularly exceeds 30 minutes when 6 clusters are created in parallel, causing premature timeouts in the self-managed e2e presubmit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by e2e |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
1 similar comment
|
/retest |
Test Resultse2e-aws
e2e-aks
|
|
/retest |
|
/test okd-scos-images |
|
Now I have the complete picture. The PR modifies Azure networking retry logic and test files — it has nothing to do with the Dockerfile or build process. The build failure is an OOM kill during Go compilation in CI. Let me calculate the timing to confirm. The build Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe Go compiler process was OOM-killed by the Linux kernel during the
This is a CI infrastructure flake — the build pod on Recommendations
Evidence
|
|
/retest |
|
@bryan-cox: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@bryan-cox: Jira Issue Verification Checks: Jira Issue OCPBUGS-98387 Jira Issue OCPBUGS-98387 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Fix included in release 5.0.0-0.nightly-2026-07-11-232605 |
5 participants
What this PR does / why we need it:
The
pull-ci-openshift-hypershift-main-e2e-azure-v2-self-managedpresubmit is failing at 100% rate across all PRs, blocking all merges. Analysis of 8 failing runs across 6 different PRs identified three systemic root causes:1. API server overload (25% of failures)
6 concurrent
hypershift create clusterprocesses overwhelm the management cluster API server.validateClusterExistencemakes a singleclient.Getwith no retry — a transient timeout aborts the entire cluster creation.Evidence from CI runs:
Runs affected: 1927044143480832000, 1927037816306180096
Fix: Add
retry.OnErrorwith exponential backoff (5 steps, 1s base, 2x factor, ~31s max) for transient API errors (timeout, server timeout, internal error, too many requests, service unavailable) invalidateClusterExistenceWithClient.2. Azure 409 Conflict (25% of failures)
Concurrent public IP creation triggers
ConflictingConcurrentWriteNotAllowederrors from Azure ARM. The Azure SDK's default retry policy retries 408/429/500/502/503/504 but not 409.Evidence from CI runs:
Runs affected: 1926697268705517568, 1927024697420627968
Fix: Add
retry.OnErrorwith exponential backoff (5 steps, 5s base, 2x factor, ~155s max) for 409 Conflict errors only inCreatePublicIPAddressForLB.BeginCreateOrUpdateis idempotent, so retries are safe.3. Premature HostedCluster Available timeout (50% of failures)
The Available condition timeout (30m) is shorter than the overall test timeout (45m). Azure VM→node registration regularly takes just over 30 minutes when 6 clusters are created in parallel.
Evidence from CI runs:
Nodes appear 1-3 minutes after timeout expiry in logs.
Runs affected: 1926627710921572352, 1926971610268430336
Fix: Align Available condition timeout from 30m to 45m to match
waitTimeoutalready used for version rollout.Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/OCPBUGS-98387
Special notes for your reviewer:
isTransientAPIErrorhelper increate.gomirrors the pattern used incontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go:3410isAzureConflictErrorhelper innetworking.gois scoped to only 409 — the Azure SDK's default retry policy already handles 429/500/502/503/504main.gomakes the Available condition timeout match the existingwaitTimeout(45m) already used for version rollout at line 352validateClusterExistenceWithClientis incmd/cluster/core/(shared cross-platform path) — reviewed for safety across all platforms (AWS, Azure, KubeVirt, etc.); retry only activates on transient errors, no behavior change for non-transient pathsChecklist:
Summary by CodeRabbit