OCPBUGS-98462: add jitter to AWS endpoint service requeue delay - #8996
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe AWS endpoint controller replaces the fixed 20-second retry interval for inactive network load balancers with a bounded randomized delay. New base and jitter constants define the calculation, retry logs report the computed duration, and tests validate both the helper and reconciliation result bounds. 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-98462, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
@jparrill: This pull request references Jira Issue OCPBUGS-98462, 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. |
openshift-ci
Bot
added
area/hypershift-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hypershift-operator/controllers/platform/aws/controller.go (1)
359-361: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a bounded retry-jitter test.
Please add or extend the controller tests to exercise this error path and assert that
RequeueAfterremains within the documented 16–25 second range, without asserting one exact random value.🤖 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 `@hypershift-operator/controllers/platform/aws/controller.go` around lines 359 - 361, Add a controller test covering the reconciliation error path that logs “reconciliation failed, retrying with jitter” and returns RequeueAfter. Execute the path and assert the result stays within the documented 16–25 second inclusive range, checking bounds rather than an exact random value; reuse existing test fixtures and symbols for the AWS controller reconciliation flow.
🤖 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 `@hypershift-operator/controllers/platform/aws/controller.go`:
- Around line 359-361: Add a controller test covering the reconciliation error
path that logs “reconciliation failed, retrying with jitter” and returns
RequeueAfter. Execute the path and assert the result stays within the documented
16–25 second inclusive range, checking bounds rather than an exact random value;
reuse existing test fixtures and symbols for the AWS controller reconciliation
flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c7817516-3d1d-4792-b165-a4ef1d77f5cf
📒 Files selected for processing (1)
hypershift-operator/controllers/platform/aws/controller.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8996 +/- ##
==========================================
+ Coverage 44.44% 44.50% +0.05%
==========================================
Files 774 774
Lines 96977 96980 +3
==========================================
+ Hits 43105 43164 +59
+ Misses 50897 50828 -69
- Partials 2975 2988 +13
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
Nice, thanks /lgtm |
|
Scheduling tests matching the |
|
All three agents have completed. Now I have the complete picture. Let me compile the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll three failures are infrastructure flakes or pre-existing gaps, not regressions introduced by this PR. The PR makes a minimal 3-line change adding jitter (15s base + 1–10s random) to the AWS endpoint service requeue delay in Root Causee2e-aws — e2e-aws-4-22 — codecov/patch: The 3 new executable lines (jitter calculation, log statement, return) sit inside the Recommendations
Evidence
|
|
/retest-required |
E2E failure analysisBoth failures are pre-existing issues unrelated to this PR's Route53 jitter change:
|
Test Resultse2e-aws
e2e-aks
|
celebdor
left a comment
celebdor
left a comment
There was a problem hiding this comment.
Some gomega usage suggestions
| g.Expect(d).To(BeNumerically(">=", min)) | ||
| g.Expect(d).To(BeNumerically("<=", max)) |
There was a problem hiding this comment.
| g.Expect(d).To(BeNumerically(">=", min)) | |
| g.Expect(d).To(BeNumerically("<=", max)) | |
| g.Expect(d).To(And( | |
| BeNumerically(">=", min), | |
| BeNumerically("<=", max), | |
| ), "iteration %d returned %s", i, d) |
I don't think we need to do two assertions here. A single gomega expression should do (I added also a suggestion on the iterations for debuggability, but feel free to leave that part out
There was a problem hiding this comment.
Done — combined into a single And() matcher with iteration context for debugging.
|
|
||
| g.Expect(err).ToNot(HaveOccurred()) | ||
| g.Expect(result.RequeueAfter).To(BeNumerically(">=", min)) | ||
| g.Expect(result.RequeueAfter).To(BeNumerically("<=", max)) |
There was a problem hiding this comment.
| g.Expect(result.RequeueAfter).To(BeNumerically("<=", max)) | |
| g.Expect(result.RequeueAfter).To(And( | |
| BeNumerically(">=", min), | |
| BeNumerically("<=", max), | |
| )) |
There was a problem hiding this comment.
Done — combined into And().
| NamespacedName: client.ObjectKeyFromObject(awsES), | ||
| }) | ||
|
|
||
| g.Expect(err).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
| g.Expect(err).ToNot(HaveOccurred()) | |
| g.Expect(err).NotTo(HaveOccurred()) |
We use NotTo instead of ToNot in the rest of the file. Let's keep it consistent.
There was a problem hiding this comment.
Done — changed to NotTo for consistency.
c6add26 to
a5dd5ee
Compare
|
Rebased with fixed Konflux pipelines |
|
/test verify-deps |
|
low risk, small change, easily reversable /lgtm |
|
Scheduling tests matching the |
|
/retest-required |
Replace the fixed 20s requeue delay in AWSEndpointServiceReconciler with a 15s base + random 1-10s jitter. When 14+ HostedClusters are created in parallel, all controllers retry Route53 ChangeResourceRecordSets at the same interval, causing API throttling cascades. The jitter desynchronizes retries across controllers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
|
/lgtm |
|
Scheduling tests matching the |
|
/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. |
|
/test e2e-kubevirt-aws-ovn-reduced |
|
/override ci/prow/e2e-kubevirt-aws-ovn-reduced |
|
@jparrill: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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 kubernetes-sigs/prow repository. |
|
@jparrill: Overrode contexts on behalf of jparrill: ci/prow/e2e-kubevirt-aws-ovn-reduced 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 kubernetes-sigs/prow repository. |
|
@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-98462 Jira Issue OCPBUGS-98462 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-22-233611 |
8 participants
Summary
Fixes
Root Cause
When 14+ HostedClusters are created in parallel (as in
e2e-aws-ovn), their AWS endpoint service controllers all call Route53ChangeResourceRecordSetssimultaneously. On failure (typically NLB not yet active), all controllers requeued after a fixed 20s — retrying at the exact same instant and sustaining Route53 API throttling (Throttling: Rate exceeded).Frequency
Searched 40 builds of
periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn(29 failures total):AWSEndpointAvailable=Falseand 9 test failuresExisting throttle handling
The CPO-side
awsprivatelinkcontroller already handles throttling with a 2min backoff (throttleRequeueDelay, line 61 ofcontrol-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go). This fix targets the HO-side endpoint service controller where the fixed 20s delay caused synchronized retries across all HCs.Changes
lbNotActiveRequeueDuration = 20s(fixed, all controllers retry simultaneously)lbNotActiveRequeueBase = 15s+rand(1-10s)jitter (16-25s range, desynchronized)log.Info("retrying in 20s")log.Info("retrying with jitter", "requeueAfter", requeueAfter)Test plan
go buildpassese2e-aws-ovnperiodic — Route53 throttling should no longer cascade across HCs🤖 Generated with Claude Code
Summary by CodeRabbit