ROSAENG-62134: Add rate-limit backoff guard for fleet mode HTTP 429 - #264
ROSAENG-62134: Add rate-limit backoff guard for fleet mode HTTP 429#264redhat-chai-bot wants to merge 3 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@redhat-chai-bot: This pull request references ROSAENG-62134 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. 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 NOT APPROVED This pull-request has been approved by: redhat-chai-bot The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughChangesRate-limit backoff
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WebhookReceiver
participant SendServiceLog
participant OCMAPI
participant BackoffStore
WebhookReceiver->>BackoffStore: Check notification and cluster backoff
BackoffStore-->>WebhookReceiver: Active or expired backoff
WebhookReceiver->>SendServiceLog: Send firing notification
SendServiceLog->>OCMAPI: Submit service log
OCMAPI-->>SendServiceLog: HTTP 429 or success
SendServiceLog-->>WebhookReceiver: RateLimitError or success
WebhookReceiver->>BackoffStore: Record or clear backoff
Possibly related issues
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… 429 When the OCM API responds with HTTP 429 (Too Many Requests) during service log sends, ocm-agent was retrying indefinitely on every AlertManager webhook delivery, creating a retry storm. This change: - Adds a RateLimitError type in pkg/ocm that wraps 429 responses from SendServiceLog, allowing callers to distinguish rate limits from other errors. - Adds an in-memory per-notification:cluster backoff map in the RHOBS webhook handler. On a 429 response the timestamp is recorded and subsequent firing alerts for that pair are silently skipped for 30 minutes, breaking the retry loop. - Clears the backoff entry on a successful send so normal operation resumes immediately once the rate limit lifts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
d7f6acc to
4d92ada
Compare
![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)
pkg/handlers/webhookrhobsreceiver.go (1)
46-51: 🩺 Stability & Availability | 🔵 TrivialConfirm the deployment scope for
rateLimitBackoffs.
rateLimitBackoffsis process-local. If the receiver runs with multiple replicas, a 429 undernotification:clusterIDin one replica will not suppress retries in another. Confirm single-replica scope is intended, or make the backoff shared.🤖 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 `@pkg/handlers/webhookrhobsreceiver.go` around lines 46 - 51, Confirm that the process-local rateLimitBackoffs in the webhook receiver is only intended for single-replica deployments; if multiple replicas are supported, replace the sync.Map-based tracking with shared backoff storage so a 429 for the same notification:clusterID suppresses retries across replicas.
🤖 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 `@pkg/handlers/webhookrhobsreceiver.go`:
- Around line 396-399: Remove the "hosted_cluster_id" field from the Warn
logging calls around the rate-limit backoff handling, including both occurrences
associated with the alert notification. Preserve the notification name and
existing warning message while ensuring no customer resource identifier is
logged.
- Around line 391-403: Protect the entire rate-limit backoff state transition
with one shared mutex: the expired-entry check/deletion in processAlert, the 429
timestamp recording, and the successful-request cleanup. Update the logic around
rateLimitBackoffs and the related branches near the existing cleanup so a
request cannot delete or clear a newer timestamp stored by another request;
alternatively, condition cleanup on removing the exact loaded value.
In `@pkg/ocm/ocm.go`:
- Around line 258-260: Update the HTTP 429 branch in the OCM send flow to
preserve the original send error for errors.As traversal. Return a
RateLimitError whose Err is the original err without replacing it with a
non-wrapping fmt.Errorf, while retaining the existing rate-limit detection.
---
Nitpick comments:
In `@pkg/handlers/webhookrhobsreceiver.go`:
- Around line 46-51: Confirm that the process-local rateLimitBackoffs in the
webhook receiver is only intended for single-replica deployments; if multiple
replicas are supported, replace the sync.Map-based tracking with shared backoff
storage so a 429 for the same notification:clusterID suppresses retries across
replicas.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 05fd039e-21d8-439e-a7d7-8bc39e52cfd3
📒 Files selected for processing (2)
pkg/handlers/webhookrhobsreceiver.gopkg/ocm/ocm.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #264 +/- ##
==========================================
+ Coverage 55.67% 56.21% +0.54%
==========================================
Files 23 23
Lines 1895 1923 +28
==========================================
+ Hits 1055 1081 +26
- Misses 785 786 +1
- Partials 55 56 +1
🚀 New features to boost your workflow:
|
Adds unit tests for both the RateLimitError type and the rate-limit backoff behavior in the fleet webhook handler: - RateLimitError: Error(), Unwrap(), errors.As() traversal - SendServiceLog: returns *RateLimitError on 429, plain error on 500 - processAlert: backoff map populated on 429, subsequent alerts skipped within window, normal operation after expiry, entry cleared on success Also addresses CodeRabbit review feedback: - Remove hosted_cluster_id from warning logs (customer data concern) - Use %w instead of %v to preserve original error in RateLimitError - Remove eager Delete of expired entries (race-safe: stale expired entries are harmless since the pre-send check only blocks within the 30-minute window) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addresses openshift/ocm-agent-operator#319 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/retest ci/prow/images AI-generated. Review for accuracy. |
|
/retest pull-ci-openshift-ocm-agent-master-images AI-generated. Review for accuracy. |
|
/test images AI-generated. Review for accuracy. |
|
@redhat-chai-bot: 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. |
4 participants
Summary
When the OCM service log API returns HTTP 429 (rate limit exceeded),
ocm-agentfleet mode enters an infinite retry loop. TherestoreNotificationStatus()function rolls backlastTransitionTimeafter every failed send, causingcanSendNotification()to returntrueon every reconciliation cycle (~15 min). This generates ~24 wasted API requests per hour, per affected cluster, indefinitely.Fixes ROSAENG-62134
Changes
1.
pkg/ocm/ocm.go— TypedRateLimitErroron 429RateLimitErrorstruct withError()andUnwrap()methodsSendServiceLogto return&RateLimitError{...}when the OCM API responds with HTTP 429, in both the SDK error path and the unexpected-status path2.
pkg/handlers/webhookrhobsreceiver.go— Rate-limit-aware backoff guardsync.Map(rateLimitBackoffs) keyed bynotificationName:clusterIDto track when 429 was last receivedrateLimitRetryIntervalconstant (30 minutes)processAlert(), before proceeding to the send path, checks the backoff map. If within the 30-minute window, logs and returnsnil(skipping the send)errors.As()to detectRateLimitError, storestime.Now()in the backoff map, and logs a warning. ExistingrestoreNotificationStatus()and metrics calls are preservedResulting Behavior
Design Notes
restoreNotificationStatus()is still called on 429 — this keepsFiringNotificationSentCountaccuratesync.Mapis ephemeral — on pod restart one extra failed attempt occurs, then the backoff is re-established (self-healing)canSendNotification()— the backoff is an early guard inprocessAlert, keeping concerns separatedSendServiceLogstill returnserror, so mock generation is unaffectedAI-generated. Review for accuracy.
@ravitri requested in Slack thread
Summary by CodeRabbit