NO-ISSUE: Remove region references from networking designs - #182
openshift-merge-bot[bot] merged 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
WalkthroughThe networking enhancement documents replace region-scoped references with deployment-scoped ChangesNetworkClass networking model
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
AI EP Review: EP-182Score: 10/10 | Verdict: PASS
Verdict: Clean terminology update that correctly replaces 'region' with 'NetworkClass' across three well-structured PRDs, improving alignment between user-facing documentation and the actual API resource model. Feedback: The terminology rename is well-executed and improves consistency. One minor point: since the design doc notes NetworkClass is 'provider-only, tenants never see it,' consider whether tenant-facing user stories should reference 'NetworkClass' directly or describe the limitation in terms tenants encounter (e.g., 'the platform prevents VM creation when the underlying infrastructure does not support virtualization'). The current phrasing works because tenants do reference NetworkClass by name via --network-class flags, but a brief note in the PRD clarifying this user-visibility model would strengthen the document. Critical (0)None. Important (0)None. Suggestions (1)
Review costModel: claude-opus-4-6 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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-1433-unified-networking/design.md`:
- Around line 368-374: Update the CLI examples around the NetworkClass and
ExternalIPPool creation steps to replace the `moc-region-1` value and matching
metadata with a NetworkClass name that contains no `region` token, keeping both
examples consistent with the no-region test.
- Around line 1101-1106: Update the “Multiple Hosting Clusters Per NetworkClass”
design to define how existing subnets are reconciled when a hosting cluster
joins after subnet creation: provision its K8s overlay and fabric bridge through
k8sManager, or explicitly restrict support to clusters present at creation.
- Around line 739-741: Document the compatibility and migration strategy for the
network_class field-number change in the VirtualNetworkSpec schema. Explicitly
state whether prior resources with region at field 1 are unsupported because the
schema is pre-release, or define the conversion/backfill process needed to
safely migrate existing data before interpreting field 1 as network_class.
In `@enhancements/OSAC-1435-vmaas-networking/design.md`:
- Line 244: Align the BM-only ComputeInstance lifecycle in the design: either
document the synchronous fulfillment-service API error returned when a
NetworkClass lacks k8sManager, or move that validation into the controller and
specify the resulting Pending NetworkingResolutionFailed status and event
contract. Update the BM-only NetworkClass check and related
lifecycle/support-coverage sections consistently.
In `@enhancements/OSAC-1436-caas-networking/design.md`:
- Around line 102-110: Update enhancements/OSAC-1436-caas-networking/design.md
lines 102-110 so CaaS cluster subnet creation always invokes the k8s_manager
path and provisions the required MetalLB IPAddressPool for endpoint VIP
allocation. Update enhancements/OSAC-1436-caas-networking/prd.md line 126 to
explicitly require the target NetworkClass to support k8s_manager and MetalLB
VIPs.
🪄 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: 9f9be4b1-1650-44b4-a166-b98600ca2b63
📒 Files selected for processing (7)
enhancements/OSAC-1433-unified-networking/design.mdenhancements/OSAC-1435-vmaas-networking/design.mdenhancements/OSAC-1435-vmaas-networking/prd.mdenhancements/OSAC-1436-caas-networking/design.mdenhancements/OSAC-1436-caas-networking/prd.mdenhancements/OSAC-1437-bmaas-networking/design.mdenhancements/OSAC-1437-bmaas-networking/prd.md
| string network_class = 1; // required, immutable | ||
| string ipv4_cidr = 2; // optional, immutable | ||
| string ipv6_cidr = 3; // optional, immutable |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 --glob '*.proto' --glob '*.md' --glob '*.yaml' --glob '*.yml' \
'VirtualNetworkSpec|network_class|region' .
rg -n -C 4 --glob '*.md' \
'migration|backfill|conversion|upgrade|stored' \
enhancements/OSAC-1433-unified-networkingRepository: osac-project/enhancement-proposals
Length of output: 50392
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the relevant design sections and nearby compatibility/migration text.
sed -n '680,785p' enhancements/OSAC-1433-unified-networking/design.md
sed -n '800,900p' enhancements/OSAC-1433-unified-networking/design.md
# Search only this design for compatibility-related concerns.
rg -n -C 3 \
'compatibility|upgrade|downgrade|migration|backfill|conversion|stored|previous|old.*data|released|pre-release|unsupported|network_class|region' \
enhancements/OSAC-1433-unified-networking/design.md
# Summarize proto field-number references to VirtualNetworkSpec in this repository.
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('*.md'):
txt=p.read_text(errors='replace')
if 'VirtualNetworkSpec' in txt or 'network_class = 1' in txt or 'region = 1' in txt:
print(f'--- {p}')
for i,line in enumerate(txt.splitlines(),1):
if 'VirtualNetworkSpec' in line or 'network_class = 1' in line or 'region = 1' in line or 'region = 3' in line:
print(f'{i}: {line}')
PYRepository: osac-project/enhancement-proposals
Length of output: 12016
Define compatibility and migration for the scope-field change.
network_class now uses field number 1. If released region data used field 1 and the proto is generated as VirtualNetworkSpec { string region = 1; ... }, old resources are parsed with the wrong semantics. Document the compatibility story, a conversion/backfill path, or state that this schema is pre-release and previous resources are unsupported.
🤖 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-1433-unified-networking/design.md` around lines 739 - 741,
Document the compatibility and migration strategy for the network_class
field-number change in the VirtualNetworkSpec schema. Explicitly state whether
prior resources with region at field 1 are unsupported because the schema is
pre-release, or define the conversion/backfill process needed to safely migrate
existing data before interpreting field 1 as network_class.
Source: Learnings
| #### Multiple Hosting Clusters Per NetworkClass | ||
|
|
||
| Multiple hosting clusters are supported per region. At subnet creation, the | ||
| Multiple hosting clusters are supported per NetworkClass. At subnet creation, the | ||
| k8sManager creates a K8s overlay on each hosting cluster and bridges it to | ||
| the fabric segment. VMs on different hosting clusters share the same subnet | ||
| via the fabric. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 --glob '*.md' \
'hosting cluster|hosting clusters|NetworkClass|subnet creation|reconcile' \
enhancements/OSAC-1433-unified-networking \
enhancements/OSAC-1436-caas-networking \
enhancements/OSAC-1437-bmaas-networkingRepository: osac-project/enhancement-proposals
Length of output: 50393
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== target section =="
sed -n '1060,1130p' enhancements/OSAC-1433-unified-networking/design.md
echo
echo "== relevant NetworkClass/subnet/provisioning sections =="
rg -n -C 4 --glob '*.md' \
'Multiple hosting clusters|Multiple Hosting Clusters|hosting cluster|hosting clusters|subnets|virtual networks|subnet provisioning|subnet creation|Create VirtualNetwork|Create Subnet' \
enhancements/OSAC-1433-unified-networking/design.md
echo
echo "== reconciliation mentions =="
rg -n -C 3 --glob '*.md' '\b(reconcile|reconciliation|requeue|watch|add|remove|backwards compatibility|existing)' \
enhancements/OSAC-1433-unified-networking/design.mdRepository: osac-project/enhancement-proposals
Length of output: 27055
Define reconciliation for hosting clusters added after subnet creation.
Multiple hosting clusters are supported per NetworkClass, but subnet provisioning calls k8sManager only at subnet creation. If a hosting cluster joins the NetworkClass later, existing subnets do not get a K8s overlay or bridge. Define a reconciliation/re-provisioning path for this case, or restrict the support claim to clusters present at creation.
🤖 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-1433-unified-networking/design.md` around lines 1101 -
1106, Update the “Multiple Hosting Clusters Per NetworkClass” design to define
how existing subnets are reconciled when a hosting cluster joins after subnet
creation: provision its K8s overlay and fabric bridge through k8sManager, or
explicitly restrict support to clusters present at creation.
| osac create virtualnetwork --network-class moc-bm-1 --cidr 10.0.0.0/16 --name my-net | ||
| ``` | ||
| Dispatcher → `osac.templates.{{ fabric_manager }}.create_virtual_network` | ||
|
|
||
| 2. **Create Subnet:** | ||
| ```bash | ||
| osac create subnet --virtual-network my-net --cidr 10.0.1.0/24 --name my-subnet | ||
| ``` | ||
| Dispatcher → TWO jobs: fabric_manager creates VLAN/fabric segment + k8s_manager creates CUDN overlay (if region hosts VMs) | ||
| Dispatcher → TWO jobs: fabric_manager creates VLAN/fabric segment + k8s_manager creates CUDN overlay (if NetworkClass hosts VMs) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Require K8s-manager capability for CaaS NetworkClasses.
CaaS uses bare-metal node sets, but endpoint VIP allocation still requires the K8s manager and its MetalLB IPAddressPool. The current design can skip this setup, while the PRD can allow a generic NetworkClass assumption.
enhancements/OSAC-1436-caas-networking/design.md#L102-L110: run the K8s-manager path for CaaS clusters and create the required VIP pool.enhancements/OSAC-1436-caas-networking/prd.md#L126-L126: state that the target NetworkClass requires K8s-manager and MetalLB VIP support.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 103-103: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 107-107: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 109-109: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
📍 Affects 2 files
enhancements/OSAC-1436-caas-networking/design.md#L102-L110(this comment)enhancements/OSAC-1436-caas-networking/prd.md#L126-L126
🤖 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-1436-caas-networking/design.md` around lines 102 - 110,
Update enhancements/OSAC-1436-caas-networking/design.md lines 102-110 so CaaS
cluster subnet creation always invokes the k8s_manager path and provisions the
required MetalLB IPAddressPool for endpoint VIP allocation. Update
enhancements/OSAC-1436-caas-networking/prd.md line 126 to explicitly require the
target NetworkClass to support k8s_manager and MetalLB VIPs.
AI Design Review: EP-182Score: 8/8 | Verdict: PASS
Verdict: Clean, architecturally sound terminology alignment replacing the informal 'region' concept with the actual OSAC resource name 'NetworkClass' across all four networking design documents and their PRDs — improves precision without altering design intent. Feedback: Strong execution on a cross-document terminology refinement. One minor inconsistency: the example NetworkClass names diverge across designs (moc-region-1 in OSAC-1433, moc-bm-virt in 1435, moc-bm-1 in 1436, moc in 1437) — while each name may be contextually appropriate, using a consistent example name across the family would make it easier for readers to follow the relationship between the unified design and its service-specific companions. Consider also adding a brief note in the unified networking design (OSAC-1433) explaining why the rename was made (NetworkClass is the typed resource, not a geographic region), since future readers won't have the PR context. Critical (0)None. Important (0)None. Suggestions (2)
Review costModel: claude-opus-4-6 |
danmanor
changed the title
|
@danmanor: This pull request explicitly references no jira issue. 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. |
The region concept was excluded from the API. Replace all region references with the appropriate term: - "NetworkClass" where the text refers to the actual API resource or its fields (e.g., proto field, CLI --network-class flag, YAML specs) - "deployment" where the text refers to the general infrastructure scope (e.g., "BM-only deployment", section titles, user stories) Affected designs: OSAC-1433 (unified, default), OSAC-1435 (VMaaS), OSAC-1436 (CaaS), OSAC-1437 (BMaaS). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
AI EP Review: EP-182Score: 9/10 | Verdict: PASS
Verdict: Solid PRDs with clear user-facing capabilities, proper persona coverage, and testable acceptance criteria. The terminology refactoring from 'region' to 'deployment' and '--region' to '--network-class' is consistently applied across all three PRDs and their companion design docs. The only weakness is business justification depth — gap descriptions are clear but lack impact quantification. Feedback: Strengthen the WHY in each PRD by adding a sentence connecting the gap to business impact — e.g., 'This blocks tenants from deploying production workloads that require network segmentation, limiting CaaS adoption for multi-tier applications.' The terminology change from 'region' to 'deployment' is clean but consider adding a one-line definition of 'deployment' in the PRDs (e.g., in a Glossary or Background section) since the term is more ambiguous than 'region' to readers unfamiliar with the OSAC model. In OSAC-1437's Assumptions section, consider rephrasing 'The NetworkClass has a fabric manager configured' to something more user-accessible like 'The deployment's network infrastructure is configured for automated switch management.' Critical (0)None. Important (1)
Suggestions (3)
Review costModel: claude-opus-4-6 |
064e10d to
b0230a8
Compare
AI EP Review: EP-182Score: 9/10 | Verdict: PASS
Verdict: The three networking PRDs are solid — clear user-facing capabilities with persona-specific stories, focused scope per service, and testable acceptance criteria. The terminology refactoring (region → deployment/NetworkClass) is a clean improvement. WHY is the weakest criterion: gaps are described but business impact is not quantified. Feedback: The PRDs describe the gap well but would benefit from a stronger 'why this matters' case — even one sentence tying to a strategic goal or quantifying the impact (e.g., 'required for multi-tenant VMaaS GA' or 'blocks N% of tenant workloads that need multi-NIC'). The terminology change from 'region' to 'deployment'/'NetworkClass' is well-executed and consistent across all documents. Consider adding a brief note in each PRD's problem statement explaining the deployment concept for readers unfamiliar with the recent terminology shift. Critical (0)None. Important (1)
Suggestions (2)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-182Score: 8/8 | Verdict: PASS
Verdict: A well-executed terminology and model refinement that aligns all four networking designs with the established 'one NetworkClass per deployment' decision, improving architectural clarity by removing the overloaded 'region' concept. Feedback: The K8s Manager description in OSAC-1433 has a semantic change beyond terminology: 'Needed for regions that host VMs or CaaS clusters' became 'Needed for deployments that host both VMs and BMs and require multi-tenancy across all' -- this changes the requirement from OR-logic (VMs/CaaS) to AND-logic (VMs AND BMs) and drops CaaS clusters as a trigger. If intentional, call this out explicitly in the PR description so reviewers don't miss it. Also, the changed line in the K8s Manager description runs long compared to surrounding text; consider re-wrapping for consistency. Critical (0)None. Important (1)
Suggestions (1)
Review costModel: claude-opus-4-6 |
|
thanks :) |
3 participants
Summary
Removed all "region" references from the five networking design documents
Summary by CodeRabbit
VirtualNetworkSpec.regionfield withnetwork_class.