OSAC-2068: Make fabricManager optional on NetworkClass and support k8s-only deployments - #146
Merged
Conversation
|
@SiddarthR56: This pull request references OSAC-2068 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe change makes ChangesNetworkClass manager contract
Manager resolution and dispatch
NAT gateway validation
Bare-metal attachment validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NetworkClass
participant Resolver
participant Dispatcher
participant Controller
NetworkClass->>Resolver: provide manager identifiers
Resolver-->>Dispatcher: return optional managers
Dispatcher->>Dispatcher: select fallback and deduplicate targets
Dispatcher-->>Controller: return dispatch result
Controller->>Controller: skip sync when no fabric manager is resolved
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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 |
Contributor
Author
|
✅ Action performedReview finished.
|
![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
🤖 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_network_classes_server.go`:
- Around line 378-381: Update hasAnyManager to require at least one non-empty
fabric_manager or k8s_manager value, rather than relying on protobuf presence
helpers. Preserve the existing validation error for requests where both
identifiers are empty or absent, and add create and update coverage for
explicitly empty optional manager fields.
In `@fulfillment-service/it/it_nat_gateway_test.go`:
- Around line 365-387: Update the deferred cleanup handlers for k8sOnlyNC and
k8sOnlyVNId to check the errors returned by networkClassesClient.Delete and
virtualNetworksClient.Delete instead of discarding them. Use assertions or
DeferCleanup so cleanup failures are reported while preserving cleanup
execution.
🪄 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: 80913a7a-5b0c-476c-8ba8-f373a6912290
⛔ Files ignored due to path filters (2)
fulfillment-service/internal/api/osac/private/v1/network_class_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/network_class_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (20)
fulfillment-service/internal/servers/default_networking_provisioner_test.gofulfillment-service/internal/servers/nat_gateways_server_test.gofulfillment-service/internal/servers/network_classes_server_test.gofulfillment-service/internal/servers/private_baremetal_instances_server.gofulfillment-service/internal/servers/private_baremetal_instances_server_test.gofulfillment-service/internal/servers/private_nat_gateways_server.gofulfillment-service/internal/servers/private_nat_gateways_server_test.gofulfillment-service/internal/servers/private_network_classes_server.gofulfillment-service/internal/servers/private_tenants_server_test.gofulfillment-service/it/it_baremetal_instance_lifecycle_test.gofulfillment-service/it/it_compute_subnet_test.gofulfillment-service/it/it_nat_gateway_test.gofulfillment-service/it/it_tenant_lifecycle_test.gofulfillment-service/proto/private/osac/private/v1/network_class_type.protoosac-aap/collections/ansible_collections/osac/service/roles/enumerate_templates/tests/test.ymlosac-operator/pkg/dispatcher/dispatch.goosac-operator/pkg/dispatcher/dispatch_test.goosac-operator/pkg/dispatcher/dispatcher.goosac-operator/pkg/dispatcher/resolver.goosac-operator/pkg/dispatcher/resolver_test.go
danmanor
approved these changes
Aug 5, 2026
|
[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 |
Contributor
|
/retest |
|
Re-triggered failed runs:
|
|
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
osac-operator/internal/controller/networkclass_capabilities_controller.go (1)
185-204: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear stale capabilities before the early return.
When a NetworkClass changes from fabric-backed to Kubernetes-only,
nccan still contain capabilities from the previous reconciliation. Theresolved.FabricManager == nilpath at Lines 199-203 returns without clearing or persisting the field, so clients can continue to see stale fabric capabilities.Clear the controller-owned capabilities to the canonical empty representation and persist that state before returning. Add a regression test for a fabric-to-Kubernetes-only transition. Apply the same cleanup to the no-manager path if that state can retain old capabilities.
🤖 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 185 - 204, Update the resolved.FabricManager == nil early-return path to clear controller-owned capabilities using the canonical empty representation, persist the updated NetworkClass, then return. Apply the same cleanup to the ErrNoManagerConfigured path if it can retain stale capabilities, and add a regression test covering a fabric-backed to Kubernetes-only transition.
🤖 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.
Outside diff comments:
In `@osac-operator/internal/controller/networkclass_capabilities_controller.go`:
- Around line 185-204: Update the resolved.FabricManager == nil early-return
path to clear controller-owned capabilities using the canonical empty
representation, persist the updated NetworkClass, then return. Apply the same
cleanup to the ErrNoManagerConfigured path if it can retain stale capabilities,
and add a regression test covering a fabric-backed to Kubernetes-only
transition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 905d66a0-fd59-4c44-bc58-f6a1b1be2cf9
⛔ Files ignored due to path filters (2)
fulfillment-service/internal/api/osac/private/v1/network_class_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/network_class_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
fulfillment-service/internal/servers/default_networking_provisioner_test.gofulfillment-service/internal/servers/nat_gateways_server_test.gofulfillment-service/internal/servers/network_classes_server_test.gofulfillment-service/internal/servers/private_baremetal_instances_server.gofulfillment-service/internal/servers/private_baremetal_instances_server_test.gofulfillment-service/internal/servers/private_nat_gateways_server.gofulfillment-service/internal/servers/private_nat_gateways_server_test.gofulfillment-service/internal/servers/private_network_classes_server.gofulfillment-service/internal/servers/private_tenants_server_test.gofulfillment-service/it/it_baremetal_instance_lifecycle_test.gofulfillment-service/it/it_compute_subnet_test.gofulfillment-service/it/it_nat_gateway_test.gofulfillment-service/it/it_tenant_lifecycle_test.gofulfillment-service/proto/private/osac/private/v1/network_class_type.protoosac-aap/collections/ansible_collections/osac/service/roles/enumerate_templates/tests/test.ymlosac-operator/internal/controller/networkclass_capabilities_controller.goosac-operator/internal/controller/networkclass_capabilities_controller_test.goosac-operator/pkg/dispatcher/dispatch.goosac-operator/pkg/dispatcher/dispatch_test.goosac-operator/pkg/dispatcher/dispatcher.goosac-operator/pkg/dispatcher/resolver.goosac-operator/pkg/dispatcher/resolver_test.go
🚧 Files skipped from review as they are similar to previous changes (19)
- fulfillment-service/it/it_tenant_lifecycle_test.go
- fulfillment-service/it/it_compute_subnet_test.go
- fulfillment-service/internal/servers/private_network_classes_server.go
- fulfillment-service/internal/servers/private_tenants_server_test.go
- osac-aap/collections/ansible_collections/osac/service/roles/enumerate_templates/tests/test.yml
- fulfillment-service/internal/servers/nat_gateways_server_test.go
- fulfillment-service/internal/servers/private_nat_gateways_server_test.go
- fulfillment-service/internal/servers/default_networking_provisioner_test.go
- fulfillment-service/proto/private/osac/private/v1/network_class_type.proto
- osac-operator/pkg/dispatcher/dispatch.go
- fulfillment-service/it/it_nat_gateway_test.go
- osac-operator/pkg/dispatcher/resolver_test.go
- osac-operator/pkg/dispatcher/dispatcher.go
- fulfillment-service/it/it_baremetal_instance_lifecycle_test.go
- fulfillment-service/internal/servers/private_nat_gateways_server.go
- fulfillment-service/internal/servers/private_baremetal_instances_server_test.go
- fulfillment-service/internal/servers/private_baremetal_instances_server.go
- osac-operator/pkg/dispatcher/dispatch_test.go
- fulfillment-service/internal/servers/network_classes_server_test.go
…s-only deployments Assisted-by: Cursor/Claude
openshift-merge-bot
Bot
merged commit Aug 8, 2026
4956560
into
osac-project:main
38 of 39 checks passed
omer-vishlitzky
added a commit
to omer-vishlitzky/osac
that referenced
this pull request
Aug 9, 2026
…kClass) A rebase mishap on this branch pulled in a full revert of already-merged PR osac-project#146 (OSAC-2068: make fabricManager optional on NetworkClass, support k8s-only deployments) across 24 files spanning fulfillment-service, osac-operator, and osac-aap. It has no relation to this PR's single-chart Kind deployment work -- Kind's own values even disable the networking subsystem the reverted dispatcher code operates under. Left as-is, the revert would have made any NetworkClass created with an empty fabric_manager permanently stuck: the reinstated validation rejects an empty fabric_manager on Update, and the reinstated immutability check rejects setting it for the first time, with no path between the two. Restore all 24 files to main's state (git checkout main -- <paths>), re-verified against origin/main: - fulfillment-service: go build ./... and go test ./internal/servers/... both pass; buf generate against the restored proto produces zero diff (generated code already in sync) - osac-operator: go build ./... and go test ./pkg/dispatcher/... pass Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
omer-vishlitzky
added a commit
to omer-vishlitzky/osac
that referenced
this pull request
Aug 9, 2026
…kClass) A rebase mishap on this branch pulled in a full revert of already-merged PR osac-project#146 (OSAC-2068: make fabricManager optional on NetworkClass, support k8s-only deployments) across 24 files spanning fulfillment-service, osac-operator, and osac-aap. It has no relation to this PR's single-chart Kind deployment work -- Kind's own values even disable the networking subsystem the reverted dispatcher code operates under. Left as-is, the revert would have made any NetworkClass created with an empty fabric_manager permanently stuck: the reinstated validation rejects an empty fabric_manager on Update, and the reinstated immutability check rejects setting it for the first time, with no path between the two. Restore all 24 files to main's state (git checkout main -- <paths>), re-verified against origin/main: - fulfillment-service: go build ./... and go test ./internal/servers/... both pass; buf generate against the restored proto produces zero diff (generated code already in sync) - osac-operator: go build ./... and go test ./pkg/dispatcher/... pass Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
omer-vishlitzky
added a commit
to omer-vishlitzky/osac
that referenced
this pull request
Aug 10, 2026
…kClass) A rebase mishap on this branch pulled in a full revert of already-merged PR osac-project#146 (OSAC-2068: make fabricManager optional on NetworkClass, support k8s-only deployments) across 24 files spanning fulfillment-service, osac-operator, and osac-aap. It has no relation to this PR's single-chart Kind deployment work -- Kind's own values even disable the networking subsystem the reverted dispatcher code operates under. Left as-is, the revert would have made any NetworkClass created with an empty fabric_manager permanently stuck: the reinstated validation rejects an empty fabric_manager on Update, and the reinstated immutability check rejects setting it for the first time, with no path between the two. Restore all 24 files to main's state (git checkout main -- <paths>), re-verified against origin/main: - fulfillment-service: go build ./... and go test ./internal/servers/... both pass; buf generate against the restored proto produces zero diff (generated code already in sync) - osac-operator: go build ./... and go test ./pkg/dispatcher/... pass Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
This was referenced Aug 10, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes