OSAC-3145: PRD: Metering for Networking Resources - #159
Conversation
|
@masayag: This pull request references OSAC-3145 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 feature 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. |
|
Warning Review limit reached
Next review available in: 42 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: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughAdds the OSAC-3145 PRD for networking metering. It defines resource scope, allocation-based usage measurement, dimensions, attribution, acceptance criteria, dependencies, risks, and the VirtualNetwork metering start-state question. ChangesNetworking Metering
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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: 4
🤖 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 `@enhancements/OSAC-3145-metering-networking/prd.md`:
- Line 47: Clarify the parent-child attribution rule for Subnets in the
“Parent-child attribution” requirement: define whether a subnet has one owner,
its allocation seconds are split among connected parents, or the resulting
parent metrics are explicitly non-additive. Ensure the selected rule prevents
the same subnet allocation seconds from being counted multiple times in
aggregated parent usage.
- Line 15: Resolve the allocation metering start-state contradiction across the
glossary, problem statement, CAP-1, acceptance criteria, open question, and
referenced sections. Choose and consistently document whether each resource’s
metering starts at creation/provisioning or when it reaches READY/ALLOCATED,
explicitly defining the VirtualNetwork behavior so implementations calculate the
same resource-second totals.
- Around line 104-105: Add acceptance criteria covering the PublicIPAttachment
and ExternalIPAttachment meters, specifying that attachment-duration usage is
emitted correctly when resources are attached and detached. Keep the existing
allocation-meter and parent-attribution criteria intact, and make the criterion
observable and verifiable.
- Around line 45-47: Update the networking metering requirements to define
attachment lifecycle independently from resource allocation: specify that
attachment usage starts when an attachment becomes active and stops when it is
detached or deleted. Clarify that IP allocation metering follows the
READY/ALLOCATED-to-deletion interval separately and must not accrue attachment
usage while an IP is unattached or after detachment, including the corresponding
sections referenced by the comment.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: df058e47-e284-4cbf-b7d0-2ff5734217ae
📒 Files selected for processing (1)
enhancements/OSAC-3145-metering-networking/prd.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@enhancements/OSAC-3145-metering-networking/prd.md`:
- Line 96: Update the metering-dimensions statement near “Each networking
resource type” to include all CAP-2/CAP-3 dimensions: resource type, network
class for VirtualNetworks, IP family, region, tenant, project, and attachment
status.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 92f32929-1dde-4ddb-a913-89c5d487cef1
📒 Files selected for processing (1)
enhancements/OSAC-3145-metering-networking/prd.md
AI EP Review: EP-159Score: 10/10 | Verdict: PASS
Verdict: A strong, well-structured PRD that clearly defines networking metering as a user-facing capability with concrete personas, specific justification, clean separation from design concerns, focused scope within the metering family, and fully testable acceptance criteria. Feedback: The user stories frame value through 'downstream systems' (e.g., 'so that downstream systems can track...') rather than direct user outcomes — consider reframing at least one story per persona to describe what the persona directly gains (e.g., 'so that I can identify which tenants are consuming the most IP address pool capacity'). The open question on PENDING vs READY metering start (11.1) is well-framed but would benefit from a decision deadline to avoid blocking design work. The minor mention of 'event pipeline, provider adapters' in Risk 10.1 could be replaced with a user-facing description of the dependency (e.g., 'Part 1 metering capability'). Critical (0)None. Important (0)None. Suggestions (3)
Review costModel: claude-opus-4-6 |
| | PublicIP | Yes | — | — | | ||
| | PublicIPAttachment | Yes (ComputeInstance) | — | — | |
There was a problem hiding this comment.
@danmanor
What should be removed in this context? the entire tracking of ExternalIPAttachment? meaning it isn't a resource that requires metering for BM, Cluster and Compute Instance?
There was a problem hiding this comment.
Resolved — removed ExternalIPAttachment from metering scope entirely in 2c5bc7d. The attachment is a binding operation that consumes no independent provider capacity. The ExternalIP meter with the attached dimension (CAP-3) is sufficient for downstream systems to track whether the IP is actively in use.
Add PRD for networking resource metering covering VirtualNetworks, Subnets, SecurityGroups, PublicIPs, ExternalIPs, and NATGateways with allocation-based metering from READY/ALLOCATED state to deletion. Assisted-by: Claude Code <noreply@anthropic.com> Co-Authored-By: Moti Asayag <masayag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Moti Asayag <masayag@redhat.com>
Replace cost/billing/pricing language with metering-focused wording. Add per-service scope table mapping each networking resource to VMaaS/CaaS/BMaaS. Broaden parent-child attribution to cover all attachment types (PublicIP, ExternalIP across all services). Clarify BMaaS out-of-scope as compute-only. Add UI out-of-scope with rationale. Assisted-by: Claude Code <noreply@anthropic.com> Co-Authored-By: Moti Asayag <masayag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Moti Asayag <masayag@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Co-Authored-By: Moti Asayag <masayag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Moti Asayag <masayag@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Co-Authored-By: Moti Asayag <masayag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Moti Asayag <masayag@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Co-Authored-By: Moti Asayag <masayag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Moti Asayag <masayag@redhat.com>
- Tighten glossary to match CAP-1 allocation start point - Clarify attachment resource lifecycle in CAP-1 - Add acceptance criterion for attachment meter duration Assisted-by: Claude Code <noreply@anthropic.com> Co-Authored-By: Moti Asayag <masayag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Moti Asayag <masayag@redhat.com>
Remove PublicIP resources per architecture change - PublicIP was removed from codebase (commits OSAC-2533/2534/2535) and replaced with ExternalIP across all services. Updated PRD to reflect current implementation where ExternalIP is the universal IP resource supporting VMaaS, CaaS, and BMaaS. Changes: - Remove PublicIP and PublicIPAttachment from services table - Replace all PublicIP references with ExternalIP throughout document - Update user stories, capabilities, usage model, and acceptance criteria - Reflect that ExternalIP now supports attachment to ComputeInstance, Cluster, and BareMetalInstance Assisted-by: Claude Code <noreply@anthropic.com>
|
Thanks @danmanor — PublicIP was removed from the codebase (commits OSAC-2533/2534/2535) and replaced with ExternalIP across all services. I've updated the PRD to reflect the current architecture where ExternalIP is the universal IP resource supporting VMaaS, CaaS, and BMaaS, and can attach to ComputeInstance, Cluster, and BareMetalInstance. Changes made:
|
AI EP Review: EP-159Score: 8/10 | Verdict: PASS
Verdict: A well-structured PRD with clear persona coverage and concrete justification, held back by non-template padding (6 extra sections) and minor design leakage in supporting sections. Feedback: Trim non-template sections: remove the standalone Capabilities (§5) and Acceptance Criteria (§7) sections — the capabilities are already stated in In Scope (§2.2) and the acceptance criteria restate them a third time; fold any unique content from Capabilities into In Scope. Move Risks, Open Questions, and the Usage Measurement Model to the design document where they belong. Remove internal implementation references ('event pipeline, provider adapters' in §10.1, 'start/stop state semantics' in §8, 'backend infrastructure, VLAN allocation' in §11.1) — describe these concerns in user-observable terms or defer to the design EP. Critical (0)None. Important (3)
Suggestions (2)
Review costModel: claude-opus-4-6 |
| | SecurityGroup | Yes | Yes | Yes | | ||
| | NATGateway | Yes | Yes | Yes | | ||
| | ExternalIP | Yes | Yes | Yes | | ||
| | ExternalIPAttachment | Yes (ComputeInstance) | Yes (Cluster) | Yes (BareMetalInstance) | |
There was a problem hiding this comment.
If we meter the ExternalIPs, why do we care about the ExternalIPAttachment?
If the tenant is already metered and charged for the ExternalIP, why should anyone care if they are using it or not (attached or not)
There was a problem hiding this comment.
Agreed — ExternalIPAttachment is a binding operation, not an independent resource that consumes provider capacity (no rack space, no address pool slot, no storage). The provider capacity consumed is the IP pool address, which is owned by the ExternalIP resource itself.
The ExternalIP meter with the attached dimension (CAP-3) already captures whether the IP is actively bound to a target, which is sufficient for downstream systems to derive attachment duration and distinguish active vs idle IP usage.
Removed ExternalIPAttachment from metering scope in 2c5bc7d — updated services table, capabilities, CAP-1, usage measurement model, and acceptance criteria.
ExternalIPAttachment is a binding operation, not an independent resource that consumes provider capacity. The ExternalIP meter with the `attached` dimension is sufficient for downstream systems to derive attachment duration and distinguish active vs idle IP usage. Addresses review feedback from ronniel1 and danmanor. Assisted-by: Claude Code <noreply@anthropic.com> Co-Authored-By: Moti Asayag <masayag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Moti Asayag <masayag@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@enhancements/OSAC-3145-metering-networking/prd.md`:
- Line 93: Clarify the sentence describing networking resource dimensions by
separating the general dimensions from the resource-specific dimensions with a
semicolon or by repeating “additionally” before the VirtualNetworks and
ExternalIPs clauses.
- Line 107: Update the networking usage acceptance criterion to include
ExternalIP attachment status, specifically distinguishing attached versus idle
usage, and clarify that network class and IP family dimensions apply to the
relevant resource types. Keep the existing breakdown dimensions while ensuring
CAP-3 coverage cannot be satisfied without reporting ExternalIP attachment
state.
🪄 Autofix
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: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e673786-6258-4a1e-a7ed-532b35159325
📒 Files selected for processing (1)
enhancements/OSAC-3145-metering-networking/prd.md
AI EP Review: EP-159Score: 8/10 | Verdict: PASS
Verdict: A well-scoped PRD with clear personas and concrete justification, held back by design leakage in supporting sections and significant restatement of the same requirements across three sections. Feedback: Consolidate the triple-stated requirements: In Scope (2.2), Capabilities (5), and Acceptance Criteria (7) all describe the same set of requirements — pick one authoritative location and reference it from the others, or merge Capabilities into In Scope and keep Acceptance Criteria as the verification lens only. Remove internal component names ('event pipeline', 'provider adapters', 'backend infrastructure', 'deduplication') from the PRD — move these to the design document where they belong. The Glossary, Risks, Open Questions, and Usage Measurement Model sections are outside the PRD template; consider moving Risks and Open Questions to the design EP, and folding the Usage Measurement Model into In Scope or Capabilities. Critical (0)None. Important (3)
Suggestions (2)
Review costModel: claude-opus-4-6 |
- Add semicolons to usage measurement model intro for unambiguous resource-to-dimension mapping - Update acceptance criterion to explicitly require attachment status for ExternalIPs (CAP-3 coverage) Assisted-by: Claude Code <noreply@anthropic.com> Co-Authored-By: Moti Asayag <masayag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Moti Asayag <masayag@redhat.com>
AI EP Review: EP-159Score: 9/10 | Verdict: PASS
Verdict: A clear, well-justified, and testable PRD for networking allocation metering that covers all four personas and three services, held back from a perfect score by significant verbosity — the same requirements are restated across In Scope, Capabilities, and Acceptance Criteria, and six non-template sections add bulk without proportional value. Feedback: Consolidate the three places where core requirements are stated (§2.2 In Scope, §5 Capabilities, §7 Acceptance Criteria) — state each requirement once in In Scope, then write acceptance criteria as end-to-end verification scenarios rather than restatements. Remove non-template sections: fold the Glossary's 'allocation metering' definition into the Problem Statement, move the Usage Measurement Model table and Risks to the design EP, and convert the Open Question into a TBD assumption. The PRD's content is strong; the issue is purely structural — a reader shouldn't need to cross-reference three sections to confirm they're reading the same requirement. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
Note: Networking resource metering vs. industry practiceI did a market survey across hyperscalers and GPU/AI cloud competitors to see how the proposed metering model compares. The industry is unanimous: virtual networks, subnets, and security groups are free everywhere. No provider — hyperscaler or GPU cloud — charges for these or meters them on an allocation basis. The only networking resources that get allocation-based metering/billing are ones that consume scarce infrastructure:
The PRD correctly states that metering is data collection only — billing decisions are deferred to a separate PRD. That said, the PRD treats all six resource types identically (same resource-seconds unit, same allocation model, same acceptance criteria) without distinguishing between resources that consume scarce infrastructure (ExternalIP, NATGateway) and resources that are configuration metadata (VirtualNetwork, Subnet, SecurityGroup). The problem statement argues that "a VirtualNetwork consumes backend network configuration and VLAN allocation from creation" — but no cloud provider treats that as a metered cost driver. This is just a note for awareness — if you want to proceed with metering all six resource types uniformly, I'm fine with approving. |
@danmanor I think this really make sense. |
The purpose of the metering service is to provide data for the billing. We don't rely on it for Quota or to show all of the resources used by specific user. This isn't the scope nor purpose of the metering. Therefore I agree - if we're not going to incur cost for the resources - then we should not report them. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, masayag 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 |
Corrects the merged PRD (#159), which metered VirtualNetwork, Subnet, and SecurityGroup on an allocation basis. Per review comment 5204380439, these are configuration metadata that incur no cost and are free across every surveyed hyperscaler and GPU/AI cloud, none of which meter them on an allocation basis. Metering is now limited to the scarce-infrastructure resources ExternalIP and NATGateway; the three free resources are moved to Out of Scope with the industry-practice rationale, and a negative acceptance criterion asserts they generate no usage data. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Moti Asayag <masayag@redhat.com>
Corrects the merged PRD (osac-project#159), which metered VirtualNetwork, Subnet, and SecurityGroup on an allocation basis. Per review comment 5204380439, these are configuration metadata that incur no cost and are free across every surveyed hyperscaler and GPU/AI cloud, none of which meter them on an allocation basis. Metering is now limited to the scarce-infrastructure resources ExternalIP and NATGateway; the three free resources are moved to Out of Scope with the industry-practice rationale, and a negative acceptance criterion asserts they generate no usage data. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Moti Asayag <masayag@redhat.com>
4 participants
Summary
Jira
Related PRDs
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit