OSAC-3282, OSAC-3283: add Volume controller and feedback controller - #340
OSAC-3282, OSAC-3283: add Volume controller and feedback controller#340akshaynadkarni wants to merge 1 commit into
Conversation
|
@akshaynadkarni: This pull request references OSAC-3282 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.1.0" version, but no target version was set. This pull request references OSAC-3283 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.1.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: akshaynadkarni 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 |
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Warning Review limit reached
Next review available in: 51 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 selected for processing (15)
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 |
|
🤖 Finished Review · ✅ Success · Started 4:38 PM UTC · Completed 4:58 PM UTC Commit: |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsHigh
Medium
Low
Labels: PR adds Volume (block storage) controller and feedback controller to osac-operator Next steps:
|
![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.
Auto-dismissed: only Prow labels gate merging
Volume controller reconciles Volume CRs on the hub cluster. On create, it calls VendorProvisioner.CreateVolume to provision storage on the vendor backend array and updates the CR status with vendorVolumeID, backend, and protocol. On delete, it calls VendorProvisioner.DeleteVolume and removes the finalizer. Volume feedback controller watches Volume CR status changes and syncs them back to fulfillment-service via the Signal RPC, mapping CRD phase to proto state (Progressing->CREATING, Ready->AVAILABLE, Failed->FAILED, Deleting->DELETING). Adds OSAC_ENABLE_VOLUME_CONTROLLER flag to gate the controllers independently. Extracts all controller setup into setupControllers() to keep main() under the gocyclo complexity limit. Moves volume_names.go from api/v1alpha1/ to internal/controller/ since the name constants are only used by the controllers. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
48b1869 to
659aebc
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
🤖 Finished Review · ✅ Success · Started 6:10 PM UTC · Completed 6:26 PM UTC Commit: |
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
osac-operator/internal/controller/volume_controller.go:88: [medium] pattern-violation
The Volume controller does not check the osac.openshift.io/management-state annotation. Every other resource controller (ClusterOrder, ComputeInstance, VirtualNetwork, Subnet, SecurityGroup, ExternalIP*, NATGateway, Storage) checks this annotation and skips reconciliation when set to Unmanaged. The only documented exception is tenant_controller.go. Without this check, operators cannot mark a Volume as Unmanaged to prevent the controller from acting on it.
Suggested fix: Add the management-state annotation check after the Get call in Reconcile, following the established pattern in other controllers (e.g., natgateway_controller.go): if val, exists := vol.Annotations[osacManagementStateAnnotation]; vol.ObjectMeta.DeletionTimestamp.IsZero() && exists && val == ManagementStateUnmanaged { return ctrl.Result{}, nil }
osac-operator/internal/controller/volume_controller.go:109: [medium] pattern-inconsistency
The VolumeReconciler uses a bare r.Status().Update(ctx, vol) for status updates. All other provisioning controllers (NATGateway, Subnet, VirtualNetwork, SecurityGroup, ExternalIP*, ComputeInstance) use updateStatusWithRetry wrapping retry.RetryOnConflict to handle concurrent updates from the feedback controller. Without retry, status updates can fail with conflict errors when the feedback controller updates the same Volume CR concurrently.
Suggested fix: Add an updateStatusWithRetry method following the established pattern (e.g., natgateway_controller.go), and call it instead of the bare r.Status().Update.
osac-operator/charts/operator/templates/clusterrole.yaml(file-level): Line 79 · [low] permission-expansion
RBAC ClusterRole expanded with volumes, volumes/finalizers, and volumes/status grants under osac.openshift.io apiGroup. Verb sets are consistent with other resources in the same role. No excessive permissions.
osac-operator/charts/operator/templates/hub-access-clusterrole.yaml(file-level): Line 34 · [low] permission-expansion
Hub-access ClusterRole expanded with volumes and volumes/status. Gated by .Values.hubAccess.enabled (default false). Consistent with other resources.
osac-operator/config/rbac/role.yaml:112: [low] permission-expansion
Generated RBAC role.yaml expanded with volumes, volumes/finalizers, and volumes/status. Generated by make manifests from controller RBAC annotations. Consistent with other resources.
osac-operator/cmd/main.go(file-level): Line 211 · [low] scope-creep
The PR extracts all controller setup into setupControllers(), refactoring existing controllers beyond the stated OSAC-3282/OSAC-3283 scope. The refactoring is clean and the behavioral change (error wrapping format) is minor.
osac-operator/internal/controller/volume_names.go:28: [low] naming-convention
osacVolumeFinalizer declared as var via fmt.Sprintf in volume_names.go. Some other controllers use const in the controller file, though the pattern is mixed (e.g., computeinstance_names.go uses the same var approach).
osac-operator/internal/controller/volume_controller.go:100: [low] error-handling
After handleDelete removes the last finalizer, the subsequent r.Status().Update may hit a NotFound error on a garbage-collected object. Consistent with existing controller pattern; not a regression.
osac-operator/internal/controller/volume_controller_test.go:97: [low] test-inadequate
Test comments (First reconcile adds finalizer, Second reconcile attempts provisioning) are misleading — both happen in the first reconcile call. Assertions are correct but test structure is confusing.
osac-operator/cmd/main.go:174: [low] fail-open
enableAllIfNoneSet() enables ALL controllers (including Volume) when no flags are explicitly set. Consistent with existing all-or-nothing pattern. Helm values.yaml defaults volume: true.
osac-operator/AGENTS.md(file-level): Line 80 · [low] stale-doc
Provisioning section states management-state is checked by every controller except tenant_controller.go. Now also omitted by the Volume controller. Does not document the VendorProvisioner pattern.
osac-operator/README.md(file-level): Line 293 · [low] stale-doc
Controller enable flags section missing OSAC_ENABLE_VOLUME_CONTROLLER and OSAC_VOLUME_NAMESPACE. Pre-existing gap: storage and bare-metal flags also missing.
|
Addressing the two medium findings from fullsend's review: management-state annotation (line 88): Intentionally omitted. Volume CRs are always provisioned through the vendor CSI driver. The management-state annotation is for resources where an admin might suppress reconciliation to manage the resource directly (e.g., manually provisioned networking). That use case does not apply to volumes. This was discussed and agreed with Roy during PR #223 review. retry-on-conflict (line 109): Keeping the direct The low-severity findings (RBAC expansion, naming convention, test comments, docs) are acknowledged. The RBAC and permissions are consistent with other resources. Docs gaps are pre-existing. |
2 participants
Summary
OSAC-3282, OSAC-3283: Add Volume controller and feedback controller to osac-operator.
The Volume controller reconciles Volume CRs on the hub cluster: provisions via
VendorProvisioner.CreateVolume on create, deprovisions via DeleteVolume on delete,
and manages the volume-protection finalizer.
The feedback controller watches Volume CR status changes and syncs them back to
fulfillment-service via Signal RPC, mapping CRD phase to proto state.
Adds OSAC_ENABLE_VOLUME_CONTROLLER flag. Extracts all controller setup into
setupControllers() to stay under the gocyclo complexity limit. Moves
volume_names.go from api/v1alpha1/ to internal/controller/.
Split from #223 for focused review. This PR covers the osac-operator side only;
the fulfillment-service reconciler is in a separate PR.
Why
PR #223 (3,300+ lines) was too large for effective review. Splitting by component
boundary makes each PR independently reviewable. Roy and Zoltan can review this PR
without needing to review the fulfillment-service reconciler changes.
Testing
All unit tests pass. Build passes:
go build ./...Ticket
OSAC-3282, OSAC-3283 (under OSAC-3280 epic, under OSAC-2872 feature)
Signed-off-by: akshaynadkarni 25892229+akshaynadkarni@users.noreply.github.com
Assisted-by: Cursor/Claude