OCPBUGS-98462: add jitter to AWS endpoint service requeue delay#8996
OCPBUGS-98462: add jitter to AWS endpoint service requeue delay#8996jparrill wants to merge 1 commit into
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 43.80% 44.29% +0.48%
==========================================
Files 772 781 +9
Lines 96100 98556 +2456
==========================================
+ Hits 42097 43654 +1557
- Misses 51084 51837 +753
- Partials 2919 3065 +146
... and 64 files with indirect coverage changes
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
Failed TestsTotal failed tests: 7
... and 2 more failed tests |
|
New changes are detected. LGTM label has been removed. |
3717e3f to
3facf0e
Compare
|
@jparrill: The following tests failed, say
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. |
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
Clean fix — jitter is the right approach for desynchronizing parallel controllers hitting the same API. One minor nit on the test structure.
| min := lbNotActiveRequeueBase + 1*time.Second | ||
| max := lbNotActiveRequeueBase + time.Duration(lbNotActiveJitterMax)*time.Second | ||
|
|
||
| if tt.name == "When called directly it should return a delay within the jitter range" { |
There was a problem hiding this comment.
Nit: the table-driven pattern with string-matching dispatch (if tt.name == "...") is unusual — consider splitting into two separate test functions or using a run func(...) field in the table struct so the test logic lives in the test case, not in a branch.
There was a problem hiding this comment.
Done — replaced the string-matching dispatch with a validate func(t *testing.T, g Gomega) field in the table struct. Each test case now carries its own logic.
| }, | ||
| { | ||
| name: "When reconcileAWSEndpointServiceStatus fails it should requeue with jitter", | ||
| validate: func(t *testing.T, g Gomega) { |
There was a problem hiding this comment.
Now that you have these as separate validation functions, it's very clear that these are completely separate tests. One is testing the lbNotActiveRequeueDelay() function, the other is testing the Reconcile function. They should be separate test functions.
There was a problem hiding this comment.
Done — split into two separate test functions: TestLbNotActiveRequeueDelay (tests the function directly) and TestReconcileRequeuesWithJitter (tests the Reconcile path). No more table struct.
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.
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>
6 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