OSAC-3273, OSAC-3280: Volume reconciler, controller, and feedback controller - #223
OSAC-3273, OSAC-3280: Volume reconciler, controller, and feedback controller#223akshaynadkarni wants to merge 13 commits into
Conversation
|
@akshaynadkarni: This pull request references OSAC-2872 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 feature 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded end-to-end Volume lifecycle support. Fulfillment reconciliation creates hub resources, the operator provisions vendor volumes, and feedback synchronization updates the fulfillment service. Startup wiring, RBAC, mocks, tests, labels, and documentation were added. ChangesVolume lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: ⚪ Minimal · up to The change adds volume lifecycle behavior, while the only remaining issue is a localized test robustness gap where setup errors are ignored. It has no direct production impact and presents no actionable merge-blocking risk beyond normal review. Sequence Diagram(s)sequenceDiagram
participant FulfillmentVolumeReconciler
participant HubKubernetesAPI
participant OperatorVolumeReconciler
participant VendorProvisioner
participant VolumeFeedbackReconciler
participant FulfillmentVolumesAPI
FulfillmentVolumeReconciler->>HubKubernetesAPI: Create or patch Volume
OperatorVolumeReconciler->>VendorProvisioner: CreateVolume
VendorProvisioner-->>OperatorVolumeReconciler: Return vendor metadata
OperatorVolumeReconciler->>HubKubernetesAPI: Persist status
VolumeFeedbackReconciler->>HubKubernetesAPI: Read Volume status
VolumeFeedbackReconciler->>FulfillmentVolumesAPI: Update remote status
Possibly related PRs
Suggested labels: 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 |
GitHub Actions
Failure
GitHub Actions
Error
GitHub Actions
Error
|
@coderabbitai review |
✅ 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: 15
🧹 Nitpick comments (11)
fulfillment-service/internal/controllers/volume/volume_reconciler_function.go (1)
270-274: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRoute the Delete error through
HandleK8sWriteErrorfor consistency.
Createat Line 196 andPatchat Line 209 both convert terminal Kubernetes write errors into a FAILED status throughcontrollers.HandleK8sWriteError.Deletereturns the raw error. A terminal rejection on delete therefore retries forever and never reaches the volume status.♻️ Proposed change
if object.GetDeletionTimestamp() == nil { err = t.hubClient.Delete(ctx, object) if err != nil { - return + return controllers.HandleK8sWriteError(ctx, t.r.logger, err, t.setFailed) }🤖 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/controllers/volume/volume_reconciler_function.go` around lines 270 - 274, Update the Delete error path in the volume reconciler to pass failures from t.hubClient.Delete through controllers.HandleK8sWriteError, matching the existing Create and Patch handling and ensuring terminal write errors update volume status instead of being returned raw.fulfillment-service/internal/controllers/volume/volume_reconciler_function_test.go (1)
626-700: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for the update branch that patches an existing CR.
This spec covers the Create failure path only. The Patch branch in
volume_reconciler_function.goLines 204-217 and the duplicate-CR error ingetKubeObjectLines 350-356 have no spec. A fake client seeded with an existing CR that carries thelabels.VolumeUuidlabel would cover the Patch branch cheaply.🤖 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/controllers/volume/volume_reconciler_function_test.go` around lines 626 - 700, Extend the “Kubernetes validation error handling” spec to seed the fake client with an existing CR labeled by labels.VolumeUuid for the test volume, then exercise f.run through the existing-object path so the Patch branch in getKubeObject/volume reconciliation is covered. Assert the Invalid error from patching produces the same FAILED state and validation message, and add coverage for the duplicate-CR error returned by getKubeObject.fulfillment-service/proto/private/osac/private/v1/volume_type.proto (1)
124-153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
VolumeStatus.hubandbackendare documented as private-only, but nothing enforces that.The comments say
backendandprotocolare "Visible only through the private API". This file is underproto/private, so that holds by placement, not by a rule. If a public Volume type is added later and copies this message, the constraint is lost. Consider recording the restriction in the shared rendering/masking metadata instead of a comment only.🤖 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/proto/private/osac/private/v1/volume_type.proto` around lines 124 - 153, Update the shared rendering/masking metadata for VolumeStatus fields backend, protocol, and hub to explicitly mark them as private-only, rather than relying solely on comments or the proto/private package location. Use the existing metadata mechanism and preserve the current field definitions and behavior.fulfillment-service/internal/servers/private_volumes_server_test.go (2)
73-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree near-identical volume factories.
createVolumeequalscreateVolumeWithName("test-volume").createStandaloneVolumeequalscreateVolumeWithName("standalone-volume")with a different size. Collapse them into one helper that takes the name and the size.♻️ Proposed consolidation
- createVolume := func() *privatev1.Volume { - response, err := server.Create(ctx, privatev1.VolumesCreateRequest_builder{ - Object: privatev1.Volume_builder{ - Metadata: privatev1.Metadata_builder{ - Name: "test-volume", - }.Build(), - Spec: privatev1.VolumeSpec_builder{ - StorageTier: "gold", - SizeGib: 100, - AccessMode: privatev1.VolumeAccessMode_VOLUME_ACCESS_MODE_READ_WRITE_ONCE, - }.Build(), - }.Build(), - }.Build()) - Expect(err).ToNot(HaveOccurred()) - return response.GetObject() - } - - createVolumeWithName := func(name string) *privatev1.Volume { + createVolumeWithNameAndSize := func(name string, sizeGib int64) *privatev1.Volume { response, err := server.Create(ctx, privatev1.VolumesCreateRequest_builder{ Object: privatev1.Volume_builder{ Metadata: privatev1.Metadata_builder{ Name: name, }.Build(), Spec: privatev1.VolumeSpec_builder{ StorageTier: "gold", - SizeGib: 100, + SizeGib: sizeGib, AccessMode: privatev1.VolumeAccessMode_VOLUME_ACCESS_MODE_READ_WRITE_ONCE, }.Build(), }.Build(), }.Build()) Expect(err).ToNot(HaveOccurred()) return response.GetObject() } - createStandaloneVolume := func() *privatev1.Volume { - response, err := server.Create(ctx, privatev1.VolumesCreateRequest_builder{ - Object: privatev1.Volume_builder{ - Metadata: privatev1.Metadata_builder{ - Name: "standalone-volume", - }.Build(), - Spec: privatev1.VolumeSpec_builder{ - StorageTier: "gold", - SizeGib: 50, - AccessMode: privatev1.VolumeAccessMode_VOLUME_ACCESS_MODE_READ_WRITE_ONCE, - }.Build(), - }.Build(), - }.Build()) - Expect(err).ToNot(HaveOccurred()) - return response.GetObject() + createVolume := func() *privatev1.Volume { + return createVolumeWithNameAndSize("test-volume", 100) } + + createVolumeWithName := func(name string) *privatev1.Volume { + return createVolumeWithNameAndSize(name, 100) + } + + createStandaloneVolume := func() *privatev1.Volume { + return createVolumeWithNameAndSize("standalone-volume", 50) + }🤖 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_volumes_server_test.go` around lines 73 - 122, Consolidate the three local volume factory closures into a single helper that accepts both the volume name and size. Update createVolume, createVolumeWithName, and createStandaloneVolume call sites to use this helper with their respective values, while preserving the shared storage tier, access mode, request construction, error assertion, and returned object behavior.
235-263: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUpdate coverage misses the immutability guarantee and the
lockpath.
VolumeSpecis documented as immutable after creation. No test attempts aspec.*field-mask update to confirm the server rejects it. No test setslock: trueto confirm optimistic locking rejects a stalemetadata.version.Add both cases. A retrieved learning asks for comprehensive Update branch coverage on private servers in this package.
🤖 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_volumes_server_test.go` around lines 235 - 263, Add Update tests alongside the existing partial field-mask test in the private volume server tests: attempt a spec.* field-mask update and assert the server rejects it without mutating the immutable VolumeSpec, then set lock: true with a stale metadata.version and assert optimistic locking rejects the update. Reuse the existing createVolume and server.Update setup and verify the expected error outcomes.Source: Learnings
fulfillment-service/proto/private/osac/private/v1/volumes_service.proto (1)
31-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe filter example uses a magic enum number.
this.status.state == 2requires the reader to look up that2isVOLUME_STATE_AVAILABLE. If the CEL evaluator accepts enum value names, use the name in the example. If it does not, name the state in the prose so the example stays readable.🤖 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/proto/private/osac/private/v1/volumes_service.proto` around lines 31 - 43, Update the filter example near the filter field to use the readable enum name VOLUME_STATE_AVAILABLE if CEL supports enum value names; otherwise keep the numeric comparison and explicitly identify 2 as VOLUME_STATE_AVAILABLE in the surrounding prose.osac-operator/api/v1alpha1/volume_types.go (1)
39-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated kubebuilder enum markers produce redundant
allOfblocks in both generated CRDs.VolumeAccessModeandVolumeProtocoleach carry a+kubebuilder:validation:Enummarker on the named type and again on every field that uses the type. controller-gen emits both, so each generated property contains anallOfwith two identicalenumlists. The schema behaves correctly, but it is larger and harder to read, and the two copies can drift when a value is added to only one marker.
osac-operator/api/v1alpha1/volume_types.go#L39-L44: remove the field-level+kubebuilder:validation:Enum=ReadWriteOnce;ReadOnlyMany;ReadWriteMany;ReadWriteOncePodonAccessMode(line 42) and the matching field-level marker onProtocol(line 152); keep the markers on theVolumeAccessModetype (line 86) and theVolumeProtocoltype (line 97), then runmake manifests.osac-operator/charts/operator-crds/templates/osac.openshift.io_volumes.yaml#L66-L78: regenerate soaccessMode(lines 66-78) andprotocol(lines 213-221) each carry a single flatenuminstead ofallOf.osac-operator/config/crd/bases/osac.openshift.io_volumes.yaml#L65-L76: regenerate soaccessMode(lines 65-76) andprotocol(lines 212-219) each carry a single flatenuminstead ofallOf.🤖 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/api/v1alpha1/volume_types.go` around lines 39 - 44, Remove the field-level Enum markers from AccessMode and Protocol in osac-operator/api/v1alpha1/volume_types.go, while retaining the Enum markers on the VolumeAccessMode and VolumeProtocol type definitions; then run make manifests. Regenerate osac-operator/charts/operator-crds/templates/osac.openshift.io_volumes.yaml (accessMode lines 66-78 and protocol lines 213-221) and osac-operator/config/crd/bases/osac.openshift.io_volumes.yaml (accessMode lines 65-76 and protocol lines 212-219) so each property has one flat enum rather than a duplicated allOf.osac-operator/api/v1alpha1/volume_names.go (1)
36-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate
VolumeCleanupFinalizerto describe only the constant’s future use.
VolumeCleanupFinalizeris declared but not added to anyClusterOrderin this PR. If cleanup logic remains pending, keep the constant but rewrite lines 36-37 to say it is intended for future use or the follow-up behavior instead of asserting it currently blocks cluster deletion.🤖 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/api/v1alpha1/volume_names.go` around lines 36 - 38, Update the comment for VolumeCleanupFinalizer to describe it as reserved or intended for future cleanup behavior, without claiming it is currently added to ClusterOrder or blocks cluster deletion. Keep the constant value unchanged.osac-operator/internal/controller/volume_mock_provisioner.go (1)
25-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the mock out of the production build.
The type carries a "test-only" comment, but the file is
volume_mock_provisioner.go, not_test.go. The mock therefore compiles into the operator binary and any caller can wire it as a realVendorProvisioner. Rename the file tovolume_mock_provisioner_test.go, or move it to atestingsubpackage if other packages need it.🤖 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/volume_mock_provisioner.go` around lines 25 - 44, Move MockVendorProvisioner and NewMockVendorProvisioner out of the production build by renaming volume_mock_provisioner.go to volume_mock_provisioner_test.go; if external packages require the mock, place it in a dedicated testing subpackage instead. Preserve its existing test behavior and VendorProvisioner compatibility.osac-operator/internal/controller/volume_controller.go (1)
250-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the embedded client for consistency.
handleUpdatecallsr.Update, but this path callsr.mgr.GetLocalManager().GetClient().Update. The constructor assigns that same client to the embeddedClient, so the behavior matches. User.Updatehere to keep one access path.♻️ Proposed refactor
if controllerutil.RemoveFinalizer(vol, v1alpha1.VolumeFinalizer) { - if err := r.mgr.GetLocalManager().GetClient().Update(ctx, vol); err != nil { + if err := r.Update(ctx, vol); err != nil { return ctrl.Result{}, 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 `@osac-operator/internal/controller/volume_controller.go` around lines 250 - 254, Update the finalizer-removal path in handleUpdate to call the controller’s embedded r.Update method instead of r.mgr.GetLocalManager().GetClient().Update, preserving the existing context, volume object, and error-return behavior.osac-operator/internal/controller/volume_feedback_controller_test.go (1)
408-461: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the assertion reads with the same mutex.
UpdateandSignalwritem.updatesandm.signalson the gRPC server goroutine underm.mu. The specs read those slices directly with no lock. The RPC completes before the read, so the values are correct, butgo test -racecan still report the access as unsynchronized. Add small accessors that take the lock.Separately,
Signalappends the ID before it checkssignalErr, so a failed signal is still recorded. Move the error check first if a test ever asserts on that.♻️ Proposed refactor
+func (m *mockVolumesServer) updateCount() int { + m.mu.Lock() + defer m.mu.Unlock() + return len(m.updates) +} + +func (m *mockVolumesServer) updateAt(i int) *privatev1.Volume { + m.mu.Lock() + defer m.mu.Unlock() + return m.updates[i] +} + +func (m *mockVolumesServer) signalIDs() []string { + m.mu.Lock() + defer m.mu.Unlock() + return append([]string(nil), m.signals...) +} + func (m *mockVolumesServer) Signal(_ context.Context, req *privatev1.VolumesSignalRequest) (*privatev1.VolumesSignalResponse, error) { m.mu.Lock() defer m.mu.Unlock() - m.signals = append(m.signals, req.GetId()) - if m.signalErr != nil { return nil, m.signalErr } + m.signals = append(m.signals, req.GetId()) return &privatev1.VolumesSignalResponse{}, nil }Note: the test at line 289 expects one signal after a successful call, and the test at line 298 does not assert signals, so this reordering is safe.
🤖 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/volume_feedback_controller_test.go` around lines 408 - 461, Protect test assertions by adding mutex-guarded accessors for the mockVolumesServer updates and signals slices, and use them instead of direct reads. In Signal, check signalErr before appending the request ID so failed signals are not recorded; preserve recording for successful calls.
🤖 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/controllers/volume/volume_reconciler_function.go`:
- Around line 304-307: Update the hub-selection logic in the reconciler function
to guard against a nil response before reading response.Items, returning the
existing “no hubs available” error when the response or its Items field is nil
or empty. Only call rand.IntN and select a hub after this validation.
In
`@fulfillment-service/internal/database/migrations/94_create_volumes_tables_test.go`:
- Around line 131-165: Update the immutability test around its existing id,
name, and tenant assertions to cover the project column as well: execute an
update of volumes.project and assert it fails with the same Z0001 error code.
Keep project in the test name only if this assertion is added.
In
`@fulfillment-service/internal/database/migrations/94_create_volumes_tables.up.sql`:
- Around line 71-75: Update the comment above trigger check_immutable_columns to
list name alongside id, tenant, and project, matching the columns passed to the
trigger and the enforced immutability behavior.
In `@fulfillment-service/internal/servers/private_volumes_server_test.go`:
- Around line 324-413: Move the five Create validation cases from the direct
server tests around server.Create into an integration test that invokes the full
gRPC interceptor chain. Add declarative buf.validate rules in the Volume schema
for required spec and non-zero/required access_mode, while preserving the
existing storage_tier, size_gib, and metadata.name constraints; do not add
duplicate validation to server handlers.
In `@fulfillment-service/internal/servers/private_volumes_server.go`:
- Around line 126-129: Update the Create flow in the private volumes server to
always replace the volume’s existing status with a new empty
privatev1.VolumeStatus, rather than only initializing it when nil. Then set the
fresh status state to VOLUME_STATE_CREATING, ensuring caller-provided
operational fields are discarded before generic.Create persists the volume.
In `@osac-operator/api/v1alpha1/volume_types.go`:
- Around line 33-37: Bound the requested volume size in the SizeGiB validation
markers using the platform’s maximum supported capacity, preventing overflow
during byte conversion. Apply the identical maximum to the size_gib field in the
volume_type.proto schema so API and fulfillment-service validation remain
consistent.
- Line 24: Align the pvcRef validation in the Volume spec with the status
contract: move the immutability rule from the pvcRef field to the Volume
struct-level validation, using optionalOldSelf when needed so API-created
volumes cannot later receive a PVC reference. If late CSI-driver population is
intentionally supported, preserve that exception and document it instead.
In `@osac-operator/cmd/main.go`:
- Around line 479-501: Resolve an empty volumeNamespace to
defaultVolumeNamespace once in setupVolumeControllers before constructing either
reconciler, so both NewVolumeFeedbackReconciler and NewVolumeReconciler receive
the same namespace; update osac-operator/cmd/main.go lines 479-501. No direct
change is required in
osac-operator/internal/controller/volume_feedback_controller.go lines 92-102
because the resolved value fixes its constructor input.
In `@osac-operator/internal/controller/volume_controller_test.go`:
- Around line 92-106: Rename the test case around the first reconcile in
volume_controller_test.go to state that it sets the phase to Ready, matching the
VolumePhaseReady assertion and its inline comment.
In `@osac-operator/internal/controller/volume_controller.go`:
- Around line 140-151: The Reconcile status update must tolerate the object
disappearing after handleDelete removes the finalizer. In
osac-operator/internal/controller/volume_controller.go:140-151, wrap
updateStatusWithRetry in client.IgnoreNotFound (or skip the write when deletion
completed). In
osac-operator/internal/controller/volume_controller_test.go:217-244, ensure the
test reaches the post-delete status write and retain the
Expect(err).ToNot(HaveOccurred()) assertion as the regression guard.
- Around line 200-205: Update the provisioning error path in the volume
reconciliation handler to return a delayed requeue result instead of an empty
result with nil error. Preserve the existing failure status and condition
updates, and use the controller’s established backoff duration if one is
available so transient vendor failures retry while the condition remains
visible.
- Around line 297-315: Update setVendorProvisionedCondition to delegate
condition insertion and updates to apimeta.SetStatusCondition, so unchanged
statuses still refresh Reason and Message while LastTransitionTime changes only
on status transitions. Preserve the existing VendorProvisioned condition type
and input fields.
In `@osac-operator/internal/controller/volume_feedback_controller_test.go`:
- Around line 80-98: Close the gRPC client connection during AfterEach by
retaining the connection created in BeforeEach and calling its close method
during teardown. Declare the connection and error separately to avoid shadowing
the outer connection variable, and guard the close when no connection was
created.
- Around line 183-201: In the volume feedback controller tests, replace the
thinking-out-loud comments in “should not overwrite remote fields when CR fields
are empty” with a concise conclusion explaining that no update is expected. In
the vendor-field propagation test, assert updated.GetStatus().GetProtocol()
equals privatev1.StorageProtocol_STORAGE_PROTOCOL_NFS to verify the protocol
mapping.
In `@osac-operator/internal/controller/volume_feedback_controller.go`:
- Around line 163-165: Replace the StorageProtocol_value lookup in
volume_feedback_controller.go:163-165 with an explicit switch mapping each
v1alpha1.VolumeProtocol value to its corresponding privatev1.StorageProtocol_*
constant. Update volume_feedback_controller_test.go:183-201 to assert the
propagated GetProtocol() value in the vendor-field test, covering the mapping
for CRD protocols such as Block and NFS.
---
Nitpick comments:
In
`@fulfillment-service/internal/controllers/volume/volume_reconciler_function_test.go`:
- Around line 626-700: Extend the “Kubernetes validation error handling” spec to
seed the fake client with an existing CR labeled by labels.VolumeUuid for the
test volume, then exercise f.run through the existing-object path so the Patch
branch in getKubeObject/volume reconciliation is covered. Assert the Invalid
error from patching produces the same FAILED state and validation message, and
add coverage for the duplicate-CR error returned by getKubeObject.
In
`@fulfillment-service/internal/controllers/volume/volume_reconciler_function.go`:
- Around line 270-274: Update the Delete error path in the volume reconciler to
pass failures from t.hubClient.Delete through controllers.HandleK8sWriteError,
matching the existing Create and Patch handling and ensuring terminal write
errors update volume status instead of being returned raw.
In `@fulfillment-service/internal/servers/private_volumes_server_test.go`:
- Around line 73-122: Consolidate the three local volume factory closures into a
single helper that accepts both the volume name and size. Update createVolume,
createVolumeWithName, and createStandaloneVolume call sites to use this helper
with their respective values, while preserving the shared storage tier, access
mode, request construction, error assertion, and returned object behavior.
- Around line 235-263: Add Update tests alongside the existing partial
field-mask test in the private volume server tests: attempt a spec.* field-mask
update and assert the server rejects it without mutating the immutable
VolumeSpec, then set lock: true with a stale metadata.version and assert
optimistic locking rejects the update. Reuse the existing createVolume and
server.Update setup and verify the expected error outcomes.
In `@fulfillment-service/proto/private/osac/private/v1/volume_type.proto`:
- Around line 124-153: Update the shared rendering/masking metadata for
VolumeStatus fields backend, protocol, and hub to explicitly mark them as
private-only, rather than relying solely on comments or the proto/private
package location. Use the existing metadata mechanism and preserve the current
field definitions and behavior.
In `@fulfillment-service/proto/private/osac/private/v1/volumes_service.proto`:
- Around line 31-43: Update the filter example near the filter field to use the
readable enum name VOLUME_STATE_AVAILABLE if CEL supports enum value names;
otherwise keep the numeric comparison and explicitly identify 2 as
VOLUME_STATE_AVAILABLE in the surrounding prose.
In `@osac-operator/api/v1alpha1/volume_names.go`:
- Around line 36-38: Update the comment for VolumeCleanupFinalizer to describe
it as reserved or intended for future cleanup behavior, without claiming it is
currently added to ClusterOrder or blocks cluster deletion. Keep the constant
value unchanged.
In `@osac-operator/api/v1alpha1/volume_types.go`:
- Around line 39-44: Remove the field-level Enum markers from AccessMode and
Protocol in osac-operator/api/v1alpha1/volume_types.go, while retaining the Enum
markers on the VolumeAccessMode and VolumeProtocol type definitions; then run
make manifests. Regenerate
osac-operator/charts/operator-crds/templates/osac.openshift.io_volumes.yaml
(accessMode lines 66-78 and protocol lines 213-221) and
osac-operator/config/crd/bases/osac.openshift.io_volumes.yaml (accessMode lines
65-76 and protocol lines 212-219) so each property has one flat enum rather than
a duplicated allOf.
In `@osac-operator/internal/controller/volume_controller.go`:
- Around line 250-254: Update the finalizer-removal path in handleUpdate to call
the controller’s embedded r.Update method instead of
r.mgr.GetLocalManager().GetClient().Update, preserving the existing context,
volume object, and error-return behavior.
In `@osac-operator/internal/controller/volume_feedback_controller_test.go`:
- Around line 408-461: Protect test assertions by adding mutex-guarded accessors
for the mockVolumesServer updates and signals slices, and use them instead of
direct reads. In Signal, check signalErr before appending the request ID so
failed signals are not recorded; preserve recording for successful calls.
In `@osac-operator/internal/controller/volume_mock_provisioner.go`:
- Around line 25-44: Move MockVendorProvisioner and NewMockVendorProvisioner out
of the production build by renaming volume_mock_provisioner.go to
volume_mock_provisioner_test.go; if external packages require the mock, place it
in a dedicated testing subpackage instead. Preserve its existing test behavior
and VendorProvisioner compatibility.
🪄 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: 2d1996f7-0a57-4cf6-8228-b79b6918df86
⛔ Files ignored due to path filters (21)
fulfillment-service/internal/api/osac/private/v1/event_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/event_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/storage_common_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/storage_common_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/storage_tier_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/storage_tier_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/volume_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/volume_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/volumes_service.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/volumes_service.pb.gw.gois excluded by!**/*.pb.gw.gofulfillment-service/internal/api/osac/private/v1/volumes_service_grpc.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/volumes_service_protoopaque.pb.gois excluded by!**/*.pb.goosac-operator/internal/api/osac/private/v1/storage_common_type.pb.gois excluded by!**/*.pb.goosac-operator/internal/api/osac/private/v1/storage_common_type_protoopaque.pb.gois excluded by!**/*.pb.goosac-operator/internal/api/osac/private/v1/storage_tier_type.pb.gois excluded by!**/*.pb.goosac-operator/internal/api/osac/private/v1/storage_tier_type_protoopaque.pb.gois excluded by!**/*.pb.goosac-operator/internal/api/osac/private/v1/volume_type.pb.gois excluded by!**/*.pb.goosac-operator/internal/api/osac/private/v1/volume_type_protoopaque.pb.gois excluded by!**/*.pb.goosac-operator/internal/api/osac/private/v1/volumes_service.pb.gois excluded by!**/*.pb.goosac-operator/internal/api/osac/private/v1/volumes_service_grpc.pb.gois excluded by!**/*.pb.goosac-operator/internal/api/osac/private/v1/volumes_service_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (37)
fulfillment-service/internal/cmd/service/start/controller/start_controller_cmd.gofulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gofulfillment-service/internal/cmd/service/start/restgateway/start_rest_gateway_cmd.gofulfillment-service/internal/controllers/volume/hubs_client_mock.gofulfillment-service/internal/controllers/volume/volume_reconciler_function.gofulfillment-service/internal/controllers/volume/volume_reconciler_function_test.gofulfillment-service/internal/controllers/volume/volume_suite_test.gofulfillment-service/internal/controllers/volume/volumes_client_mock.gofulfillment-service/internal/database/migrations.sha256fulfillment-service/internal/database/migrations/94_create_volumes_tables.up.sqlfulfillment-service/internal/database/migrations/94_create_volumes_tables_test.gofulfillment-service/internal/kubernetes/labels/kubernetes_labels.gofulfillment-service/internal/rendering/tables/osac.private.v1.Volume.yamlfulfillment-service/internal/servers/private_volumes_server.gofulfillment-service/internal/servers/private_volumes_server_test.gofulfillment-service/proto/private/osac/private/v1/event_type.protofulfillment-service/proto/private/osac/private/v1/storage_common_type.protofulfillment-service/proto/private/osac/private/v1/storage_tier_type.protofulfillment-service/proto/private/osac/private/v1/volume_type.protofulfillment-service/proto/private/osac/private/v1/volumes_service.protoosac-operator/api/v1alpha1/groupversion_info.goosac-operator/api/v1alpha1/volume_names.goosac-operator/api/v1alpha1/volume_types.goosac-operator/api/v1alpha1/zz_generated.deepcopy.goosac-operator/charts/operator-crds/templates/osac.openshift.io_volumes.yamlosac-operator/charts/operator/templates/clusterrole.yamlosac-operator/charts/operator/templates/deployment.yamlosac-operator/charts/operator/templates/hub-access-clusterrole.yamlosac-operator/cmd/main.goosac-operator/config/crd/bases/osac.openshift.io_volumes.yamlosac-operator/config/rbac/role.yamlosac-operator/internal/controller/volume_controller.goosac-operator/internal/controller/volume_controller_test.goosac-operator/internal/controller/volume_feedback_controller.goosac-operator/internal/controller/volume_feedback_controller_test.goosac-operator/internal/controller/volume_mock_provisioner.goosac-operator/internal/controller/volume_names.go
| It("Enforces immutability of id, name, tenant, and project", func(ctx context.Context) { | ||
| err := tool.Migrate(ctx, 94) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| _, err = conn.Exec(ctx, | ||
| `insert into volumes (id, name, tenant, data) values ($1, $2, $3, $4)`, | ||
| "immutable-id", "immutable-vol", "system", `{}`, | ||
| ) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| var pgErr *pgconn.PgError | ||
|
|
||
| _, err = conn.Exec(ctx, | ||
| `update volumes set id = $1 where id = $2`, | ||
| "changed-id", "immutable-id", | ||
| ) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(errors.As(err, &pgErr)).To(BeTrue()) | ||
| Expect(pgErr.Code).To(Equal("Z0001")) | ||
|
|
||
| _, err = conn.Exec(ctx, | ||
| `update volumes set name = $1 where id = $2`, | ||
| "changed-name", "immutable-id", | ||
| ) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(errors.As(err, &pgErr)).To(BeTrue()) | ||
| Expect(pgErr.Code).To(Equal("Z0001")) | ||
|
|
||
| _, err = conn.Exec(ctx, | ||
| `update volumes set tenant = $1 where id = $2`, | ||
| "other-tenant", "immutable-id", | ||
| ) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(errors.As(err, &pgErr)).To(BeTrue()) | ||
| Expect(pgErr.Code).To(Equal("Z0001")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the stated project immutability.
The test name includes project, but the test only updates id, name, and tenant. Add a failing update volumes set project = ... assertion if the migration makes project immutable. Otherwise, remove project from the test name.
🤖 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/database/migrations/94_create_volumes_tables_test.go`
around lines 131 - 165, Update the immutability test around its existing id,
name, and tenant assertions to cover the project column as well: execute an
update of volumes.project and assert it fails with the same Z0001 error code.
Keep project in the test name only if this assertion is added.
There was a problem hiding this comment.
This is on code from the base PR #201. Will address in that PR's review cycle.
There was a problem hiding this comment.
@akshaynadkarni, acknowledged. This test originates in base PR #201. I will leave the change for that review cycle.
🧠 Learnings used
Learnt from: akshaynadkarni
Repo: osac-project/osac PR: 201
File: fulfillment-service/internal/database/migrations/94_create_volumes_tables_test.go:35-46
Timestamp: 2026-08-07T20:01:15.978Z
Learning: In Go migration tests under fulfillment-service/internal/database/migrations, do not flag SQL constructed with fmt.Sprintf and pgx.Identifier.Sanitize() as SQL injection when the table-name identifier comes from a hardcoded test constant and is not externally controlled. This established pattern is used in files such as 73_create_nat_gateways_tables_test.go and 62_create_storage_backends_tables_test.go.
You are interacting with an AI system.
| -- Enforce immutability of id, tenant, and project columns at the database level. | ||
| create trigger check_immutable_columns | ||
| before update on volumes | ||
| for each row | ||
| execute function check_immutable_columns('id', 'name', 'tenant', 'project'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The comment omits name, which the trigger does enforce.
Line 71 lists "id, tenant, and project". Line 75 passes 'id', 'name', 'tenant', 'project'. The test in 94_create_volumes_tables_test.go confirms that a name update raises Z0001. Fix the comment.
📝 Proposed comment fix
--- Enforce immutability of id, tenant, and project columns at the database level.
+-- Enforce immutability of the id, name, tenant, and project columns at the database level.
create trigger check_immutable_columns📝 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.
| -- Enforce immutability of id, tenant, and project columns at the database level. | |
| create trigger check_immutable_columns | |
| before update on volumes | |
| for each row | |
| execute function check_immutable_columns('id', 'name', 'tenant', 'project'); | |
| -- Enforce immutability of the id, name, tenant, and project columns at the database level. | |
| create trigger check_immutable_columns | |
| before update on volumes | |
| for each row | |
| execute function check_immutable_columns('id', 'name', 'tenant', 'project'); |
🤖 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/database/migrations/94_create_volumes_tables.up.sql`
around lines 71 - 75, Update the comment above trigger check_immutable_columns
to list name alongside id, tenant, and project, matching the columns passed to
the trigger and the enforced immutability behavior.
There was a problem hiding this comment.
This is on code from the base PR #201. Will address in that PR's review cycle.
There was a problem hiding this comment.
@akshaynadkarni, understood. This comment applies to the migration introduced in base PR #201. I will leave it for that PR’s review cycle.
You are interacting with an AI system.
| Describe("Validation", func() { | ||
| It("Create without metadata.name fails", func() { | ||
| _, err := server.Create(ctx, privatev1.VolumesCreateRequest_builder{ | ||
| Object: privatev1.Volume_builder{ | ||
| Spec: privatev1.VolumeSpec_builder{ | ||
| StorageTier: "gold", | ||
| SizeGib: 100, | ||
| AccessMode: privatev1.VolumeAccessMode_VOLUME_ACCESS_MODE_READ_WRITE_ONCE, | ||
| }.Build(), | ||
| }.Build(), | ||
| }.Build()) | ||
| Expect(err).To(HaveOccurred()) | ||
| st, ok := status.FromError(err) | ||
| Expect(ok).To(BeTrue()) | ||
| Expect(st.Code()).To(Equal(codes.InvalidArgument)) | ||
| Expect(st.Message()).To(ContainSubstring("metadata.name")) | ||
| }) | ||
|
|
||
| It("Create without storage_tier fails", func() { | ||
| _, err := server.Create(ctx, privatev1.VolumesCreateRequest_builder{ | ||
| Object: privatev1.Volume_builder{ | ||
| Metadata: privatev1.Metadata_builder{ | ||
| Name: "test-volume", | ||
| }.Build(), | ||
| Spec: privatev1.VolumeSpec_builder{ | ||
| SizeGib: 100, | ||
| AccessMode: privatev1.VolumeAccessMode_VOLUME_ACCESS_MODE_READ_WRITE_ONCE, | ||
| }.Build(), | ||
| }.Build(), | ||
| }.Build()) | ||
| Expect(err).To(HaveOccurred()) | ||
| st, ok := status.FromError(err) | ||
| Expect(ok).To(BeTrue()) | ||
| Expect(st.Code()).To(Equal(codes.InvalidArgument)) | ||
| Expect(st.Message()).To(ContainSubstring("storage_tier")) | ||
| }) | ||
|
|
||
| It("Create without size_gib fails", func() { | ||
| _, err := server.Create(ctx, privatev1.VolumesCreateRequest_builder{ | ||
| Object: privatev1.Volume_builder{ | ||
| Metadata: privatev1.Metadata_builder{ | ||
| Name: "test-volume", | ||
| }.Build(), | ||
| Spec: privatev1.VolumeSpec_builder{ | ||
| StorageTier: "gold", | ||
| AccessMode: privatev1.VolumeAccessMode_VOLUME_ACCESS_MODE_READ_WRITE_ONCE, | ||
| }.Build(), | ||
| }.Build(), | ||
| }.Build()) | ||
| Expect(err).To(HaveOccurred()) | ||
| st, ok := status.FromError(err) | ||
| Expect(ok).To(BeTrue()) | ||
| Expect(st.Code()).To(Equal(codes.InvalidArgument)) | ||
| Expect(st.Message()).To(ContainSubstring("size_gib")) | ||
| }) | ||
|
|
||
| It("Create without access_mode fails", func() { | ||
| _, err := server.Create(ctx, privatev1.VolumesCreateRequest_builder{ | ||
| Object: privatev1.Volume_builder{ | ||
| Metadata: privatev1.Metadata_builder{ | ||
| Name: "test-volume", | ||
| }.Build(), | ||
| Spec: privatev1.VolumeSpec_builder{ | ||
| StorageTier: "gold", | ||
| SizeGib: 100, | ||
| }.Build(), | ||
| }.Build(), | ||
| }.Build()) | ||
| Expect(err).To(HaveOccurred()) | ||
| st, ok := status.FromError(err) | ||
| Expect(ok).To(BeTrue()) | ||
| Expect(st.Code()).To(Equal(codes.InvalidArgument)) | ||
| Expect(st.Message()).To(ContainSubstring("access_mode")) | ||
| }) | ||
|
|
||
| It("Create without spec fails", func() { | ||
| _, err := server.Create(ctx, privatev1.VolumesCreateRequest_builder{ | ||
| Object: privatev1.Volume_builder{ | ||
| Metadata: privatev1.Metadata_builder{ | ||
| Name: "test-volume", | ||
| }.Build(), | ||
| }.Build(), | ||
| }.Build()) | ||
| Expect(err).To(HaveOccurred()) | ||
| st, ok := status.FromError(err) | ||
| Expect(ok).To(BeTrue()) | ||
| Expect(st.Code()).To(Equal(codes.InvalidArgument)) | ||
| Expect(st.Message()).To(ContainSubstring("spec")) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
These validation tests call the server directly, so the protovalidate interceptor never runs.
All five cases assert codes.InvalidArgument for constraints declared with buf.validate in volume_type.proto. server.Create is invoked directly here, which bypasses the gRPC interceptor chain that applies those rules.
Two of the cases cannot pass even with the interceptor. access_mode is only guarded by enum.defined_only, which accepts the zero value, and Volume.spec carries no required rule. See the comment on fulfillment-service/proto/private/osac/private/v1/volume_type.proto lines 101-122.
Move these cases to an integration test that exercises the full gRPC chain, and add the missing declarative rules.
Based on learnings: "treat validation expressible with buf.validate annotations as enforced by the protovalidate gRPC interceptor. Do not duplicate that validation in server handlers or direct server unit tests; test negative declarative Create-request cases in integration tests that exercise the complete gRPC chain".
🤖 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_volumes_server_test.go` around
lines 324 - 413, Move the five Create validation cases from the direct server
tests around server.Create into an integration test that invokes the full gRPC
interceptor chain. Add declarative buf.validate rules in the Volume schema for
required spec and non-zero/required access_mode, while preserving the existing
storage_tier, size_gib, and metadata.name constraints; do not add duplicate
validation to server handlers.
Source: Learnings
There was a problem hiding this comment.
This is on code from the base PR #201, not this controller PR. Will address there.
There was a problem hiding this comment.
@akshaynadkarni, acknowledged. This validation issue belongs to base PR #201. I will treat it as out of scope for this controller PR.
🧠 Learnings used
Learnt from: ajamias
Repo: osac-project/osac PR: 40
File: fulfillment-service/internal/servers/private_baremetal_instance_types_server_test.go:59-73
Timestamp: 2026-07-31T18:16:29.969Z
Learning: In fulfillment-service Go tests, treat validation expressible with buf.validate annotations as enforced by the protovalidate gRPC interceptor. Do not duplicate that validation in server handlers or direct server unit tests; test negative declarative Create-request cases in integration tests that exercise the complete gRPC chain, consistent with fulfillment-service/AGENTS.md.
You are interacting with an AI system.
| if vol.GetStatus() == nil { | ||
| vol.SetStatus(&privatev1.VolumeStatus{}) | ||
| } | ||
| vol.GetStatus().SetState(privatev1.VolumeState_VOLUME_STATE_CREATING) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Discard caller-provided status during Create.
This code preserves caller-provided vendor_volume_id, backend, protocol, hub, and PV/PVC references. It only overwrites state. generic.Create can then persist unverified operational status data.
Create a fresh VolumeStatus before setting VOLUME_STATE_CREATING.
Proposed fix
- if vol.GetStatus() == nil {
- vol.SetStatus(&privatev1.VolumeStatus{})
- }
- vol.GetStatus().SetState(privatev1.VolumeState_VOLUME_STATE_CREATING)
+ status := &privatev1.VolumeStatus{}
+ status.SetState(privatev1.VolumeState_VOLUME_STATE_CREATING)
+ vol.SetStatus(status)📝 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.
| if vol.GetStatus() == nil { | |
| vol.SetStatus(&privatev1.VolumeStatus{}) | |
| } | |
| vol.GetStatus().SetState(privatev1.VolumeState_VOLUME_STATE_CREATING) | |
| status := &privatev1.VolumeStatus{} | |
| status.SetState(privatev1.VolumeState_VOLUME_STATE_CREATING) | |
| vol.SetStatus(status) |
🤖 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_volumes_server.go` around lines
126 - 129, Update the Create flow in the private volumes server to always
replace the volume’s existing status with a new empty privatev1.VolumeStatus,
rather than only initializing it when nil. Then set the fresh status state to
VOLUME_STATE_CREATING, ensuring caller-provided operational fields are discarded
before generic.Create persists the volume.
There was a problem hiding this comment.
This is on code from the base PR #201, not this controller PR. Will address there.
There was a problem hiding this comment.
@akshaynadkarni Understood. This finding belongs to base PR #201, not this controller PR. I will leave this comment open for validation against the fix in #201. No separate follow-up issue is needed while #201 tracks the change.
You are interacting with an AI system.
| if err != nil { | ||
| log.Error(err, "vendor provisioning failed") | ||
| vol.Status.Phase = v1alpha1.VolumePhaseFailed | ||
| setVendorProvisionedCondition(&vol.Status.Conditions, metav1.ConditionFalse, "ProvisioningFailed", err.Error()) | ||
| return ctrl.Result{}, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Failed provisioning never retries.
The handler returns nil, so controller-runtime does not requeue. No further event arrives for this CR, because nothing else changes it. A transient vendor outage therefore strands the volume in Failed forever. The condition is still visible if you requeue with a delay.
🐛 Proposed fix: requeue after a backoff delay
if err != nil {
log.Error(err, "vendor provisioning failed")
vol.Status.Phase = v1alpha1.VolumePhaseFailed
setVendorProvisionedCondition(&vol.Status.Conditions, metav1.ConditionFalse, "ProvisioningFailed", err.Error())
- return ctrl.Result{}, nil
+ return ctrl.Result{RequeueAfter: vendorProvisioningRetryInterval}, nil
}🤖 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/volume_controller.go` around lines 200 -
205, Update the provisioning error path in the volume reconciliation handler to
return a delayed requeue result instead of an empty result with nil error.
Preserve the existing failure status and condition updates, and use the
controller’s established backoff duration if one is available so transient
vendor failures retry while the condition remains visible.
There was a problem hiding this comment.
Fixed in f0a6f21. Added an explicit guard so Failed volumes skip provisioning. Re-provisioning is triggered by Signal or periodic sync after the admin fixes the vendor issue.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- current revision ---'
git rev-parse --short HEAD
printf '%s\n' '--- volume controller outline ---'
ast-grep outline osac-operator/internal/controller/volume_controller.go --items all --type function,method --view expanded
printf '%s\n' '--- provisioning and phase guards ---'
rg -n -C 8 'VolumePhase(Failed|Ready)|handleProvisioning|CreateVolume' \
osac-operator/internal/controller/volume_controller.go
printf '%s\n' '--- volume Signal and periodic sync references ---'
rg -n -i -C 5 'signal|periodic|sync' osac-operator \
-g '*.go' -g '*.yaml' -g '*.yml'Length of output: 201148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- f0a6f211 diff ---'
git show --format=fuller --stat --oneline f0a6f211
git show --format= --unified=30 f0a6f211 -- osac-operator/internal/controller/volume_controller.go
printf '%s\n' '--- current volume controller, relevant range ---'
sed -n '145,235p' osac-operator/internal/controller/volume_controller.go
printf '%s\n' '--- Volume-specific signal and sync source references (exclude generated files) ---'
rg -n -i -C 4 'VolumesSignal|Volume.*Signal|Signal.*Volume|periodic.{0,80}(volume|sync)|(volume|sync).{0,80}periodic' \
osac-operator/internal/controller \
fulfillment-service/internal \
-g '*.go' -g '!**/*.pb.go' -g '!**/*mock*.go'
printf '%s\n' '--- volume controller tests for failed phase and re-provisioning ---'
rg -n -C 6 'Failed|fail|Signal|re.?provision|CreateCallCount' \
osac-operator/internal/controller/volume_controller_test.goLength of output: 30529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- f0a6f211 changes outside VolumeReconciler ---'
git show --format= --unified=35 f0a6f211 -- \
fulfillment-service/internal/controllers/volume/volume_reconciler_function.go \
osac-operator/internal/controller/volume_feedback_controller.go \
osac-operator/internal/controller/volume_feedback_controller_test.go
printf '%s\n' '--- fulfillment Volume reconciler lifecycle mapping ---'
rg -n -C 8 'Signal|VOLUME_STATE_(FAILED|PENDING|CREATING)|VolumeState|Failed|Progressing|Patch|Update' \
fulfillment-service/internal/controllers/volume/volume_reconciler_function.go
printf '%s\n' '--- feedback reconciler status-to-remote mapping ---'
sed -n '45,180p' osac-operator/internal/controller/volume_feedback_controller.go
printf '%s\n' '--- all Volume periodic runnable registrations ---'
rg -n -i -C 3 'volume.*(runnable|periodic|ticker|resync)|((runnable|periodic|ticker|resync).*volume)' \
osac-operator/cmd osac-operator/internal/controller \
-g '*.go' -g '!**/*.pb.go'Length of output: 23022
@akshaynadkarni, I verified f0a6f211. The finding remains unresolved.
handleUpdate now returns early when vol.Status.Phase == VolumePhaseFailed. Volumes.Signal causes fulfillment-service reconciliation, but that reconciler only creates or patches the hub Volume specification. It does not reset Volume.Status.Phase to Progressing.
I also found no Volume periodic-sync runnable. Therefore, after a vendor error, a Volume stays Failed and every later reconcile skips CreateVolume.
Please reset the hub CR phase to Progressing through the intended retry action, or return a delayed requeue for retryable vendor errors.
You are interacting with an AI system.
Fix five issues identified by CodeRabbit on PR osac-project#223: 1. (Critical) Protocol enum lookup: CRD uses "Block"/"NFS" but the proto map is keyed by "STORAGE_PROTOCOL_BLOCK". Replaced the map lookup with an explicit switch in syncVolumeVendorFields. Added test coverage for both Block and NFS protocol mapping. 2. (Major) Feedback controller namespace default: NewVolumeFeedbackReconciler now defaults to "osac-volume" when the namespace parameter is empty, matching NewVolumeReconciler's behavior. Previously the feedback controller would watch "" (matching nothing) when OSAC_VOLUME_NAMESPACE was unset. 3. (Major) Status update after delete: after handleDelete removes the finalizer, the API server may delete the object before updateStatusWithRetry runs. Now ignores NotFound during deletion since the status was already persisted in the finalizer-removal Update call. 4. (Major) Failed provisioning retry: added an explicit guard so Failed volumes are not re-provisioned on every reconcile (avoids spamming the vendor API). Re-provisioning is triggered by Signal or periodic sync after the admin fixes the vendor issue. 5. (Minor) Nil-check on hub list response before selecting a hub. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
GitHub Actions
Failure
GitHub Actions
Inactive
GitHub Actions
Inactive
Fix five issues identified by CodeRabbit on PR osac-project#223: 1. (Critical) Protocol enum lookup: CRD uses "Block"/"NFS" but the proto map is keyed by "STORAGE_PROTOCOL_BLOCK". Replaced the map lookup with an explicit switch in syncVolumeVendorFields. Added test coverage for both Block and NFS protocol mapping. 2. (Major) Feedback controller namespace default: NewVolumeFeedbackReconciler now defaults to "osac-volume" when the namespace parameter is empty, matching NewVolumeReconciler's behavior. Previously the feedback controller would watch "" (matching nothing) when OSAC_VOLUME_NAMESPACE was unset. 3. (Major) Status update after delete: after handleDelete removes the finalizer, the API server may delete the object before updateStatusWithRetry runs. Now ignores NotFound during deletion since the status was already persisted in the finalizer-removal Update call. 4. (Major) Failed provisioning retry: added an explicit guard so Failed volumes are not re-provisioned on every reconcile (avoids spamming the vendor API). Re-provisioning is triggered by Signal or periodic sync after the admin fixes the vendor issue. 5. (Minor) Nil-check on hub list response before selecting a hub. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
f0a6f21 to
126d2dc
Compare
GitHub Actions
Failure
GitHub Actions
Inactive
GitHub Actions
Inactive
| default: | ||
| log := ctrllog.FromContext(ctx) | ||
| log.Info("Unknown phase, will ignore it", "phase", obj.Status.Phase) | ||
| } |
There was a problem hiding this comment.
this would leave remote.Status.State unchanged, maybe it should return an error so the controller re-queues?
(same pattern in all feedback controllers, so this is a generic issue. we can ignore for now, just might consider handling as a follow-up)
There was a problem hiding this comment.
Agreed. The default case is a defensive guard for phases that don't exist yet. Returning an error would requeue indefinitely for a phase the controller genuinely doesn't know how to handle, so logging and leaving the state unchanged is the safer default. We can revisit this across all feedback controllers as a follow-up if we want consistent error-or-log behavior.
akshaynadkarni
changed the title
|
@akshaynadkarni: This pull request references OSAC-3273 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 epic to target the "5.0.0" version, but no target version was set. This pull request references OSAC-3280 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 epic 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. |
Fix five issues identified by CodeRabbit on PR osac-project#223: 1. (Critical) Protocol enum lookup: CRD uses "Block"/"NFS" but the proto map is keyed by "STORAGE_PROTOCOL_BLOCK". Replaced the map lookup with an explicit switch in syncVolumeVendorFields. Added test coverage for both Block and NFS protocol mapping. 2. (Major) Feedback controller namespace default: NewVolumeFeedbackReconciler now defaults to "osac-volume" when the namespace parameter is empty, matching NewVolumeReconciler's behavior. Previously the feedback controller would watch "" (matching nothing) when OSAC_VOLUME_NAMESPACE was unset. 3. (Major) Status update after delete: after handleDelete removes the finalizer, the API server may delete the object before updateStatusWithRetry runs. Now ignores NotFound during deletion since the status was already persisted in the finalizer-removal Update call. 4. (Major) Failed provisioning retry: added an explicit guard so Failed volumes are not re-provisioned on every reconcile (avoids spamming the vendor API). Re-provisioning is triggered by Signal or periodic sync after the admin fixes the vendor issue. 5. (Minor) Nil-check on hub list response before selecting a hub. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
126d2dc to
7589a7d
Compare
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Failure
|
🤖 Review · Commit: |
| func (r *VolumeReconciler) handleProvisioning(ctx context.Context, vol *v1alpha1.Volume) (ctrl.Result, error) { | ||
| log := ctrllog.FromContext(ctx) | ||
|
|
||
| resp, err := r.VendorProvisioner.CreateVolume(ctx, VendorCreateVolumeRequest{ |
There was a problem hiding this comment.
I think the CreateVolume request needs to say which storageBackend to use and not the tier. Meaning that the tier resolution already executed by the osac volume creation, at the volume-api level.
The vendor provisioner would route the request to the right backend.
The Volume reconciler watches for Volume events via gRPC streaming and creates corresponding Volume CRs on the hub cluster. It follows the same three-struct pattern (FunctionBuilder/function/task) as NATGateway and other hub-based reconcilers. Key behaviors: - Adds the fulfillment-controller finalizer before any K8s work - Selects a hub randomly from available hubs (volumes are independent resources with no parent to inherit from, same as ComputeInstance) - Creates Volume CRs with the volume-uuid label for lookup - Converts the proto VolumeAccessMode enum to the CRD typed string - On delete, removes the CR from the hub and cleans up the finalizer - Handles decommissioned hubs by removing the finalizer gracefully Tests cover buildSpec (all access mode enum values, PVC reference), setDefaults, validateTenant, finalizer lifecycle, delete paths (missing hub, missing CR, active deletion, decommissioned hub), selectHub (existing hub, random selection, no hubs), and K8s validation error handling. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
…sioner Volume controller reconciles Volume CRs on the hub cluster through the vendor provisioning lifecycle (Progressing -> Ready or Failed). Unlike networking and compute controllers that use AAP, the Volume controller calls a VendorProvisioner interface directly because storage provisioning is a synchronous vendor CSI gRPC call. For this PR the implementation is a MockVendorProvisioner; the real vendor CSI client is wired in PR osac-project#3. The feedback controller syncs Volume CR status back to the fulfillment- service using the shared feedback.Bridge. Phase-to-state mapping: Progressing->CREATING, Ready->AVAILABLE, Failed->FAILED, Deleting->DELETING. It also syncs vendorVolumeID, backend, and protocol so the fulfillment- service inventory reflects the actual storage array state. Generated gRPC client from local fulfillment-service proto (BSR version will be bumped after PR osac-project#1 merges and publishes the Volume proto). Tests cover the full lifecycle: finalizer addition, provisioning success and failure, idempotent re-reconcile after Ready, management-state skip, deletion with vendor deprovisioning, and not-found handling. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Register the Volume resource controller and feedback controller under the Storage controller flag (ctrlFlags.Storage). The VendorProvisioner is nil until the real vendor CSI client is integrated in a follow-up PR; the controller sets phase to Progressing and skips provisioning. Helm chart changes: - deployment.yaml: add OSAC_VOLUME_NAMESPACE env var (metadata.namespace) - clusterrole.yaml: add volumes, volumes/finalizers, volumes/status - hub-access-clusterrole.yaml: add volumes, volumes/status so the fulfillment-service controller can manage Volume CRs on the hub Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Tests cover the full feedback lifecycle using an in-process gRPC mock server (bufconn pattern, same as VirtualNetwork feedback tests): Phase-to-state mapping: - Progressing -> CREATING, Ready -> AVAILABLE, Failed -> FAILED Vendor field syncing: - vendorVolumeID, backend, protocol copied to fulfillment-service - Empty CR fields do not overwrite existing remote values Deletion handling: - Deleting phase maps to DELETING state - Failed phase during deletion maps to FAILED - Last-finalizer removal triggers Signal RPC - Signal failure does not block finalizer removal - NotFound remote record during deletion removes finalizer gracefully Idempotency: - No Update RPC when remote state already matches CR Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Fix five issues identified by CodeRabbit on PR osac-project#223: 1. (Critical) Protocol enum lookup: CRD uses "Block"/"NFS" but the proto map is keyed by "STORAGE_PROTOCOL_BLOCK". Replaced the map lookup with an explicit switch in syncVolumeVendorFields. Added test coverage for both Block and NFS protocol mapping. 2. (Major) Feedback controller namespace default: NewVolumeFeedbackReconciler now defaults to "osac-volume" when the namespace parameter is empty, matching NewVolumeReconciler's behavior. Previously the feedback controller would watch "" (matching nothing) when OSAC_VOLUME_NAMESPACE was unset. 3. (Major) Status update after delete: after handleDelete removes the finalizer, the API server may delete the object before updateStatusWithRetry runs. Now ignores NotFound during deletion since the status was already persisted in the finalizer-removal Update call. 4. (Major) Failed provisioning retry: added an explicit guard so Failed volumes are not re-provisioned on every reconcile (avoids spamming the vendor API). Re-provisioning is triggered by Signal or periodic sync after the admin fixes the vendor issue. 5. (Minor) Nil-check on hub list response before selecting a hub. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Move Volume label, finalizer, and namespace constants from api/v1alpha1/ to internal/controller/volume_names.go using the existing pattern (unexported vars with fmt.Sprintf and osacPrefix). All references are within the controller package so no external consumers are affected. Doc updates: - AGENTS.md Resources Managed: add Volume (vol) - README.md CRD list: add Volume with VendorProvisioner description - architecture-patterns.md Resource Hierarchy: add Storage Resources Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
MockVendorProvisioner is only used in tests. Move from volume_mock_provisioner.go to volume_mock_provisioner_test.go so it is not compiled into the production binary. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
- Rename misleading test: "should set phase to Progressing on first reconcile" correctly reflects Ready, not Progressing, since the mock provisioner succeeds immediately - Close gRPC client connection in AfterEach of feedback controller tests to prevent goroutine leaks across specs - Add ReadOnlyMany access mode test in volume reconciler to cover the explicit switch case in protoAccessModeToCRD - Add test for handleDelete when DeleteVolume returns an error: verifies the finalizer is kept and Phase is Deleting when vendor fails - Replace r.mgr.GetLocalManager().GetClient().Update with r.Update in handleDelete for consistency with all other controllers - Rename oldStatus to oldstatus in volume_controller.go to match the convention used across all 11 other resource controllers - Replace thinking-out-loud comment in feedback controller test with a concise explanation of why no Update RPC is sent Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Remove management-state annotation check: volumes are always provisioned by the vendor CSI driver and have no manual management path, unlike AAP- provisioned resources where the annotation makes sense. Use Backend instead of StorageTier in VendorCreateVolumeRequest: tier resolution happens in the fulfillment-service at CreateVolume time (OSAC-3277). By the time the Volume CR reaches the operator, the backend is already resolved and stored in vol.Status.Backend. The vendor provisioner routes by backend name, not tier name. Type AccessMode as VolumeAccessMode instead of string in VendorCreateVolumeRequest, matching the CRD type and removing the unsafe string conversion at the call site. Add comment on the nil VendorProvisioner guard explaining it is temporary until the real vendor CSI client is wired. Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Volume provisioning (vendor CSI) has different operational dependencies from tenant storage management (StorageClass lifecycle, ClusterOrder storage). Bundling both under OSAC_ENABLE_STORAGE_CONTROLLER prevents deploying one without the other and establishes a bad precedent for future storage types (file, object) that will each need independent enable flags. Add a dedicated OSAC_ENABLE_VOLUME_CONTROLLER / --enable-volume-controller flag that gates setupVolumeControllers independently of the storage controller. Both default to true when no flags are set. Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Replace updateStatusWithRetry with a direct r.Status().Update() call, matching the pattern used by all other resource controllers. The retry wrapper introduced a deletion race where re-fetching the object after finalizer removal returned NotFound, requiring a novel guard that no other controller needs. Write conflicts on status are handled by controller-runtime requeue, which is sufficient. Assert reconcile errors in setup loops so a silent failure cannot put the test in an unexpected state before the actual assertion. Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
The error "client is mandatory" was misleading since the field being checked is a grpc.ClientConn, not a client. Changed to "connection is mandatory" to match the field name and SetConnection() method. Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
f13b742 to
af83279
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
🤖 Finished Review · ✅ Success · Started 9:07 PM UTC · Completed 9:25 PM UTC Commit: |
…exity main() exceeded gocyclo's limit of 30 after adding the separate OSAC_ENABLE_VOLUME_CONTROLLER if block. Extract all controller setup into a dedicated setupControllers() function, reducing main()'s branch count and making the enable-flag logic easier to follow. Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
🤖 Finished Review · ✅ Success · Started 2:54 AM UTC · Completed 3:12 AM UTC Commit: |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
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:
fulfillment-service/internal/controllers/volume/volume_reconciler_function.go:165: [medium] error-handling-gap
validateTenant() returns a hard error when the tenant is empty, but the error propagates to the reconciler loop which retries indefinitely. Unlike K8s validation errors that go through HandleK8sWriteError and call setFailed(), a missing tenant will never set the volume status to FAILED, leaving the user with no visibility into why the volume is stuck.
Suggested fix: Call t.setFailed(err) before returning the error from validateTenant failure, similar to how HandleK8sWriteError handles validation errors.
osac-operator/internal/controller/volume_controller.go(file-level): Line 558 · [medium] missing-management-state-check
The VolumeReconciler.Reconcile method does not check the osac.openshift.io/management-state annotation before reconciling. Per AGENTS.md, all resource controllers (except tenant_controller.go) check this annotation and skip reconciliation when set to Unmanaged. Without this check, operators cannot halt Volume reconciliation for debugging or emergency intervention.
Suggested fix: Add the management-state annotation check after the Get call, consistent with other controllers (e.g., computeinstance_controller.go:145).
osac-operator/internal/controller/volume_controller.go(file-level): Line 580 · [medium] pattern-inconsistency
Direct r.Status().Update(ctx, vol) instead of the updateStatusWithRetry helper used by all other resource controllers. Other controllers wrap status updates in retry.RetryOnConflict to handle optimistic locking conflicts. The direct call is susceptible to conflict errors under concurrent modification.
Suggested fix: Add an updateStatusWithRetry method matching the pattern in other controllers (e.g., computeinstance_controller.go:173-186).
fulfillment-service/internal/controllers/volume/volume_reconciler_function.go:306: [low] edge-case
selectHub accesses response.Items via rand.IntN. If a Hub in the Items slice has an empty ID, the reconciler assigns an empty hubId and likely fails with a confusing error. Low-probability edge case that applies equally to ComputeInstance.
osac-operator/internal/controller/volume_names.go:28: [low] naming-convention
Finalizer uses -finalizer suffix (osac.openshift.io/volume-finalizer). Other resources are inconsistent: NATGateway uses -finalizer, ComputeInstance uses just the resource name. Volume follows the majority pattern.
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
|
||
| resp, err := r.VendorProvisioner.CreateVolume(ctx, VendorCreateVolumeRequest{ | ||
| Name: vol.Name, | ||
| Backend: vol.Status.Backend, |
There was a problem hiding this comment.
I might getting this wrong but I think vol.Status.Backend is always empty on first provisioning. Status.Backend originates from the CreateVolume response (at least for now), not from any input. Neither does the fulfillment-service reconciler Status fields.
I saw the struct comment claiming OSAC-3277 will pre-populate Backend via tier resolution before the CR is created, but it's not listed as a dependency of this PR and nothing in this diff implements that path.
Again, maybe I missed something, or this is something that will be covered in the third PR so just raising it here so it doesn’t get overlooked.
There was a problem hiding this comment.
Discussed offline. It's true that backend will be empty on first provisioning for now. However, I am putting up a PR to tackle tier -> backend resolution. So this will change soon.
|
Closing this PR in favor of smaller, focused PRs split by component boundary:
All code from this PR is preserved in the split PRs (final file states copied, all review fixes from CodeRabbit, fullsend, Roy, and Zoltan are included). Each PR is independently mergeable against main. |
4 participants
Summary
Adds the Volume lifecycle controllers (PR 2 of 3 for the storage control
plane), building on the data model from
#201.
streaming, creates/patches/deletes Volume CRs on the hub cluster. Maps proto
VolumeAccessMode enum to CRD typed strings. Selects a hub randomly (same as
ComputeInstance, since volumes have no parent resource to inherit from).
(nil for this PR, real vendor CSI integration in a follow-up). Feedback
controller syncs CR status back to the fulfillment-service
(Progressing->CREATING, Ready->AVAILABLE, Failed->FAILED,
Deleting->DELETING) plus vendorVolumeID, backend, and protocol.
OSAC_VOLUME_NAMESPACEenv var,hub-access clusterrole updated.
Why
#201 established the Volume
data model (proto, DB, server, CRD types). This PR adds the controllers that
drive the provisioning lifecycle end-to-end: the fulfillment-service creates
Volume CRs on the hub, the operator controller calls the vendor to provision,
and the feedback controller syncs the result back. The VendorProvisioner is nil
for now (volumes stay in Progressing), with the real vendor CSI client planned
for a follow-up PR once the CSI driver integration PRs land.
What's included
fulfillment-service
internal/controllers/volume/(reconciler function, tests, mocks)internal/kubernetes/labels/kubernetes_labels.go(VolumeUuid label)internal/cmd/service/start/controller/start_controller_cmd.go(wiring)osac-operator
internal/controller/volume_controller.go(VendorProvisioner interface,provisioning lifecycle, management-state skip, status retry with NotFound
handling on delete)
internal/controller/volume_feedback_controller.go(feedback.Bridge,phase-to-state mapping, vendor field sync with explicit protocol enum
conversion via crdProtocolToProto)
internal/controller/volume_mock_provisioner.go(test mock withdeterministic IDs and call counting)
internal/controller/volume_names.go(labels, finalizers, namespace default)cmd/main.go(setupVolumeControllers under ctrlFlags.Storage)charts/operator/templates/(deployment, clusterrole, hub-access)Docs
AGENTS.mdResources Managed: Volume addedREADME.mdCRD list: Volume addedarchitecture-patterns.mdResource Hierarchy: Storage Resources sectionDependencies
#141 (merged),
#94 (open)
Testing
Ticket
Feature: OSAC-2872
(Storage Control Plane)
Epics:
Inventory & Storage Logic
Operator
Tasks covered in this PR:
(fulfillment-service to hub Volume CR)
controller
controller
Userflow: OSAC-3371
(hub PVC create happy path)
Signed-off-by: akshaynadkarni 25892229+akshaynadkarni@users.noreply.github.com
Assisted-by: Cursor/Claude
Summary by CodeRabbit