OCPBUGS-98718: retry CreateVpcEndpoint on AWS throttle errors#9012
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthrough
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jparrill
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@jparrill: This pull request references Jira Issue OCPBUGS-98718, 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. |
|
/area ci-tooling |
openshift-ci
Bot
added
area/ci-tooling
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/infra/aws/ec2.go`:
- Around line 138-143: Update CreateVpcEndpoint to generate a single stable
ClientToken before entering its retry loop, then pass that same token on every
attempt, including throttling retries. Preserve the existing invalidRouteTableID
and DefaultThrottleErrorCodes handling while ensuring retries cannot create
duplicate VPC endpoints.
🪄 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: 320e1f48-ba84-4263-b05f-b7f7a6086122
📒 Files selected for processing (1)
cmd/infra/aws/ec2.go
| code := apiErr.ErrorCode() | ||
| if strings.EqualFold(code, invalidRouteTableID) { | ||
| return true | ||
| } | ||
| _, isThrottle := awsretry.DefaultThrottleErrorCodes[code] | ||
| return isThrottle |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 'CreateVpcEndpointInput|ClientToken|retry\.OnError|DefaultThrottleErrorCodes' cmd/infra/awsRepository: openshift/hypershift
Length of output: 7566
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the relevant sections in cmd/infra/aws/ec2.go and the other VPC endpoint creator.
sed -n '110,190p' cmd/infra/aws/ec2.go
printf '\n----\n'
sed -n '490,540p' cmd/infra/aws/create.go
printf '\n----\n'
rg -n 'ClientToken|CreateVpcEndpointInput|VpcEndpoint' cmd/infra/aws/*.goRepository: openshift/hypershift
Length of output: 10235
Make the CreateVpcEndpoint retry idempotent. CreateVpcEndpoint is retried on throttling without a stable ClientToken, so a lost response can create a duplicate endpoint. Generate one token before the retry loop and reuse it on every attempt.
🤖 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/aws/ec2.go` around lines 138 - 143, Update CreateVpcEndpoint to
generate a single stable ClientToken before entering its retry loop, then pass
that same token on every attempt, including throttling retries. Preserve the
existing invalidRouteTableID and DefaultThrottleErrorCodes handling while
ensuring retries cannot create duplicate VPC endpoints.
Source: MCP tools
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9012 +/- ##
=======================================
Coverage 44.11% 44.12%
=======================================
Files 772 772
Lines 96226 96231 +5
=======================================
+ Hits 42448 42458 +10
+ Misses 50832 50827 -5
Partials 2946 2946
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jparrill: This pull request references Jira Issue OCPBUGS-98718, 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. |
The isRetriable predicate for the S3 VPC endpoint creation only matched invalidRouteTableID. When 14+ HCs are created in parallel, AWS throttles ec2:CreateVpcEndpoint with RequestLimitExceeded, which was not retried — the retry.OnError returned immediately. Add AWS SDK's DefaultThrottleErrorCodes to isRetriable so the existing backoff (5 steps, 3s base, 3x factor, 0.1 jitter) covers all standard AWS rate limiting error codes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
|
@jparrill: This pull request references Jira Issue OCPBUGS-98718, 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.
🧹 Nitpick comments (1)
cmd/infra/aws/ec2_test.go (1)
3-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep external imports in one group.
The blank line between the two external dependencies splits them into separate groups. As per coding guidelines, imports must be grouped and ordered as stdlib, external, then internal.
🤖 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/aws/ec2_test.go` around lines 3 - 10, Update the import block in ec2_test.go to place github.com/onsi/gomega and github.com/aws/smithy-go in the same external-import group, while preserving the standard-library group and required ordering.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.
Nitpick comments:
In `@cmd/infra/aws/ec2_test.go`:
- Around line 3-10: Update the import block in ec2_test.go to place
github.com/onsi/gomega and github.com/aws/smithy-go in the same external-import
group, while preserving the standard-library group and required ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e2a472dd-0cbf-4a01-9b5c-011179c7e22f
📒 Files selected for processing (2)
cmd/infra/aws/ec2.gocmd/infra/aws/ec2_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/infra/aws/ec2.go
|
/acknowledge-critical-fixes-only |
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
LGTM — clean fix, good test coverage. One minor nit.
| } | ||
| return false | ||
| } | ||
| isRetriable := isRetriableVPCEndpointError |
There was a problem hiding this comment.
nit: the intermediate variable isn't needed — you can pass the function reference directly:
| isRetriable := isRetriableVPCEndpointError | |
| if err = retry.OnError(retryBackoff, isRetriableVPCEndpointError, func() error { |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng, jparrill 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 Resultse2e-aws
e2e-aks
|
|
/retest-required Failures are infrastructure-related, unrelated to the isRetriableVPCEndpointError fix:
|
|
/verified by e2e |
openshift-ci-robot
added
the
verified
|
@jparrill: 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. |
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
@jparrill: 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. |
|
@jparrill: Jira Issue Verification Checks: Jira Issue OCPBUGS-98718 Jira Issue OCPBUGS-98718 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. |
4 participants
Summary
isRetriablepredicate inCreateVPCS3Endpointso rate-limitedec2:CreateVpcEndpointcalls are retried with the existing exponential backoffFixes
Root Cause
When 14+ HostedClusters are created in parallel (as in
e2e-aws-ovn), AWS throttlesec2:CreateVpcEndpointwithRequestLimitExceeded. TheisRetriablepredicate only matchedinvalidRouteTableID, so the throttle error was treated as non-retriable andretry.OnErrorreturned immediately.Evidence from build 2077041517976883200 (Jul 14, PR #8996 CI):
Same class of issue as OCPBUGS-98462 (Route53 throttle) but different AWS API.
Changes
isRetriablematches onlyinvalidRouteTableIDawsretry.DefaultThrottleErrorCodes(14 standard AWS throttle codes includingRequestLimitExceeded,Throttling,EC2ThrottledException, etc.)The existing
retryBackoff(5 steps, 3s base, 3x factor, 0.1 jitter) already provides exponential backoff with jitter — only the predicate was missing.Test plan
go buildpassesmake lint— 0 issuese2e-aws-4-22presubmit should no longer fail onRequestLimitExceededduring VPC endpoint creation🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests