OSAC-1755: Add capabilities intersection to NetworkClass reconciler - #127
Conversation
|
@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. |
WalkthroughThe operator now synchronizes NetworkClass capabilities from resolved network managers. It updates manager discovery labels, adds configurable periodic resynchronization, classifies missing fabric managers, wires the controller, computes capability intersections, paginates NetworkClass listing, and adds tests. ChangesNetworkClass capability synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ManagerConfigMap
participant Operator
participant NetworkClassCapabilitiesReconciler
participant DispatcherResolver
participant NetworkClassesClient
ManagerConfigMap->>NetworkClassCapabilitiesReconciler: manager registration event
NetworkClassCapabilitiesReconciler->>NetworkClassesClient: list NetworkClasses
NetworkClassCapabilitiesReconciler->>DispatcherResolver: resolve managers
DispatcherResolver-->>NetworkClassCapabilitiesReconciler: manager capabilities
NetworkClassCapabilitiesReconciler->>NetworkClassesClient: update changed capabilities
Operator->>NetworkClassCapabilitiesReconciler: periodic resync
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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: 1
🧹 Nitpick comments (4)
osac-operator/internal/controller/networkclass_capabilities_controller_test.go (2)
190-210: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis spec covers a skip path, not a failure path.
badNCreferences an unregistered manager.syncOneclassifies that throughnetworkmanager.IsManagerNotFoundat controller line 189 and returnsnil. SoReconcilenever populateserrs, and line 206 asserting no error is guaranteed regardless of the loop's error handling.The
errors.Joinaccumulation at controller lines 141-148 therefore has no coverage. Add a spec where one NetworkClass produces a genuine error — for example agetFuncthat returns a gRPC failure for one ID — and assert both thatReconcilereturns that error and that the healthy NetworkClass was still updated. Then rename this spec to state that it skips NetworkClasses with unregistered managers.🤖 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 `@osac-operator/internal/controller/networkclass_capabilities_controller_test.go` around lines 190 - 210, Rename the existing spec to describe skipping NetworkClasses with unregistered managers, preserving its current no-error and healthy-update assertions. Add a separate Reconcile test using the listing client’s getFunc to return a genuine gRPC error for one NetworkClass, then assert Reconcile returns that error while the healthy NetworkClass is still updated, covering errors.Join accumulation.
254-262: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case where the server over-reports
Total.The controller comment at lines 169-170 states that the
resp.GetSize() == 0check exists to stop an infinite loop when the server reports a total larger than what it returns.pagingNetworkClassClientalways derivesTotalfromlen(all), so it cannot produce that state, and the guard is untested.Add a stub that returns
Total: 100with an empty page and assert thatlistAllNetworkClassesreturns after one call.🧪 Suggested spec
It("stops when the server reports a total larger than the items it returns", func() { calls := 0 stub := &stubNetworkClassesClient{ listFunc: func(_ context.Context, _ *privatev1.NetworkClassesListRequest, _ ...grpc.CallOption) (*privatev1.NetworkClassesListResponse, error) { calls++ return &privatev1.NetworkClassesListResponse{Size: 0, Total: 100}, nil }, } reconciler := NewNetworkClassCapabilitiesReconciler(stub, nil, "default") items, err := reconciler.listAllNetworkClasses(ctx) Expect(err).NotTo(HaveOccurred()) Expect(items).To(BeEmpty()) Expect(calls).To(Equal(1)) })🤖 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 `@osac-operator/internal/controller/networkclass_capabilities_controller_test.go` around lines 254 - 262, Add a test alongside the existing listAllNetworkClasses paging cases using a stubNetworkClassesClient whose listFunc returns an empty response with Size 0 and Total 100. Assert that listAllNetworkClasses returns no error, an empty item list, and exactly one client call, covering the over-reported Total termination guard.osac-operator/internal/controller/networkclass_capabilities_controller.go (2)
85-88: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffScope the ConfigMap watch to manager-registration ConfigMaps.
For(&corev1.ConfigMap{}, ...)caches ConfigMaps across the cluster, andmanagerConfigMapPredicateonly drops events after storage. Use the manager'scache.Options.ByObjectforcorev1.ConfigMapwith the networking namespace plus the manager labels in the predicate. Alignosac-operator/config/rbac/role.yamlwith the selector and keep read-only RBAC narrow, so the object cache and RBAC surface do not exceed what this controller needs.🤖 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 `@osac-operator/internal/controller/networkclass_capabilities_controller.go` around lines 85 - 88, Scope the ConfigMap cache in the controller setup around managerConfigMapPredicate by configuring the manager cache’s cache.Options.ByObject entry for corev1.ConfigMap with the networking namespace and manager-label predicate. Update osac-operator/config/rbac/role.yaml to match that namespace and object scope while retaining only read-only permissions required by the controller.
202-208: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSet a field mask and lock the NetworkClass update.
Updatehasupdate_maskandlockfields, but this request sends onlyObject; the capabilities-only change is lost if the server performs a full replacement, and concurrent writer changes can be overwritten. Setupdate_masktocapabilitiesand enablelockwith the currentmetadata.version.🤖 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 `@osac-operator/internal/controller/networkclass_capabilities_controller.go` around lines 202 - 208, Update the NetworkClass request in the capabilities update flow after nc.SetCapabilities to set update_mask to capabilities and enable lock using the current metadata.version. Preserve the existing object and error handling while ensuring the Update request protects concurrent changes and applies only the capabilities field.
🤖 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 `@osac-operator/cmd/main.go`:
- Around line 582-586: Update the validator passed to GetEnvWithDefault for
envNetworkClassSyncInterval so it accepts only durations within the required
minimum resync floor and a defined upper bound, preserving the default fallback
for invalid or out-of-range values before syncInterval reaches time.NewTicker.
---
Nitpick comments:
In
`@osac-operator/internal/controller/networkclass_capabilities_controller_test.go`:
- Around line 190-210: Rename the existing spec to describe skipping
NetworkClasses with unregistered managers, preserving its current no-error and
healthy-update assertions. Add a separate Reconcile test using the listing
client’s getFunc to return a genuine gRPC error for one NetworkClass, then
assert Reconcile returns that error while the healthy NetworkClass is still
updated, covering errors.Join accumulation.
- Around line 254-262: Add a test alongside the existing listAllNetworkClasses
paging cases using a stubNetworkClassesClient whose listFunc returns an empty
response with Size 0 and Total 100. Assert that listAllNetworkClasses returns no
error, an empty item list, and exactly one client call, covering the
over-reported Total termination guard.
In `@osac-operator/internal/controller/networkclass_capabilities_controller.go`:
- Around line 85-88: Scope the ConfigMap cache in the controller setup around
managerConfigMapPredicate by configuring the manager cache’s
cache.Options.ByObject entry for corev1.ConfigMap with the networking namespace
and manager-label predicate. Update osac-operator/config/rbac/role.yaml to match
that namespace and object scope while retaining only read-only permissions
required by the controller.
- Around line 202-208: Update the NetworkClass request in the capabilities
update flow after nc.SetCapabilities to set update_mask to capabilities and
enable lock using the current metadata.version. Preserve the existing object and
error handling while ensuring the Update request protects concurrent changes and
applies only the capabilities field.
🪄 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: 8346f7bf-ffc9-430e-a3f0-1fa42ae027ef
📒 Files selected for processing (12)
osac-operator/.claude/rules/configuration.mdosac-operator/charts/operator/templates/deployment.yamlosac-operator/charts/operator/templates/network-managers.yamlosac-operator/charts/operator/values.yamlosac-operator/cmd/main.goosac-operator/config/samples/network-manager-fabric-netris.yamlosac-operator/internal/controller/dispatcher_resolver_helpers_test.goosac-operator/internal/controller/networkclass_capabilities_controller.goosac-operator/internal/controller/networkclass_capabilities_controller_test.goosac-operator/pkg/dispatcher/resolver.goosac-operator/pkg/networkmanager/doc.goosac-operator/pkg/networkmanager/types.go
|
/retest |
|
Re-triggered failed runs:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@osac-operator/internal/controller/networkclass_capabilities_controller_test.go`:
- Line 107: Report cleanup failures instead of discarding Delete errors in
osac-operator/internal/controller/networkclass_capabilities_controller_test.go:107-107
for fabricCM, 111-111 for k8sCM, 139-139 for fabricCM, and 193-193 for fabricCM.
Update each deferred cleanup around k8sClient.Delete to assert that deletion
succeeds.
In `@osac-operator/internal/controller/networkclass_capabilities_controller.go`:
- Around line 94-100: Update managerConfigMapPredicate to handle update events
using both ObjectOld and ObjectNew: accept the event when either ConfigMap has
the target namespace and at least one manager registration label, including when
the last label is removed. Preserve the existing filtering for other event
types, and add a regression test covering removal of the final manager label and
reconciliation triggering.
🪄 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: 7837528f-3d65-491c-905b-0b3258b2a449
📒 Files selected for processing (12)
osac-operator/.claude/rules/configuration.mdosac-operator/charts/operator/templates/deployment.yamlosac-operator/charts/operator/templates/network-managers.yamlosac-operator/charts/operator/values.yamlosac-operator/cmd/main.goosac-operator/config/samples/network-manager-fabric-netris.yamlosac-operator/internal/controller/dispatcher_resolver_helpers_test.goosac-operator/internal/controller/networkclass_capabilities_controller.goosac-operator/internal/controller/networkclass_capabilities_controller_test.goosac-operator/pkg/dispatcher/resolver.goosac-operator/pkg/networkmanager/doc.goosac-operator/pkg/networkmanager/types.go
🚧 Files skipped from review as they are similar to previous changes (10)
- osac-operator/pkg/networkmanager/types.go
- osac-operator/charts/operator/values.yaml
- osac-operator/.claude/rules/configuration.md
- osac-operator/charts/operator/templates/deployment.yaml
- osac-operator/pkg/networkmanager/doc.go
- osac-operator/cmd/main.go
- osac-operator/pkg/dispatcher/resolver.go
- osac-operator/internal/controller/dispatcher_resolver_helpers_test.go
- osac-operator/charts/operator/templates/network-managers.yaml
- osac-operator/config/samples/network-manager-fabric-netris.yaml
| It("computes the intersection and updates the NetworkClass when capabilities changed", func() { | ||
| fabricCM := newFabricManagerConfigMap("fm-caps-fabric", namespace, "fabric-caps-1") | ||
| Expect(k8sClient.Create(ctx, fabricCM)).To(Succeed()) | ||
| defer func() { _ = k8sClient.Delete(ctx, fabricCM) }() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report ConfigMap cleanup failures.
These deferred cleanup calls discard k8sClient.Delete errors. A failed cleanup can leave a manager registration ConfigMap in the test environment and affect later specs or retries. Assert that each cleanup succeeds.
osac-operator/internal/controller/networkclass_capabilities_controller_test.go#L107-L107: report deletion failure forfabricCM.osac-operator/internal/controller/networkclass_capabilities_controller_test.go#L111-L111: report deletion failure fork8sCM.osac-operator/internal/controller/networkclass_capabilities_controller_test.go#L139-L139: report deletion failure forfabricCM.osac-operator/internal/controller/networkclass_capabilities_controller_test.go#L193-L193: report deletion failure forfabricCM.
As per path instructions, “Never ignore error returns.”
📍 Affects 1 file
osac-operator/internal/controller/networkclass_capabilities_controller_test.go#L107-L107(this comment)osac-operator/internal/controller/networkclass_capabilities_controller_test.go#L111-L111osac-operator/internal/controller/networkclass_capabilities_controller_test.go#L139-L139osac-operator/internal/controller/networkclass_capabilities_controller_test.go#L193-L193
🤖 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
`@osac-operator/internal/controller/networkclass_capabilities_controller_test.go`
at line 107, Report cleanup failures instead of discarding Delete errors in
osac-operator/internal/controller/networkclass_capabilities_controller_test.go:107-107
for fabricCM, 111-111 for k8sCM, 139-139 for fabricCM, and 193-193 for fabricCM.
Update each deferred cleanup around k8sClient.Delete to assert that deletion
succeeds.
Source: Path instructions
| return predicate.NewPredicateFuncs(func(obj clnt.Object) bool { | ||
| if obj.GetNamespace() != namespace { | ||
| return false | ||
| } | ||
| labels := obj.GetLabels() | ||
| return labels[networkmanager.LabelFabricManager] == "true" || labels[networkmanager.LabelK8sManager] == "true" | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
go_mod="$(fd -a '^go\.mod$' . | head -n 1)"
rg -n -C 3 'sigs\.k8s\.io/controller-runtime' "$go_mod"
rg -n -C 8 'managerConfigMapPredicate|NewPredicateFuncs' osac-operator/internal/controllerRepository: osac-project/osac
Length of output: 13883
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Fetch the relevant controller-runtime source for v0.24.1's NewPredicateFuncs behavior.
curl -fsSL 'https://raw.githubusercontent.com/kubernetes-sigs/controller-runtime/v0.24.1/pkg/predicate/predicate.go' | sed -n '1,220p'
# Inspect the NetworkClass capabilities controller relevant methods around sync/reconcile.
wc -l osac-operator/internal/controller/networkclass_capabilities_controller.go
sed -n '1,240p' osac-operator/internal/controller/networkclass_capabilities_controller.goRepository: osac-project/osac
Length of output: 18609
Handle ConfigMap label removal in managerConfigMapPredicate.
With controller-runtime NewPredicateFuncs, Update events run the filter against ObjectNew only. Removing the last manager registration label can make the Update predicate return false while ObjectOld still matched, so the ConfigMap change skips Reconcile and resyncAllLocked() until the periodic sync. For Update events, accept the event when either ObjectOld or ObjectNew has a manager registration label, and add a regression test for label removal.
🤖 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 `@osac-operator/internal/controller/networkclass_capabilities_controller.go`
around lines 94 - 100, Update managerConfigMapPredicate to handle update events
using both ObjectOld and ObjectNew: accept the event when either ConfigMap has
the target namespace and at least one manager registration label, including when
the last label is removed. Preserve the existing filtering for other event
types, and add a regression test covering removal of the final manager label and
reconciliation triggering.
danmanor
left a comment
danmanor
left a comment
There was a problem hiding this comment.
Reviewed the full diff against the unified networking design (OSAC-1433).
Design alignment — good:
- Capabilities intersection logic directly implements the design: "Capabilities are inferred from the assigned managers and published in the NetworkClass status — the provider does not set them manually. The operator computes the intersection of capabilities declared by the fabric manager and k8sManager ConfigMaps and populates status.capabilities automatically."
- fabric ∩ k8s when both present, fabric-only when no k8s manager — correct per design
- All 4 capabilities covered:
supports_ipv4,supports_ipv6,supports_dual_stack,dpu_support - ConfigMap-triggered reconciliation + periodic resync (for NC-side spec changes with no k8s event source) is a sound approach
- Pagination on
listAllNetworkClassesavoids the silent truncation from the server's default page size disable_capabilitiessubtraction deferred to OSAC-2030 — noted in code, matches the design'sspec.disableCapabilitiesfieldresyncMu+TryLockon the periodic path avoids races without blocking context cancellation
Label rename (osac.openshift.io/network/fabric-manager → osac.openshift.io/network-fabric-manager): K8s labels allow at most one /, so the old format was never valid. This is a bug fix. The design doc examples (OSAC-1433) and any existing ConfigMap samples will need updating to match — but since the old labels would have silently failed selectors, no deployment could have been relying on them.
Coordination note: PR #126 (OSAC-1460, dispatcher wiring) shares the dispatcher_resolver_helpers_test.go file and the ErrFabricManagerNotSet sentinel. These two PRs need to merge in a coordinated order or rebase on each other to avoid conflicts.
Assisted-by: Claude Code noreply@anthropic.com
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, SiddarthR56 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 |
|
/retest |
|
Re-triggered failed runs:
|
|
/retest |
|
Re-triggered failed runs:
|
Assisted-by: Cursor/Claude
|
New changes are detected. LGTM label has been removed. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
osac-operator/internal/controller/networkclass_capabilities_controller_test.go (1)
190-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise a real resolver failure.
The bad NetworkClass references an unregistered manager.
syncOnehandlesnetworkmanager.IsManagerNotFound(err)as a successful skip. This test does not verify that a real resolver error is joined while the valid NetworkClass still updates.Make the resolver return an error that is not
ErrFabricManagerNotSetorIsManagerNotFound, then assert both the valid update and the returned error.🤖 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 `@osac-operator/internal/controller/networkclass_capabilities_controller_test.go` around lines 190 - 206, Update the test around syncOne and the stub resolver so the bad NetworkClass produces a genuine resolver error other than ErrFabricManagerNotSet or networkmanager.IsManagerNotFound. After Reconcile, assert the valid NetworkClass was updated and the returned error is present and contains the resolver failure, verifying the error is joined while processing continues.
🤖 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.
Nitpick comments:
In
`@osac-operator/internal/controller/networkclass_capabilities_controller_test.go`:
- Around line 190-206: Update the test around syncOne and the stub resolver so
the bad NetworkClass produces a genuine resolver error other than
ErrFabricManagerNotSet or networkmanager.IsManagerNotFound. After Reconcile,
assert the valid NetworkClass was updated and the returned error is present and
contains the resolver failure, verifying the error is joined while processing
continues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 28b4c816-d999-43a6-be04-a685539e09c1
📒 Files selected for processing (12)
osac-operator/.claude/rules/configuration.mdosac-operator/charts/operator/templates/deployment.yamlosac-operator/charts/operator/templates/network-managers.yamlosac-operator/charts/operator/values.yamlosac-operator/cmd/main.goosac-operator/config/samples/network-manager-fabric-netris.yamlosac-operator/internal/controller/dispatcher_resolver_helpers_test.goosac-operator/internal/controller/networkclass_capabilities_controller.goosac-operator/internal/controller/networkclass_capabilities_controller_test.goosac-operator/pkg/dispatcher/resolver.goosac-operator/pkg/networkmanager/doc.goosac-operator/pkg/networkmanager/types.go
🚧 Files skipped from review as they are similar to previous changes (10)
- osac-operator/pkg/networkmanager/doc.go
- osac-operator/charts/operator/templates/network-managers.yaml
- osac-operator/.claude/rules/configuration.md
- osac-operator/config/samples/network-manager-fabric-netris.yaml
- osac-operator/charts/operator/templates/deployment.yaml
- osac-operator/pkg/dispatcher/resolver.go
- osac-operator/charts/operator/values.yaml
- osac-operator/pkg/networkmanager/types.go
- osac-operator/internal/controller/dispatcher_resolver_helpers_test.go
- osac-operator/cmd/main.go
3 participants
Summary by CodeRabbit
New Features
Configuration
Bug Fixes