OSAC-1755: Add capabilities intersection to NetworkClass reconciler - #406
OSAC-1755: Add capabilities intersection to NetworkClass reconciler#406SiddarthR56 wants to merge 1 commit into
Conversation
Assisted-by: Cursor/Claude
|
@SiddarthR56: This pull request references OSAC-1755 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 NOT APPROVED This pull-request has been approved by: SiddarthR56 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe PR adds NetworkClass capability synchronization from resolved network-manager capabilities. It introduces ConfigMap-triggered and periodic reconciliation, configurable sync timing, updated manager discovery labels, and tests for capability computation, pagination, updates, and resolution errors. ChangesNetworkClass capability synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigMap
participant NetworkClassCapabilitiesReconciler
participant dispatcher.Resolver
participant FulfillmentService
participant NetworkClassesClient
ConfigMap->>NetworkClassCapabilitiesReconciler: trigger resynchronization
NetworkClassCapabilitiesReconciler->>NetworkClassesClient: list NetworkClasses
NetworkClassCapabilitiesReconciler->>dispatcher.Resolver: resolve manager capabilities
dispatcher.Resolver->>FulfillmentService: query manager capabilities
FulfillmentService-->>dispatcher.Resolver: return capabilities
dispatcher.Resolver-->>NetworkClassCapabilitiesReconciler: return resolved capabilities
NetworkClassCapabilitiesReconciler->>NetworkClassesClient: update changed capabilities
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 |
![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 @.claude/rules/configuration.md:
- Around line 27-28: Update the ConfigMap changes entry to state that
synchronization is active only when both the fulfillment-service gRPC connection
is configured and the networking namespace is non-empty, matching the gate in
cmd/main.go.
In `@cmd/main.go`:
- Around line 581-584: Update the GetEnvWithDefault call for
envNetworkClassSyncInterval to pass a validator that accepts only strictly
positive durations, rejecting zero and negative values before syncInterval
reaches NewNetworkClassCapabilitiesSyncRunnable. Reuse the existing duration
parsing/validation conventions or helper available in the codebase.
- Around line 572-576: Instantiate privatev1.NewNetworkClassesClient(grpcConn)
once before creating the resolver, assign it to a shared client variable, and
pass that same instance to both dispatcher.NewResolver and
controller.NewNetworkClassCapabilitiesReconciler.
In `@internal/controller/networkclass_capabilities_controller_test.go`:
- Around line 84-99: Opaque protobuf messages are being instantiated with
unsupported non-empty struct literals. In
internal/controller/networkclass_capabilities_controller_test.go at lines 84-99,
117-121, 144-148, 160-210, and 225-229, replace NetworkClassCapabilities,
NetworkClass, and NetworkClassesListResponse literals with their corresponding
_builder{...}.Build() calls; in
internal/controller/dispatcher_resolver_helpers_test.go at lines 76-92, do the
same for NetworkClassesListResponse, NetworkClassesGetResponse, and
NetworkClassesUpdateResponse. Preserve all existing field values and nesting.
In `@internal/controller/networkclass_capabilities_controller.go`:
- Around line 251-267: Prevent concurrent resyncAll executions by adding shared
synchronization between networkClassCapabilitiesSyncRunnable.Start and the
reconciler’s ConfigMap-triggered resync path. Gate each full resync with a mutex
or in-progress guard, ensuring a periodic tick skips or waits when another
resync is active while preserving existing error logging and cancellation
behavior.
🪄 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: 31ccf270-f2b5-4ed0-8644-224b89974ed3
📒 Files selected for processing (12)
.claude/rules/configuration.mdcharts/operator/templates/deployment.yamlcharts/operator/templates/network-managers.yamlcharts/operator/values.yamlcmd/main.goconfig/samples/network-manager-fabric-netris.yamlinternal/controller/dispatcher_resolver_helpers_test.gointernal/controller/networkclass_capabilities_controller.gointernal/controller/networkclass_capabilities_controller_test.gopkg/dispatcher/resolver.gopkg/networkmanager/doc.gopkg/networkmanager/types.go
| ConfigMap changes (default: 5m). Only active when the fulfillment-service gRPC | ||
| connection is configured. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the namespace prerequisite too.
This entry states the sync is "Only active when the fulfillment-service gRPC connection is configured." The actual gate in cmd/main.go also requires a non-empty networking namespace (grpcConn != nil && networkingNamespace != ""). Mention both prerequisites so operators do not troubleshoot a silently inactive sync when only the gRPC connection is set.
📝 Proposed doc fix
- `OSAC_NETWORK_CLASS_SYNC_INTERVAL` — periodic full resync interval for NetworkClass
capabilities intersection, in addition to the immediate resync triggered by manager
- ConfigMap changes (default: 5m). Only active when the fulfillment-service gRPC
- connection is configured.
+ ConfigMap changes (default: 5m). Only active when the fulfillment-service gRPC
+ connection and `OSAC_NETWORKING_NAMESPACE` are both configured.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ConfigMap changes (default: 5m). Only active when the fulfillment-service gRPC | |
| connection is configured. | |
| ConfigMap changes (default: 5m). Only active when the fulfillment-service gRPC | |
| connection and `OSAC_NETWORKING_NAMESPACE` are both configured. |
🤖 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 @.claude/rules/configuration.md around lines 27 - 28, Update the ConfigMap
changes entry to state that synchronization is active only when both the
fulfillment-service gRPC connection is configured and the networking namespace
is non-empty, matching the gate in cmd/main.go.
| resolver := dispatcher.NewResolver(privatev1.NewNetworkClassesClient(grpcConn), disc) | ||
|
|
||
| ncReconciler := controller.NewNetworkClassCapabilitiesReconciler( | ||
| privatev1.NewNetworkClassesClient(grpcConn), resolver, networkingNamespace, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Reuse a single NetworkClassesClient instance.
privatev1.NewNetworkClassesClient(grpcConn) is called twice: once for resolver and once for ncReconciler. Both wrap the same grpcConn, so a single client instance can be shared.
♻️ Proposed refactor
- resolver := dispatcher.NewResolver(privatev1.NewNetworkClassesClient(grpcConn), disc)
+ ncClient := privatev1.NewNetworkClassesClient(grpcConn)
+ resolver := dispatcher.NewResolver(ncClient, disc)
ncReconciler := controller.NewNetworkClassCapabilitiesReconciler(
- privatev1.NewNetworkClassesClient(grpcConn), resolver, networkingNamespace,
+ ncClient, resolver, networkingNamespace,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resolver := dispatcher.NewResolver(privatev1.NewNetworkClassesClient(grpcConn), disc) | |
| ncReconciler := controller.NewNetworkClassCapabilitiesReconciler( | |
| privatev1.NewNetworkClassesClient(grpcConn), resolver, networkingNamespace, | |
| ) | |
| ncClient := privatev1.NewNetworkClassesClient(grpcConn) | |
| resolver := dispatcher.NewResolver(ncClient, disc) | |
| ncReconciler := controller.NewNetworkClassCapabilitiesReconciler( | |
| ncClient, resolver, networkingNamespace, | |
| ) |
🤖 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 `@cmd/main.go` around lines 572 - 576, Instantiate
privatev1.NewNetworkClassesClient(grpcConn) once before creating the resolver,
assign it to a shared client variable, and pass that same instance to both
dispatcher.NewResolver and controller.NewNetworkClassCapabilitiesReconciler.
| syncInterval := helpers.GetEnvWithDefault(envNetworkClassSyncInterval, defaultNetworkClassSyncInterval) | ||
| if err := localMgr.Add(controller.NewNetworkClassCapabilitiesSyncRunnable(ncReconciler, syncInterval)); err != nil { | ||
| return fmt.Errorf("networkclass capabilities sync runnable: %w", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate that the sync interval is positive before use.
helpers.GetEnvWithDefault(envNetworkClassSyncInterval, defaultNetworkClassSyncInterval) accepts a validator argument, but none is passed here. time.ParseDuration accepts "0s" or negative durations like "-5m" as valid parses, so an operator setting OSAC_NETWORK_CLASS_SYNC_INTERVAL to zero or negative reaches NewNetworkClassCapabilitiesSyncRunnable unchecked. time.NewTicker panics for a non-positive duration in Start() (internal/controller/networkclass_capabilities_controller.go, line 254), crashing the manager process from a single misconfigured env var.
Add a positive-duration validator to the GetEnvWithDefault call.
🛡️ Proposed fix
- syncInterval := helpers.GetEnvWithDefault(envNetworkClassSyncInterval, defaultNetworkClassSyncInterval)
+ syncInterval := helpers.GetEnvWithDefault(
+ envNetworkClassSyncInterval, defaultNetworkClassSyncInterval,
+ func(d time.Duration) bool { return d > 0 },
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| syncInterval := helpers.GetEnvWithDefault(envNetworkClassSyncInterval, defaultNetworkClassSyncInterval) | |
| if err := localMgr.Add(controller.NewNetworkClassCapabilitiesSyncRunnable(ncReconciler, syncInterval)); err != nil { | |
| return fmt.Errorf("networkclass capabilities sync runnable: %w", err) | |
| } | |
| syncInterval := helpers.GetEnvWithDefault( | |
| envNetworkClassSyncInterval, defaultNetworkClassSyncInterval, | |
| func(d time.Duration) bool { return d > 0 }, | |
| ) | |
| if err := localMgr.Add(controller.NewNetworkClassCapabilitiesSyncRunnable(ncReconciler, syncInterval)); err != nil { | |
| return fmt.Errorf("networkclass capabilities sync runnable: %w", err) | |
| } |
🤖 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 `@cmd/main.go` around lines 581 - 584, Update the GetEnvWithDefault call for
envNetworkClassSyncInterval to pass a validator that accepts only strictly
positive durations, rejecting zero and negative values before syncInterval
reaches NewNetworkClassCapabilitiesSyncRunnable. Reuse the existing duration
parsing/validation conventions or helper available in the codebase.
| It("treats nil as equivalent to all-false", func() { | ||
| Expect(capabilitiesEqual(nil, &privatev1.NetworkClassCapabilities{})).To(BeTrue()) | ||
| }) | ||
|
|
||
| It("returns false when any field differs", func() { | ||
| a := &privatev1.NetworkClassCapabilities{SupportsIpv4: true} | ||
| b := &privatev1.NetworkClassCapabilities{} | ||
| Expect(capabilitiesEqual(a, b)).To(BeFalse()) | ||
| }) | ||
|
|
||
| It("returns true when all fields match", func() { | ||
| a := &privatev1.NetworkClassCapabilities{SupportsIpv4: true, SupportsDualStack: true} | ||
| b := &privatev1.NetworkClassCapabilities{SupportsIpv4: true, SupportsDualStack: true} | ||
| Expect(capabilitiesEqual(a, b)).To(BeTrue()) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Non-empty struct literals on Opaque API messages likely fail to build.
internal/api/osac/private/v1 generates NetworkClass with the protogen:"opaque.v1" state tag and only hidden xxx_hidden_* fields (per the upstream contract snippet). The Opaque API hides struct fields and requires either the generated <Type>_builder{...}.Build() companion or setters on an empty literal — this PR's own production code follows that pattern (privatev1.NetworkClassesListRequest_builder{...}.Build(), privatev1.NetworkClassesUpdateRequest_builder{Object: nc}.Build(), and caps := &privatev1.NetworkClassCapabilities{} followed by caps.SetSupportsIpv4(...) etc.). The new test code instead constructs NetworkClass, NetworkClassCapabilities, and the NetworkClasses*Response types with non-empty struct literals, which is inconsistent with the Opaque API and, per the protobuf-go documentation, is not supported for opaque messages.
internal/controller/networkclass_capabilities_controller_test.go#L84-L99: replace&privatev1.NetworkClassCapabilities{SupportsIpv4: true}(and similar) withprivatev1.NetworkClassCapabilities_builder{SupportsIpv4: true}.Build(), or an empty literal plusSet*calls.internal/controller/networkclass_capabilities_controller_test.go#L117-L121: replace&privatev1.NetworkClass{Id: ..., FabricManager: ..., K8SManager: ...}withprivatev1.NetworkClass_builder{...}.Build().internal/controller/networkclass_capabilities_controller_test.go#L144-L148: same fix for theNetworkClassand nestedNetworkClassCapabilitiesliterals.internal/controller/networkclass_capabilities_controller_test.go#L160-L210: same fix for&privatev1.NetworkClass{Id: "nc-caps-no-fabric"},{Id: "nc-caps-bad-fabric", FabricManager: ...},{Id: "nc-caps-good", FabricManager: ...}, and{Id: "nc-caps-bad", FabricManager: ...}.internal/controller/networkclass_capabilities_controller_test.go#L225-L229: replace&privatev1.NetworkClassesListResponse{Items: page, Size: ..., Total: ...}withprivatev1.NetworkClassesListResponse_builder{...}.Build().internal/controller/dispatcher_resolver_helpers_test.go#L76-L92: replace&privatev1.NetworkClassesListResponse{Items: items},&privatev1.NetworkClassesGetResponse{Object: item}, and&privatev1.NetworkClassesUpdateResponse{Object: in.GetObject()}with the corresponding_builder{...}.Build()calls.
Run the verification script below to confirm whether these types are opaque and whether _builder companions exist, before applying fixes.
#!/bin/bash
set -euo pipefail
# Locate the generated private API package.
fd -t d 'v1$' internal/api/osac/private
# Check whether NetworkClass, NetworkClassCapabilities, and the List/Get/Update
# response types are generated with the Opaque API (hidden fields + _builder companions).
rg -n --type=go -A2 'type (NetworkClass|NetworkClassCapabilities|NetworkClassesListResponse|NetworkClassesGetResponse|NetworkClassesUpdateResponse) struct' internal/api/osac/private/v1
rg -n --type=go '(NetworkClass|NetworkClassCapabilities|NetworkClassesListResponse|NetworkClassesGetResponse|NetworkClassesUpdateResponse)_builder' internal/api/osac/private/v1
rg -n --type=go 'protogen:"opaque' internal/api/osac/private/v1📍 Affects 2 files
internal/controller/networkclass_capabilities_controller_test.go#L84-L99(this comment)internal/controller/networkclass_capabilities_controller_test.go#L117-L121internal/controller/networkclass_capabilities_controller_test.go#L144-L148internal/controller/networkclass_capabilities_controller_test.go#L160-L210internal/controller/networkclass_capabilities_controller_test.go#L225-L229internal/controller/dispatcher_resolver_helpers_test.go#L76-L92
🤖 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 `@internal/controller/networkclass_capabilities_controller_test.go` around
lines 84 - 99, Opaque protobuf messages are being instantiated with unsupported
non-empty struct literals. In
internal/controller/networkclass_capabilities_controller_test.go at lines 84-99,
117-121, 144-148, 160-210, and 225-229, replace NetworkClassCapabilities,
NetworkClass, and NetworkClassesListResponse literals with their corresponding
_builder{...}.Build() calls; in
internal/controller/dispatcher_resolver_helpers_test.go at lines 76-92, do the
same for NetworkClassesListResponse, NetworkClassesGetResponse, and
NetworkClassesUpdateResponse. Preserve all existing field values and nesting.
| // Start runs the periodic resync loop until the context is canceled. | ||
| func (s *networkClassCapabilitiesSyncRunnable) Start(ctx context.Context) error { | ||
| log := ctrllog.FromContext(ctx).WithName("networkclass-capabilities-sync") | ||
| ticker := time.NewTicker(s.interval) | ||
| defer ticker.Stop() | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil | ||
| case <-ticker.C: | ||
| if err := s.reconciler.resyncAll(ctx); err != nil { | ||
| log.Error(err, "periodic network class capabilities resync failed") | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Overlapping resyncs are possible between the ConfigMap-triggered reconciler and the periodic runnable.
Start calls s.reconciler.resyncAll(ctx) directly from its own goroutine on every tick, independent of the controller-runtime reconcile queue (which serializes Reconcile calls via MaxConcurrentReconciles). A ConfigMap event and a tick can therefore trigger two concurrent full resyncAll passes, each listing and potentially updating every NetworkClass. The result is still correct (each pass computes the same capabilities independently), but under frequent ConfigMap churn this doubles fulfillment-service List/Update traffic.
Consider gating resyncAll behind a mutex or an "in-progress" flag shared between the reconciler and the runnable to avoid redundant concurrent passes.
🤖 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 `@internal/controller/networkclass_capabilities_controller.go` around lines 251
- 267, Prevent concurrent resyncAll executions by adding shared synchronization
between networkClassCapabilitiesSyncRunnable.Start and the reconciler’s
ConfigMap-triggered resync path. Gate each full resync with a mutex or
in-progress guard, ensuring a periodic tick skips or waits when another resync
is active while preserving existing error logging and cancellation behavior.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation