OSAC-3273, OSAC-3280: Volume private API, DB migration, and CRD types - #201
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:
WalkthroughAdds private Volume protobuf contracts, database persistence, fulfillment-service CRUD and signal handling, REST/gRPC registration, table rendering, and the Kubernetes Volume API with CRDs. ChangesVolume contracts
Volume persistence rules
Operator Volume resource
Private volumes service
Volume API exposure and rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client as Volumes API client
participant Gateway as REST gateway
participant Server as PrivateVolumesServer
participant Generic as GenericServer
Client->>Gateway: Send Volume API request
Gateway->>Server: Route request to Volumes RPC
Server->>Generic: Delegate volume operation
Generic-->>Server: Return response or error
Server-->>Gateway: Return RPC response
Gateway-->>Client: Return HTTP response
Possibly related PRs
Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
|
@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: 7
🤖 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/database/migrations/93_create_volumes_tables_test.go`:
- Around line 141-158: Update the immutable-column test around the existing id,
name, and tenant updates to assert each error has PostgreSQL SQLSTATE Z0001 from
check_immutable_columns(), rather than only asserting an error occurred. Add a
corresponding project update using a valid alternate project value and apply the
same Z0001 assertion, ensuring foreign-key errors cannot satisfy the test.
- Around line 35-46: Replace the fmt.Sprintf-based queries in the test with
static SQL statements targeting the volumes table for both the insert and count
assertion. Remove the quotedTable interpolation from these assertions; if
additional tables are later supported, use explicitly allowlisted complete query
strings.
In
`@fulfillment-service/internal/database/migrations/93_create_volumes_tables.up.sql`:
- Line 14: Rename the migration version used by the volume tables migration from
93 to an unused version, and update the corresponding migration filename and
identifiers in 93_create_volumes_tables_test.go, including all tool.Migrate(ctx,
93) calls, to keep the migration and tests synchronized.
In `@fulfillment-service/proto/private/osac/private/v1/volumes_service.proto`:
- Around line 87-95: Update PrivateVolumesServer.Update to reject any FieldMask
paths targeting VolumeSpec fields, including spec.size_gib, spec.storage_tier,
spec.access_mode, spec.cluster, spec.pvc_ref, and future spec fields, before
invoking the DAO save. Preserve updates to mutable fields and return the
service’s established validation error for invalid immutable-field changes; add
data-layer enforcement only if required by the existing architecture.
In `@osac-operator/api/v1alpha1/volume_types.go`:
- Around line 116-118: Update osac-operator/api/v1alpha1/volume_types.go:116-118
on the Conditions field with +listType=map, +listMapKey=type,
+patchStrategy=merge, and +patchMergeKey=type markers, and remove its protobuf
struct tag. Regenerate
osac-operator/config/crd/bases/osac.openshift.io_volumes.yaml:141-198 so
conditions uses a map list keyed by type, then re-sync
osac-operator/charts/operator-crds/templates/osac.openshift.io_volumes.yaml:143-200
with the regenerated CRD.
- Around line 32-36: Update the SizeGiB validation in the Volume API type to add
a kubebuilder Maximum constraint matching the largest volume supported by all
storage backends. Keep the existing Required, Minimum, and immutability
validations unchanged, and use an integer-safe bound to prevent oversized
requests from reaching vendor CSI CreateVolume.
- Around line 45-56: Replace the field-level XValidation rules on Cluster and
PVCRef with a spec-level transition validation rule using optionalOldSelf: true,
enforcing that once either optional field is set it cannot be changed or unset
while still allowing an initially absent value to be added. Apply the rule at
the containing spec type and preserve the existing field optionality and JSON
definitions.
🪄 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: 64accf26-f9e2-42f5-b90d-68426bfdff89
⛔ Files ignored due to path filters (8)
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/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.go
📒 Files selected for processing (16)
fulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gofulfillment-service/internal/cmd/service/start/restgateway/start_rest_gateway_cmd.gofulfillment-service/internal/database/migrations/93_create_volumes_tables.up.sqlfulfillment-service/internal/database/migrations/93_create_volumes_tables_test.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/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/config/crd/bases/osac.openshift.io_volumes.yaml
ef40810 to
ab0b709
Compare
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
eb8a2cb to
d4bc854
Compare
d4bc854 to
461d0b5
Compare
GitHub Actions
Failure
Volume CRD for the osac-operator, representing block storage provisioned on vendor arrays through the storage control plane. VolumeSpec (immutable after creation): - storageTier: name of the StorageTier for backend/protocol resolution - sizeGiB: requested capacity - accessMode: Kubernetes access mode (ReadWriteOnce, ReadWriteMany) - cluster: optional, cluster where the PVC exists - pvcRef: optional PVC reference set by the CSI driver VolumeStatus: - phase: Progressing, Ready, Failed, Deleting - conditions: VendorProvisioned (set True by Volume controller after vendor CSI CreateVolume succeeds) - vendorVolumeID, backend, protocol: resolved by tier resolution - pvcRef, pvRef: operator-confirmed PVC/PV on tenant cluster Constants (volume_names.go): namespace (osac-volume), labels (volume, volume-uuid), finalizers (volume, feedback, cleanup). Scheme registration in groupversion_info.go. Print columns show Tier, Size, Access, Cluster, Phase (default) and Backend, VendorID (wide). Generated: CRD manifest, deepcopy, Helm chart CRD sync. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
- Remove spec.cluster from proto and CRD: cluster association will be tracked via status or a future attach/detach model, not as an immutable spec field. This keeps the volume representation consistent regardless of creation flow (PVC-driven vs standalone). - Add list-map markers to CRD Conditions field (+listType=map, +listMapKey=type) so server-side apply merges conditions by type instead of overwriting the entire array. - Assert SQLSTATE Z0001 in migration immutability tests to verify the check_immutable_columns trigger fires (not a foreign key error). Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Address reviewer feedback from Zoltan: 1. VolumeAccessMode enum (proto + CRD): access_mode is now a typed enum matching Kubernetes PersistentVolume access modes (ReadWriteOnce, ReadOnlyMany, ReadWriteMany, ReadWriteOncePod) instead of a raw string. 2. StorageProtocol reuse (proto + CRD): status.protocol now uses the existing StorageProtocol enum instead of a raw string. Moved StorageProtocol from storage_tier_type.proto to a new shared storage_common_type.proto to avoid a false dependency between Volume and StorageTier. 3. Create validation: added validateVolumeCreate method to the server, matching the pattern used by StorageBackends and StorageTiers. Validates metadata.name, spec.storage_tier, spec.size_gib, and spec.access_mode before persisting. Tests: all 85 fulfillment-service suites pass, osac-operator lint 0 issues and all tests pass. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
The fulfillment-service reconciler needs to track which hub cluster hosts the Volume CR so the delete path can look up the correct hub client. Every other hub-based resource (ComputeInstance, NATGateway, Subnet, etc.) has this field for the same reason. 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 hub from field 8 to field 6 in VolumeStatus proto, grouping it with the other system-resolved fields (vendor_volume_id, backend, protocol) and before the reference fields (pvc_ref, pv_ref). Add a spec-level CEL transition rule on VolumeSpec that prevents pvcRef from being removed once set. The existing field-level rule (self == oldSelf) prevents value changes but is skipped when the field is absent. The new rule (!has(oldSelf.pvcRef) || has(self.pvcRef)) blocks removal, completing the immutability guarantee. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
f070c47 to
9de3972
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
🤖 Finished Review · ✅ Success · Started 1:00 PM UTC · Completed 1:17 PM UTC Commit: |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Labels: PR adds Volume resource to the storage control plane (proto, DB migration, CRD types) |
![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.
fullsend-ai-review
Bot
added
the
requires-manual-review
Add Entry("archived_volumes") to the migration DescribeTable to verify
the archived table is created and accepts inserts.
Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Assisted-by: Cursor/Claude
Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
|
/lgtm |
|
/lgtm |
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, rgolangh, zszabo-rh 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 |
|
🤖 Finished Review · ✅ Success · Started 4:24 PM UTC · Completed 4:41 PM UTC Commit: |
|
/lgtm |
|
@akshaynadkarni: you cannot LGTM your own PR. 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 kubernetes-sigs/prow repository. |
|
/lgtm |
6 participants
Summary
Adds the Volume resource data model and private API to the fulfillment-service,
with Volume CRD types in osac-operator. This is the foundation for the storage
control plane feature (OSAC-2872).
Why
The storage control plane needs a Volume resource to track block storage provisioned on
backend storage arrays. This PR establishes the proto definitions, database schema,
passthrough API server, and event integration. The Volume API serves both PVC-driven
flows (CSI driver creates volumes in response to PVCs) and API-driven flows (CSP admin
creates standalone volumes as inventory). Cluster association is intentionally not part
of the Volume spec: it will be tracked via status or a future attach/detach model
(OSAC-984), keeping the volume representation consistent across all creation flows.
What's included
fulfillment-service
volume_type.proto(Volume, VolumeSpec, VolumeStatus, VolumeState,VolumeAccessMode enum, PVCReference, PVReference) and
volumes_service.proto(List, Get, Create, Update, Delete, Signal RPCs with REST transcoding)
storage_common_type.protowith StorageProtocol enum (moved fromstorage_tier_type.proto to avoid false dependency between Volume and StorageTier)
buf.validateannotations on spec fields (storage_tier min_len,size_gib gt 0, access_mode defined_only) + server-side
validateVolumeCreatetenant/project FKs, immutability triggers on id/name/tenant/project
private_volumes_server.gousing GenericServer pattern, Create validatesrequired fields and sets initial state to CREATING
osac-operator
(phase, conditions with list-map markers, vendorVolumeID, backend, protocol as enum, hub,
pvcRef, pvRef)
Not in this PR
Testing
Ticket
Feature: OSAC-2872 (Storage Control Plane)
Epics:
Tasks covered in this PR:
Userflow: OSAC-3371 (hub PVC create happy path)
Signed-off-by: akshaynadkarni 25892229+akshaynadkarni@users.noreply.github.com
Assisted-by: Cursor/Claude