docs: add design doc for SNS/SQS scalable change notifications - #708
docs: add design doc for SNS/SQS scalable change notifications#708rrp-bot wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @rrp-bot. Thanks for your PR. I'm waiting for a openshift-online member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe design document specifies replacing DynamoDB Streams with cross-account SNS/SQS fan-out for specs and status change notifications, including message schemas, routing, Terraform provisioning, reliability guarantees, startup behavior, operational considerations, and future optimizations. ChangesSNS/SQS change notification architecture
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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)
docs/design/sns-sqs-change-notifications.md (1)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit ADR status and ownership metadata.
Include
Status(Proposed/Accepted), decision date, owners/reviewers, and related or superseded ADRs.Last Updated Datealone does not establish the decision’s approval state.As per path instructions, this file should comply with ADR decision-document conventions.
🤖 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 `@docs/design/sns-sqs-change-notifications.md` around lines 1 - 5, Update the ADR metadata near the document title and existing Last Updated Date to include explicit Status, decision date, owners/reviewers, and related or superseded ADR references, using the repository’s established ADR conventions and valid Proposed/Accepted status values.Source: Path instructions
🤖 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 `@docs/design/sns-sqs-change-notifications.md`:
- Around line 503-504: Update the “Low-latency notifications” statement to avoid
claiming an end-to-end latency bound of 20 seconds; describe 20 seconds
specifically as the SQS long-poll interval, or replace it with a measured
end-to-end latency SLO if one is established.
- Around line 581-585: Update the SQS cost description in the messaging cost
model to state that the free tier applies account-wide rather than per queue,
and clarify the SNS-to-SQS cost description to distinguish free delivery from
applicable region-specific payload data-transfer charges. Preserve the existing
request and API pricing details.
- Around line 336-340: The documented KMS key and IAM inline policy must include
the required KMS permissions for encrypted messaging: grant
`kms:GenerateDataKey*` for SNS publishing and `kms:Decrypt` for SQS consumption,
covering both the SNS topic CMK and every subscribed SQS queue CMK. Update the
module/config symbols defining the CMK policies and service-principal grants
while preserving the existing SNS/SQS permissions.
---
Nitpick comments:
In `@docs/design/sns-sqs-change-notifications.md`:
- Around line 1-5: Update the ADR metadata near the document title and existing
Last Updated Date to include explicit Status, decision date, owners/reviewers,
and related or superseded ADR references, using the repository’s established ADR
conventions and valid Proposed/Accepted status values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 76f48a28-f5d1-4f09-81ae-846f80acb0a1
📒 Files selected for processing (1)
docs/design/sns-sqs-change-notifications.md
|
|
||
| This document describes the replacement of DynamoDB Streams with an SNS/SQS fan-out architecture | ||
| for bidirectional change notifications between the hyperfleet-operator (RC account) and | ||
| kube-applier-aws (MC account). DynamoDB remains the authoritative data store; SNS/SQS replaces |
There was a problem hiding this comment.
Actually postgres is the authoritative source
|
|
||
| ### Problem Statement | ||
|
|
||
| The hyperfleet-operator runs as a Kubernetes StatefulSet and uses DynamoDB Streams to learn when |
There was a problem hiding this comment.
The problem is more around multiple controllers, the two streams is absolute, not per controller
| DynamoDB Streams enforces a hard limit of **two concurrent consumers per stream shard**. A | ||
| three-replica operator deployment violates this limit, causing stream throttling and missed events. | ||
|
|
||
| The equivalent limit exists in kube-applier-aws, which tails DynamoDB Streams on the specs tables |
There was a problem hiding this comment.
Ensure this is regarding the scaling of replicas
|
|
||
| ### Assumptions | ||
|
|
||
| - The hyperfleet-operator is deployed as a Kubernetes StatefulSet. Pod hostnames encode the |
There was a problem hiding this comment.
However - every queue gets ALL notifications and each pod just responds to the ones it owns/is sharded to
| - The hyperfleet-operator is deployed as a Kubernetes StatefulSet. Pod hostnames encode the | ||
| replica ordinal (e.g., `hyperfleet-operator-2`), which is used to determine which pre-provisioned | ||
| SQS queue a pod should poll. | ||
| - kube-applier-aws is leader-elected per MC: only one active replica polls the specs SQS queue |
There was a problem hiding this comment.
Currently a single replica - but we wanted options
…ine#708 - Reframe two-consumer limit: clarify it is absolute across the entire stream; multiple controller goroutines per replica means even a single replica can approach the limit with multiple MCs - Clarify kube-applier Streams removal enables future horizontal scaling - Note every per-pod SQS queue receives all MC status notifications; EventRouter silently drops IDs the pod does not own - Note kube-applier is currently single-replica (leader-elected) but the design keeps options open for future scaling - Add kms:GenerateDataKey* and kms:Decrypt to IAM inline policy descriptions for both RC and MC modules, and to the Security summary - Fix low-latency bullet: 20 s is the empty-queue long-poll wait, not end-to-end latency; actual delivery is typically sub-second - Fix Cost section: SQS free tier is account-wide not per-queue; SNS-to-SQS delivery has no per-notification fee but cross-account data transfer charges apply Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
2 participants
Summary
Adds an Architecture Decision Record documenting the replacement of DynamoDB Streams with SNS/SQS fan-out for bidirectional change notifications between the hyperfleet-operator (RC account) and kube-applier-aws (MC account).
What's in the doc
Related
Summary by CodeRabbit