OSAC-4291: PRD - K8s Manager — OVN EVPN Phase 1 - #242
Conversation
|
@bkopilov: This pull request references OSAC-4291 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.1.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 EP Review: EP-242Score: 7/10 | Verdict: PASS
Verdict: The PRD describes a clear, well-justified capability with good persona coverage, but design leakage in acceptance criteria and restatement of scope across sections hold it to a borderline pass. Feedback: Rewrite acceptance criteria to describe user-observable outcomes: replace 'FRR diagnostic commands show correct VNI state' with 'Cloud Infrastructure Admin can verify connectivity status using documented tools,' replace 'provisions both Netris VNet and overlay network' with 'VMs deployed on the subnet are reachable from bare-metal servers,' and drop 'dual-dispatch provisioning' language. Consider removing the standalone Acceptance Criteria section entirely — fold any unique verification scenarios into User Stories as acceptance conditions, or move them to the design document. The CI integration test AC belongs in the design document, not the PRD. Critical (0)None. Important (4)
Suggestions (2)
Structural notes (0)None. Review costModel: claude-opus-4-6 |
|
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: Team Run ID: Comment |
- Line 19: Change 'cudn_evpn, ipv4 or dualstack' to clearly separate manager identifier (cudn_evpn) from capability (IPv4 address family) - Line 53: Use specific manager name 'cudn_evpn' instead of generic 'OVN EVPN k8s manager' - Line 81: Add assumption about NetworkClass configuration pairing fabric_manager (netris) with k8s_manager (cudn_evpn) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Benny Kopilov <bkopilov@bkopilov-thinkpadp1gen3.raanaii.csb>
Route targets are automatically calculated using (leaf ASN % 65536):VNI formula - they are not a configuration input or propagated value. Only VNI needs to be propagated from Netris to CUDN. Changes: - Line 20: 'VNI and route-target propagation' → 'VNI propagation' - Line 22: Remove FRRConfiguration design leakage, focus on outcome - Line 59: Reframe user story around VM route advertisement outcome - Line 65: Remove route targets from tenant-facing EVPN details list - Line 75: Clarify only VNI is returned, route targets auto-calculated - Line 94-98: Remove 'route target return via API' from Netris dependency Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Benny Kopilov <bkopilov@bkopilov-thinkpadp1gen3.raanaii.csb>
1. Capability format consistency (line 19, 53):
- 'IPv4 address family' → 'addressFamily:ipv4'
- Matches OSAC-1433 ConfigMap format exactly
2. Remove CUDN design leakage (line 21):
- 'Automatic CUDN creation' → 'Automatic overlay network provisioning'
- Adds context that ClusterUserDefinedNetwork is the mechanism used
3. Add Acceptance Criteria section:
- 7 testable checkpoints for Phase 1 completion
- Covers NetworkClass creation, dual-dispatch provisioning, VM
connectivity (L2/L3), FRR diagnostics, constraint validation, CI
4. Add missing user story (line 62):
- Cloud Infrastructure Admin needs visibility into which
VirtualNetworks hit the single-subnet limit for Phase 2 planning
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Benny Kopilov <bkopilov@bkopilov-thinkpadp1gen3.raanaii.csb>
danmanor
left a comment
danmanor
left a comment
There was a problem hiding this comment.
PRD Review — OSAC-4291: K8s Manager — OVN EVPN Phase 1
Good PRD overall — clear scope, strong persona separation (D1, D2 are excellent), thorough clarification rounds. The locked decisions are coherent with OSAC-1433's two-manager architecture.
Key findings
-
Missing requirement: fabric-before-k8s data dependency for subnet provisioning — The k8s manager needs network segment identifiers (VNI, route targets) that only exist after the fabric manager provisions the network. The PRD should state this ordering requirement and that the interface between managers must be fabric-agnostic. See inline comment on the "Automatic VNI propagation" bullet.
-
Single-subnet validation is NetworkClass-conditional — D4 says "enforced at Subnet API creation time" but this constraint only applies when the k8s manager is
cudn_evpn. Other NetworkClasses support multiple subnets per VirtualNetwork. The PRD should clarify this is conditional on the k8s manager, not a universal constraint. -
Dual gateway MAC understated — Listed as a known limitation but it's effectively a deployment blocker. Should be a documented prerequisite step, not a footnote.
-
IPv4-only capability — More restrictive than existing
cudn_net(which supports dual-stack). If deliberate Phase 1 scope cut, the PRD should state why. -
SecurityGroup and NATGateway enforcement gaps — No mention of whether Netris ACLs and SNAT cover EVPN-bridged VM traffic. Should be confirmed or listed as known limitations.
-
Too much implementation detail — Several bullets contain protocol-level specifics (BGP route types, VNI/VRF internals, FRR CLI commands, ConfigMap schemas) that belong in the design doc, not the PRD. See inline comments marked "Too technical for a PRD."
| The OVN EVPN spike (OSAC-1717) validated the technical approach: VMs can join the fabric via BGP EVPN route advertisements, enabling MAC/IP learning (Type-2 routes), VTEP discovery (Type-3 routes), and cross-subnet reachability (Type-5 routes). Phase 1 delivers single-cluster EVPN bridging with a constraint that OVN-Kubernetes does not currently route between separate CUDNs on the same cluster (the Connectors feature is pending). [Clarify: R1.Q4] | ||
|
|
||
| ## In Scope | ||
|
|
There was a problem hiding this comment.
Missing requirement: fabric-to-k8s data dependency
The k8s manager needs network segment identifiers (VNI, route targets) that are only available after the fabric manager provisions the VPC/VNet. Today the dispatcher runs fabric and k8s manager jobs independently — the k8s manager cannot assume those identifiers exist when it starts.
The PRD should add a requirement along these lines:
- The k8s manager requires network segment identifiers (VNI, route targets) produced by the fabric manager during subnet provisioning
- Subnet provisioning must ensure the fabric manager completes and those identifiers are available before the k8s manager begins
- The identifiers must be passed through a manager-agnostic interface so the k8s manager works with any fabric manager that produces them, not just Netris
This is the most critical gap — without it, the design phase starts from an open architectural question about how VNI/RT data flows between managers.
|
|
||
| ## In Scope | ||
|
|
||
| - **K8s manager registration** via ConfigMap (identifier: `cudn_evpn`, capabilities: `addressFamily:ipv4`) [Clarify: R2.Q4] |
There was a problem hiding this comment.
Too technical for a PRD: "route targets are calculated automatically from VNI" is an implementation detail. The PRD should say that the k8s manager receives the identifiers it needs from the fabric manager without manual configuration. How route targets relate to VNIs is a design concern.
| ## In Scope | ||
|
|
||
| - **K8s manager registration** via ConfigMap (identifier: `cudn_evpn`, capabilities: `addressFamily:ipv4`) [Clarify: R2.Q4] | ||
| - **Automatic VNI propagation** from Netris VPC/VNet creation to CUDN configuration (route targets are calculated automatically from VNI) [Clarify: R1.Q3, R2.Q5, D7] [User] |
There was a problem hiding this comment.
Too technical for a PRD: "ClusterUserDefinedNetwork with EVPN transport" names a specific K8s CRD and OVN topology mode. The PRD requirement is: "overlay network provisioned on hosting clusters that bridges VMs to the physical fabric." The specific mechanism (CUDN, EVPN transport topology, RouteAdvertisements) belongs in the design.
| - **K8s manager registration** via ConfigMap (identifier: `cudn_evpn`, capabilities: `addressFamily:ipv4`) [Clarify: R2.Q4] | ||
| - **Automatic VNI propagation** from Netris VPC/VNet creation to CUDN configuration (route targets are calculated automatically from VNI) [Clarify: R1.Q3, R2.Q5, D7] [User] | ||
| - **Automatic overlay network provisioning** on hosting clusters (via ClusterUserDefinedNetwork with EVPN transport) when a VirtualNetwork/Subnet is created [Clarify: R2.Q1] | ||
| - **Automatic BGP EVPN route advertisement** for VMs, using VNI mappings from Netris [Clarify: R2.Q1, D5] |
There was a problem hiding this comment.
Too technical for a PRD: "Type-2 MAC/IP routes, Type-3 VTEP discovery, Type-5 prefix routes" are BGP EVPN protocol details. The PRD requirement is: "VMs are reachable from the physical fabric (L2 same-subnet and L3 cross-subnet)." The BGP route types are design/implementation detail.
| - **Automatic VNI propagation** from Netris VPC/VNet creation to CUDN configuration (route targets are calculated automatically from VNI) [Clarify: R1.Q3, R2.Q5, D7] [User] | ||
| - **Automatic overlay network provisioning** on hosting clusters (via ClusterUserDefinedNetwork with EVPN transport) when a VirtualNetwork/Subnet is created [Clarify: R2.Q1] | ||
| - **Automatic BGP EVPN route advertisement** for VMs, using VNI mappings from Netris [Clarify: R2.Q1, D5] | ||
| - **VM-to-fabric connectivity** via BGP EVPN (Type-2 MAC/IP routes, Type-3 VTEP discovery, Type-5 prefix routes) |
There was a problem hiding this comment.
Too technical for a PRD: "Layer 2 VNI (macVRF)" and "Layer 3 VNI (ipVRF)" are fabric implementation details. The PRD should state the user-facing capability: VMs can share the same subnet as bare-metal hosts (L2 connectivity) or use a different subnet within the same VPC (L3 routing). How VNIs and VRFs achieve that is design territory.
| - As a Cloud Infrastructure Admin, I want documented installation prerequisites (underlay link setup, VTEP configuration, BGP peering with Netris including VTEP subnets/prefixes advertisement) so that I can prepare the infrastructure before enabling EVPN for the first time. [Clarify: R2.Q2, R2.Q3, D6] [User] | ||
|
|
||
| - As a Cloud Infrastructure Admin, I want FRR diagnostic commands documented (show evpn vni, show bgp l2vpn evpn, show bgp vni <VNI>, show bgp l2vpn evpn summary) so that I can verify VNI creation and troubleshoot VNI/route-target mismatches. [Clarify: R3.Q2] | ||
|
|
There was a problem hiding this comment.
Too technical for a PRD: "ConfigMap (with addressFamily:ipv4 capability)" references the specific K8s registration mechanism. The PRD user story should be about the capability: "register the EVPN k8s manager so that OSAC can provision fabric-bridged subnets for VMs." How it's registered (ConfigMap, labels, capability fields) is design.
| - As a Tenant Admin or Tenant User, I want VMs I provision on an EVPN-bridged subnet to receive IP addresses via OVN DHCP and be reachable from bare-metal servers in the same Netris VPC so that my workloads can span VMs and physical hosts — either via Layer 2 connectivity when using the same subnet, or via Layer 3 routing when using different subnets within the same VPC. [User] | ||
|
|
||
| - As a Tenant Admin or Tenant User, I want the system to reject my second Subnet creation attempt under the same VirtualNetwork with a clear error message referencing the OVN Connectors limitation so that I understand the constraint and can structure my networks accordingly. [Clarify: R1.Q4, D4] | ||
|
|
There was a problem hiding this comment.
Missing: SecurityGroup behavior
The PRD covers VirtualNetwork and Subnet but says nothing about SecurityGroup enforcement for EVPN-bridged VMs.
With cudn_evpn as k8s manager and netris as fabric manager, SecurityGroups dispatch to Netris (ACL rules). But does the Netris ACL enforcement plane see and filter EVPN-originated traffic from OVN? If not, VMs provisioned on EVPN-bridged subnets have no firewall — a security gap.
The PRD should either:
- Confirm that fabric-level SecurityGroups cover EVPN-bridged VM traffic (add to assumptions)
- Or acknowledge this as a known limitation / out of scope for Phase 1
|
|
||
| - As a Tenant Admin or Tenant User, I want the system to reject my second Subnet creation attempt under the same VirtualNetwork with a clear error message referencing the OVN Connectors limitation so that I understand the constraint and can structure my networks accordingly. [Clarify: R1.Q4, D4] | ||
|
|
||
| ## Assumptions |
There was a problem hiding this comment.
Missing: NATGateway interaction
NATGateway dispatches to the fabric manager only (Netris SNAT via softgate). Does Netris SNAT work for EVPN-bridged VM traffic? The softgate needs to see the VM's source IP on the fabric for SNAT to apply.
If confirmed working: add to assumptions. If untested: add to known limitations or out of scope.
| ## Acceptance Criteria | ||
|
|
||
| - [ ] A NetworkClass with `fabric_manager: "netris"` and `k8s_manager: "cudn_evpn"` can be created and transitions to READY state | ||
| - [ ] Creating a VirtualNetwork + Subnet with this NetworkClass provisions both Netris VNet (with L2/L3 VNI) and OCP CUDN with EVPN transport |
There was a problem hiding this comment.
Silent failure — needs acceptance criteria or known limitation entry
R3.Q2 established that VNI/route-target mismatches cause silent connectivity failure: VM boots, gets an IP, shows as Ready, but can't reach the fabric. The diagnostic commands story partially addresses this, but the PRD should either:
- Add an acceptance criterion that the system surfaces a warning when EVPN state is inconsistent, OR
- Explicitly list "silent connectivity failure on VNI/RT mismatch" as a known limitation with a cross-reference to the troubleshooting documentation requirement
| - **OSAC-1717 (K8s Manager — OVN-Kubernetes EVPN Spike):** Validates OVN EVPN technical approach. Status: Closed. | ||
|
|
||
| - **OSAC-1433 (Unified Networking Architecture):** Provides foundation for NetworkClass, dispatcher, k8s manager registration pattern. This design extends OSAC-1433 with the OVN EVPN k8s manager section. [Clarify: R3.Q4, D10] | ||
|
|
There was a problem hiding this comment.
Missing dependencies: The Assumptions section mentions "FRR operator and NMState operator are installed" but the Dependencies section doesn't list them. These are hard infrastructure prerequisites — if they're not installed, nothing works. Add them with minimum version requirements if known.
… fixes) Addressed 4 Important findings + 2 Suggestions from AI review bot: **Important fixes:** 1. Line 23: Remove BGP route type internals (Type-2/Type-3/Type-5) - Before: 'via BGP EVPN (Type-2 MAC/IP routes...)' - After: 'VMs are discoverable and directly reachable from bare-metal' 2. Line 24: Remove macVRF/ipVRF internal terminology - Before: 'Layer 2 VNI (macVRF) for L2 connectivity' - After: 'Layer 2 bridging for direct connectivity' 3. Line 44: Fix route-target sourcing contradiction - Consistent: route targets calculated from VNI, not returned by API 4. CI test user story moved to Acceptance Criteria (non-functional) - Was: Cloud Infrastructure Admin user story osac-project#5 - Now: Under 'Non-Functional' section in acceptance criteria **Suggestions applied:** 1. Line 26: Simplify DHCP coordination to user-observable outcome - Before: 'DHCP range coordination between OCP CUDN IPAM and Netris' - After: 'VMs receive IP addresses that do not conflict with Netris' 2. Line 98-101: Simplify dispatcher dependency - Before: Listed OSAC-1457/1458/1460 internals - After: Single line describing dispatcher purpose Also updated Acceptance Criteria to use user-observable language (removed 'OVN DHCP', 'CUDN with EVPN transport', 'route-target state'). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Benny Kopilov <bkopilov@bkopilov-thinkpadp1gen3.raanaii.csb>
…TED) Addressed all findings from reviewer's CHANGES_REQUESTED review: **Critical architectural fixes:** 1. Added fabric-to-k8s data dependency requirement (line 20) - K8s manager needs network segment identifiers from fabric manager - Provisioning must ensure fabric completes before k8s manager starts - Interface must be manager-agnostic 2. Fixed single-subnet validation scope (line 24) - Was: universal API constraint - Now: conditional on this k8s manager only - Other NetworkClasses support multiple subnets per VirtualNetwork 3. Promoted dual gateway MAC to prerequisite (line 29, removed from line 46) - Was: known limitation - Now: documented prerequisite (prevents L3 traffic failure) 4. Added IPv4-only justification (Out of Scope) - Phase 1: IPv4 only (EVPN route advertisement untested with IPv6) - Phase 2: IPv6/dual-stack 5. Added SecurityGroup and NATGateway assumptions (line 75-76) - Fabric-level ACLs apply to fabric-bridged VM traffic - Fabric-level SNAT applies to VM egress 6. Added FRR and NMState operator dependencies (line 107-110) - Both required, must be installed before EVPN configuration **Removed technical details from PRD (moved to design):** - Line 19-23: Removed BGP route types, VRF terminology, protocol details - Line 27-28: Removed specific prerequisites (VTEP, BGP peering commands) - Line 29: Removed FRR CLI commands - Line 43: Removed route-target formula - Line 52-57: Removed ConfigMap/capability specifics from user stories - Line 102-109: Simplified dependency descriptions **Added Known Limitations section:** - Silent connectivity failure on network segment mismatch - VMs boot but can't reach fabric (no automatic warnings) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Benny Kopilov <bkopilov@bkopilov-thinkpadp1gen3.raanaii.csb>
Addressed Review FeedbackThank you for the detailed review! I've addressed all 16 inline comments: Critical Architectural Fixes1. Fabric-to-k8s data dependency (line 20) ✅
2. Single-subnet validation scope (line 24) ✅
3. Dual gateway MAC (line 29, was line 46) ✅
4. IPv4-only justification ✅
5. SecurityGroup/NATGateway coverage (line 75-76) ✅
6. FRR/NMState operator dependencies (line 107-110) ✅
Design Leakage RemovedLines 19-29: Removed BGP route types, VRF terminology, VTEP/BGP specifics, FRR commands, ConfigMap details Known LimitationsAdded new section: Silent connectivity failure on network segment mismatch
All changes pushed in commit 8e36a2c. |
|
|
||
| #### Answer | ||
|
|
||
| Phase 1 will not include `0:VNI_ID` route-target standardization. VNI extraction happens automatically - when creating VPC on Netris, the VNI is automatically set in CUDN. |
There was a problem hiding this comment.
I guess it is more of a design comment but how does this happen ?
Clarifications file needs the same cleanupThe PRD itself looks good now, but
The clarifications file ships with the PRD as an audit trail. If someone reads D4 as-is, they'll implement a universal subnet limit that breaks other NetworkClasses. And if they read the technical details, they may treat implementation choices as locked decisions when they should be design-phase decisions. Please update the clarifications to match the PRD's abstraction level. |
Remove BGP route types, VNI/VRF terminology, and infrastructure specifics per review feedback. The PRD should describe capabilities (L2/L3 reachability), not protocols (Type-2/Type-3/Type-5 routes). Changes: - Problem Statement: replace BGP route types with "L2 same-subnet and L3 cross-subnet reachability" - Acceptance Criteria: remove "(with L2/L3 VNI)" - Dependencies: remove infrastructure details "(underlay ports, BGP sessions)" Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Benny Kopilov <bkopilov@bkopilov-thinkpadp1gen3.raanaii.csb>
Rewrite R1.Q3 answer to state the requirement (automatic VNI propagation, no manual steps) without implying a specific mechanism. The original phrasing "VNI is automatically set in CUDN" sounded like implementation detail rather than a PRD-level requirement. Acknowledge that the mechanism for passing VNI from fabric manager output to k8s manager input is a design decision, not a PRD concern. Also remove ASN formula from D3 - already removed from PRD as "too technical." Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Benny Kopilov <bkopilov@bkopilov-thinkpadp1gen3.raanaii.csb>
|
D3 Impact/Decision updated — looks good, correctly frames VNI handoff as fabric→k8s with mechanism as a design decision. Two remaining items:
The Q&A content in the rounds (FRR commands, VTEP details, etc.) is fine as-is — that's the actual conversation record. It's the Decisions, Impact lines, and Summary that need to match the PRD. |
Update D4 Impact, Decision, and Summary to match PRD's conditional scoping: "when a VirtualNetwork uses a NetworkClass whose k8s manager has this limitation" rather than universal constraint. Other NetworkClasses (cudn_net, fabric-only netris) support multiple subnets per VirtualNetwork today. The single-subnet constraint is specific to cudn_evpn due to missing OVN Connectors. Also update D3 summary: "to k8s manager" instead of "to CUDN" to match the revised D3 wording from previous commit. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Benny Kopilov <bkopilov@bkopilov-thinkpadp1gen3.raanaii.csb>
danmanor
left a comment
danmanor
left a comment
There was a problem hiding this comment.
All feedback addressed across both PRD and clarifications.
PRD: Clean separation between requirements and implementation detail. Critical items (fabric-to-k8s data dependency, conditional subnet validation, gateway MAC prerequisite, SecurityGroup/NATGateway assumptions, silent failure mode) all properly captured.
Clarifications: D3 and D4 now match the PRD — D3 uses "fabric manager to k8s manager" (not CUDN), D4 is correctly scoped as conditional on NetworkClass's k8s manager.
Ready for design phase.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bkopilov, 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 |
3 participants
PRD: K8s Manager — OVN EVPN Phase 1: Single-Cluster VM-to-Fabric Bridging
Jira: https://redhat.atlassian.net/browse/OSAC-4291
Summary
Implements the OVN EVPN k8s manager for single-cluster deployments, enabling VMs to be first-class fabric participants via BGP EVPN route advertisement. VMs running on OVN-Kubernetes can share the same L2 subnet with bare-metal servers and are reachable from the physical fabric through Type-2/Type-3/Type-5 EVPN routes.
Phase 1 delivers automatic VNI/route-target propagation from Netris to CUDN, automatic FRRConfiguration creation for EVPN overlay, VM-to-fabric connectivity, and DHCP range coordination. Includes single-subnet-per-VirtualNetwork constraint enforcement (OVN Connectors limitation) and comprehensive installation prerequisites documentation.
Requesting Review On
How to Review
osac-docs/personas.md