ROSAENG-62134: Add rate-limit backoff guard for fleet mode HTTP 429 - #264
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
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
WebhookReceiver->>SendServiceLog: Send firing notification
SendServiceLog->>OCMAPI: Submit service log
OCMAPI-->>SendServiceLog: Return HTTP 429 or success
SendServiceLog-->>WebhookReceiver: Return RateLimitError or success
WebhookReceiver->>BackoffStore: Record or clear backoff
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 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.28% +0.60%
==========================================
Files 23 23
Lines 1895 1926 +31
==========================================
+ Hits 1055 1084 +29
- 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. |
| return nil | ||
| } | ||
|
|
||
| // Skip firing alerts that are within the rate-limit backoff window. |
There was a problem hiding this comment.
The rateLimitBackoffs map entry for a notification:clusterID pair is only cleared on a successful firing send (rateLimitBackoffs.Delete in the success path). If the alert auto-resolves while the entry is in the backoff window, the resolved webhook passes through processAlert with isCurrentlyFiring=false and never touches the map.
Clear the backoff entry when a resolved alert is received for the same key.
There was a problem hiding this comment.
Fixed in commit aaad6bc.
When a resolved alert is received (isCurrentlyFiring=false), the backoff entry for that notificationName:clusterID pair is now cleared from the rateLimitBackoffs map:
if !isCurrentlyFiring {
rateLimitKey := alert.Labels[AMLabelTemplateName] + ":" + alert.Labels[AMLabelAlertHCID]
rateLimitBackoffs.Delete(rateLimitKey)
}This ensures that if an alert auto-resolves while rate-limited, the stale backoff entry doesn't persist and suppress the first send attempt when the alert fires again.
Added a test: "clears backoff entry when a resolved alert is received" — seeds a backoff entry, processes a resolved alert, and verifies the entry was deleted.
AI-generated. Review for accuracy.
AI-generated. Review for accuracy.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/test lint 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. |
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: redhat-chai-bot, TheUndeadKing 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
the
approved
5 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