OSAC-1382: Design - Multi-Fabric East-West Networking - #179
Conversation
|
@vladikr: This pull request references OSAC-1382 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. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 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 design introduces an independent, tenant-scoped ChangesFabricDomain east-west networking
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant FulfillmentService
participant OperatorController
participant AAP
participant Netris
Admin->>FulfillmentService: Create FabricDomain
FulfillmentService->>OperatorController: Reconcile FabricDomain
OperatorController->>AAP: Start provisioning
AAP->>Netris: Create or update Server Cluster
Netris-->>AAP: Return provisioning status
AAP-->>OperatorController: Report status
OperatorController-->>FulfillmentService: Persist FabricDomain status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
AI Design Review: EP-179Score: 6/8 | Verdict: PASS
Verdict: A technically strong design that extends VirtualNetwork with fabric bindings using sound OSAC patterns and deep implementation detail, but lacks coverage of the Installation and Documentation dimensions and has vague graduation criteria. Feedback: Two actionable improvements would strengthen this design: (1) Add an Installation section addressing osac-installer changes — the new CRD fields (fabricBindings on VirtualNetwork) must flow through Helm charts/Kustomize overlays, and per AGENTS.md, failing to update osac-installer after cross-component changes causes CI failures. (2) Replace the graduation criteria with measurable conditions (e.g., 'Tech Preview: all CRUD operations pass E2E on netris-lab, error paths tested, VNet coexistence validated; GA: production deployment with N tenants, no regressions in existing networking tests'). Also align the proto FabricBindingStatus fields with the CRD Go struct — the ProvisioningJobs field appears in the CRD but not the proto, and the message field is in the proto but not the CRD. Critical (0)None. Important (5)
Suggestions (3)
Review costModel: claude-opus-4-6 |
danmanor
left a comment
danmanor
left a comment
There was a problem hiding this comment.
Design review — four questions about extensibility and scope beyond Phase 1.
| // Fabric-manager-specific template identifier. | ||
| string template_id = 3; | ||
| } | ||
|
|
There was a problem hiding this comment.
Why is fabric_bindings on VirtualNetwork instead of Subnet?
In OSAC's model, servers attach to Subnets (via network_attachments), not directly to VirtualNetworks. A tenant may have one VN with multiple Subnets where only some need east/west connectivity:
VirtualNetwork "my-vn" (10.0.0.0/16)
├── Subnet "gpu-training" (10.0.1.0/24) ← needs east/west
├── Subnet "web-services" (10.0.2.0/24) ← does NOT need east/west
└── Subnet "storage" (10.0.3.0/24) ← different east/west need
With bindings at the VN level, you can't scope east/west to a specific Subnet — the servers list is the only scoping mechanism, but it's not tied to which Subnet those servers are in.
I understand this maps to how Netris Server Clusters work today (scoped to a VPC). But is this the right abstraction level for Phase 2+ when different Subnets may need different fabric treatment?
There was a problem hiding this comment.
You are right, and that's tricky. I thought to go with a simple model first and then creating a separate resource.
Regarding subnets... I agree that going forward we need a separation, but Subnets themselves are just addresses inside an isolation domain, right? So there can be multiple attached to one isolation domain.
In terms of ownership, I didn't think that a deletion of a subnet should lead to the destruction of an isolation domain...
| repeated FabricBindingStatus fabric_binding_statuses = 4; | ||
| } | ||
|
|
||
| message FabricBindingStatus { |
There was a problem hiding this comment.
How will the tenant know what template_id to use?
This is a raw Netris Server Cluster Template ID. The tenant has no way to discover available templates or understand what they mean. Even for admin-only Phase 1, this creates a dependency on out-of-band knowledge of the Netris Controller's inventory.
Should this be derived from the NetworkClass configuration instead? The admin could configure the template once on the NetworkClass ("when ethernet_ew is requested, use template X"), and the fulfillment-service would resolve it automatically — similar to how implementationStrategy is resolved from NetworkClass today. That would also make default_fabric_bindings cleaner (no need to repeat template_id in defaults).
There was a problem hiding this comment.
Yes, I agree. You're right, it's in the wrong place and should be in the NetworkClass :/
I'll rework that.
| When InfiniBand and NVLink bindings are introduced, this is insufficient. IB requires servers with HCAs visible to UFM; NVLink requires GPUs in the correct NVLink domain (NVL72/NVL144). Handing a server list with missing hardware to UFM or NMX results in either silent partial failures or active domains with only a subset of requested servers — unacceptable for expensive GPU infrastructure. | ||
|
|
||
| The validation model for Phase 2+: | ||
|
|
There was a problem hiding this comment.
Is this generic enough to work with a non-Netris fabric manager?
The type and servers fields are fabric-manager-agnostic, but template_id leaks Netris semantics. For other fabric managers:
| Fabric Manager | What template_id maps to |
Needed? |
|---|---|---|
| Netris | Server Cluster Template ID | Yes |
| UFM (InfiniBand) | Nothing — P_Key partitions don't use templates | No |
| Neutron | Network profile? | Maybe |
Would a map<string, string> parameters field (or deriving config from NetworkClass) be more future-proof than a Netris-specific template_id? The Fabric Manager Capability Contract section defines a generic interface, but the proto shape doesn't fully match that genericity.
| | Partial binding success (A ok, B fail) | VN Failed; A shows Ready, B shows Failed | Fix B; A is not re-provisioned | | ||
| | Delete binding job fails | VN stuck in Deleting | Manually delete from fabric manager, remove finalizer | | ||
|
|
||
| All AAP jobs are idempotent. |
There was a problem hiding this comment.
How does this model fit separate east/west use cases like storage?
A GPU server often has three distinct fabrics: north/south Ethernet, east/west GPU (IB or RoCE), and east/west storage (NVMe-oF). GPU and storage east/west are different isolation domains on different physical networks with potentially different tenancy boundaries.
With bindings on VN, you'd model this as:
fabric_bindings:
- type: "ethernet_ew" # GPU
servers: ["hgx-00"]
- type: "storage_ew" # Storage
servers: ["hgx-00"]This forces both to share the same VN lifecycle — you can't independently manage the storage domain without touching the GPU VN, and different teams can't own them separately. A standalone resource (like Nebius's GpuCluster or Crusoe's ib-partition) would decouple these lifecycles.
Is this an acceptable trade-off for Phase 1, or should the design acknowledge this as a known limitation that may require factoring out a dedicated resource in Phase 2+?
There was a problem hiding this comment.
yes, I don't think it's enough for phase 2+, I was just worried that no matter what we'll design now may not 100% fit..
I was debating with myself whether we should introduce a separate EastWestDomain resource now or do phase 1 and then.
Let me think about this again and see if I can propose a different approach.
![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/OSAC-1382-multi-fabric-east-west-networking/design.md`:
- Around line 122-132: Update FabricBinding admission validation and the
corresponding CRD schema so ethernet_ew requires a valid Netris Server Cluster
template_id, rejects missing or invalid values, and enforces immutability after
creation. Apply the same validation to the referenced FabricBinding definitions
and add tests covering missing, invalid, and changed template_id values.
- Around line 134-154: Align the VirtualNetworkStatus and CRD binding-status
contracts by defining one canonical binding status model and an explicit
conversion between the proto FabricBindingStatus and CRD status fields.
Reconcile type, phase, message, backend ID, and provisioningJobs so no status
information is silently lost, constrain phase values to the shared lifecycle,
and add a DELETING phase or explicitly define when binding status is removed.
- Around line 215-242: Extend the VirtualNetwork status model and provisioning
flow around ComputeDesiredConfigVersion to persist each fabric binding’s
last-applied configuration hash keyed by a stable binding identity. Use this
durable per-binding state to selectively detect updates across reconciles and
restarts, while correctly handling server changes, additions, removals, and
reordered bindings without treating unchanged bindings as modified.
- Around line 157-170: Define supports_east_west_ethernet as a derived
capability on NetworkClass, inferred from assigned fabric-manager metadata and
published through status.capabilities rather than relying on the Pydantic
default or manual values. Apply the existing disableCapabilities behavior during
derivation, and update VN validation to consume the published derived
capability. Align the implementation with the established unified-networking
capability flow.
- Around line 261-267: Update the fenced code block containing the VPC and
server-cluster API sequence to specify the text language, using ```text as the
opening fence while preserving the existing sequence unchanged.
- Around line 52-56: Make the supports_east_west_ethernet capability rename
backward-compatible by retaining supports_east_west as a deprecated alias or
adding an explicit migration across AAP metadata, the operator, and
fulfillment-service. Add mixed-version coverage for old and new capability keys,
and document the required upgrade order for AAP, operator, and
fulfillment-service independently of the fabricBindings skew strategy.
- Around line 293-297: Update the fabric_bindings authorization design to
require admin-only access for both servers and template_id on API and CRD paths,
rather than relying on VirtualNetwork tenant/OPA scoping. Add validation against
authoritative inventory to confirm selected servers are owned or assigned
appropriately before provisioning.
- Around line 94-96: Update the Tenant Onboarding via NetworkDefaults design so
default_fabric_bindings cannot reuse concrete provider-level servers across
tenants. Define a tenant-scoped binding profile with per-tenant server
resolution, or explicitly disable automatic defaults until tenant-scoped
allocation exists, and document an explicit opt-out path when defaults are
configured.
🪄 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: c70f6ae0-c765-423c-8550-91e7288ec286
📒 Files selected for processing (1)
enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md
| message VirtualNetworkStatus { | ||
| // ... existing fields 1-3 ... | ||
|
|
||
| // Per-binding provisioning status. | ||
| repeated FabricBindingStatus fabric_binding_statuses = 4; | ||
| } | ||
|
|
||
| message FabricBindingStatus { | ||
| string type = 1; | ||
| FabricBindingPhase phase = 2; | ||
| optional string message = 3; | ||
| string backend_id = 4; | ||
| } | ||
|
|
||
| enum FabricBindingPhase { | ||
| FABRIC_BINDING_PHASE_UNSPECIFIED = 0; | ||
| FABRIC_BINDING_PHASE_PENDING = 1; | ||
| FABRIC_BINDING_PHASE_PROVISIONING = 2; | ||
| FABRIC_BINDING_PHASE_READY = 3; | ||
| FABRIC_BINDING_PHASE_FAILED = 4; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Align the proto and CRD binding-status contracts.
The proto status contains type, enum phase, message, and backend_id. The CRD status contains type, string phase, provisioningJobs, and backendId. No conversion is defined.
provisioningJobs cannot be represented by the proto. message cannot be represented by the CRD. The free-form CRD phase can diverge from the proto enum. The proto enum also lacks DELETING, although deletion is a supported lifecycle.
Define one canonical status model and an explicit conversion. Add a deletion phase or document when binding status is removed.
Also applies to: 187-197
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around
lines 134 - 154, Align the VirtualNetworkStatus and CRD binding-status contracts
by defining one canonical binding status model and an explicit conversion
between the proto FabricBindingStatus and CRD status fields. Reconcile type,
phase, message, backend ID, and provisioningJobs so no status information is
silently lost, constrain phase values to the shared lifecycle, and add a
DELETING phase or explicitly define when binding status is removed.
| #### Operator Two-Phase Provisioning | ||
|
|
||
| The `VirtualNetworkReconciler.handleProvisioning` is extended: | ||
|
|
||
| **Phase 1 (existing):** Standard VN provisioning fires `osac-create-virtual-network` (creates VPC). Tracked via `status.provisioningJobs`. | ||
|
|
||
| **Phase 2 (new):** For each `spec.fabricBindings` entry, the operator looks up the binding type in a constant map: | ||
|
|
||
| ```go | ||
| var fabricBindingTemplates = map[string]struct { | ||
| createTemplate string | ||
| deleteTemplate string | ||
| }{ | ||
| "ethernet_ew": { | ||
| createTemplate: "osac-create-server-cluster", | ||
| deleteTemplate: "osac-delete-server-cluster", | ||
| }, | ||
| } | ||
| ``` | ||
|
|
||
| It constructs a server_cluster payload from the VN metadata + binding fields and fires the AAP job using a dedicated `AAPProvider` with explicit template names. Each binding's job is tracked independently in `fabricBindingStatuses`. | ||
|
|
||
| VN Phase = Ready only when ALL jobs (standard + all bindings) succeed. | ||
|
|
||
| #### Spec Change Detection (Resize) | ||
|
|
||
| `fabric_bindings` is included in the spec hash computed by `ComputeDesiredConfigVersion`. When an admin updates `fabricBindings[0].servers`, the hash changes, and the operator re-triggers provisioning for the changed binding only. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Persist per-binding desired state for selective resize.
ComputeDesiredConfigVersion is a whole-VN hash. The status model has no per-binding configuration hash or stable binding identity. After changing one of multiple bindings, or after a controller restart, the operator cannot determine which binding changed from this contract.
Store the last-applied hash keyed by binding identity, or define an equivalent durable comparison mechanism. Test server updates, additions, removals, and list reordering.
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around
lines 215 - 242, Extend the VirtualNetwork status model and provisioning flow
around ComputeDesiredConfigVersion to persist each fabric binding’s last-applied
configuration hash keyed by a stable binding identity. Use this durable
per-binding state to selectively detect updates across reconciles and restarts,
while correctly handling server changes, additions, removals, and reordered
bindings without treating unchanged bindings as modified.
| #### Phase 1 Limitations | ||
|
|
||
| - **No server eligibility validation.** The admin is trusted to specify servers that have the correct NICs for the fabric binding. There is no check against HostType interfaces or Netris inventory. If servers lack the required NICs, the Server Cluster activates but VNet ports remain inactive. | ||
| - **Server list membership is not restricted** beyond existing VirtualNetwork tenancy. The admin is trusted to supply correct hostnames for the binding type. | ||
| - **`template_id` is Netris-specific.** It references a Netris Server Cluster Template by ID. Future phases will replace this with a more abstract profile or capability selector once multiple fabric managers are supported. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
'fabricBindings|fabric_bindings|templateId|template_id|osac.openshift.io/tenant|Role|ClusterRole|OPA' \
--glob '*.go' \
--glob '*.yaml' \
--glob '*.yml' \
--glob '*.proto'Repository: osac-project/enhancement-proposals
Length of output: 172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,200p'
echo "== design file snippet =="
if [ -f enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md ]; then
wc -l enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md
sed -n '240,410p' enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md | nl -ba -v240
fi
echo "== related searches =="
rg -n -C 3 \
'fabricBindings|fabric_bindings|templateId|template_id|PhysicalServer|HostType|Netris|Netris inventory|virtualnetwork|VirtualNetwork|tenancy|admin-only|admin|RBAC|OPA|Role|ClusterRole|validate|authorization|tenant' \
. || trueRepository: osac-project/enhancement-proposals
Length of output: 4599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status/stat =="
git status --short
git diff --stat || true
echo "== tracked design files =="
git ls-files | grep -F 'enhancements/OSAC-1382-multi-fabric-east-west-networking' || trueRepository: osac-project/enhancement-proposals
Length of output: 352
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== design.md relevant sections =="
sed -n '280,390p' enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md
echo "== prd.md relevant searches =="
rg -n -C 4 \
'fabricBindings|fabric_bindings|templateId|template_id|PhysicalServer|Netris|VirtualNetwork|tenant|admin|authorization|RBAC|OPA|role|cluster role|permissions|servers' \
enhancements/OSAC-1382-multi-fabric-east-west-networking/prd.md || true
echo "== design.md mentions =="
rg -n -C 4 \
'fabricBindings|fabric_bindings|templateId|template_id|PhysicalServer|Netris|VirtualNetwork|tenant|admin|authorization|RBAC|OPA|role|cluster role|permissions|servers' \
enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md || trueRepository: osac-project/enhancement-proposals
Length of output: 41328
Enforce authorization for servers and template_id.
fabric_bindings is proposed as admin-only, but the RBAC section says the field only inherits VirtualNetwork tenant/OPA scoping. Tenant scoping does not authorize selecting physical server inventory or Netris templates. Add explicit admin-only authorization on both API and CRD paths, and validate server ownership/assignment against authoritative inventory before provisioning.
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around
lines 293 - 297, Update the fabric_bindings authorization design to require
admin-only access for both servers and template_id on API and CRD paths, rather
than relying on VirtualNetwork tenant/OPA scoping. Add validation against
authoritative inventory to confirm selected servers are owned or assigned
appropriately before provisioning.
d69d7de to
d6d3a03
Compare
AI Design Review: EP-179Score: 5/8 | Verdict: PASS
Verdict: The design is technically sound and follows established OSAC patterns with strong feasibility, but scores are pulled down by spec/status ownership violations, a phase enum on a new resource (should prefer conditions), thin integration test details, vague graduation criteria, and unaddressed cross-cutting dimensions (Installation, Documentation, UI deferral). Feedback: Three concrete improvements to strengthen this design: (1) Move Critical (0)None. Important (6)
Suggestions (4)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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-1382-multi-fabric-east-west-networking/design.md`:
- Around line 89-91: Define the VPC cardinality and ownership rules for
FabricDomain.virtual_networks: specify behavior for zero associations,
deterministic mapping for multiple associations, and ownership of the single VPC
recorded in status.vpc_id. Update the “With VirtualNetwork Association” design
and related sections to enforce one VN or explicitly model per-VN resources, and
add coverage for zero, one, multiple, and removed associations.
- Around line 227-237: Define the resolved template transport field in
FabricDomainSpec and carry it consistently through the proto, CRD, and AAP
payload mapping. Keep it write-protected from user configuration, populate it
from NetworkClass during fulfillment, and update
playbook_osac_create_fabric_domain.yml to consume the declared field when
invoking create_server_cluster.
- Around line 288-293: Update the code fence containing the Netris API workflow
near the POST /api/v2/vpc and POST /api/v2/server-cluster lines to specify the
text language, changing the opening fence to ```text while preserving the block
contents.
- Around line 101-103: Define the canonical
NetworkDefaults.default_fabric_domains schema and document how each entry
expands into a tenant-scoped FabricDomainSpec, including required servers,
networkClass, and tenant-scoped virtualNetworks. Ensure the OSAC-2341 onboarding
path can populate all required fields before describing automatic FabricDomain
creation as supported.
- Around line 93-99: Update the Resize and Deletion sections to define the AAP
idempotency key as a stable value derived from the immutable FabricDomain
identity and tenant, and specify that FabricDomainStatus.backend_id is persisted
and reused across retries. Document retry-after-success behavior, ensure renames
do not change the key or target existing cluster, and state how the persisted
backend_id is handled during deletion.
🪄 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: 708ee7e4-2926-4e37-af47-5c60af0b396d
📒 Files selected for processing (1)
enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md
53aec02 to
a22e7ff
Compare
AI Design Review: EP-179Score: 5/8 | Verdict: PASS
Verdict: The design is a solid, pattern-following enhancement proposal that passes on the strength of its detailed proto schemas, concrete validation rules, specific risk mitigations, and thorough alternatives analysis, but is held back by spec/status ownership issues, missing conditions-based lifecycle, thin graduation criteria, and unaddressed cross-cutting dimensions (Installation, Documentation). Feedback: Three high-impact improvements: (1) Move implementation_strategy from Spec to Status and use Conditions (not a phase enum) as the primary lifecycle indicator for this new resource — this aligns with the stated preference for conditions on new resources and fixes the spec/status ownership issue. (2) Add measurable graduation criteria (e.g., 'all CRUD operations pass e2e with <2% flake rate, error paths tested, no regressions in existing networking tests') instead of bare stage names. (3) Address the Installation dimension — specify what Helm chart changes osac-installer needs for the new FabricDomain CRD, and add a Terminology section defining 'Server Cluster', 'VPC', and 'east-west' upfront to prevent reviewer confusion. Critical (0)None. Important (5)
Suggestions (4)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md (3)
167-190: 🗄️ Data Integrity & Integration | 🟠 MajorDefine capability derivation from fabric-manager metadata.
The design declares
supports_east_west_ethernetbut does not define how the value is derived from assigned manager metadata. Validation can therefore consume a manually set or stale capability. Define the derivation path anddisableCapabilitiesbehavior. Test capability changes and validation decisions.Based on the supplied upstream unified-networking contract, NetworkClass capabilities are inferred from assigned manager metadata and published through status.
Also applies to: 217-225
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around lines 167 - 190, Define the derivation of NetworkClassCapabilities.supports_east_west_ethernet from assigned fabric-manager metadata, rather than allowing it to be manually or stale-set. Specify how disableCapabilities affects the derived and published capability, ensure status exposes the resulting value, and add tests covering metadata-driven capability changes and the corresponding validation decisions.
104-110: 🗄️ Data Integrity & Integration | 🟠 MajorPersist a stable provisioning identity and generation.
Resize relies on
desiredConfigVersion, but the proto, CRD, and database sections do not define its source or persistence. The AAP role finds clusters by name, but the design does not define a stable name or idempotency key. Concurrent updates and retry-after-success can target the wrong cluster or accept a stale callback.Persist an immutable FabricDomain-derived key,
backend_id, and desired generation. Reject stale callbacks. Cover retries, renames, and overlapping updates.Also applies to: 232-236, 254-262, 316-325
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around lines 104 - 110, Define in the design a stable immutable FabricDomain-derived provisioning identity (`backend_id` or equivalent), its source, and persistence across the proto, CRD, and database; use it as the Netris/AAP idempotency and lookup key rather than a mutable name. Specify how desired generations are persisted, incremented, associated with jobs, and validated so stale or duplicate callbacks are rejected, including retry-after-success, renames, and overlapping updates.
112-114: 🗄️ Data Integrity & Integration | 🟠 MajorDefine tenant-scoped expansion for default FabricDomains.
NetworkDefaults.default_fabric_domainsis referenced without a schema or expansion rule. The design does not define how requiredservers,network_class, andvirtual_networksvalues are resolved per tenant. Concrete server lists could be reused across tenants.Define a tenant-scoped profile with per-tenant server resolution, or disable automatic defaults until allocation exists. Add an explicit opt-out path.
Also applies to: 177-180, 381-397
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around lines 112 - 114, Define the schema and tenant-scoped expansion rules for NetworkDefaults.default_fabric_domains, including how servers, network_class, and virtual_networks are resolved for each tenant without reusing concrete server lists. Specify the required allocation/profile mechanism, or disable automatic defaults until that mechanism exists, and document an explicit tenant opt-out path in the related sections.
🤖 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-1382-multi-fabric-east-west-networking/design.md`:
- Line 33: Update the “FabricDomain ↔ VirtualNetwork Relationship” heading in
the design document to use the consecutive ### level after the preceding ##
heading, unless an intermediate section is intentionally added.
- Around line 193-202: Update FabricDomainSpec and its
fulfillment-service/operator mapping to explicitly handle the resolved
template_id and implementation_strategy values end to end. Ensure these fields
are derived from the proto or resolved only by the operator, and reject or
ignore user-provided overrides rather than exposing writable derived
configuration in the CRD.
- Around line 281-295: Update the Phase 1 authorization design and RBAC sections
to require explicit Cloud Infrastructure Admin authorization for physical server
assignments and NetworkClass/template selection on both API and CRD paths. Add
authoritative inventory validation for server ownership or assignment before
dispatch, and replace the statement that any admin may assign any server;
preserve tenant scoping for virtual network resources.
- Around line 213-225: Expand NC-VAL-09 and the associated admission/dispatch
validation to require a valid ethernet_ew template_id before dispatch, including
missing, malformed, and nonexistent template references. Define whether
template_id is immutable after creation or requires explicit versioning, then
document and test missing, invalid, and changed values so invalid configurations
cannot reach AAP.
- Around line 63-67: Update the capability rename described in the design so
version-skew compatibility is explicit: retain a deprecated supports_east_west
alias or define a migration to supports_east_west_ethernet, covering AAP
metadata and older consumers in addition to fulfillment-service and
osac-operator. Add mixed-version tests and document the required AAP,
fulfillment-service, and operator upgrade order, while preserving the
additive-change claim only if the compatibility path supports it.
- Around line 35-42: Define deterministic VPC mapping for
FabricDomain–VirtualNetwork associations across the design’s API, provisioning,
and status sections: specify behavior for zero, one, multiple, added, and
removed VNs, including ownership and cleanup of VPCs created without a VN and
migration semantics when associations change. Prefer enforcing a single-VN rule
unless shared-VPC ownership and destructive migration are explicitly defined,
and add tests covering every association mode.
- Around line 149-162: Define a single canonical lifecycle-status contract
across the FabricDomain proto, CRD status, feedback synchronization, and
workflow states. Extend FabricDomainState to cover progressing and deleting,
then document explicit proto-to-CRD mappings for state/phase, messages,
provisioningJobs, conditions, and backend identifiers across the referenced
status sections.
- Around line 263-295: Define FabricDomain server-assignment semantics for
active ethernet_ew domains by rejecting any server already assigned to another
active domain unless authoritative fabric-manager inventory explicitly supports
multi-attachment. Update validation for both create and update paths, and add
conflict tests covering overlapping servers across active FabricDomains.
---
Duplicate comments:
In `@enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md`:
- Around line 167-190: Define the derivation of
NetworkClassCapabilities.supports_east_west_ethernet from assigned
fabric-manager metadata, rather than allowing it to be manually or stale-set.
Specify how disableCapabilities affects the derived and published capability,
ensure status exposes the resulting value, and add tests covering
metadata-driven capability changes and the corresponding validation decisions.
- Around line 104-110: Define in the design a stable immutable
FabricDomain-derived provisioning identity (`backend_id` or equivalent), its
source, and persistence across the proto, CRD, and database; use it as the
Netris/AAP idempotency and lookup key rather than a mutable name. Specify how
desired generations are persisted, incremented, associated with jobs, and
validated so stale or duplicate callbacks are rejected, including
retry-after-success, renames, and overlapping updates.
- Around line 112-114: Define the schema and tenant-scoped expansion rules for
NetworkDefaults.default_fabric_domains, including how servers, network_class,
and virtual_networks are resolved for each tenant without reusing concrete
server lists. Specify the required allocation/profile mechanism, or disable
automatic defaults until that mechanism exists, and document an explicit tenant
opt-out path in the related sections.
🪄 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: f11a1570-921a-4b49-9880-4808d52adb76
📒 Files selected for processing (1)
enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md
| #### Multiple FabricDomains | ||
|
|
||
| A tenant can have multiple FabricDomains with independent lifecycles: | ||
|
|
||
| ```yaml | ||
| # GPU east-west | ||
| - name: tenant-a-gpu-ew | ||
| type: ethernet_ew | ||
| servers: ["hgx-00", "hgx-01", "hgx-02", "hgx-03"] | ||
|
|
||
| # Storage east-west (different servers, same tenant) | ||
| - name: tenant-a-storage-ew | ||
| type: ethernet_ew | ||
| servers: ["storage-00", "storage-01"] | ||
| ``` | ||
|
|
||
| Each has independent create/resize/delete lifecycle. In Phase 2, a tenant might have `ethernet_ew` + `infiniband_ew` FabricDomains — separate resources with separate controllers. | ||
|
|
||
| #### Phase 1 Limitations | ||
|
|
||
| - **No server eligibility validation.** Admin is trusted to supply correct hostnames. Server list membership is not restricted beyond existing tenant scoping — any Cloud Infrastructure Admin can assign any server to a FabricDomain. Phase 2+ will add HostType-based validation. | ||
| - **No automatic server pool assignment.** Explicit server lists only. | ||
| - **Depends on pre-existing Server Cluster Templates** on the Netris Controller. | ||
| - **`template_id` resolved from NetworkClass** — still Netris-specific in Phase 1, but scoped to NC config rather than appearing on every FabricDomain. | ||
| - **VPC-Server Cluster VNet coexistence validated** on zeus12 — no conflicts. | ||
|
|
||
| #### Virtual Clusters and VMaaS | ||
|
|
||
| Phase 1 targets bare-metal servers whose NICs are directly attached to the fabric. VMs need SR-IOV for high-performance EW. The FabricDomain model extends naturally via new `type` values (e.g., `ethernet_ew_sriov`). Basic VM EW isolation (without RDMA) works today via CUDN (OSAC-1589). | ||
|
|
||
| #### Hardware Eligibility Validation (Phase 2+) | ||
|
|
||
| Phase 1 relies on admin knowledge + Netris discovery. Phase 2+ adds HostType-based validation and fabric manager authoritative checks. The FabricDomain API shape does not change — only admission validation becomes stricter. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files matching design/security/fabric =="
git ls-files | rg -i '(^|/)design\.md$|fabric|fabricdomain|multifabric|netris|OSAC-1382|enhancements' | head -200
echo
echo "== target file excerpt =="
file="enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md"
if [ -f "$file" ]; then
nl -ba "$file" | sed -n '1,380p'
else
fd -a 'design.md' . | sed -n '1,50p'
fi
echo
echo "== search server eligibility/overlap validation terms =="
rg -n -i "multiple fabricdomains|FabricDomains|fabricdomain|fabric manager|multi-domain|overlapping|share|server list|servers|eligibility|multi-attachment|SR-IOV|infiniband|ethernet_ew|validation" enhancements . --glob '!vendor' --glob '!node_modules' --glob '!dist' --glob '!build' | head -300Repository: osac-project/enhancement-proposals
Length of output: 3996
🏁 Script executed:
#!/bin/sh
set -euo pipefail
file="enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md"
echo "== target file excerpt (1-420) =="
awk '{ printf "%6d\t%s\n", NR, $0 }' "$file" | sed -n '1,420p'
echo
echo "== target file excerpt (420-800) =="
awk '{ printf "%6d\t%s\n", NR, $0 }' "$file" | sed -n '420,800p'
echo
echo "== search FabricDomain/multi-domain/overlap terms in enhancements =="
rg -n -i "Multiple FabricDomains|FabricDomains|fabricdomain|FabricDomain|fabric-manager|fabric manager|multi-domain|multi-attachment|overlap|server eligibility|Server Cluster Templates|HostType|Server Cluster|ethernet_ew|infiniband_ew|SR-IOV|VPC-Server Cluster" enhancements/OSAC-1382-multi-fabric-east-west-networking enhancements --glob '*.md' | head -400Repository: osac-project/enhancement-proposals
Length of output: 50393
Define exclusive server assignment semantics.
FabricDomain validation currently requires only a non-empty servers list, and multiple FabricDomains are independent. A server can therefore be advertised in multiple active ethernet_ew Server Clusters unless the spec defines supported multi-attachment. Reject overlapping servers by default, or explicitly support cross-cluster multi-attachment and enforce it against authoritative fabric-manager inventory. Add create and update conflict tests for overlaps across active domains.
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around
lines 263 - 295, Define FabricDomain server-assignment semantics for active
ethernet_ew domains by rejecting any server already assigned to another active
domain unless authoritative fabric-manager inventory explicitly supports
multi-attachment. Update validation for both create and update paths, and add
conflict tests covering overlapping servers across active FabricDomains.
| #### Phase 1 Limitations | ||
|
|
||
| - **No server eligibility validation.** Admin is trusted to supply correct hostnames. Server list membership is not restricted beyond existing tenant scoping — any Cloud Infrastructure Admin can assign any server to a FabricDomain. Phase 2+ will add HostType-based validation. | ||
| - **No automatic server pool assignment.** Explicit server lists only. | ||
| - **Depends on pre-existing Server Cluster Templates** on the Netris Controller. | ||
| - **`template_id` resolved from NetworkClass** — still Netris-specific in Phase 1, but scoped to NC config rather than appearing on every FabricDomain. | ||
| - **VPC-Server Cluster VNet coexistence validated** on zeus12 — no conflicts. | ||
|
|
||
| #### Virtual Clusters and VMaaS | ||
|
|
||
| Phase 1 targets bare-metal servers whose NICs are directly attached to the fabric. VMs need SR-IOV for high-performance EW. The FabricDomain model extends naturally via new `type` values (e.g., `ethernet_ew_sriov`). Basic VM EW isolation (without RDMA) works today via CUDN (OSAC-1589). | ||
|
|
||
| #### Hardware Eligibility Validation (Phase 2+) | ||
|
|
||
| Phase 1 relies on admin knowledge + Netris discovery. Phase 2+ adds HostType-based validation and fabric manager authoritative checks. The FabricDomain API shape does not change — only admission validation becomes stricter. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
Enforce authorization for physical servers and fabric configuration.
The Phase 1 limitation permits any Cloud Infrastructure Admin to assign any server. The RBAC section only repeats VirtualNetwork tenant scoping. Tenant scoping does not authorize physical inventory claims or NetworkClass template selection.
Require explicit admin authorization on API and CRD paths. Validate server ownership or assignment against authoritative inventory before dispatch.
Also applies to: 312-330
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around
lines 281 - 295, Update the Phase 1 authorization design and RBAC sections to
require explicit Cloud Infrastructure Admin authorization for physical server
assignments and NetworkClass/template selection on both API and CRD paths. Add
authoritative inventory validation for server ownership or assignment before
dispatch, and replace the statement that any admin may assign any server;
preserve tenant scoping for virtual network resources.
AI Design Review: EP-179Score: 5/8 | Verdict: PASS
Verdict: A well-structured design that follows OSAC patterns and provides strong implementation detail, but lands at the pass threshold (5/8) due to gaps in spec/status ownership, vague graduation criteria, and unaddressed cross-cutting dimensions (Installation, Documentation). Feedback: Three targeted fixes would strengthen this significantly: (1) Move Critical (0)None. Important (5)
Suggestions (5)
Review costModel: claude-opus-4-6 |
a22e7ff to
113e52f
Compare
AI Design Review: EP-179Score: 5/8 | Verdict: PASS
Verdict: A solid design that follows core OSAC patterns and provides strong technical detail, passing at the threshold (5/8) with no zeros — held back by spec/status ownership issues, vague graduation criteria, and unaddressed installation dimension. Feedback: Move resolved_template_id and implementation_strategy from FabricDomainSpec to FabricDomainStatus (or a separate resolved/computed section) since they are system-set at creation time, not user-controlled desired state — this aligns with the OSAC convention that spec is user-owned and status is system-owned. Add measurable graduation criteria for each stage (e.g., 'Dev Preview: all CRUD operations pass e2e on netris-lab; Tech Preview: multi-tenant isolation verified, resize tested under load; GA: no regressions in existing networking tests, support procedures validated'). Address the installation dimension — describe what osac-installer Helm chart changes are needed for the new FabricDomain CRD and any new configuration values. Critical (0)None. Important (5)
Suggestions (4)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md (7)
271-292: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAuthorization Bypass (CWE-863): Incorrect Authorization
Reachability: External
Reject server overlap by default.
The design permits one server in multiple
ethernet_ewFabricDomains because Netris accepts it. Backend acceptance does not prove that traffic remains isolated between clusters or tenants. Reject overlap across active domains, or define supported multi-attachment with authoritative inventory checks and explicit traffic semantics. Add create and update conflict tests.Also applies to: 315-315
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around lines 271 - 292, Update the Multiple FabricDomains and Phase 1 limitations design to reject server overlap by default across active FabricDomains, including create and update operations. If multi-attachment remains supported, define authoritative inventory validation and explicit traffic-isolation semantics instead; add conflict tests covering both create and update paths.
289-292: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAuthorization Bypass (CWE-862): Missing Authorization
Reachability: External
Authorize physical server assignment separately from tenant scoping.
Line 291 allows any Cloud Infrastructure Admin to assign any server. Line 350 provides only tenant and OPA scoping. A tenant-scoped request can therefore select a server assigned to another tenant. Validate assignment against authoritative inventory and enforce explicit server-assignment permission on API and CRD paths before AAP dispatch.
Also applies to: 333-350
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around lines 289 - 292, Update the server-assignment flows described in the Phase 1 limitations and the tenant/OPA scoping section to validate each selected server against authoritative inventory and its tenant ownership, rather than trusting supplied hostnames. Enforce a distinct physical-server assignment permission on both API and CRD paths before dispatching to AAP, while preserving existing tenant and OPA authorization checks.
112-114: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine
default_fabric_domainsbefore enabling onboarding.
NetworkDefaults.default_fabric_domainsis referenced but not declared in the shown schema. EachFabricDomainSpecalso requiresnetwork_classand concreteservers. Define the default entry, tenant-scoped expansion, server resolution, and opt-out behavior. If Phase 1 has no allocator, disable automatic defaults. Otherwise, OSAC-2341 cannot create valid tenant resources.Also applies to: 184-187
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around lines 112 - 114, Define NetworkDefaults.default_fabric_domains in the schema before enabling tenant onboarding, including a valid FabricDomainSpec with network_class and concrete servers. Specify how defaults expand per tenant, how server references resolve, and how tenants opt out; if Phase 1 lacks an allocator or server-resolution path, disable automatic default provisioning until OSAC-2341 supports valid resources.
104-110: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist and reuse a stable backend identity.
Line 106 uses cluster-name lookup, but the design does not define a stable idempotency key or reuse of
FabricDomainStatus.backend_id. Derive the key from immutableFabricDomain.idand tenant. Persist the backend ID after success and reuse it for retry, rename, resize, and delete. Otherwise, a retry can create a duplicate cluster or target the wrong cluster.Also applies to: 337-346
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around lines 104 - 110, Define a stable backend idempotency key from immutable FabricDomain.id and tenant, and use it for all create, retry, rename, resize, and delete operations instead of relying on cluster-name lookup. After successful creation or reconciliation, persist the backend identifier in FabricDomainStatus.backend_id and reuse that value for subsequent AAP jobs, including deletion, while preserving it across renames and resizes.
174-182: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine the source of
supports_east_west_ethernet.
FD-VAL-01consumes this capability, but the design only adds a boolean field. Define how fabric-manager metadata derives the value and how the value is published and refreshed. A false default can reject a supported NetworkClass. A stale true value can dispatch to an unsupported backend.Also applies to: 221-232
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around lines 174 - 182, Define the authoritative fabric-manager metadata source and derivation rules for NetworkClassCapabilities.supports_east_west_ethernet, including how the value is published, refreshed, and handled when metadata is missing or stale. Update the related design sections and FD-VAL-01 behavior to prevent false defaults from rejecting supported classes or stale true values from selecting unsupported backends.
154-169: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine one lifecycle-status contract.
The proto exposes
state,message,hub,backend_id, andvpc_id. The CRD exposesphase,provisioningJobs,conditions,backendId, andvpcId. Feedback only synchronizes part of this state. Define the conversion for every field and legal phase value. AlignPROVISIONINGwithProgressingand define deletion and status-removal behavior.Also applies to: 212-218, 242-244, 337-345
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around lines 154 - 169, Define a complete lifecycle-status contract across FabricDomainStatus and the CRD status: map state/message/hub/backend_id/vpc_id to phase/provisioningJobs/conditions/backendId/vpcId, enumerate legal phase values, and specify conversion behavior for every field. Ensure PROVISIONING maps to Progressing, and explicitly define deletion handling and when status is removed, covering the related status sections as well.
221-232: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftValidate
template_idbefore dispatch.
NC-VAL-09does not require a non-empty, valid, or existing template ID. It also does not define update behavior. A bad template can reach AAP and fail after FabricDomain creation. Validate the template before dispatch, define immutability or versioning, and test missing, invalid, and changed values.Also applies to: 360-366
🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around lines 221 - 232, Extend the Validation Rules around NC-VAL-09 to require template_id to be present, valid, and refer to an existing template before dispatch to AAP, preventing FabricDomain creation with a bad template. Define the update behavior by making template_id immutable or specifying its versioning rules, and add coverage for missing, invalid, and changed values in the relevant validation tests.
🧹 Nitpick comments (1)
enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md (1)
398-418: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftAdd tests for the cross-layer failure paths.
Add tests for VPC reassignment, retry-after-success with persisted
backend_id, status conversion, default expansion, resolved-field override rejection, template validation, server ownership, and overlap conflicts. The current plan does not cover these contracts.🤖 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/OSAC-1382-multi-fabric-east-west-networking/design.md` around lines 398 - 418, Add cross-layer tests to the Test Plan covering VPC reassignment, retry-after-success using persisted backend_id, status conversion, default expansion, rejection of resolved-field overrides, template validation, server ownership, and overlap conflicts. Place these scenarios across the appropriate unit, integration, or E2E sections and preserve the existing lifecycle and error-path coverage.
🤖 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-1382-multi-fabric-east-west-networking/design.md`:
- Around line 35-42: Define explicit update semantics for the singular
FabricDomainSpec.virtual_network field and align the virtual_networks add/remove
update section accordingly. Either specify the complete mutable transition
behavior, including VPC migration, ownership and cleanup, and vpc_id updates, or
state that changes are rejected after creation and document that validation
rule.
- Line 23: Resolve the contradiction in the design between the statement on Line
23 that backend fields are not exposed and the description on Line 252 where
fulfillment-service writes `resolved_template_id` and `implementation_strategy`
as public FabricDomainSpec fields. Either move these resolved fields to status
or internal state instead of the public spec, or define an explicit validation
mechanism that rejects user-supplied values for these fields while explaining
the proto-to-CRD-to-AAP mapping. Ensure the resolution is consistently reflected
across all affected sections including Lines 146-151, 203-210, 248-256, and
333-335.
- Around line 65-67: Document explicit rollout gating for FabricDomain creation:
define the behavior when the FabricDomain CRD is absent from the API and when
the operator/controller is not installed, including whether creation is
rejected, deferred, or ignored. Update the FabricDomains/Create flow and the
skew-handling guidance so FS persistence and CR creation follow the same
supported-state rules, while preserving normal creation once the CRD and
controller are available.
---
Duplicate comments:
In `@enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md`:
- Around line 271-292: Update the Multiple FabricDomains and Phase 1 limitations
design to reject server overlap by default across active FabricDomains,
including create and update operations. If multi-attachment remains supported,
define authoritative inventory validation and explicit traffic-isolation
semantics instead; add conflict tests covering both create and update paths.
- Around line 289-292: Update the server-assignment flows described in the Phase
1 limitations and the tenant/OPA scoping section to validate each selected
server against authoritative inventory and its tenant ownership, rather than
trusting supplied hostnames. Enforce a distinct physical-server assignment
permission on both API and CRD paths before dispatching to AAP, while preserving
existing tenant and OPA authorization checks.
- Around line 112-114: Define NetworkDefaults.default_fabric_domains in the
schema before enabling tenant onboarding, including a valid FabricDomainSpec
with network_class and concrete servers. Specify how defaults expand per tenant,
how server references resolve, and how tenants opt out; if Phase 1 lacks an
allocator or server-resolution path, disable automatic default provisioning
until OSAC-2341 supports valid resources.
- Around line 104-110: Define a stable backend idempotency key from immutable
FabricDomain.id and tenant, and use it for all create, retry, rename, resize,
and delete operations instead of relying on cluster-name lookup. After
successful creation or reconciliation, persist the backend identifier in
FabricDomainStatus.backend_id and reuse that value for subsequent AAP jobs,
including deletion, while preserving it across renames and resizes.
- Around line 174-182: Define the authoritative fabric-manager metadata source
and derivation rules for NetworkClassCapabilities.supports_east_west_ethernet,
including how the value is published, refreshed, and handled when metadata is
missing or stale. Update the related design sections and FD-VAL-01 behavior to
prevent false defaults from rejecting supported classes or stale true values
from selecting unsupported backends.
- Around line 154-169: Define a complete lifecycle-status contract across
FabricDomainStatus and the CRD status: map state/message/hub/backend_id/vpc_id
to phase/provisioningJobs/conditions/backendId/vpcId, enumerate legal phase
values, and specify conversion behavior for every field. Ensure PROVISIONING
maps to Progressing, and explicitly define deletion handling and when status is
removed, covering the related status sections as well.
- Around line 221-232: Extend the Validation Rules around NC-VAL-09 to require
template_id to be present, valid, and refer to an existing template before
dispatch to AAP, preventing FabricDomain creation with a bad template. Define
the update behavior by making template_id immutable or specifying its versioning
rules, and add coverage for missing, invalid, and changed values in the relevant
validation tests.
---
Nitpick comments:
In `@enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md`:
- Around line 398-418: Add cross-layer tests to the Test Plan covering VPC
reassignment, retry-after-success using persisted backend_id, status conversion,
default expansion, rejection of resolved-field overrides, template validation,
server ownership, and overlap conflicts. Place these scenarios across the
appropriate unit, integration, or E2E sections and preserve the existing
lifecycle and error-path coverage.
🪄 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: f7e04747-3029-47a8-92d9-bad706d09ae6
📒 Files selected for processing (1)
enhancements/OSAC-1382-multi-fabric-east-west-networking/design.md
113e52f to
634fffb
Compare
AI Design Review: EP-179Score: 6/8 | Verdict: PASS
Verdict: Solid design with clear scope, strong alternatives analysis, and deep implementation detail, but held back from top marks by a phase enum on a new resource (conditions preferred), vague graduation criteria, and missing terminology definitions. Feedback: Define measurable graduation criteria for each maturity stage (e.g., 'Tech Preview: all CRUD operations pass e2e, error paths tested, no regressions in existing networking tests; GA: two production tenants using FabricDomain for 30+ days with no manual intervention'). For the FabricDomainState enum, either make conditions the primary lifecycle mechanism (the OSAC convention for new resources) and demote the phase enum, or justify why this resource needs phase-based state. Add a Terminology section near the top defining 'fabric domain,' 'east-west traffic,' 'isolation domain,' and 'Server Cluster' — the networking EP (review-patterns.md reference) set the precedent. Critical (0)None. Important (3)
Suggestions (5)
Review costModel: claude-opus-4-6 |
4e34f98 to
b4c6cf7
Compare
|
Below is a point-by-point response to the arguments for FabricDomain over ServerCluster.
|
Phase 1: does a separate FabricDomain resource earn its keep?Following the discussion on this PR, a few observations lead to a simpler conclusion for Phase 1:
Given this, introducing a full new resource (CRD, DB table, gRPC service, controller, AAP playbooks) for something that's tightly coupled to VirtualNetwork in Phase 1 may not be justified. The pragmatic alternative: extend VirtualNetwork with east-west properties for Phase 1. Learn from real usage. Then design the independent resource for Phase 2 when non-Ethernet fabrics bring concrete requirements. |
|
Hi @vladikr I am happy we are converging and you plan to include uniform isolation in your FabricDomain solution. Comments from @ori-amizur is also interesting, I for sure agree with creating OSAC networking resources (VPC, VirtualNetwork) does not trigger fabric manager actions — there's nothing for the backend to actually do. NetworkAttachment triggers backend actions. Details matter, so I figure concrete example will speak for itself. OSAC CLI: OSAC CLuster CR: Expected Status (after reconciliation) Complete details including osac-operator reconciliation flows are added in my writing: https://github.com/jingczhang/osac-enhancements/tree/networking/enhancements/OSAC-1382-multi-fabric-east-west-networking#implementation-details |
- Remove network_class from FabricDomainSpec — inherited from VN's NetworkClass (one NC per deployment) - Use protobuf enum FabricDomainType instead of free-form string - Add per-member status (FabricDomainMemberStatus) for server-level visibility on provisioning failures - Lock resource name to FabricDomain (resolved from open questions) - Clarify template V-Net vs OSAC Subnet coexistence with zeus12 validation details - Add CLI commands section (create, get, describe, edit, delete) - Reserve fabric_domain field on BareMetalInstance/ClusterOrder as Phase 2 open question - Update DB schema, validation rules, and test plan accordingly Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>
AI Design Review: EP-179Score: 8/8 | Verdict: PASS
Verdict: A thorough and well-validated design that introduces a clean architectural separation between N-S (VirtualNetwork) and E-W (FabricDomain) isolation planes, with strong real-infrastructure validation, concrete test plans, and well-constrained Phase 1 scope — minor gaps in operator reconciliation detail and protobuf naming consistency are addressable without structural changes. Feedback: Three items to address before merge: (1) Add an explicit finalizer section for the FabricDomain reconciler — the design references the standard OSAC operator pattern ('finalizer → status update → provisioning/deprovisioning lifecycle') everywhere else but never names the FabricDomain finalizer or describes the deprovisioning sequence that ensures the Netris Server Cluster is deleted before the CR is removed; without this, implementers may miss the cleanup guarantee. (2) Fix the protobuf enum naming: ETHERNET_EW, INFINIBAND_EW, and NVLINK should be FABRIC_DOMAIN_TYPE_ETHERNET_EW, FABRIC_DOMAIN_TYPE_INFINIBAND_EW, and FABRIC_DOMAIN_TYPE_NVLINK per protobuf and OSAC conventions (the UNSPECIFIED value is already correctly prefixed) — this will fail buf lint as written. (3) Describe how the operator propagates backend_id and vpc_id back to the fulfillment-service database after provisioning — the request path (FS → operator → AAP → Netris) is clear, but the return path for status fields sto Critical (0)None. Important (3)
Suggestions (4)
Review costModel: claude-opus-4-6 |
Thanks Ori. I think we already discussed it and the agreement was not to extend the VirtualNetowrk |
Thank you! On one hand, scale is an issue, since if you have many nodes each would need to have a CR * nics * by number of FabricDomains... For Phase 2+ BMI/VM integration, FabricDomain can evolve to reference BareMetalInstance CRs directly (via bmi_refs or label selectors) without needing per-interface attachment CRs. Let's revisit this in Phase 2. |
Thanks for your comment. The networkAttachments list includes both EW and NS subnets in one object. The nvlink: true as a boolean... I mean, this is exactly the lifecycle coupling point. Regarding the nodeSelector. I think it's an interesting idea for Phase 2, but it's a separate concern from the isolation model itself. |
|
Hi @vladikr, The OSAC ServerCluster solution applies to both BM and VM nodes. For VM ServerClusters with GPU support, enabling NVLink places them in the same NVLink partition — same semantics as BM. I'd like to ask again to add the OSAC ServerCluster solution back as a design alternative. The description can be kept simple — the key differentiator is: single lifecycle for nodes, uniform isolation across all fabrics. The name is not important (ServerCluster, HostGroup, NodeGroup — group compute resources with uniform characteristics and manage them as a unit with a single lifecycle and a single security perimeter). |
|
Thanks @jingczhang I think there is some kind of confusion. I agree that per-job NVLink sub-partitioning inside a tenant is a scheduling concern (IMEX), not OSAC infrastructure. What OSAC manages is which servers are in this tenant’s isolation domain on a given fabric. Hardware capability is a node attribute, for sure. However, partition/VRF/PKey membership is a provisioning decision when nodes are allocated or returned. For the common uniform case (same nodes, all fabrics, one lifecycle), we don’t need to force multiple domains. one FabricDomain and a multi-fabric NetworkClass template (or type: multi later) gives a single membership list and atomic resize. That gives the same operational shape as you're describing. But the API should stay flexible to accommodate use cases where membership and/or lifetime is different (I already gave examples: storage off NVLink, or a domain that must be torn down without touching the long-lived Ethernet/N-S) or managers that have different models. ServerCluster-as-peer is already documented as Alternative #2 in the design doc. - Please take a look. For VMs with GPU, NVLink membership is still a host/partition concern. VMs don’t independently join an NVLink domain the way a BM node does. |
Alternative: FabricDomain as a definition, server membership via
|
|
Hi @vladikr, I think enough details are discussed on discrete isolation vs uniform isolation now, and my request has been for you to document uniform isolation as a design alternative. There is good reason that the majority of GPU-aaS providers use uniform isolation. For the fabric domain solution, two things feel off, I wish you can improve them: (2) Your osac-cli to create a Fabric domain has to specify the N-S network in the command line, this feels very twisted. It will become common sense if you attach the fabric domain to a VPC. isn't it? |
AI Design Review: EP-179Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured design that follows all OSAC architectural patterns, provides deep implementation detail with proto schemas and database schemas, offers five substantive alternatives, and includes specific testable scenarios at every level — one of the stronger EPs reviewed. Feedback: The SignalFabricDomain RPC appears in the gRPC service definition but is never explained — add a brief description of what signal types it supports and when it would be invoked. Integration tests should specify test infrastructure details (kind cluster with mocked Netris backend, or real netris-lab) to distinguish them from E2E tests. Since osac-installer Helm values for NetworkClass east_west_config are being extended, consider noting whether the Enclave Wizard pipeline applies for the new schema fields. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
|
I feel the need to add this comment. I have looked at a number of mainstream CaaS, BMaaS, VMaaS, and GPUaaS offerings and have not found any that models tenant networking using separate North-South and East-West tenancy memberships. |
|
Thanks @jingczhang . On uniform isolation: Regarding VPC vs multiple networks:In OSAC today VirtualNetwork is the VPC-like object. regarding CLI:--virtual-network is the Phase 1 handle for that VPC (VN = VPC in the current model). On industry practice:Yes, I agree that most GPU-aaS UX is uniform node allocation, or EW hidden as a provider detail. OSAC still needs an API object for fabric isolation in a multi-tenant sovereign cloud. Hyperscalers hiding EW does not remove the need to program VRF/PKey/partition isolation; it only hides it from the tenant API. |
|
Hi @vladikr, thanks. To avoid any confusion, below is the alternative I advocate and I am done commenting -) Use node-based isolation. A tenant is assigned nodes (bare metal or virtual). For mixed hardware, use separate node groups under the same VPC.
Per-job E-W does not change that. The NVLink partition is static ownership of the GPU node group. The AI job scheduler’s dynamic isolation runs inside that partition; it is not a second multi-tenancy lifecycle. |
danmanor
left a comment
danmanor
left a comment
There was a problem hiding this comment.
Approval — Phase 1 design is sound, ship it
I've reviewed this design extensively, including deep dives into how 10 industry providers model east/west networking (AWS, GCP, Azure, OCI, Nebius, Crusoe, CoreWeave, Rafay, NICo, Netris), OSAC's existing networking architecture, and the full discussion between Vladik, Jing, and Ori.
Why I'm approving
The two-plane model (N/S via VirtualNetwork, E/W via FabricDomain) is architecturally correct. NVIDIA's own infrastructure controller (NICo), which manages the same backends OSAC will use (Netris, UFM, NMX-C), chose three independent isolation planes (VPC, InfiniBandPartition, NVLinkLogicalPartition). Nebius and Crusoe independently arrived at the same orthogonal model. The design is well-validated on real hardware (zeus12), follows established OSAC patterns, and has clear Phase 1 boundaries.
The Jing/Vladik debate has converged. Both agree on the core behavior; the disagreement is about resource naming and lifecycle granularity. Vladik's type: multi proposal for Phase 2/3 addresses Jing's atomic-resize concern for uniform deployments, while separate FabricDomains per type handle non-uniform cases (SuperPOD-style). ServerCluster-as-peer is documented as Alternative #2.
Items addressed in earlier reviews (Aug 3 + Aug 13)
All four concerns from my first review round have been resolved:
Why VN instead of Subnet?→ FabricDomain is now standalone, not on VNHow will tenant know template_id?→ Resolved from NetworkClassGeneric enough without Netris?→ Backend config scoped to NetworkClass, not FabricDomainStorage east/west?→ Multiple FabricDomains with independent lifecycles
Items from my second review (Aug 13) — status
- Enum for
type— still recommend. Every AI review flagged this. Proto enum prevents typos, provides compile-time safety. Addressable before or shortly after merge. - Per-member status — still recommend for Phase 2 (IB partial membership visibility). Not blocking for Phase 1.
- Resolve naming — FabricDomain vs IsolationDomain. Recommend deciding before merge. FabricDomain is the stronger choice.
- Template scope clarification — the design now explains that the template programs both EW and NS V-Nets, with OSAC Subnets coexisting alongside. This is clear.
- CLI commands — Phase 1 is CLI-only, so documenting the CLI surface matters. Can be a follow-up.
- Reserved
fabric_domainfield on BareMetalInstance/Cluster — Phase 2 item.
On Ori's latest proposal (fabricDomains array on BareMetalInstance)
Ori's Aug 16 proposal — FabricDomain as a definition with membership declared on BareMetalInstance via fabricDomains: [] — is the most promising evolution path for Phase 2. It eliminates server-list drift, makes non-uniform membership natural (gpu-01 in both ethernet_ew and nvlink, storage-01 in ethernet_ew only), and aligns with how networkAttachments already work. Worth revisiting when BMI/VM integration is scoped.
On Jing's remaining concerns
-
API/implementation gap (OSAC models N/S and E/W separately, but Netris Server Cluster handles both): This is intentional — the design models intent, not implementation. A multi-backend platform should not let one backend's holistic API shape the tenant-facing abstraction. When OSAC supports direct UFM or NICo (where there's no holistic "Server Cluster" concept), the FabricDomain API works unchanged.
-
--virtual-networkon FabricDomain create feels unnatural: Acknowledged. The VN reference is a Phase 1 product constraint (Netris needs VPC context + N/S reachability). Phase 2 can relax this to optional when non-VPC fabrics (IB, NVLink) are added. The current design explicitly documents this as a Phase 1 requirement, not a permanent architectural choice.
Recommendation
Merge Phase 1 as designed. Evolve in Phase 2 based on real usage:
type: multifor uniform deployments (Vladik's proposal, addresses Jing's atomic-resize)fabricDomains: []on BareMetalInstance for server-side membership (Ori's proposal, addresses drift)- Relax VN requirement to optional when IB/NVLink land
- Consider
network_classremoval from FabricDomainSpec (one NC per deployment, can be resolved from VN)
danmanor
left a comment
danmanor
left a comment
There was a problem hiding this comment.
Approval — Phase 1 design is sound, ship it
I've reviewed this design extensively, including deep dives into how industry providers model east/west networking (hyperscalers, neoclouds, and infrastructure controllers), OSAC's existing networking architecture, and the full discussion thread.
Why I'm approving
The two-plane model (N/S via VirtualNetwork, E/W via FabricDomain) is architecturally correct. Industry infrastructure controllers that manage the same backends OSAC will use chose independent isolation planes for VPC, InfiniBand, and NVLink. Multiple neoclouds independently arrived at the same orthogonal model. The design is well-validated on real hardware (zeus12), follows established OSAC patterns, and has clear Phase 1 boundaries.
The design discussion has converged. The core behavior is agreed; the remaining disagreement is about resource naming and lifecycle granularity. The type: multi proposal for Phase 2/3 addresses the atomic-resize concern for uniform deployments, while separate FabricDomains per type handle non-uniform cases (e.g., storage off NVLink, ephemeral NVLink partitions). ServerCluster-as-peer is documented as Alternative #2.
Items addressed in earlier reviews (Aug 3 + Aug 13)
All four concerns from my first review round have been resolved:
Why VN instead of Subnet?→ FabricDomain is now standalone, not on VNHow will tenant know template_id?→ Resolved from NetworkClassGeneric enough without a specific fabric manager?→ Backend config scoped to NetworkClass, not FabricDomainStorage east/west?→ Multiple FabricDomains with independent lifecycles
Items from my second review (Aug 13) — status
- Enum for
type— still recommend. Every AI review flagged this. Proto enum prevents typos, provides compile-time safety. Addressable before or shortly after merge. - Per-member status — still recommend for Phase 2 (IB partial membership visibility). Not blocking for Phase 1.
- Resolve naming — FabricDomain vs IsolationDomain. Recommend deciding before merge. FabricDomain is the stronger choice.
- Template scope clarification — the design now explains that the template programs both EW and NS V-Nets, with OSAC Subnets coexisting alongside. This is clear.
- CLI commands — Phase 1 is CLI-only, so documenting the CLI surface matters. Can be a follow-up.
- Reserved
fabric_domainfield on BareMetalInstance/Cluster — Phase 2 item.
On the NetworkAttachment / fabricDomains[] proposals
The Aug 16 proposal — FabricDomain as a definition with membership declared on BareMetalInstance via fabricDomains: [] — is the most promising evolution path for Phase 2. It eliminates server-list drift, makes non-uniform membership natural (gpu-01 in both ethernet_ew and nvlink, storage-01 in ethernet_ew only), and aligns with how networkAttachments already work. Worth revisiting when BMI/VM integration is scoped.
On remaining architectural concerns
-
API/implementation gap (OSAC models N/S and E/W separately, but the fabric manager handles both holistically): This is intentional — the design models intent, not implementation. A multi-backend platform should not let one backend's holistic API shape the tenant-facing abstraction. When OSAC supports backends that don't have a holistic concept, the FabricDomain API works unchanged.
-
--virtual-networkon FabricDomain create feels unnatural: Acknowledged. The VN reference is a Phase 1 product constraint (backend needs VPC context + N/S reachability). Phase 2 can relax this to optional when non-VPC fabrics (IB, NVLink) are added. The current design explicitly documents this as a Phase 1 requirement, not a permanent architectural choice.
Recommendation
Merge Phase 1 as designed. Evolve in Phase 2 based on real usage:
type: multifor uniform deployments (addresses atomic-resize concern)fabricDomains: []on BareMetalInstance for server-side membership (addresses drift)- Relax VN requirement to optional when IB/NVLink land
- Consider
network_classremoval from FabricDomainSpec (one NC per deployment, can be resolved from VN)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, vladikr 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 |
Replaced with updated review (removed vendor references)
|
/retest |
- Membership is static in Phase 1 (explicit hostnames); document that Phase 2 should support inventory-driven membership via selectors or BMI references - FabricDomain membership is host/device-scoped; VMs attach to SR-IOV VFs or GPUs on hosts already in the domain, not as FD members Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>
ccdfe1f to
e9511a0
Compare
|
New changes are detected. LGTM label has been removed. |
AI Design Review: EP-179Score: 8/8 | Verdict: PASS
Verdict: A strong, comprehensive design that follows all OSAC architectural patterns, provides deep technical detail (proto schemas, SQL DDL, validation rules with error codes, failure handling matrix), and includes an unusually thorough alternatives analysis with five options evaluated. Feedback: The SignalFabricDomain RPC is declared in the gRPC service but never described anywhere in the design — add request/response types, the use case (e.g., trigger re-reconciliation), and which persona invokes it, or remove it if it's not needed for Phase 1. Consider explicitly positioning FabricDomain within the two-manager model from the unified networking decisions (fabricManager handles the Netris/UFM backend; k8sManager is N/A for bare-metal Phase 1) to help reviewers connect this design to the established networking architecture. The Phase 1 limitation of no server overlap validation across FabricDomains is well-documented, but consider adding a brief note on the blast radius — what happens to existing workloads if Netris accepts overlapping servers and applies conflicting port configs. Critical (0)None. Important (2)
Suggestions (3)
Structural notes (0)None. Review costModel: claude-opus-4-6 |
5 participants
Design: Multi-Fabric East-West Networking
Jira: OSAC-1382
PRD: prd.md (merged in PR #117)
Summary
Introduces FabricDomain as a first-class OSAC resource for east-west fabric isolation, separate from VirtualNetwork (which remains the north-south / IP isolation boundary). Phase 1 delivers Ethernet east-west
via Netris Server Clusters.
Each FabricDomain requires exactly one VirtualNetwork in Phase 1 — the Server Cluster is created in that VN's Netris VPC. Backend config (template_id) lives on NetworkClass, not on the domain object.
Core operational risk retired: VPC-first → Server Cluster in existing VPC → OSAC Subnet coexistence and tenant isolation validated on zeus12 netris-lab.
Key Design Decisions
Validation