OSAC-2244: resolve fabric_interface from HostType for CaaS node sets - #294
OSAC-2244: resolve fabric_interface from HostType for CaaS node sets#294ori-amizur wants to merge 1 commit into
Conversation
|
@ori-amizur: This pull request references OSAC-2244 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 task 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ori-amizur 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 |
|
🤖 Finished Review · ✅ Success · Started 4:01 PM UTC · Completed 4:22 PM UTC Commit: |
|
Warning Review limit reached
Next review available in: 112 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
WalkthroughCluster creation now resolves each attached node set’s fabric interface from its referenced host type. The selected interface name is system-populated in ChangesFabric interface resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CreateCluster
participant resolveFabricInterfaces
participant HostType
participant ClusterNodeSet
CreateCluster->>resolveFabricInterfaces: resolve attached node-set interfaces
resolveFabricInterfaces->>HostType: look up referenced host type
HostType-->>resolveFabricInterfaces: return host type interfaces
resolveFabricInterfaces->>ClusterNodeSet: set fabric interface name
resolveFabricInterfaces-->>CreateCluster: return success or FailedPrecondition
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
fulfillment-service/internal/servers/private_clusters_server_test.go (1)
133-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the new
fabric_interfacecontract.The fixture now provides
data-0, but the test file does not assert that a network-attached cluster storesGetFabricInterface() == "data-0". It also does not cover first-match ordering, no attachment, or a host type without a fabric interface returningFailedPrecondition. Add focused tests for these cases.🤖 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 `@fulfillment-service/internal/servers/private_clusters_server_test.go` around lines 133 - 137, Add focused tests in the private cluster server test suite covering the new fabric_interface contract: assert a network-attached cluster stores GetFabricInterface() as "data-0", verify the first matching fabric interface is selected when multiple interfaces exist, verify no attachment leaves the field unset, and verify a host type without a fabric interface returns FailedPrecondition. Reuse the existing fixture and server/test helpers rather than changing production code.fulfillment-service/internal/servers/private_clusters_server.go (1)
705-735: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse resolved
HostTyperecords.
Createalready looks up request node sets at Lines [190-200]. Both transformation paths also load host types before this helper.resolveFabricInterfacesthen performs another DAO lookup for every final node set. Pass the resolved records into this step or resolve the interface during the existing transformation to avoid redundant DAO I/O.🤖 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 `@fulfillment-service/internal/servers/private_clusters_server.go` around lines 705 - 735, Update resolveFabricInterfaces and its callers to reuse HostType records already loaded during Create and the node-set transformation paths, rather than calling lookupHostType for each final node set. Pass the resolved records into the helper or assign fabric interfaces during transformation while preserving the existing missing-host-type and missing-fabric-interface behavior.
🤖 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 `@fulfillment-service/internal/servers/private_clusters_server.go`:
- Around line 230-237: Update the Update flow around GenericServer.Update to
retain each stored node set’s system-populated fabric_interface when
spec.node_sets is masked and the request omits it. Validate any client-supplied
fabric_interface against the persisted value and reject changes rather than
accepting them. Add regression tests covering both preservation of omitted
values and rejection of attempted modifications.
- Around line 230-237: Before the network-attachment check in the server
handler, apply the tenant default network_attachment to spec when the request
omits one, using the existing defaulting/transformation mechanism. Ensure this
occurs before resolveFabricInterfaces is called so the defaulted attachment
triggers fabric interface resolution and is persisted; retain the existing
error-return behavior.
---
Nitpick comments:
In `@fulfillment-service/internal/servers/private_clusters_server_test.go`:
- Around line 133-137: Add focused tests in the private cluster server test
suite covering the new fabric_interface contract: assert a network-attached
cluster stores GetFabricInterface() as "data-0", verify the first matching
fabric interface is selected when multiple interfaces exist, verify no
attachment leaves the field unset, and verify a host type without a fabric
interface returns FailedPrecondition. Reuse the existing fixture and server/test
helpers rather than changing production code.
In `@fulfillment-service/internal/servers/private_clusters_server.go`:
- Around line 705-735: Update resolveFabricInterfaces and its callers to reuse
HostType records already loaded during Create and the node-set transformation
paths, rather than calling lookupHostType for each final node set. Pass the
resolved records into the helper or assign fabric interfaces during
transformation while preserving the existing missing-host-type and
missing-fabric-interface behavior.
🪄 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: a0d1a553-d96f-4585-a901-ae4ba439c392
⛔ Files ignored due to path filters (2)
fulfillment-service/internal/api/osac/private/v1/cluster_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/cluster_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
fulfillment-service/internal/servers/private_clusters_server.gofulfillment-service/internal/servers/private_clusters_server_test.gofulfillment-service/proto/private/osac/private/v1/cluster_type.proto
| // Resolve fabric_interface for each node set when the cluster has a | ||
| // network attachment. The HostType's interfaces list is searched for | ||
| // the first interface with role "fabric". | ||
| if spec.GetNetworkAttachment() != nil { | ||
| if err = s.resolveFabricInterfaces(ctx, spec); err != nil { | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'fabric_interface|FabricInterface|SetFabricInterface|Update|spec\.node_sets' \
fulfillment-service/internal/servers/private_clusters_server.go \
fulfillment-service/internal/servers/private_clusters_server_test.goRepository: osac-project/osac
Length of output: 50374
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Create/update and fabric resolution ---'
sed -n '200,280p' fulfillment-service/internal/servers/private_clusters_server.go
sed -n '700,760p' fulfillment-service/internal/servers/private_clusters_server.go
printf '%s\n' '--- Node-set validation and transformation ---'
sed -n '403,440p' fulfillment-service/internal/servers/private_clusters_server.go
sed -n '737,860p' fulfillment-service/internal/servers/private_clusters_server.go
printf '%s\n' '--- Generic update implementation and field-mask semantics ---'
rg -n -C 5 'func .*Update|proto\.Merge|UpdateMask|FieldMask|Merge' fulfillment-service/internal -g '*.go' | head -n 240
printf '%s\n' '--- Fabric-interface declarations and update tests ---'
rg -n -C 6 'fabric_interface|FabricInterface|resolveFabricInterfaces|network_attachment' fulfillment-service -g '*.go' -g '*.proto' | head -n 320Repository: osac-project/osac
Length of output: 50373
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- GenericServer definition ---'
rg -l 'type GenericServer|func \(.*GenericServer.*\) Update' fulfillment-service
rg -n -C 12 'type GenericServer|func \(.*GenericServer.*\) Update' fulfillment-service -g '*.go' | head -n 220
printf '%s\n' '--- Exact cluster field declarations ---'
rg -l 'fabric_interface|message ClusterNodeSet|ClusterNodeSet' fulfillment-service/proto fulfillment-service/internal | head -n 80
rg -n -C 10 'fabric_interface|message ClusterNodeSet' fulfillment-service/proto -g '*.proto'
printf '%s\n' '--- Cluster test cases containing fabric or network attachment ---'
rg -n -C 12 'fabric_interface|FabricInterface|network_attachment|NetworkAttachment' \
fulfillment-service/internal/servers/private_clusters_server_test.go
printf '%s\n' '--- Update mask helper and merge behavior ---'
rg -n -C 10 'Apply.*Update|apply.*Update|update mask|updateMask|proto.Merge|MergeFrom|FieldMask' \
fulfillment-service/internal fulfillment-service/pkg fulfillment-service -g '*.go' \
| rg -v 'mock|_test.go' | head -n 300Repository: osac-project/osac
Length of output: 50375
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Complete GenericServer.Update merge and persistence flow ---'
sed -n '608,790p' fulfillment-service/internal/servers/generic_server.go
printf '%s\n' '--- Field-path implementation ---'
rg -n -C 8 'func .*compilePaths|type .*FieldPath|func .*Set|func .*Clear|func .*Get' \
fulfillment-service/internal/masks fulfillment-service/internal/servers -g '*.go' \
| head -n 260
printf '%s\n' '--- All cluster update validators ---'
sed -n '560,710p' fulfillment-service/internal/servers/private_clusters_server.go
sed -n '1000,1095p' fulfillment-service/internal/servers/private_clusters_server_test.go
printf '%s\n' '--- Relevant source invariant probe ---'
python3 - <<'PY'
from pathlib import Path
server = Path("fulfillment-service/internal/servers/private_clusters_server.go").read_text()
generic = Path("fulfillment-service/internal/servers/generic_server.go").read_text()
proto = Path("fulfillment-service/proto/private/osac/private/v1/cluster_type.proto").read_text()
checks = {
"create_resolves_fabric_only": (
"resolveFabricInterfaces(ctx, spec)" in server
and "func (s *PrivateClustersServer) Update" in server
and server.index("resolveFabricInterfaces(ctx, spec)")
< server.index("func (s *PrivateClustersServer) Update")
),
"generic_mask_replaces_masked_value": (
"fieldPath.Set(tmpObject, value)" in generic
and 'tmpObject = requestObject' in generic
),
"fabric_is_not_user_settable_by_contract_only": (
"Not user-settable." in proto
and "string fabric_interface = 3;" in proto
),
"no_update_fabric_validation": "fabric_interface" not in server[
server.index("func (s *PrivateClustersServer) Update"):
server.index("func (s *PrivateClustersServer) Delete")
],
}
for name, result in checks.items():
print(f"{name}: {'PASS' if result else 'FAIL'}")
PYRepository: osac-project/osac
Length of output: 39446
Preserve and protect fabric_interface during Update.
When spec.node_sets is masked, GenericServer.Update replaces the node-set map with the request map. Existing fabric_interface values are therefore cleared because update requests omit this system-populated field. The update path also accepts client-supplied fabric_interface values. Preserve stored values and reject client changes, with regression tests for both cases.
🤖 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 `@fulfillment-service/internal/servers/private_clusters_server.go` around lines
230 - 237, Update the Update flow around GenericServer.Update to retain each
stored node set’s system-populated fabric_interface when spec.node_sets is
masked and the request omits it. Validate any client-supplied fabric_interface
against the persisted value and reject changes rather than accepting them. Add
regression tests covering both preservation of omitted values and rejection of
attempted modifications.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'setDefaults|injectDefaultNetworkAttachment|GetNetworkAttachment|resolveFabricInterfaces' \
fulfillment-service/internal/servers/private_clusters_server.goRepository: osac-project/osac
Length of output: 4043
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Create and defaulting flow ---'
sed -n '180,245p' fulfillment-service/internal/servers/private_clusters_server.go
sed -n '280,300p' fulfillment-service/internal/servers/private_clusters_server.go
printf '%s\n' '--- Fabric resolution implementation ---'
sed -n '705,755p' fulfillment-service/internal/servers/private_clusters_server.go
printf '%s\n' '--- Relevant symbols across the repository ---'
rg -n -C 4 \
'injectDefaultNetworkAttachment|DefaultNetworkingReady|network_attachment|fabric_interface' \
fulfillment-service/internal fulfillment-service/proto \
-g '*.go' -g '*.proto' | head -n 400Repository: osac-project/osac
Length of output: 50373
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Cluster transformation paths ---'
rg -n -C 12 \
'func \(s \*PrivateClustersServer\) validateAndTransform(CatalogItem|Cluster)|SetNetworkAttachment|NetworkAttachment_builder|networkAttachment' \
fulfillment-service/internal/servers/private_clusters_server.go
printf '%s\n' '--- Cluster-specific default-network symbols ---'
rg -n -C 5 \
'PrivateClustersServer|findDefaultSubnet|findDefaultSecurity|default subnet|default security|ClusterNetworkAttachment_builder|SetNetworkAttachment' \
fulfillment-service/internal/servers \
-g '*.go' | grep -E 'private_clusters|Cluster|cluster|default|NetworkAttachment|network_attachment' | head -n 300
printf '%s\n' '--- All direct writes to ClusterSpec.network_attachment ---'
rg -n -C 3 \
'SetNetworkAttachment|NetworkAttachment_builder|network_attachment' \
fulfillment-service/internal/servers/private_clusters_server.go \
fulfillment-service/internal/controllers \
-g '*.go' 2>/dev/null | head -n 300Repository: osac-project/osac
Length of output: 33559
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- validateAndTransformCluster ---'
sed -n '737,947p' fulfillment-service/internal/servers/private_clusters_server.go
printf '%s\n' '--- validateAndTransformCatalogItem ---'
sed -n '948,1085p' fulfillment-service/internal/servers/private_clusters_server.goRepository: osac-project/osac
Length of output: 12232
Inject the tenant default network_attachment before resolving fabric_interface. setDefaults only initializes spec and status, and neither transformation path injects tenant networking. A request without network_attachment therefore skips resolveFabricInterfaces and persists no fabric_interface, despite the API contract.
🤖 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 `@fulfillment-service/internal/servers/private_clusters_server.go` around lines
230 - 237, Before the network-attachment check in the server handler, apply the
tenant default network_attachment to spec when the request omits one, using the
existing defaulting/transformation mechanism. Ensure this occurs before
resolveFabricInterfaces is called so the defaulted attachment triggers fabric
interface resolution and is persisted; retain the existing error-return
behavior.
ReviewFindingsHigh
Low
Next steps:
Previous runReviewFindingsMedium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
Auto-dismissed: only Prow labels gate merging
Add fabric_interface field (field 3) to the private ClusterNodeSet proto. During cluster creation, when network_attachment is set, the server resolves each node set's HostType and selects the first interface with role "fabric". If no fabric interface exists, creation fails with FailedPrecondition. The resolved value flows through the feedback controller to the ClusterOrder CRD's NodeSetStatus.FabricInterface (added by OSAC-2346), which the operator uses during reconcileNetworking to configure the correct physical NIC on each bare-metal worker. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ori Amizur <oamizur@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 10:21 AM UTC · Completed 10:39 AM UTC Commit: |
| if hostTypeKey == "" { | ||
| continue | ||
| } | ||
| hostType, err := s.lookupHostType(ctx, hostTypeKey) |
There was a problem hiding this comment.
[low] logic-error
resolveFabricInterfaces has two defensive guards that are effectively unreachable: (1) continue on empty hostTypeKey -- mergeNodeSetsWithTemplate already populates every node set's HostType from the template; (2) hostType == nil guard -- lookupHostType returns NotFound error, not nil, so the preceding err != nil check already returns.
|
|
||
| // Name of the network interface on the HostType used for tenant fabric traffic. | ||
| // | ||
| // System-populated during cluster creation by selecting the first interface with |
There was a problem hiding this comment.
[low] edge-case
fabric_interface is documented as 'Not user-settable' but validateNodeSetsUpdate does not check its immutability on Update. Create path is fine (always overwrites via resolveFabricInterfaces).
| if fabricInterface == "" { | ||
| return grpcstatus.Errorf(grpccodes.FailedPrecondition, | ||
| "node_sets[%s]: host type '%s' has no interface with role 'fabric'", | ||
| name, hostTypeKey) |
There was a problem hiding this comment.
[low] pattern-inconsistency
Error message uses node_sets[%s]: map-index notation; all other node set errors in this file use natural language 'node set' phrasing.
Suggested fix: Reword to match established pattern, e.g.: "host type '%s' for node set '%s' has no interface with role 'fabric'".
| int32 size = 2; | ||
|
|
||
| // Name of the network interface on the HostType used for tenant fabric traffic. | ||
| // |
There was a problem hiding this comment.
[low] pattern-inconsistency
Comment uses 'System-populated' / 'Not user-settable' instead of the established (system-provided, read-only) idiom used elsewhere in proto files.
Suggested fix: Adopt the established (system-provided, read-only) phrasing.
fullsend-ai-review
Bot
removed
the
requires-manual-review
Auto-dismissed: only Prow labels gate merging
2 participants
Add fabric_interface field (field 3) to the private ClusterNodeSet proto. During cluster creation, when network_attachment is set, the server resolves each node set's HostType and selects the first interface with role "fabric". If no fabric interface exists, creation fails with FailedPrecondition.
The resolved value flows through the feedback controller to the ClusterOrder CRD's NodeSetStatus.FabricInterface (added by OSAC-2346), which the operator uses during reconcileNetworking to configure the correct physical NIC on each bare-metal worker.
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit