OSAC-1029: Per-service networking EPs and unified EP restructuring - #107
Conversation
|
@danmanor: This pull request references OSAC-1029 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. |
|
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:
WalkthroughThe PR adds BMaaS, CaaS, and VMaaS networking design and PRD documents, revises unified networking contracts and lifecycle descriptions, and defines tenant default networking with automatic external access and NAT behavior. ChangesUnified Networking Base Updates
BMaaS Networking Proposal
CaaS Networking Proposal
VMaaS Networking Proposal
Default Networking Proposal
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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 |
AI EP Review: EP-107Score: 9/10 | Verdict: PASS
Verdict: Strong set of PRDs with clear user outcomes, concrete justification, well-scoped per service type, and testable acceptance criteria; weakened by design leakage in some functional requirements (BMaaS FR-8/FR-11) and inconsistent NFR coverage across PRDs. Feedback: Remove implementation details from PRD requirements: BMaaS FR-8 should say 'network connectivity is configured for each attachment before provisioning begins' rather than describing switch port configuration, fabric server identification, and DHCP/static allocation. BMaaS FR-11 describes an internal configuration parameter that belongs in the design doc, not the PRD. Add quantitative success metrics to the CaaS and VMaaS PRDs (BMaaS sets a good example with provisioning time and success rate targets), and add at least one NFR to the unified networking PRD to establish baseline performance expectations for the cross-service networking model. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: A comprehensive, well-structured set of design documents that introduces consistent networking support across VMaaS, CaaS, and BMaaS with sound architectural principles, thorough test plans, and clearly defined scope boundaries. Feedback: The NATGateway reuse-regardless-of-state behavior is an open question across all three designs and should be resolved centrally before implementation begins, since it directly affects user experience in failure scenarios. Consider consolidating the duplicated HostType proto definition (repeated in BMaaS and CaaS designs) by referencing the unified-networking HostType section to reduce drift risk. Adding mermaid sequence diagrams for the more complex cross-component flows (especially the CaaS VIP feedback loop spanning template, ClusterOrder, feedback controller, fulfillment-service, and ExternalIPAttachment controller) would significantly improve reviewability. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI EP Review: EP-107Score: 9/10 | Verdict: PASS
Verdict: Strong set of PRDs with clear user-facing requirements, concrete justification, well-scoped features, and testable acceptance criteria; minor design leakage in BMaaS PRD (switch port terminology, fabric manager config detail) prevents a perfect score. Feedback: Replace implementation-specific terminology in BMaaS PRD: FR-8's 'switch port configuration' should be 'network connectivity configuration' and FR-11 should describe the user-facing outcome (avoiding name collision with networking resources) rather than the mechanism (static configuration string). Also resolve the auto NAT gateway reuse-regardless-of-state design decision that appears as both a risk and open question in all three service-specific PRDs — either commit to a direction or explicitly mark it as a blocking open question. Finally, VMaaS Non-Goals incorrectly states bare-metal multi-interface support is 'out of scope' when the BMaaS PRD explicitly covers it — reword to 'covered in BMaaS Networking PRD'. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI EP Review: EP-107Score: 8/10 | Verdict: PASS
Verdict: A well-structured set of networking PRDs with clear user outcomes, specific requirements, and thorough acceptance criteria, held back slightly by gap-descriptive (rather than impact-quantified) business justification and the bundling of three independently deliverable service-type PRDs into a single submission. Feedback: Strengthen the WHY in each PRD by quantifying the impact of the current gaps — e.g., how many tenants are blocked, how much manual coordination time is required per BM provisioning, or tie explicitly to a strategic goal like 'CaaS adoption is blocked for tenants requiring network isolation.' Consider splitting the per-service PRDs (BMaaS, CaaS, VMaaS) into separate PRs since they are independently deliverable and serve different service types, which would make each easier to review, prioritize, and track independently. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
|
@danmanor: This pull request references OSAC-1029 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. |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: A comprehensive, well-structured set of per-service networking designs that build cleanly on the unified networking architecture, with honest dependency tracking, thorough test plans, and clear graduation criteria. Feedback: The auto NATGateway reuse-regardless-of-state design is the weakest point across all three documents -- consider resolving open question #1 (check state before reusing) before merging, as it affects user experience in all service types identically. The 5 untracked GAPs in BMaaS and 5 in CaaS should get Jira tickets before implementation begins to prevent scope creep. The CaaS VIP feedback loop (template -> ClusterOrder status -> Signal RPC -> fulfillment-service -> Cluster -> ExternalIPAttachment controller) spans 5 components and is the highest-risk integration path; consider adding a sequence diagram and explicit timeout/retry semantics for each hop. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/bmaas-networking/design.md`:
- Around line 126-141: The tenant-facing interface validation for
BareMetalNetworkAttachment needs to explicitly reject lifecycle/BMC ports, not
just verify the name exists in HostType.interfaces. Update the validation logic
that resolves the HostType from the catalog_item/template so it filters out or
errors on interfaces marked as lifecycle, and ensure network_attachments only
accept tenant-attachable interfaces such as fabric, management, and storage.
In `@enhancements/bmaas-networking/prd.md`:
- Around line 114-117: FR-12 only defines cleanup for auto-provisioned external
IP resources, but it does not state what happens to NAT gateways when a server
is deleted. Update the deletion cleanup section in the PRD to explicitly define
ownership for NAT gateway resources referenced by FR-7 and the networking flow,
using the same AUTO vs reused/shared distinction as the external IP rules. Make
it clear that server deletion deletes only system-created NAT gateways and
leaves reused/shared gateways intact, or specify the alternative policy
consistently across the networking requirements.
In `@enhancements/caas-networking/design.md`:
- Around line 124-127: Persist the parent ClusterOrder record before allocating
dependent networking resources, or add rollback for any allocations made first.
Update the provisioning flow described around external_ip_mode and
nat_gateway_mode so the Cluster record/ClusterOrder exists before creating
ExternalIPs, ExternalIPAttachments, or NATGateway, and use the existing
ClusterOrder/network_attachments flow to keep ownership clear if a later step
fails.
- Around line 403-407: The Auto-Provisioned Resource Lifecycle summary is
missing NATGateway from the cleanup contract, so update the lifecycle
description to include NATGateway alongside the other auto-provisioned
resources. Make sure the cleanup order and permanent-failure behavior in the
design section explicitly reference NATGateway so it is removed with the cluster
and not left orphaned; use the existing lifecycle terminology in the
Auto-Provisioned Resource Lifecycle section to keep it consistent.
In `@enhancements/caas-networking/prd.md`:
- Around line 66-67: Update the FR-1 requirement in the networking PRD so
nodeSet is required for v0.2 instead of optional, and align the wording with
FR-11’s one-attachment-per-node-set, bare-metal-only scope. Make sure the
cluster creation/network attachment description and any related references to
nodeSet in the same requirements block are consistent so interface selection is
no longer ambiguous for mixed HostTypes.
- Around line 78-79: The NAT gateway reuse behavior in FR-4 should be narrowed
so cluster creation only reuses a healthy gateway. Update the requirement around
automatic NAT gateway provisioning to reference the relevant cluster creation
flow and NAT gateway handling so that only Ready resources are reused, and add
an explicit rejection or creation-failure path for existing Failed or Deleting
gateways.
In `@enhancements/unified-networking/prd.md`:
- Around line 337-338: Update the PRD checklist in this section to match the
design’s per-resource attachment contracts: replace the shared
`NetworkAttachment` wording with the specific `ComputeNetworkAttachment`,
`ClusterNetworkAttachment`, and `BareMetalNetworkAttachment` types, and refer to
`HostType` instead of `BaremetalInstanceTemplate`. Keep the language aligned
with the resource-specific API shapes so the requirements mirror the design
exactly.
In `@enhancements/vmaas-networking/prd.md`:
- Around line 71-74: Clarify the Auto NAT Gateway failure contract in the PRD:
FR-5 currently says `--nat-gateway=auto` reuses an existing gateway regardless
of state, which conflicts with the outbound connectivity promise. Update the
`Auto NAT Gateway` requirement so the behavior is explicit when an existing
gateway is failed, deleting, or otherwise unusable—either require the
create/retry flow to fail in that case, or state that degraded connectivity is
acceptable. Use the `FR-5` requirement text and the `--nat-gateway=auto`
acceptance language as the places to revise.
🪄 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: Enterprise
Run ID: 4148b28a-cd27-4c4f-9598-fd73bed371ed
📒 Files selected for processing (8)
enhancements/bmaas-networking/design.mdenhancements/bmaas-networking/prd.mdenhancements/caas-networking/design.mdenhancements/caas-networking/prd.mdenhancements/unified-networking/design.mdenhancements/unified-networking/prd.mdenhancements/vmaas-networking/design.mdenhancements/vmaas-networking/prd.md
AI EP Review: EP-107Score: 9/10 | Verdict: PASS
Verdict: Strong set of PRDs with clear user-facing needs, compelling justification, well-identified personas, and testable acceptance criteria; minor design leakage in BMaaS and CaaS requirements (FR-8, FR-11, FR-9) keeps the 'how' score at 1. Feedback: Move implementation-specific language out of PRD requirements into design docs: BMaaS FR-8 should describe the user-observable outcome ('network connectivity is configured for each interface before OS provisioning begins') without specifying HOW ('identifies the physical server in the network fabric, adds the server's interface to the subnet's network segment'). BMaaS FR-11 should describe the user-observable effect of fabric manager configuration, not its internal representation ('static configuration string set at deployment time'). CaaS FR-9 should separate 'endpoint addresses are available in cluster status' (user-observable) from 'performs DNS record creation' (implementation detail). Critical (0)None. Important (3)
Suggestions (2)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: A comprehensive, well-structured set of per-service networking enhancement proposals that consistently extend the unified networking architecture across VMaaS, CaaS, BMaaS, and default networking with thorough test plans, clear dependency tracking, and sound architectural patterns. Feedback: The NATGateway reuse-regardless-of-state decision appears as both a risk and an open question in all four designs -- resolve this cross-cutting concern in one place (e.g., the unified EP or default networking EP) and reference it from the others to avoid divergent implementations. The 'Not tracked' GAPs in dependency tables (CRD updates, mutateBMI, IP feedback, HostType interfaces, agent selection logic) should be tracked as Jira tickets before implementation begins to prevent discovery-phase delays. Consider adding a sequence diagram (mermaid) for the VIP feedback loop in CaaS (template -> ClusterOrder status -> Signal RPC -> fulfillment-service -> Cluster -> ExternalIPAttachment controller) since it spans 5+ components and is called out as a complexity risk. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: A comprehensive and well-structured set of per-service networking design documents that extend a unified architecture with consistent patterns, concrete API definitions, thorough test plans, and honest dependency tracking. Feedback: The open questions about NATGateway state-aware reuse appear in all four designs and the PRDs — resolving this cross-cutting decision before implementation would eliminate ambiguity for all service teams simultaneously. Consider adding idempotency guarantees for dispatcher calls (create_network_attachment, delete_network_attachment) since reconciliation loops will retry on transient failures. The 5 untracked GAPs in both BMaaS and CaaS dependency tables should be converted to Jira tickets before work begins to avoid items falling through the cracks. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: A comprehensive, well-structured set of per-service networking design documents that decompose the OSAC unified networking architecture into clear, implementable per-service flows with thorough API definitions, failure handling, test plans, and operational procedures. Feedback: The NATGateway reuse-regardless-of-state design is flagged as an open question in all four documents — resolve this cross-cutting concern once (perhaps in the unified EP) rather than leaving identical open questions in each per-service doc. Consider adding mermaid sequence diagrams for the multi-component flows (especially CaaS VIP feedback loop: template -> ClusterOrder status -> Signal RPC -> fulfillment-service -> Cluster -> ExternalIPAttachment controller) as the text-based workflow descriptions are harder to follow for complex interactions. The 5 untracked GAPs in BMaaS and 5 in CaaS dependency tables should be converted to Jira tickets before implementation begins to avoid work falling through the cracks. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 7/8 | Verdict: PASS
Verdict: A comprehensive and architecturally sound set of enhancements that extend the OSAC unified networking API to all three service types (VMaaS, CaaS, BMaaS) with default networking, auto external access, and multi-NIC support; the main weakness is the sheer breadth of the PR and several unresolved cross-cutting design questions. Feedback: The repeated open question about NATGateway reuse semantics (reuse regardless of state vs. state-aware reuse) appears identically in all four service-specific enhancements and the default-networking EP — resolve this once in the unified networking EP and reference it from the others to avoid divergence during implementation. The dependency tables show 5+ untracked GAP items per enhancement (CRD updates, mutateBMI, IP feedback, HostType extensions, agent selection logic); these should be tracked in Jira before merging to prevent implementation surprises. Consider splitting this PR into the unified networking updates (including HostType) as one PR and the per-service EPs as follow-ups to make review more tractable. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 7/8 | Verdict: PASS
Verdict: A comprehensive, well-architected networking enhancement suite that cleanly decomposes a unified networking API into per-service flows with consistent patterns; the main concern is the large scope with many untracked implementation gaps and several identical open questions that need resolution before implementation begins. Feedback: Resolve the NATGateway state-aware reuse open question once and reference it from all four EPs rather than repeating the same open question and risk in each document — this is a cross-cutting design decision that should be settled at the unified networking level. Create Jira tickets for the 10+ untracked gaps (marked GAP in dependency tables) before implementation starts, as these represent real work that could surprise the schedule. Consider adding a sequence diagram or state machine for the auto-provisioning two-phase lifecycle, as the text description spans multiple documents and the interaction between ExternalIP Pending->Allocated and ExternalIPAttachment Pending->Ready across different target types (VM/BM/Cluster) would benefit from a visual representation. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: A thorough and well-structured set of per-service networking enhancement proposals that consistently extend the unified networking architecture with clear implementation paths, comprehensive test plans, and explicit dependency tracking — the main gaps are untracked Jira items and a few unresolved open questions that should be closed before implementation begins. Feedback: Resolve the three recurring open questions before implementation: (1) NATGateway Deleting-state reuse semantics — the current 'reuse regardless of state' behavior is a documented weakness across all four designs; at minimum, treat Deleting as 'does not exist' to avoid silently attaching to a disappearing resource. (2) Capacity exhaustion behavior (API error vs Failed resource) — pick one and apply consistently. (3) IP address feedback mechanism for BMaaS (Option A is recommended but still listed as an open question in design.md OQ#3 — promote to a decision). Additionally, create Jira tickets for all items marked as 'GAP' in the dependency tables (5 in BMaaS, 5 in CaaS) — these represent real implementation work that is invisible to planning without tracking. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: A comprehensive, well-structured suite of networking enhancement designs that decompose a complex cross-service feature into modular, per-service EPs with consistent patterns, detailed API definitions, thorough test plans, and honest dependency/risk tracking. Feedback: The designs are strong overall. Three areas for improvement: (1) The NATGateway 'reuse regardless of state' decision should be resolved before implementation rather than deferred — silently attaching to a Deleting NATGateway is a user-facing bug, and the open question about treating Deleting as 'does not exist' should be closed in this design phase. (2) The CaaS design's agent selection migration from template to operator (reconcileAgentSelection) is the highest-risk change and could benefit from a more detailed specification of the selection algorithm rather than 'port existing logic.' (3) Consider adding a dependency tracking summary table across all EPs showing the critical path — the individual dependency tables are thorough but the cross-EP ordering (e.g., which EP blocks which) would help with implementation planning. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
enhancements/unified-networking/design.md (1)
835-839: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClarify
node_setvsfabric_interfacefor cluster attachments.node_setis optional here, butfabric_interfaceis a single immutable value derived from a node set’s HostType. If one attachment can cover multiple node sets, the spec needs to say how that field is resolved for differing HostTypes; otherwise makenode_setrequired for v0.2.🤖 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 `@enhancements/unified-networking/design.md` around lines 835 - 839, The cluster attachment spec is ambiguous about how an optional node_set interacts with the single immutable fabric_interface value. Update the unified-networking design text to either require node_set for v0.2 or explicitly define how fulfillment-service resolves fabric_interface when one attachment applies to multiple node sets with different HostTypes. Refer to the cluster attachment entry, node_set, fabric_interface, and fulfillment-service resolution rules so the behavior is unambiguous.enhancements/bmaas-networking/prd.md (1)
96-96: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestrict AUTO NAT gateway reuse to ready resources. Reusing an existing NAT gateway while it’s deleting or otherwise unavailable can leave the new server with broken outbound connectivity. Define a READY/ACTIVE gate, or fail/create a replacement when the current gateway isn’t usable.
🤖 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 `@enhancements/bmaas-networking/prd.md` at line 96, Update the NAT gateway AUTO behavior in the BMAAS networking spec so reuse only applies to gateways that are actually ready/active; if the existing gateway is deleting or otherwise unavailable, the server should not attach to it. Adjust the FR-7 wording in the NAT gateway mode section to reference the readiness gate and clarify that create-or-replace behavior is required when the current gateway is unusable, while keeping the existing AUTO/NONE semantics in the same spec area.enhancements/vmaas-networking/design.md (1)
304-305: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep the finalizer until cleanup succeeds.
Dropping the finalizer after N retries can delete the parent while auto-provisioned ExternalIP/Attachment records are still allocated, which leaks capacity and leaves public endpoints orphaned. Keep the finalizer and surface a terminal cleanup condition, or hand off orphan cleanup to a separate reconciler.
🤖 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 `@enhancements/vmaas-networking/design.md` around lines 304 - 305, The cleanup flow currently removes the finalizer after N retries in the auto-provisioned resource cleanup path, which can let the parent resource delete while ExternalIP/ExternalIPAttachment records are still allocated. Update the finalizer/reconciliation design so the finalizer stays until cleanup actually succeeds, and use a terminal cleanup condition or a separate reconciler to handle orphaned ExternalIP/Attachment cleanup; refer to the auto-provisioned resource cleanup transient/permanent failure behavior in the design doc.
♻️ Duplicate comments (2)
enhancements/caas-networking/prd.md (2)
78-78: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestrict NAT gateway reuse to healthy gateways.
“Reuse any existing NAT gateway” still allows Failed/Deleting gateways to be reused, which can leave new clusters without outbound connectivity. Limit reuse to Ready gateways or fail creation explicitly.
🤖 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 `@enhancements/caas-networking/prd.md` at line 78, Restrict NAT gateway reuse in the cluster creation flow so only healthy gateways are reused: update the FR-4 behavior around automatic NAT gateway provisioning to check the NAT gateway’s status before reusing it, and treat Failed or Deleting gateways as unusable. In the cluster creation logic that implements the NAT gateway lookup/reuse path, ensure only Ready gateways are selected; otherwise fail creation explicitly instead of silently reusing the unhealthy gateway.
66-66: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDefine precedence when
nodeSetis omitted.The attachment scope is still ambiguous: FR-1 allows
nodeSetto be optional, but the PRD never says whether an omitted value applies cluster-wide or how it interacts with node-set-specific attachments. Make that explicit for v0.2.🤖 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 `@enhancements/caas-networking/prd.md` at line 66, Clarify FR-1’s network attachment scope by explicitly stating the default behavior when nodeSet is omitted, since it is currently ambiguous. Update the FR-1 requirement so it says whether an omitted nodeSet applies cluster-wide or only to a default node pool, and define its precedence relative to nodeSet-specific attachments in v0.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/bmaas-networking/design.md`:
- Around line 204-205: The NATGateway auto-provisioning rule is too broad
because `nat_gateway_mode == AUTO` currently says to reuse an existing
NATGateway regardless of state, which can bind a new BaremetalInstance to a
gateway that is deleting or otherwise unstable. Update the `nat_gateway_mode`
flow in the design so reuse is limited to stable, ready-like states only, and
explicitly state that non-stable states must not be reused; keep the wording
aligned with the NATGateway state lifecycle sections referenced later in the
same document.
- Around line 244-247: The workflow description is inconsistent about who owns
primary-IP writeback: `ExternalIPAttachment`/`reconcileNetworking` says the
fabric manager writes `status.networkAttachments[].ipAddress`, while the “Subnet
IPAM — Resolved” section implies the operator does it. Update the design so a
single component is clearly responsible for allocating and writing back the
primary IP, and then make both the step 6b text and the later subnet/IPAM
section match that owner consistently.
- Around line 216-217: The current subnet IPAM flow in the design is race-prone
because `list existing allocations → pick next free IP` can let concurrent
reconciles assign the same address. Update the operator-managed allocation path
for `status.networkAttachments[].ipAddress` to use an atomic reservation
mechanism keyed on the Subnet CR before writing status back. Keep the
switch-side dispatch in `osac.templates.{{ fabric_manager
}}.create_network_attachment` unchanged, but ensure the IP reservation happens
first and only one BaremetalInstance can claim a given subnet IP at a time.
In `@enhancements/bmaas-networking/prd.md`:
- Around line 145-148: The defaulted-server networking requirements in the PRD
still assume a fabric-role interface exists when attachments are omitted, but
the host-type interface list may be empty. Update the default networking
requirement around the tenant onboarding/defaults and server creation flow to
explicitly cover the no-fabric-interface case for omitted attachments, and
define the same clear failure behavior used for explicit attachments when the
host type has no fabric interface. Reference the FR-5/defaulted server wording
and the host type physical network interface list requirement so the rule is
consistent in both paths.
In `@enhancements/caas-networking/design.md`:
- Around line 138-140: The IP/VIP allocation flow in the operator-managed IPAM
section is subject to a race because Subnet CR reads and status writes are not
atomic. Update the provisioning logic around the operator allocation path to use
a transactional claim/lease or other conflict-safe reservation mechanism when
assigning `status.nodeSets[].agents[].ipAddress`, `status.apiVIP`, and
`status.ingressVIP`, and make the reconcile retry/handle conflicts cleanly in
the same allocation routine that currently computes the next available IPs.
- Line 125: The AUTO NATGateway flow currently allocates a separate ExternalIP
before creating the NATGateway in a later transaction, but it does not define
cleanup if that second step fails. Update the design around the NATGateway AUTO
path to either make the ExternalIP and NATGateway creation atomic or add a
compensating delete/release for the allocated ExternalIP on failure, and ensure
the logic tied to nat_gateway_mode == AUTO and the auto-provisioned
ExternalIP/NATGateway handling preserves pool capacity.
In `@enhancements/default-networking/design.md`:
- Around line 566-570: The Tenant readiness story is inconsistent: the risk note
says onboarding can still complete with Tenant becoming READY even when
NetworkClass defaults are missing, while the main flow around
DefaultNetworkingReady implies READY should be gated on default networking being
configured. Align the design by choosing one contract and updating the relevant
sections in the document, especially the DefaultNetworkingReady readiness gating
and the deployment risk/mitigation text, so implementers do not wire conflicting
readiness checks.
- Around line 459-466: Update the NATGateway reuse behavior in the
default-networking design so `nat_gateway_mode=AUTO` only reuses an existing
NATGateway when it is READY. The current reuse rule in the NATGateway reuse
logic incorrectly allows Failed or Deleting gateways to be selected; change the
described flow to filter by ready state before reusing the first matching
NATGateway by name, and if no READY gateway exists, describe the
create-or-replace/recovery path explicitly instead of reusing unhealthy
resources.
In `@enhancements/unified-networking/design.md`:
- Around line 630-637: The Cluster row in the ExternalIPAttachment preconditions
uses the wrong status owner and should be updated to match the Cluster-backed
contract. In the target-type table, replace references to
ClusterOrder.status.apiVIP/ingressVIP with
Cluster.status.api_endpoint/ingress_endpoint, and keep the description aligned
with the CaaS flow and API ownership model so implementers read the correct
source of target IP.
In `@enhancements/vmaas-networking/design.md`:
- Around line 159-163: Update the NATGateway reuse logic in the
fulfillment-service flow described under the NATGateway creation/reuse step so
it only reuses an existing NATGateway when it is Ready. Treat Failed, Deleting,
or any non-Ready state as absent and create a replacement NATGateway instead,
while keeping the separate ExternalIP and separate-transaction behavior intact.
Use the existing NATGateway state checks in the fulfillment-service path and the
dispatcher/fabric-manager create_nat_gateway flow to locate the change.
---
Outside diff comments:
In `@enhancements/bmaas-networking/prd.md`:
- Line 96: Update the NAT gateway AUTO behavior in the BMAAS networking spec so
reuse only applies to gateways that are actually ready/active; if the existing
gateway is deleting or otherwise unavailable, the server should not attach to
it. Adjust the FR-7 wording in the NAT gateway mode section to reference the
readiness gate and clarify that create-or-replace behavior is required when the
current gateway is unusable, while keeping the existing AUTO/NONE semantics in
the same spec area.
In `@enhancements/unified-networking/design.md`:
- Around line 835-839: The cluster attachment spec is ambiguous about how an
optional node_set interacts with the single immutable fabric_interface value.
Update the unified-networking design text to either require node_set for v0.2 or
explicitly define how fulfillment-service resolves fabric_interface when one
attachment applies to multiple node sets with different HostTypes. Refer to the
cluster attachment entry, node_set, fabric_interface, and fulfillment-service
resolution rules so the behavior is unambiguous.
In `@enhancements/vmaas-networking/design.md`:
- Around line 304-305: The cleanup flow currently removes the finalizer after N
retries in the auto-provisioned resource cleanup path, which can let the parent
resource delete while ExternalIP/ExternalIPAttachment records are still
allocated. Update the finalizer/reconciliation design so the finalizer stays
until cleanup actually succeeds, and use a terminal cleanup condition or a
separate reconciler to handle orphaned ExternalIP/Attachment cleanup; refer to
the auto-provisioned resource cleanup transient/permanent failure behavior in
the design doc.
---
Duplicate comments:
In `@enhancements/caas-networking/prd.md`:
- Line 78: Restrict NAT gateway reuse in the cluster creation flow so only
healthy gateways are reused: update the FR-4 behavior around automatic NAT
gateway provisioning to check the NAT gateway’s status before reusing it, and
treat Failed or Deleting gateways as unusable. In the cluster creation logic
that implements the NAT gateway lookup/reuse path, ensure only Ready gateways
are selected; otherwise fail creation explicitly instead of silently reusing the
unhealthy gateway.
- Line 66: Clarify FR-1’s network attachment scope by explicitly stating the
default behavior when nodeSet is omitted, since it is currently ambiguous.
Update the FR-1 requirement so it says whether an omitted nodeSet applies
cluster-wide or only to a default node pool, and define its precedence relative
to nodeSet-specific attachments in v0.2.
🪄 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: Enterprise
Run ID: b30acebd-4acc-492f-8199-2a86e5ddedbf
📒 Files selected for processing (10)
enhancements/bmaas-networking/design.mdenhancements/bmaas-networking/prd.mdenhancements/caas-networking/design.mdenhancements/caas-networking/prd.mdenhancements/default-networking/design.mdenhancements/default-networking/prd.mdenhancements/unified-networking/design.mdenhancements/unified-networking/prd.mdenhancements/vmaas-networking/design.mdenhancements/vmaas-networking/prd.md
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: A comprehensive, architecturally sound set of design documents that define a unified networking model across VMaaS, CaaS, and BMaaS with consistent patterns for auto-provisioning, IPAM, tenant isolation, and resource lifecycle management. Feedback: The designs are strong overall. Two areas to tighten before implementation: (1) The operator IPAM approach allocates IPs by scanning existing allocations on the subnet, but the concurrency model isn't specified — clarify how two concurrent reconcileNetworking runs on different resources sharing the same subnet avoid allocating the same IP (K8s optimistic concurrency on the Subnet CR, or a dedicated IPAllocation sub-resource?). (2) The NATGateway 'reuse regardless of state' decision is flagged as an open question in all four designs — resolve this before implementation, as silently attaching to a Deleting NATGateway is a user-facing footgun that will generate support tickets. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 7/8 | Verdict: PASS
Verdict: A comprehensive, architecturally sound design suite for unified networking across VMaaS, CaaS, and BMaaS — well-specified with clear API contracts, test plans, and dependency tracking — held back slightly by aggregate scope breadth and several unresolved open questions that could affect implementation. Feedback: The individual designs are strong, but consider staging the delivery more explicitly — the dependency tables show most CaaS and BMaaS work items are 'New' while prerequisites like dispatcher core and NATGateway full stack are only partially in progress. A phased delivery plan (e.g., VMaaS auto-access first, then BMaaS networking, then CaaS with VIP feedback) with explicit gates would reduce integration risk. Resolve the NATGateway Deleting-state open question before implementation — silently attaching to a disappearing resource is a user-facing bug, not a design tradeoff to defer. Finally, the cross-operator IPAM concurrency section mentions optimistic locking on Subnet CR status but lacks detail on the allocation tracking structure; specify whether allocations are tracked as a list in Subnet status or via separate IPAllocation CRs, as this affects conflict behavior at scale. Critical (0)None. Important (4)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: A comprehensive, well-structured design family that extends unified networking across VMaaS, CaaS, and BMaaS with consistent architectural patterns, concrete API definitions, and thorough test plans. Feedback: Resolve the three shared open questions (NATGateway Deleting-state behavior, capacity exhaustion error-vs-Failed-resource, CaaS agent selection mechanism) before implementation begins, as they affect cross-cutting behavior in all service types. Create JIRA tickets for the 11 untracked GAP items identified in the dependency tables (CRD updates, mutateBMI, operator IPAM, HostType NetworkInterface list, etc.) to avoid planning gaps. Consider adding explicit metrics for IPAM allocation failures and auto-provisioning latency rather than relying solely on existing provisioning metrics. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: An exceptionally thorough set of five interconnected networking design documents that define a unified, consistent networking architecture across VMaaS, CaaS, and BMaaS with well-justified design decisions, comprehensive test plans, and clear dependency tracking. Feedback: The cross-operator IPAM concurrency section (Unified Networking) states 'implementation should use optimistic concurrency on a shared tracking resource' but doesn't specify the concrete mechanism — clarify which resource (Subnet CR status with allocated IPs list?), what fields, and how conflicts are resolved, since osac-operator and bare-metal-fulfillment-operator allocating from the same subnet simultaneously is a critical correctness concern. The NATGateway Deleting-state open question appears in all four per-service documents and the Default Networking document — consider resolving it now (treating Deleting as 'does not exist' seems clearly correct since the SNAT rule is being removed) or at minimum consolidating it into the Unified Networking document with cross-references to reduce redundancy. Several implementation work items are marked 'Not tracked | GAP' across documents (CRD updates, mutateBMI, operator IPAM, agent selection logic) — track these in Jira before implementation b Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: A comprehensive, well-architected set of enhancement proposals that cleanly decompose networking for VMaaS, CaaS, and BMaaS into per-service EPs built on a shared Unified Networking foundation, with consistent patterns for auto-provisioning, IPAM, cleanup, and feedback loops. Feedback: Three areas to strengthen: (1) The cross-operator IPAM concurrency design (osac-operator vs bare-metal-fulfillment-operator allocating from the same subnet) needs more detail — specify the tracking resource schema, conflict detection mechanism, and retry behavior rather than deferring to implementation. (2) The duplicated open questions (NATGateway Deleting state, capacity exhaustion behavior) appear in 4 separate EPs — resolve these at the Unified Networking EP level and reference the decision from service EPs to avoid divergent implementations. (3) Track the 10+ untracked GAP dependencies in Jira now to prevent implementation surprises — several (CRD updates, mutateBMI changes, operator RBAC for Subnet/NetworkClass CRs) are on the critical path. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
enhancements/default-networking/design.md (1)
196-198: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd rollback for the split NATGateway create path.
The separate ExternalIP/NATGateway transaction has no compensating cleanup if NATGateway creation fails, so pool capacity and the reserved IP can leak. Make the pair atomic or define the release path explicitly.
🤖 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 `@enhancements/default-networking/design.md` around lines 196 - 198, The split NATGateway creation path needs compensating cleanup because the separate ExternalIP and NATGateway transaction can fail after reserving capacity, leaking both the IP and pool usage. Update the NATGateway provisioning flow in the design around the separate transaction so that the ExternalIP reservation is either made atomic with NATGateway creation or explicitly rolled back on any failure, and ensure the release path covers the auto-provisioned resource labels and pool decrement/release logic.enhancements/vmaas-networking/design.md (1)
161-161: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd rollback for the split NATGateway create path.
If the NATGateway write fails after the ExternalIP is allocated, the design leaks the IP and pool capacity. Make the pair atomic or spell out the compensating delete/release path.
🤖 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 `@enhancements/vmaas-networking/design.md` at line 161, The split NATGateway creation path in the design needs a rollback/compensation plan because `NATGateway` and its auto-provisioned `ExternalIP` are created separately and can leak pool capacity if the second step fails. Update the `NATGateway` create flow to either make the `ExternalIP` allocation and `NATGateway` persist atomic, or explicitly document and implement a compensating delete/release path that cleans up the allocated IP and restores pool capacity when the `NATGateway` write fails. Refer to the `external_ip_mode=AUTO`/`osac.openshift.io/auto-provisioned` path and the separate DB transaction described for the NATGateway creation.
♻️ Duplicate comments (1)
enhancements/bmaas-networking/design.md (1)
205-205: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGate NATGateway reuse on Ready state.
nat_gateway_mode == AUTOstill reuses a gateway regardless of state, so a deleting or failed NATGateway can strand outbound connectivity for the next BaremetalInstance.🤖 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 `@enhancements/bmaas-networking/design.md` at line 205, Update the `nat_gateway_mode == AUTO` design in the NATGateway provisioning flow so reuse only happens when an existing NATGateway is in Ready state, and treat deleting/failed gateways as non-reusable. Adjust the `AUTO` branch description around NATGateway lookup/reuse to reference the Ready condition explicitly, while keeping the separate ExternalIP and separate DB transaction behavior unchanged.
🤖 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/caas-networking/design.md`:
- Line 125: Update the NATGateway reuse logic in the AUTO branch so it only
reuses an existing gateway when it is in Ready state; if an existing NATGateway
is deleting, failed, or otherwise not Ready, treat it as unavailable and proceed
with creating a new one after the parent resource is persisted. Adjust the
decision point in the NATGateway provisioning flow to check the gateway’s status
before reuse, and keep the separate DB transaction and auto-provisioned labeling
behavior unchanged.
In `@enhancements/unified-networking/design.md`:
- Around line 565-569: Update the ExternalIPAttachment flow to read the VIPs
from the Cluster object, not from ClusterOrder status, so ownership stays with
the Cluster-backed source of truth. Adjust the design text around the
ExternalIPAttachment controller to reference the synced `api_endpoint` and
`ingress_endpoint` fields on Cluster, and remove any mention of `apiVIP` /
`ingressVIP` being consumed from ClusterOrder. Keep the wording aligned with the
Feedback controller and the ExternalIPAttachment controller to avoid implying
the wrong status contract.
---
Outside diff comments:
In `@enhancements/default-networking/design.md`:
- Around line 196-198: The split NATGateway creation path needs compensating
cleanup because the separate ExternalIP and NATGateway transaction can fail
after reserving capacity, leaking both the IP and pool usage. Update the
NATGateway provisioning flow in the design around the separate transaction so
that the ExternalIP reservation is either made atomic with NATGateway creation
or explicitly rolled back on any failure, and ensure the release path covers the
auto-provisioned resource labels and pool decrement/release logic.
In `@enhancements/vmaas-networking/design.md`:
- Line 161: The split NATGateway creation path in the design needs a
rollback/compensation plan because `NATGateway` and its auto-provisioned
`ExternalIP` are created separately and can leak pool capacity if the second
step fails. Update the `NATGateway` create flow to either make the `ExternalIP`
allocation and `NATGateway` persist atomic, or explicitly document and implement
a compensating delete/release path that cleans up the allocated IP and restores
pool capacity when the `NATGateway` write fails. Refer to the
`external_ip_mode=AUTO`/`osac.openshift.io/auto-provisioned` path and the
separate DB transaction described for the NATGateway creation.
---
Duplicate comments:
In `@enhancements/bmaas-networking/design.md`:
- Line 205: Update the `nat_gateway_mode == AUTO` design in the NATGateway
provisioning flow so reuse only happens when an existing NATGateway is in Ready
state, and treat deleting/failed gateways as non-reusable. Adjust the `AUTO`
branch description around NATGateway lookup/reuse to reference the Ready
condition explicitly, while keeping the separate ExternalIP and separate DB
transaction behavior unchanged.
🪄 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: Enterprise
Run ID: 04dc9314-b038-4793-b612-11abfcc47e2d
📒 Files selected for processing (5)
enhancements/bmaas-networking/design.mdenhancements/caas-networking/design.mdenhancements/default-networking/design.mdenhancements/unified-networking/design.mdenhancements/vmaas-networking/design.md
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured BMaaS networking enhancement with concrete APIs, clear operator responsibilities, honest dependency tracking, and comprehensive test coverage — ready for implementation pending resolution of three well-scoped open questions. Feedback: The IPAM concurrency model needs explicit treatment: when multiple BaremetalInstances allocate IPs from the same subnet concurrently in reconcileNetworking, how are races prevented (optimistic locking on Subnet CR, lease-based allocation, etc.)? This is a real operational concern for multi-tenant environments. Second, PRD risk 8.3 ('IP address feedback mechanism fails — if the fabric manager does not write the allocated IP to server status') contradicts the design which has the operator managing IPAM, not the fabric manager — the PRD risk should be updated to match the resolved design. Third, consider resolving open question #1 (Deleting NATGateway reuse) before implementation, as silently attaching to a disappearing resource will create user-facing confusion that's hard to diagnose. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: Exceptionally thorough set of five inter-related networking design documents with complete proto schemas, detailed per-service workflows, specific risks with concrete mitigations, and comprehensive test plans — a strong pass across all four dimensions. Feedback: The designs are well-structured and remarkably consistent across all five EPs. Two actionable improvements: (1) The tenant onboarding failure recovery ('delete and re-create tenant') is destructive and worth exploring a repair/retry mechanism that preserves tenant data. (2) The 'No new metrics or alerts' stance across all EPs is insufficient for a feature set this large — add at least default networking provisioning duration, auto-ExternalIP allocation rate, and pool utilization metrics to enable proactive capacity management. Critical (0)None. Important (4)
Suggestions (3)
Review costModel: claude-opus-4-6 |
Unified and default networking directories use OSAC-1433 prefix but Jira tracking links pointed to OSAC-1029. Updated to OSAC-1433 for consistency. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
AI EP Review: EP-107Score: 10/10 | Verdict: PASS
Verdict: Exceptionally well-structured PRD suite that cleanly separates a large networking initiative into five focused, individually coherent PRDs with clear user-facing requirements, concrete business justification, and testable acceptance criteria across all four OSAC personas. Feedback: This is a strong PRD suite. Minor improvement opportunities: (1) The CaaS PRD's FR-6 ('The system selects and reserves suitable bare-metal hosts...to prevent allocation conflicts') borders on describing internal orchestration — consider reframing as 'The system ensures sufficient hosts are available before cluster provisioning begins' to keep focus on the user-observable outcome. (2) Consider adding explicit 'In Scope / Out of Scope' sections to each per-service PRD to make scope boundaries even clearer, since each PRD's non-goals reference the others. (3) The BMaaS PRD's success metrics (FR-7 < 5min, NFR-2 < 2min) are excellent — consider adding similar measurable targets to the other per-service PRDs for consistency. Critical (0)None. Important (0)None. Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: Exceptionally thorough five-document networking design suite covering unified architecture, default networking, and per-service flows for VMaaS/CaaS/BMaaS — deeply technical with proto schemas, controller reconciliation patterns, IP discovery mechanisms, and comprehensive test plans across all dimensions. Feedback: The main actionable items are: (1) assign concrete proto field numbers for placeholder 'N' fields before implementation (ComputeNetworkAttachmentStatus, BareMetalNetworkAttachmentStatus) to avoid field number conflicts; (2) track the GAP items in the CaaS and BMaaS dependency tables as Jira tickets — untracked work is invisible to planning; (3) establish the dual-field migration timeline for VMaaS (OSAC-1471 is 'TBD') to give consumers a deprecation horizon. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
BMaaS PRD: "primary-traffic" → "fabric" role example in FR-2. CaaS PRD: removed k8s_manager/MetalLB implementation details from resolved OQ 9.3. Unified PRD: removed proto field name from Gap osac-project#1, removed proto jargon (oneof, compute_instance) from Gap osac-project#4. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
AI EP Review: EP-107Score: 10/10 | Verdict: PASS
Verdict: Exceptionally well-structured set of PRDs with clear user-facing needs, concrete business justification, clean separation from design, well-scoped decomposition, and fully testable acceptance criteria across all five documents. Feedback: The unified networking PRD groups provider stories under a generic 'Provider Stories' heading rather than splitting by OSAC persona (Cloud Infrastructure Admin vs Cloud Provider Admin) — align with the per-service PRDs for consistency. Consider cleaning up resolved open questions (marked with strikethrough) across all PRDs, as they add length without informational value in the published document. The CaaS PRD FR-8 mentions 'performs DNS record creation' while DNS API is listed as a non-goal — add a clarifying note that DNS remains template-based (not tenant-controlled) to avoid reviewer confusion. Critical (0)None. Important (0)None. Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: An exceptionally comprehensive family of 5 interrelated networking designs that follow all OSAC patterns, provide deep implementation detail with proto schemas and Go types, clearly delineate scope with specific non-goals and real alternatives, and include concrete test plans with measurable graduation criteria — scoring a perfect 8/8. Feedback: The designs are production-quality and ready for implementation. Two minor items worth addressing: (1) Replace placeholder proto field numbers ('= N') in ComputeInstanceStatus and BareMetalInstanceStatus with concrete field numbers to avoid ambiguity during implementation. (2) The CaaS and BMaaS dependency tables list several items as 'Not tracked / GAP' — consider creating Jira tickets for these before merge to avoid implementation blind spots (HostType NetworkInterface fields, agent selection logic in operator, mutateBMI network_attachments copy, etc.). Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
…uture BMaaS design: reverted to HostType for interface validation (v0.2). Added "Future: BareMetalInstanceType Integration" section documenting the migration plan when PR osac-project#119 lands with enhanced network_ports. HostType is the system-level resource that exists today and works for both CaaS and BMaaS. BareMetalInstanceType will be the tenant-facing catalog once it's available. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
AI EP Review: EP-107Score: 9/10 | Verdict: PASS
Verdict: A strong set of PRDs with clear user-facing capabilities, concrete justification, and verifiable acceptance criteria across all five documents. The only weakness is that the PR bundles five independent PRDs (unified, default, VMaaS, CaaS, BMaaS) that could be reviewed and shipped as separate PRs. Feedback: Consider splitting the per-service PRDs (VMaaS, CaaS, BMaaS) into separate PRs to simplify review and allow independent prioritization — each is self-contained with its own user stories and acceptance criteria. The unified networking and default networking PRDs could remain together as they form the foundation. The unified networking PRD's requirements section (Section 4.1) lost its per-requirement acceptance criteria during the restructuring from the old format — adding inline testability markers (e.g., 'verified by...') to each FR would strengthen traceability between requirements and the acceptance criteria in Section 5. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: Exceptionally thorough design suite covering unified networking across VMaaS, CaaS, and BMaaS with consistent architectural patterns, detailed proto schemas, comprehensive failure handling, and concrete test plans — minor gaps around unspecified field number placeholders and missing observability metrics do not materially weaken the design. Feedback: Three areas to tighten before implementation: (1) Replace all '= N' proto field number placeholders with actual assigned numbers in ComputeNetworkAttachmentStatus, BareMetalNetworkAttachmentStatus, and ClusterStatus messages — implementers need concrete field assignments. (2) Specify the retry count and backoff strategy for 'permanent failure' during auto-provisioned resource cleanup instead of 'after N retries' — this is a production-critical parameter that affects orphan cleanup behavior. (3) All five designs state 'No new metrics or alerts' despite introducing significant new failure modes (pool exhaustion, default networking provisioning failure, cleanup orphans); add at least pool capacity gauge metrics and default networking provisioning failure counters to enable operational alerting. Critical (0)None. Important (4)
Suggestions (3)
Review costModel: claude-opus-4-6 |
…on non-goal Default design: clarified that NATGateway creation also fails without ExternalIPPool (not just auto external access). Default PRD: added non-goal about existing tenant migration matching the design's upgrade section. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
AI EP Review: EP-107Score: 9/10 | Verdict: PASS
Verdict: A strong, well-structured set of PRDs that clearly define user-facing networking capabilities across VMaaS, CaaS, and BMaaS with concrete justification and testable requirements; minor design leakage in the unified networking PRD's acceptance criteria prevents a perfect score. Feedback: The unified networking PRD's acceptance criteria should be rewritten to describe user-observable outcomes rather than architectural properties. Replace 'A single networking backend handles all physical networking operations' with something a PM could verify (e.g., 'A provider can add a new networking backend by deploying configuration without modifying the API or operator code'). Similarly, 'VM networking is integrated into the same networking layer' should describe the user-visible consequence (e.g., 'A VM and a bare-metal server on the same subnet can communicate at their subnet IPs'). The per-service PRDs are exemplary — apply the same discipline to the unified networking PRD's acceptance criteria. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-107Score: 8/8 | Verdict: PASS
Verdict: Exceptionally thorough design submission covering five EPs (unified + per-service + default networking) with deep technical detail, consistent OSAC patterns, comprehensive test plans, and well-defined scope boundaries — one of the strongest design submissions in terms of completeness and cross-component coordination. Feedback: Track all 'GAP' dependency items in Jira (CaaS has 5 untracked items, BMaaS has 6) — these represent real implementation work that should be visible in project planning. The default networking EP's failure recovery strategy ('delete and re-create tenant') is a blunt instrument; consider adding a reconciliation retry mechanism or partial re-creation flow so admins don't lose tenant state on transient failures. Check whether matching temp-api files exist in osac-ux for affected resources — if they do, a UX Alignment section with field-by-field mapping is required by the design template. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
|
@danmanor Thank you for the clarification :) |
|
/lgtm |
networking/README.md's superseded-by field was N/A despite unified-networking/README.md already declaring replaces: /enhancements/networking. Points at the current unified-networking path (pre-restructure) rather than a post-osac-project#107/post-merge path, since that work hasn't landed yet. Signed-off-by: Tommy Hughes <tohughes@redhat.com>
|
@aminhussainbarbhuiya17-art: changing LGTM is restricted to collaborators 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aminhussainbarbhuiya17-art, danmanor 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 |
8 participants
Summary
Changes
Unified networking EP/PRD (restructured):
ComputeNetworkAttachmentStatusandBareMetalNetworkAttachmentStatusprotos for IP visibilityComputeNetworkAttachment,BareMetalNetworkAttachment,ClusterNetworkAttachmentauto-provisioned-forlabelVMaaS networking (new):
ComputeNetworkAttachmentwithprimaryfield for multi-NIC (field 18 on ComputeInstanceSpec)ComputeNetworkAttachmentStatuswithip_addressfor IP visibility (from KubeVirt VMI feedback)compute_network_attachmentswith tenant defaults; dual-field migration from old field 14compute_network_attachment_statusesIP (not VirtualMachineReference)CaaS networking (new):
ClusterNetworkAttachmentwithsubnet+security_groupsonly (field 9 on ClusterSpec)fabricInterfaceresolved per node set from HostType, stored on node set definition (NOT on attachment)BMaaS networking (new):
BareMetalNetworkAttachmentwith tenant-specifiedinterface+primary(field 8 on BaremetalInstanceSpec)BareMetalNetworkAttachmentStatuswithip_address(field name:network_attachment_statuses)osac.openshift.io/auto-provisionedandauto-provisioned-forfabricfrom HostType (when attachments omitted)Default networking (new):
DefaultsNotConfiguredwhen no defaults configured (tenant still becomes READY)Key architectural decisions
ComputeNetworkAttachmentStatus; BMaaS:BareMetalNetworkAttachmentStatus; CaaS:api_endpoint/ingress_endpointauto-provisioned-forlabel for orphan detectionOpen questions (unresolved)
Jira