Repository navigation
OSAC-2876: Storage Control Plane design - #151
openshift-merge-bot[bot] merged 12 commits into
Conversation
Design for the vendor-agnostic storage layer covering the OSAC CSI meta-driver, fulfillment-service Volume API and storage logic layer, Helm chart packaging, and automated cluster storage deployment via AAP. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
|
@akshaynadkarni: This pull request references OSAC-2876 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
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:
WalkthroughThe design document specifies a vendor-agnostic storage control plane for OSAC tenant clusters, covering Volume APIs, CSI integration, reconciliation workflows, security, deployment, observability, testing, and rollout procedures. ChangesStorage control plane
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant CSI as OSAC CSI driver
participant API as Fulfillment-service Volume API
participant Operator as osac-operator
participant Vendor as Vendor CSI controller
CSI->>API: CreateVolume request
API->>Operator: Create hub Volume CR
Operator->>Vendor: Provision vendor volume
Vendor-->>Operator: Update Volume status
Operator->>API: Signal volume state
API-->>CSI: Return AVAILABLE volume
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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 |
AI Design Review: EP-151Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured design document (635 lines) that provides deep technical detail across all dimensions — full proto schemas, step-by-step handler logic, comprehensive failure handling, real alternatives, and specific test scenarios at every level. Feedback: Two minor improvements would strengthen this already strong design: (1) Add a brief documentation dimension note — even if deferred, state what docs are needed (architecture update for storage control plane, API reference for private Volume API, admin guide for CSI driver deployment). (2) Define concrete graduation criteria instead of deferring — e.g., 'Dev Preview: all CRUD operations pass e2e with VAST backend, error paths tested; Tech Preview: multi-tenant isolation verified, no regressions in existing storage conditions.' The access_mode field in VolumeSpec should ideally be an enum (AccessMode) rather than a string, per OSAC API conventions. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
|
@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: 11
🤖 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 `@enhancements/OSAC-2872-storage-control-plane/design.md`:
- Line 170: Remove the trailing whitespace from the PVC with unconfigured
StorageClass line in the design document, preserving its wording and formatting
otherwise.
- Around line 156-166: Update the design to require durable orphan cleanup
before GA: define ownership metadata for volumes, add cleanup during
cluster-termination/teardown workflows, and specify periodic reconciliation that
safely detects and removes vendor volumes whose inventory remains AVAILABLE or
CREATING when CSI DeleteVolume was skipped. Apply the same requirement to the
referenced deletion, teardown, and reconciliation sections.
- Around line 389-404: Update the deployment artifact configuration in the
driver and vendor image sections to replace every latest image tag with the
tested immutable image digest. Also update the associated Helm chart dependency
constraint from >=0.0.0 to an exact or bounded compatible version, preserving
the documented version-skew and rollback strategy.
- Around line 337-340: Remove the design that returns reusable VAST
username/password credentials through the CreateVolume response or any
tenant-cluster-facing Volume API. Update CredentialManager and the CSI
interaction so vendor authentication remains hub-side, or replace it with a
brokered short-lived mechanism that never exposes reusable vendor secrets to
tenant infrastructure; ensure the isolation claim remains accurate.
- Around line 174-175: Update the vendor provisioning flow described for Volume
API CreateVolume retries so VAST volume creation is idempotent even when the
controller crashes before UpdateVolume. Persist and reuse a deterministic vendor
request identity, or reconcile existing vendor state before issuing a new
creation request, ensuring retries cannot create duplicate vendor volumes when
the inventory record lacks a vendor ID.
- Around line 6-11: Align the PR/commit title with the canonical Jira identifier
OSAC-2872 referenced by the design metadata and PRD. If the work is actually for
OSAC-2876, update the linked issue references in the design and PRD consistently
instead.
- Line 27: The Volume API flow must explicitly treat provisioning’s CreateVolume
and UpdateVolume calls, and deprovisioning’s DeleteVolume and UpdateVolume
calls, as separate gRPC operations rather than one call per volume operation.
Update the corresponding provisioning and deprovisioning sections to define
retry and timeout behavior for each call, including the second UpdateVolume
call, and ensure monitoring and alerting cover both calls consistently.
- Around line 356-367: The vendor CSI proxy flow must require authenticated,
identity-validated transport rather than unrestricted configurable gRPC. Update
the design around proxyMgr.GetConnection and the CreateVolume/DeleteVolume proxy
operations to define mTLS/server identity validation, credential handling, and
an allowlist for permitted vendor endpoints before implementation.
- Around line 442-444: Update the Cross-cluster authentication and Tenant
isolation sections to replace long-lived admin service-account tokens with
short-lived, signed, rotatable tenant-bound credentials. Add server-side
cluster-to-tenant identity mapping and enforce least-privilege, per-method
authorization so the CSI driver cannot bypass tenant OPA/JWT checks or receive
broad admin access.
- Around line 317-319: Update the StorageBackend credential design to use an
explicit secret-management path instead of inline username/password values in
data, including secure secret references and envelope encryption for stored
credentials. Define access auditing plus credential revocation and rotation
workflows, including handling compromised credentials, before permitting backend
credential storage.
- Around line 221-240: Update the CreateVolume/CreateVolumeResponse contract to
return backend-resolution data directly for the vendor proxy flow, including
endpoint, volume parameters, and credentials. Keep the persisted Volume object
and VolumeStatus limited to non-secret state and backend identifiers; do not add
these response details to Volume by default. Mark any credential fields as
write-only or redacted, and preserve existing persistence/write paths without
storing secrets.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 66c1cbf6-b737-4f0d-8c24-243e64b940c5
⛔ Files ignored due to path filters (2)
enhancements/OSAC-2872-storage-control-plane/osac-csi-flow-caas.pngis excluded by!**/*.pngenhancements/OSAC-2872-storage-control-plane/osac-storage-components.pngis excluded by!**/*.png
📒 Files selected for processing (1)
enhancements/OSAC-2872-storage-control-plane/design.md
Clarify the Volume API call contract (two calls per operation, not one). Add reclaim policy and cluster teardown cleanup for volume orphans. Add idempotency mechanism (deterministic volume name from PVC). Add transient routing fields to CreateVolumeResponse. Update Open Questions to reflect shared VAST controller with per-tenant credential delivery as open decision. Add spike recommendation for cross-cluster auth. Fix trailing whitespace. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
AI Design Review: EP-151Score: 7/8 | Verdict: PASS
Verdict: A thorough, well-architected design that follows OSAC patterns closely and provides deep implementation detail, held back only by deferred graduation criteria in the test plan. Feedback: Add concrete graduation criteria instead of deferring them — specify measurable conditions like 'all CRUD operations pass e2e, policy denial tested, no regressions in existing storage conditions, volume inventory matches vendor state for test tenants.' Also address the documentation dimension from osac-dimensions.md (even if just to defer it explicitly). Finally, consider whether Open Question 2 (credential delivery mechanism) should be resolved before merge, since option (b) would move vendor proxy logic into fulfillment-service and fundamentally change the repo split described in the proposal. Critical (0)None. Important (2)
Suggestions (4)
Review costModel: claude-opus-4-6 |
|
|
||
| Starting state: a tenant cluster has been provisioned via ClusterOrder, the OSAC CSI driver and VAST node plugins are deployed, and StorageClasses matching the tenant's configured tiers exist on the cluster. | ||
|
|
||
| **Actors:** Tenant User (creates PVC), Kubernetes (external-provisioner, external-attacher, kubelet), OSAC CSI Driver, fulfillment-service (Volume API), VAST CSI Controller, VAST Array. |
There was a problem hiding this comment.
Clarification: tenant User here is the tenant user that created the cluster, and never a specific openshift cluster.
I thought to add that so we understand that the call to the volume-api in the fulfillment-service is authenticated and authroized as one tenant user per all the storage operations for the osac-csi driver on a cluster.
There was a problem hiding this comment.
Thanks for the clarification. Added a note that the tenant user who created the cluster is the authenticated identity for all CSI-initiated Volume API calls on that cluster.
| K8s->>K8s: Create PV, bind PVC | ||
| ``` | ||
|
|
||
| The CSI controller makes two Volume API calls per provision: CreateVolume (persists record, resolves tier, checks policy, returns credentials) and UpdateVolume (records vendor volume ID, transitions to AVAILABLE). |
There was a problem hiding this comment.
I'm not sure it is the csi controller that needs to call the UpdateVolume after the vendor creation. I thinmk that should be internally reconciled by the volume-api
There was a problem hiding this comment.
Makes sense. I've reworked the flow so the Volume API calls the VAST controller internally and returns the completed volume. The CSI driver just makes one CreateVolume call. This also solves the credential isolation issue since vendor creds never leave the fulfillment-service.
| **Deletion flow:** | ||
|
|
||
| 1. Tenant User deletes PVC. | ||
| 2. Unmount (reverse of mount): kubelet -> OSAC Node -> VAST Node (NodeUnpublishVolume, NodeUnstageVolume). |
There was a problem hiding this comment.
Where is the osac.backend type coming now? we should probably persist that as a volumeAttribute on the PersistentVolume when the CSI controller see the volume in fulfillment-service is ready, and creates the PersistentVolume
There was a problem hiding this comment.
I think it is already like that, just need to make sure
There was a problem hiding this comment.
Yes, that's how it works. The Volume API populates volume_context with osac.backend, osac.volume-id, and osac.protocol in the CreateVolume response. Kubernetes stores these as spec.csi.volumeAttributes on the PV. The node plugin reads them at mount time for vendor routing. I've made this more explicit in the updated workflow.
There was a problem hiding this comment.
Confirmed, see updated sequence diagram.
| 5. OSAC CSI Controller calls Volume API `DeleteVolume` (updates state to deleting, verifies ownership). | ||
| 6. OSAC CSI Controller proxies DeleteVolume to VAST CSI Controller. | ||
| 7. VAST deletes volume on array. | ||
| 8. OSAC CSI Controller calls Volume API `UpdateVolume` (state: deleted). |
There was a problem hiding this comment.
- it is the volume-api that calls internally the vast controller
- I Don't think the controller needs to update the volume API again after in 5 we called DeleteVolume. I expect the volume-api to be declerative and eventually will delete or make the volume as deleted by its owne reconciler. Osac controller would need to follow the state update untill the volumme disapears, or with state=deleted.
There was a problem hiding this comment.
Agreed, updated both the create and delete flows. The Volume API is now fully declarative: it handles the vendor call internally and reconciles state. The CSI driver doesn't call UpdateVolume.
|
|
||
| **CreateVolume handler logic:** | ||
|
|
||
| 1. Validate required fields (`storage_tier_id`, `size_gib`, `cluster_id`). |
There was a problem hiding this comment.
The assumption here is that the CSI, when calling into the CreateVolume, knows the tier already, and the storageClass has the needed tier annotation. is this right?
There was a problem hiding this comment.
Right. The StorageClass carries tier and tenant as parameters, which Kubernetes passes to the driver in req.GetParameters(). That's how the driver knows the tier.
| **CreateVolume handler logic:** | ||
|
|
||
| 1. Validate required fields (`storage_tier_id`, `size_gib`, `cluster_id`). | ||
| 2. Resolve tier: call `tierResolver.Resolve(ctx, tenant, storageTierID)` which reads StorageTier and its associated StorageBackend from the DB, returns backend endpoint, protocol, and volume parameters. |
There was a problem hiding this comment.
I guess we can change this to tier GET, cecause tht is merely a get tier by ID, and tenant ID.
There was a problem hiding this comment.
Yeah, simplified the naming. It's just a GET with a backend join.
| 5. Delegate to `generic.Create()` to persist the volume record. | ||
| 6. Return the created volume (with backend details in status) to the CSI driver. | ||
|
|
||
| The CSI driver then uses the returned backend details to proxy the vendor CreateVolume call, and follows up with an UpdateVolume to set `vendor_volume_id` and transition to `AVAILABLE`. |
There was a problem hiding this comment.
It is the volume API that will forward the call to the CSI backends (the vendor CSi) to create the volume. the details of thata reconciliation will be put on the Volume object.
The flow looks something like:
k8s calls csi.CreateVolume PVC123
csi calls fulfillment.CreateVolume -> return 200 OK volume status transits to CREATING and calls vendor.Create in the volume controller
poll fulfillment.GetVolume
return when volume is ready
if timeout:
k8s calls csi.CreaetVolume PVC123 (same PVC name)
csi calls fulfillmentCreateVolume return 409 conflict
poll fulfillment.GetVolume till state=READY
Flow is idempotenet, and the volume API takes care of creation
There was a problem hiding this comment.
Adopted this. The design now follows exactly this flow: CreateVolume returns 200/CREATING, reconciler handles the vendor call, CSI polls GetVolume, 409 on duplicate. See the updated sequence diagram and the new Volume Reconciler section.
Volume API now calls the VAST CSI controller internally for CreateVolume and DeleteVolume. Vendor credentials never leave the fulfillment-service process. The CSI driver makes a single declarative CreateVolume call and receives the completed volume with routing metadata. Attach/detach remains proxied by the CSI driver. Adds vendor proxy package to fulfillment-service storage logic layer. Updates sequence diagram, deletion flow, error handling, failure table, alternatives, and open questions to reflect the new model. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
AI Design Review: EP-151Score: 7/8 | Verdict: PASS
Verdict: A thorough, implementation-ready design that follows OSAC patterns well with detailed proto schemas, failure handling, and test scenarios; the main weakness is implementation-focused goals and a missing Documentation dimension. Feedback: Rewrite the Goals section to state user-visible outcomes rather than engineering tasks — e.g., 'Tenants can create PVCs on CaaS clusters without exposure to vendor-specific storage details' instead of 'Reuse the existing fulfillment-service GenericServer.' Add a Documentation line in the cross-cutting dimensions (even if deferred) and define graduation criteria with measurable conditions (e.g., 'All CRUD operations pass E2E, error paths tested, no regressions in existing storage conditions'). Consider typing Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
Volume API is now declarative: CreateVolume persists in CREATING state and returns immediately. A background reconciler in the fulfillment- service picks up CREATING volumes, calls the vendor CSI controller, and transitions to AVAILABLE. The CSI driver polls GetVolume until ready. Same pattern for deletion (DELETING -> reconciler -> DELETED). Adds Volume Reconciler section documenting the reconciler loop, concurrency handling (row-level locking for multi-replica), and restart behavior. Adds CSI driver poll loop with 409 Conflict handling for retry idempotency. Notes the StorageBackend schema limitation: no CSI controller endpoint field (only management API endpoint). Single-hub deployments use config; multi-hub needs a schema addition. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
AI Design Review: EP-151Score: 6/8 | Verdict: PASS
Verdict: A strong, deeply detailed design that follows OSAC patterns and provides thorough implementation specifics, held back by implementation-focused goals (not user-visible outcomes), a deferred graduation criteria section, and a missing Documentation dimension. Feedback: Rewrite the Goals section to state user-visible outcomes (e.g., 'Tenants can provision persistent volumes via opaque storage tiers without vendor knowledge', 'Vendor credentials never leave the hub cluster') and move the current implementation-focused goals to an Implementation Constraints subsection. Add concrete graduation criteria — at minimum, tie them to the E2E scenarios already defined (e.g., 'All 5 E2E scenarios pass, volume reconciler handles concurrent replicas without duplicate vendor calls'). Address the Documentation dimension from osac-dimensions.md, even if only to explicitly defer it (e.g., 'Documentation deferred to GA — API is private and not user-facing'). Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
CSI driver identity is tenant-scoped, not admin. Volume API authorization uses a dedicated CSI role in OPA (not the admin allowlist) enforcing tenant ownership and method-scoped access. AAP teardown playbook now explicitly queries Volume API by cluster_id and deletes volumes before Helm/StorageClass cleanup. Periodic orphan scan added as a pre-GA hardening item in Risks table. Removes duplicate failure handling row. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
AI Design Review: EP-151Score: 7/8 | Verdict: PASS
Verdict: A thorough, well-architected design that follows OSAC patterns closely and provides deep implementation detail; the only material gap is deferred graduation criteria with no measurable conditions. Feedback: Define concrete graduation criteria — e.g., 'Dev Preview: all CRUD operations pass e2e on a single-tenant kind cluster; Tech Preview: multi-tenant tenant isolation verified, reconciler handles 100+ concurrent volumes, stale volume detection exercised; GA: 30-day production soak with no orphaned volumes.' Also explicitly address or defer the Documentation dimension from osac-dimensions.md — the Support Procedures section partially covers operational docs, but user-facing documentation for the private Volume API and CSI driver operational guide should be scoped or deferred. Consider specifying the reconciler's poll interval and stale threshold as concrete values (or at least ranges) rather than 'N seconds' and 'threshold.' Critical (0)None. Important (2)
Suggestions (4)
Review costModel: claude-opus-4-6 |
Commits to osac-{tenant}-{tier} naming convention (e.g.,
osac-acme.com-gold). Drops vendor name and protocol from the
StorageClass name. Protocol remains on the storage-protocol label.
Removes Open Question 3 (resolved).
Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Assisted-by: Cursor/Claude
Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
AI Design Review: EP-151Score: 7/8 | Verdict: PASS
Verdict: A thorough, well-architected design that follows OSAC patterns closely, provides deep implementation detail with full proto schemas and specific error handling, and clearly scopes boundaries with real alternatives -- held back from a perfect score only by deferred graduation criteria. Feedback: Define concrete graduation criteria instead of deferring them -- even preliminary conditions like 'all CRUD operations pass e2e with tenant isolation, error paths tested, no regressions in existing storage conditions' would satisfy the requirement. The access_mode field in VolumeSpec should be an enum rather than a bare string to ensure validation and documentation of valid values. The Volume reconciler's poll interval, backoff strategy, and stale timeout threshold should be specified as configurable defaults rather than left implicit. Critical (0)None. Important (3)
Suggestions (4)
Review costModel: claude-opus-4-6 |
Document the mapping from VolumeStatus fields to volume_context keys in the CSI driver controller section: backend_id -> osac.backend, vendor_volume_id -> osac.volume-id, protocol -> osac.protocol. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
AI Design Review: EP-151Score: 6/8 | Verdict: PASS
Verdict: A strong, deeply technical design that follows OSAC architectural patterns and provides thorough implementation detail, held back from a higher score by implementation-focused goals, a missing Documentation dimension, and deferred graduation criteria. Feedback: Reframe the Goals section as user-visible outcomes rather than implementation tasks — e.g., 'Tenants can create persistent volumes using opaque storage tiers without vendor-specific knowledge' instead of 'Reuse GenericServer patterns.' Add a Documentation subsection addressing what admin guides, architecture docs, or API reference updates are needed (or explicitly defer them with a ticket). Provide at least preliminary graduation criteria with measurable conditions — e.g., 'All CRUD operations pass e2e, error paths tested, no regressions in existing storage conditions' — rather than fully deferring to a future release. Critical (0)None. Important (4)
Suggestions (3)
Review costModel: claude-opus-4-6 |
Volume lifecycle now follows the established OSAC pattern: the
fulfillment-service creates a Volume CR on the hub, the osac-operator
reconciles it (calls the vendor CSI controller), and a feedback
controller syncs status back.
Key changes:
- Volume CRD in osac-operator with conditions (VendorProvisioned,
PVCBound), phases (Progressing, Ready, Failed, Deleting)
- Dual-controller pattern: resource controller + feedback controller
- Vendor controller discovery via in-cluster DNS (vast.osac-csi-backend.svc)
- PVC/PV tracking: spec.pvcRef (input), status.pvcRef/pvRef (operator
confirmed via cross-cluster GET + requeue), annotations for OpenShift UI
- ClusterOrder finalizer (volume-cleanup) blocks deletion until volumes
processed using PV persistentVolumeReclaimPolicy
- Credentials derived from existing hub Secret (vast-tenant-config-{tenant})
- Cross-cluster auth: tenant user credentials for first release
- Replaced PNG diagrams with Mermaid
- Removed vendor REST adapter references
Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Assisted-by: Cursor/Claude
Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
AI Design Review: EP-151Score: 7/8 | Verdict: PASS
Verdict: A thorough, well-architected design that follows OSAC patterns consistently across all four repos, with deep implementation detail, comprehensive failure handling, and strong alternatives analysis — held back from a perfect score only by deferred graduation criteria. Feedback: The main gap is graduation criteria: replace 'will be defined when targeting a release' with measurable conditions (e.g., 'All CRUD volume operations pass e2e, CSI sanity suite passes, tenant isolation verified, no regressions in existing storage conditions'). Consider reframing Goals as user-visible outcomes ('Tenants can provision persistent volumes through standard PVC workflows without vendor-specific knowledge') rather than implementation constraints ('Reuse GenericServer'). Finally, address the Documentation dimension from osac-dimensions.md — either state what documentation is needed (API reference, architecture updates) or explicitly defer it. Critical (0)None. Important (3)
Suggestions (4)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 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 `@enhancements/OSAC-2872-storage-control-plane/design.md`:
- Line 65: Update the “Architecture: Volume lifecycle components” heading from
level five to level three, using the proposal’s existing heading hierarchy.
- Line 662: Update the fenced code block in the design document to include an
explicit language identifier, using text for plain-text content, while
preserving the block’s existing contents.
- Around line 542-546: Update the DeleteVolume handler logic to be idempotent:
after verifying tenant ownership, return success for volumes already in DELETING
or DELETED state, and for missing volumes, while transitioning only active
volumes to DELETING. Document these outcomes explicitly so CSI retries never
fail or strand PV cleanup.
- Around line 193-196: Standardize the tier identifier contract across the
design: choose one canonical representation and apply it consistently to the
proto, StorageClass parameter, validation, policy lookup, API flow, and
persisted volume record. Update all references to storage_tier, storage_tier_id,
tier names, and ID-based lookups—including the cited sections—so request
handling and persistence use the same identifier semantics.
- Around line 193-195: Define a single immutable cluster UUID as the ownership
and cleanup key in the volume lifecycle documented around CreateVolume and the
teardown flows. Replace interchangeable uses of clusterID, cluster_id, and
spec.cluster with this canonical UUID, while retaining cluster names only as
non-identity metadata; apply the same consistency to the referenced sections.
- Around line 730-733: Update the volume_context and VolumeStatus field
definitions to use the proto’s exact backend field name, replacing
status.backend_id with status.backend. Distinguish the OSAC fulfillment-service
volume UUID from the vendor_volume_id, and populate osac.volume-id with the OSAC
UUID used for CR tracking while retaining vendor_volume_id as the vendor
identifier.
- Around line 560-564: The volume reconciler must process deletion transitions
immediately rather than relying on the periodic sync. Extend the event handling
described in the volume event subscription to recognize the event emitted by
DeleteVolume when a record enters DELETING, then update or delete the
corresponding Volume CR through the existing hubClient flow while preserving
periodic reconciliation as a fallback.
- Around line 634-636: Define an explicit bounded cleanup path for the Failed
state with VendorProvisioned=True and PVCBound=False: after a binding timeout,
transition the volume to DELETING and invoke the existing vendor-volume removal
flow, or specify an equivalent administrative cleanup workflow with ownership,
trigger, and deadline. Apply the same behavior to the corresponding state
definition around the other referenced section, while preserving the existing
terminal Failed behavior for volumes that were never provisioned.
- Around line 405-411: Update the operator RBAC requirements for ClusterOrder
finalizer handling to include get, watch, update, and patch permissions on
ClusterOrder resources, including finalizer updates. Also document and verify
the required cross-cluster kubeconfig/credential access used to inspect PV
reclaim policies during ClusterOrder deletion.
- Line 115: Define the tenant-side authentication flow for the vendor node
plugin, including how it receives non-reusable iSCSI connection and
authentication material without exposing reusable vendor credentials; otherwise
revise the claim that tenant clusters have no vendor credentials. Keep the
statements consistent across the tenant data-plane description and the
referenced deployment and node-plugin sections.
- Around line 562-566: Update the VolumeSpec-to-Volume CR creation flow to
explicitly propagate the authenticated volume record’s tenant value into the
osac.openshift.io/tenant annotation. Populate this mapping server-side before
hubClient.Create(), and ensure reconciliation preserves the authenticated tenant
rather than accepting a client-supplied value.
- Around line 534-538: Define a machine-readable duplicate-volume response for
the 409 path: either add a GetVolume-by-name flow that returns the existing
volume ID, or specify a structured gRPC conflict error detail carrying that ID.
Document how the CSI driver extracts and uses the ID, and apply the same
behavior to the corresponding duplicate-handling section.
- Around line 364-366: Update the Volume idempotency design to use a globally
unique key derived from the full PVC identity—tenant, cluster, namespace, and
name or UID—instead of PVCReference.name alone. Apply the same derived key to
both VolumeSpec.pvc_ref and the active-name uniqueness constraint, ensure
retries for the same PVC remain idempotent without cross-cluster collisions, and
document this contract near the CreateVolume retry behavior.
- Line 164: Update the attach/detach flow represented by the `csictrl
-->|"attach/detach"| vendors` diagram edge to document a reachable tenant-side
CSI controller endpoint, including its DNS/service identity, namespace, network
policy, authentication, TLS, and failure behavior; alternatively relocate the
proxying to the hub-side and document those same operational details there.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 3217e2bd-9a3c-480c-a0c4-002f42cbd69a
📒 Files selected for processing (1)
enhancements/OSAC-2872-storage-control-plane/design.md
Fix tier identifier consistency (storage_tier_id -> storage_tier). Add ClusterOrder RBAC for volume-cleanup finalizer. Make DeleteVolume idempotent (success for DELETING, DELETED, not-found). Process deletion events immediately (subscribe to all volume events, not just CREATED/SIGNALED). Make tenant annotation propagation explicit on Volume CR. Add language identifier to fenced code block. Fix stale volume_context field reference (backend_id -> backend). Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
AI Design Review: EP-151Score: 6/8 | Verdict: PASS
Verdict: A thorough and architecturally sound design that follows OSAC patterns consistently across all four repositories, with detailed proto schemas, comprehensive failure handling, and strong test scenarios, held back from a higher score by placeholder graduation criteria and implementation-focused goals. Feedback: Two concrete improvements: (1) Replace the Goals section with user-visible outcomes ('Tenants can provision persistent volumes on CaaS clusters without vendor awareness', 'Platform admins have central volume inventory and policy enforcement') and move the current implementation-focused goals to the Proposal section as design constraints. (2) Add measurable graduation criteria, e.g., 'All CRUD operations pass e2e with VAST backend, tenant isolation verified (tenant A cannot see tenant B volumes), no regressions in existing storage conditions.' Also add a Documentation subsection addressing or deferring user-facing docs (API reference, admin guides for storage tier configuration). Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@enhancements/OSAC-2872-storage-control-plane/design.md`:
- Line 901: Update the Operator RBAC requirement to explicitly grant Secret get
access in the osac-system namespace, matching the tenant-credential mapping
described elsewhere in the design. If retaining “storage config namespace,”
define it consistently as osac-system and use that alias throughout the
document.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 342a4446-19a1-48df-a24f-8c9ef1d25904
📒 Files selected for processing (1)
enhancements/OSAC-2872-storage-control-plane/design.md
Fix heading levels for architecture/components diagrams (h5 -> h3). Clarify volume name is PVC-UID-based from external-provisioner (globally unique, no collision risk). Add ListVolumes with name filter for 409 duplicate resolution. Add three open questions: immutable cluster identity (UUID solution proposed), attach/detach routing, data-plane credentials for node mount (CHAP). Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
AI Design Review: EP-151Score: 6/8 | Verdict: PASS
Verdict: A thorough and well-architected design that follows OSAC patterns consistently, provides deep implementation detail with proto schemas and CRD types, and covers all lifecycle operations — held back from a higher score by implementation-focused goals, a deferred graduation criteria placeholder, and a silently ignored documentation dimension. Feedback: Rewrite the Goals section to state user-visible outcomes (e.g., 'Tenants can provision block storage on CaaS clusters without exposure to vendor-specific details') rather than implementation decisions. Add concrete graduation criteria with measurable conditions (e.g., 'All CRUD operations pass E2E, error paths tested, no regressions in existing storage conditions'). Address the documentation dimension from osac-dimensions.md — either specify what docs are needed (user guide for storage tiers, API reference for private Volume API, architecture doc updates) or explicitly defer it with a Jira reference. Critical (0)None. Important (6)
Suggestions (3)
Review costModel: claude-opus-4-6 |
Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
AI Design Review: EP-151Score: 7/8 | Verdict: PASS
Verdict: A thorough, well-structured design that follows OSAC patterns closely and provides exceptional implementation detail across 5 repositories; the only material gap is deferred graduation criteria in the test plan. Feedback: Add concrete graduation criteria — specify measurable conditions for each stage (Dev Preview, Tech Preview, GA), such as 'all CRUD operations pass E2E, error paths tested, no regressions in existing storage tests, volume inventory matches vendor state for >95% of volumes.' Address the Documentation dimension explicitly — even if deferred, state what docs are needed and when. Consider reframing at least some Goals as user-visible outcomes (e.g., 'Tenants can create persistent volumes without exposure to vendor-specific details') alongside the implementation goals. Critical (0)None. Important (3)
Suggestions (4)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
enhancements/OSAC-2872-storage-control-plane/design.md (1)
117-165: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the topology with the VAST-only scope.
This diagram deploys NetApp and Pure node/controllers, while the non-goals explicitly exclude multi-vendor support beyond VAST. Either remove those components for this release or label them as future architecture; otherwise the deployment scope and acceptance criteria are ambiguous.
🤖 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 `@enhancements/OSAC-2872-storage-control-plane/design.md` around lines 117 - 165, Update the “Components: OSAC storage and CSI deployment topology” diagram to reflect the VAST-only release scope: remove the NetApp and Pure node plugins/controllers and their topology connections, or clearly label them as future architecture. Keep the VAST components and existing VAST-related relationships unchanged.
🤖 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 `@enhancements/OSAC-2872-storage-control-plane/design.md`:
- Around line 364-366: Document the vendor volume name/ID recovery contract in
the retry and duplicate CreateVolume sections: require the external-provisioner
PVC-UID name to reach the CSI CreateVolume request unchanged, and define
tenant-scoped ListVolumes CEL name-filter behavior, including exactly one
matching volume. Specify the handling for no match or multiple matches so
recovery resumes the existing volume without creating duplicates.
---
Outside diff comments:
In `@enhancements/OSAC-2872-storage-control-plane/design.md`:
- Around line 117-165: Update the “Components: OSAC storage and CSI deployment
topology” diagram to reflect the VAST-only release scope: remove the NetApp and
Pure node plugins/controllers and their topology connections, or clearly label
them as future architecture. Keep the VAST components and existing VAST-related
relationships unchanged.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 25c71628-16a6-4ff8-8d3b-8e54c240aba7
📒 Files selected for processing (1)
enhancements/OSAC-2872-storage-control-plane/design.md
| **Cons:** Tenants see vendor-specific StorageClasses. Vendor credentials stored on tenant clusters. No central inventory. No policy enforcement point. | ||
| **Rejected because:** Does not meet the PRD requirements for vendor abstraction, credential isolation, or volume inventory. | ||
|
|
||
| ## Open Questions |
|
|
||
| ## Summary | ||
|
|
||
| This design introduces a vendor-agnostic storage layer for OSAC CaaS tenant clusters. A single CSI driver (`csi.osac.openshift.io`) presents opaque storage tiers to tenants, while a storage logic layer inside the fulfillment-service handles tier resolution, policy enforcement, credential management, and volume inventory. The CSI driver is a thin gRPC client that delegates every storage decision to the fulfillment-service via a private Volume API, then proxies volume operations to vendor CSI controllers running on the hub cluster. See [PRD](prd.md) for detailed requirements. |
There was a problem hiding this comment.
In the google doc we discussed a non-OSAC name to leave the door open to make this a generic driver with a community around it. I suggested facade.csi.io, shadow.csi.io, or broker.csi.io - but am open to other options.
There was a problem hiding this comment.
See my comment here.
We can discuss naming separately and adjust the design once we have agreement.
|
|
||
| ## Summary | ||
|
|
||
| This design introduces a vendor-agnostic storage layer for OSAC CaaS tenant clusters. A single CSI driver (`csi.osac.openshift.io`) presents opaque storage tiers to tenants. The fulfillment-service handles tier resolution, policy enforcement, and volume inventory via a private Volume API. The osac-operator reconciles Volume CRs on the hub cluster, calling vendor CSI controllers to create and delete volumes on storage arrays. See [PRD](prd.md) for detailed requirements. |
There was a problem hiding this comment.
We discussed a generic name for the CSI driver, like facade-csi instead of csi.osac.openshift.io.
There was a problem hiding this comment.
Thing is that a csi driver also needs to say a vendor in its name.
Also the osac project is upstream first and open source, and osac isn't a product name.
btw I just saw that the convention {name}.csi.{domain}
There was a problem hiding this comment.
Looked at how other CSI drivers are named (ebs.csi.aws.com, pd.csi.storage.gke.io, rook-ceph.rbd.csi.ceph.com). Roy is right about the naming order: the convention is {name}.csi.{domain}.
Updated all references in the design to osac.csi.openshift.io.
I don't have a strong opinion on whether the name should be OSAC-specific or generic. Happy to defer to you two on this. If the name changes, one of us can update the design accordingly.
There was a problem hiding this comment.
Link from @avishayt for k8s CSI driver names:
https://kubernetes-csi.github.io/docs/drivers.html
Rename CSI driver from csi.osac.openshift.io to osac.csi.openshift.io
to follow the {name}.csi.{domain} convention used by production CSI
drivers (ebs.csi.aws.com, pd.csi.storage.gke.io, etc.).
Add volume snapshots and clones to Non-Goals (Avishay's comment).
Expand the duplicate CreateVolume recovery section to specify the
name pass-through contract (CSI spec guarantee) and ListVolumes
filter edge cases (0 matches = fatal, >1 = impossible due to
uniqueness constraint).
Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Assisted-by: Cursor/Claude
Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
AI Design Review: EP-151Score: 7/8 | Verdict: PASS
Verdict: Strong design with exceptional implementation depth spanning 5 repos, clear architectural alignment with OSAC patterns, and thorough failure handling — held back from a higher score only by absent graduation criteria in the test plan. Feedback: Add concrete graduation criteria to the test plan — e.g., 'All CRUD operations pass E2E on a real VAST backend, error paths tested, no regressions in existing storage conditions, volume inventory matches vendor array state.' Either add osac.openshift.io/owner-reference annotation on Volume CRs or explicitly explain why the cleanup finalizer on ClusterOrder is preferred over the standard ownership pattern. Remove the duplicated RBAC/Security content (CSI driver identity and OPA policy updates appear verbatim in both Security Considerations and RBAC/Tenancy sections). Critical (0)None. Important (5)
Suggestions (3)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@enhancements/OSAC-2872-storage-control-plane/design.md`:
- Line 367: The Duplicate CreateVolume 409 recovery flow must bound zero-match
ListVolumes failures instead of returning them to Kubernetes indefinitely.
Update the CreateVolume recovery logic to limit reconciliation attempts, then
persist or emit a reconciliation failure and trigger the established repair
alert; only return CSI retryable errors for transient API unavailability, while
treating the data-integrity failure as terminal after the bound.
- Line 367: Update the “Duplicate CreateVolume (retry after timeout)” section to
distinguish CSI guarantees from external-provisioner behavior: state only that
the CO reuses CreateVolumeRequest.name and provisioning is idempotent by name,
and qualify pvc-{PVC-UID} as an external-provisioner convention rather than a
CSI guarantee. If retaining the PVC-UID naming requirement, add the relevant
sidecar/version-specific coverage.
- Line 844: Update the Volume API provisioning flow described near StorageClass
creation so the tenant is derived from the authenticated CSI identity, not
trusted from the mutable StorageClass tenant parameter. Reject requests when the
parameter is present and does not match the authenticated tenant, and use
matching or absent parameters only as a consistency check.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 89c5c716-fe91-4288-b97f-c428e2cfb262
📒 Files selected for processing (1)
enhancements/OSAC-2872-storage-control-plane/design.md
|
|
||
| **Vendor volume creation failure:** The operator Volume controller retries via the standard `provisioning.RunProvisioningLifecycle()` pattern (backoff, retry, status update). The volume name is generated by external-provisioner from the PVC's Kubernetes UID (e.g., `pvc-{PVC-UID}`), which is globally unique across clusters and namespaces. The vendor CSI CreateVolume is idempotent by name per the CSI spec. | ||
|
|
||
| **Duplicate CreateVolume (retry after timeout):** If Kubernetes retries and the CSI driver calls CreateVolume again for the same PVC, external-provisioner sends the same PVC-UID-based name. The Volume API returns 409 Conflict (volume already exists). The CSI driver resolves the existing volume ID by calling `ListVolumes` with a CEL filter on the volume name, then polls `GetVolume(id)` until the volume reaches AVAILABLE. The name pass-through is guaranteed by the CSI spec: external-provisioner generates the volume name from `pvc-{PVC-UID}` and passes it unchanged through `CreateVolumeRequest.name` to the CSI driver, which forwards it to the Volume API. The `ListVolumes` name filter must return exactly one match (the volume name has a unique constraint in the database). Zero matches indicates a data integrity issue (the 409 said the volume exists, but it cannot be found); the CSI driver treats this as a fatal error and returns it to Kubernetes for retry. Multiple matches are impossible due to the uniqueness constraint. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the 409 recovery failure path.
A 409 followed by zero ListVolumes matches is explicitly a data-integrity failure, not a normal transient retry. Returning it to Kubernetes for unlimited retries can leave the PVC Pending indefinitely and repeatedly load the API. After bounded recovery attempts, persist/emit a reconciliation failure and alert for repair; reserve CSI retries for transient unavailability.
🤖 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 `@enhancements/OSAC-2872-storage-control-plane/design.md` at line 367, The
Duplicate CreateVolume 409 recovery flow must bound zero-match ListVolumes
failures instead of returning them to Kubernetes indefinitely. Update the
CreateVolume recovery logic to limit reconciliation attempts, then persist or
emit a reconciliation failure and trigger the established repair alert; only
return CSI retryable errors for transient API unavailability, while treating the
data-integrity failure as terminal after the bound.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'external-provisioner|pvc-\{?PVC|CreateVolumeRequest|provisioner' .Repository: osac-project/enhancement-proposals
Length of output: 20348
🌐 Web query:
Kubernetes external-provisioner csi-provisioner pvc-{UID} CreateVolume name pass-through CSI spec
💡 Result:
In the Kubernetes CSI external-provisioner, the volume name passed to the CSI CreateVolume request is generated by the provisioner itself using a configurable prefix and the PersistentVolumeClaim (PVC) UID [1][2][3]. By default, this name follows the format pvc-{UID}, where the prefix "pvc" is configurable via the --volume-name-prefix flag [4][3]. Regarding "name pass-through" and CSI specification compliance: 1. Volume Naming: The CSI specification requires the CreateVolumeRequest to include a Name field, which serves as a unique identifier for the volume being provisioned. The external-provisioner manages this requirement by automatically generating a unique name based on the PVC's UID to ensure global uniqueness and prevent collisions [5][2][6]. This name is not "passed through" from the PVC object directly as the primary identifier; rather, it is computed by the provisioner [2][3]. 2. Metadata Pass-through: If you require the actual Kubernetes PVC name, namespace, or the resulting PV name to be available to your CSI driver during the CreateVolume call, the external-provisioner provides a mechanism via the --extra-create-metadata flag [7][4]. When enabled, the provisioner injects the following keys into the parameters map of the CreateVolumeRequest [8][7]: - csi.storage.k8s.io/pvc/name - csi.storage.k8s.io/pvc/namespace - csi.storage.k8s.io/pv/name These parameters allow CSI drivers to associate the underlying storage resource with the specific Kubernetes objects that requested it [9]. This is the standard, authoritative approach for passing Kubernetes-specific metadata into the CSI CreateVolume process [7][4].
Citations:
- 1: https://raw.githubusercontent.com/kubernetes-csi/external-provisioner/master/pkg/controller/controller.go
- 2: https://github.com/kubernetes-csi/external-provisioner/blob/v3.1.0/pkg/controller/controller.go
- 3: https://github.com/kubernetes-csi/external-provisioner/blob/master/cmd/csi-provisioner/csi-provisioner.go
- 4: https://pkg.go.dev/github.com/kubernetes-csi/external-provisioner
- 5: PVC.UID for volume name unique but hard for humans to differentiate kubernetes-csi/external-provisioner#67
- 6: https://github.com/kubernetes-csi/external-provisioner/blob/master/pkg/controller/controller.go
- 7: https://kubernetes-csi.github.io/docs/external-provisioner.html
- 8: add pvc metadata to createvolume req kubernetes-csi/external-provisioner#399
- 9: Pass k8s API metadata through parameters in CreateVolume kubernetes-csi/external-provisioner#370
🌐 Web query:
"CSI spec" "CreateVolumeRequest" "Name" "CSI driver"
💡 Result:
In the Container Storage Interface (CSI) specification, the Name field within the CreateVolumeRequest message is a required string that identifies the volume to be provisioned [1][2]. Key details regarding the Name field: 1. Function: It serves as the unique identifier for the volume request [3]. According to the CSI specification, the CreateVolume operation must be idempotent [3]. If a volume with the specified name already exists and meets the requirements defined in the other fields (such as capacity_range, volume_capabilities, and parameters), the CSI driver must return a successful response rather than attempting to create a new volume [3]. 2. Usage in Kubernetes: When using a CSI driver with Kubernetes, the Kubernetes external-provisioner (a common sidecar container) typically generates this name [4][5]. It often derives the name from the PersistentVolumeClaim (PVC) name to ensure consistency and prevent collisions [4][3]. 3. Implementation: In the generated Go code for the CSI specification, this field is accessed via the GetName method on the CreateVolumeRequest struct [1]. It is important to note that while the Name field is used to identify the volume, it is distinct from any opaque parameters passed in the parameters field [6][7]. Some implementations may pass additional metadata, such as the PVC name or namespace, within the parameters map to help the driver contextually manage the volume, particularly if the driver needs to relate the storage resource back to a specific Kubernetes workload [7][4][5].
Citations:
- 1: https://pkg.go.dev/github.com/container-storage-interface/spec/lib/go/csi
- 2: https://static.sched.com/hosted_files/kccncchina2018english/d0/2018%20KubeCon%20Shanghai%20-%20K8S%20Snapshot%20-%20Final.pdf
- 3: AlreadyExists error cause repeated provisioning a volume kubernetes-csi/external-provisioner#124
- 4: https://github.com/kubernetes-csi/external-provisioner/blob/master/pkg/controller/controller.go
- 5: https://github.com/kubernetes-csi/external-provisioner/blob/v3.1.0/pkg/controller/controller.go
- 6: https://github.com/container-storage-interface/spec/blob/master/spec.md
- 7: CreateVolumeRequest should provide CO related volume resource information container-storage-interface/spec#248
🌐 Web query:
github.com container-storage-interface spec CreateVolumeRequest Name idempotent CSI
💡 Result:
In the Container Storage Interface (CSI) specification, the CreateVolumeRequest operation is required to be idempotent [1][2]. The name field in CreateVolumeRequest is the primary mechanism used by the Container Orchestrator (CO) to ensure this idempotency [3][1]. Key requirements and behaviors for CreateVolume idempotency include: 1. Idempotency Guarantee: If a volume with the specified name already exists, and it is compatible with the requested accessibility_requirements, capacity_range, volume_capabilities, and parameters, the CSI plugin must return a successful response (0 OK) containing the existing volume's details [4][5]. 2. Error Handling for Incompatibility: If a volume with the same name exists but is incompatible with the requested parameters (e.g., different capacity range, capabilities, accessibility requirements, or source snapshot), the plugin must return an ALREADY_EXISTS error code [6][4]. 3. CO Responsibilities: The CO generates the name to identify the storage space. If a CreateVolume call fails (e.g., due to a network timeout), the CO may safely retry the request using the same name to determine if the volume was successfully provisioned or to retrieve its volume_id [3][1]. 4. Plugin Obligations: The plugin should ensure that multiple CreateVolume calls with the same name do not result in the provisioning of multiple physical volumes [3][1]. If a plugin cannot enforce this, it risks creating orphaned or redundant volumes [3][1]. The name field serves a dual purpose: it acts as a tool for achieving idempotency and, if the underlying storage system allows it, it may optionally serve as the permanent identifier for the provisioned storage [3][7].
Citations:
- 1: https://github.com/container-storage-interface/spec/blob/master/spec.md
- 2: https://github.com/container-storage-interface/spec/blob/v1.12.0/spec.md
- 3: https://github.com/container-storage-interface/spec/blob/master/csi.proto
- 4: https://groups.google.com/g/container-storage-interface-community/c/k5EAsH5LvMc
- 5: https://github.com/container-storage-interface/spec/blob/v0.3.0/spec.md
- 6: Fix error code on wrong snapshot ID or topology container-storage-interface/spec#412
- 7: https://github.com/container-storage-interface/spec/blob/v1.12.0/csi.proto
Distinguish the CSI contract from external-provisioner specifics.
The CSI spec guarantees CreateVolumeRequest.name is reused by the CO and that provisioning must be idempotent by name, but pvc-{PVC-UID} is an external-provisioner convention, not the spec. Either replace that guarantee with the CSI idempotency guarantee, or add the sidecar/version-specific coverage before documenting this behavior as required.
🤖 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 `@enhancements/OSAC-2872-storage-control-plane/design.md` at line 367, Update
the “Duplicate CreateVolume (retry after timeout)” section to distinguish CSI
guarantees from external-provisioner behavior: state only that the CO reuses
CreateVolumeRequest.name and provisioning is idempotent by name, and qualify
pvc-{PVC-UID} as an external-provisioner convention rather than a CSI guarantee.
If retaining the PVC-UID naming requirement, add the relevant
sidecar/version-specific coverage.
| 1. Deploy the OSAC CSI driver Helm chart to the target cluster (instead of installing the VAST CSI operator via OLM). | ||
| 2. Deploy the VAST CSI controller as a separate Deployment on the hub cluster (in `osac-csi-backend` namespace) if not already running. The service name matches the provider name (e.g., `vast`), so it's reachable at `vast.osac-csi-backend.svc.cluster.local`. | ||
| 3. Deploy the VAST node plugin as a co-located container in the OSAC CSI node DaemonSet on the target cluster. | ||
| 4. Create StorageClasses with provisioner `osac.csi.openshift.io` and parameters `tier=<tierName>`, `tenant=<tenantName>`. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Bind tenant to the authenticated identity.
tenant=<tenantName> is mutable StorageClass configuration and must not authorize cross-tenant operations. The Volume API should derive the tenant from the authenticated CSI identity, reject mismatches with the parameter, and use the parameter only as a consistency check.
🤖 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 `@enhancements/OSAC-2872-storage-control-plane/design.md` at line 844, Update
the Volume API provisioning flow described near StorageClass creation so the
tenant is derived from the authenticated CSI identity, not trusted from the
mutable StorageClass tenant parameter. Reject requests when the parameter is
present and does not match the authenticated tenant, and use matching or absent
parameters only as a consistency check.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, rgolangh 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 |
Publish and unpublish operations (ControllerPublishVolume, ControllerUnpublishVolume) now route through the fulfillment-service instead of proxying directly to the vendor CSI controller. This gives the CSI driver a single cross-cluster connection to the fulfillment-service for all controller operations. Introduces osac.internal.v1.StorageControlPlane, a new gRPC service in a new proto package restricted to OSAC components only (not accessible to CSP admins, unlike osac.private.v1). The fulfillment- service orchestrates vendor calls through the operator, following the same async pattern as create/delete. This resolves the former Open Question 3 (attach/detach routing). Also addresses CodeRabbit review comments from PR osac-project#151: - Distinguish CSI spec idempotency guarantee from external-provisioner pvc-{PVC-UID} naming convention - Bound the 409 recovery failure path (terminal error after retries) - Document StorageClass tenant parameter as a consistency check against the authenticated CSI identity (JWT), not a source of authorization - Deduplicate CSI identity text between Security and RBAC sections Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
| // PVCRef is set when the volume was triggered by a PVC creation. | ||
| // Empty for API-driven volume creation (OSAC-984). | ||
| // +kubebuilder:validation:Optional | ||
| PVCRef *PVCReferenceType `json:"pvcRef,omitempty"` |
There was a problem hiding this comment.
This is strange here. What if I create a volume via API and then attach it to a cluster? I would expect this information to possibly be on a VolumeAttachment but definitely not here.
| Phase PhaseType `json:"phase,omitempty"` | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` | ||
| VendorVolumeID string `json:"vendorVolumeID,omitempty"` | ||
| Backend string `json:"backend,omitempty"` |
There was a problem hiding this comment.
Note: Should not be in the public API when we create it (I know it's out of scope)
| PVCRef *PVCReferenceType `json:"pvcRef,omitempty"` | ||
|
|
||
| // PVRef is set after the PV is created on the tenant cluster. | ||
| // +kubebuilder:validation:Optional | ||
| PVRef *PVReferenceType `json:"pvRef,omitempty"` |
There was a problem hiding this comment.
Part of VolumeAttachment? Not here...
Publish and unpublish operations (ControllerPublishVolume, ControllerUnpublishVolume) now route through the fulfillment-service instead of proxying directly to the vendor CSI controller. This gives the CSI driver a single cross-cluster connection to the fulfillment-service for all controller operations. Introduces osac.internal.v1.StorageControlPlane, a new gRPC service in a new proto package restricted to OSAC components only (not accessible to CSP admins, unlike osac.private.v1). The fulfillment- service orchestrates vendor calls through the operator, following the same async pattern as create/delete. This resolves the former Open Question 3 (attach/detach routing). Also addresses CodeRabbit review comments from PR osac-project#151: - Distinguish CSI spec idempotency guarantee from external-provisioner pvc-{PVC-UID} naming convention - Bound the 409 recovery failure path (terminal error after retries) - Document StorageClass tenant parameter as a consistency check against the authenticated CSI identity (JWT), not a source of authorization - Deduplicate CSI identity text between Security and RBAC sections Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
4 participants
Design: Storage Control Plane
Jira: OSAC-2872
PRD: prd.md (merged in PR #134)
Summary
Introduces a vendor-agnostic storage layer for OSAC CaaS tenant clusters. A single CSI driver (
csi.osac.openshift.io) presents opaque storage tiers to tenants. The fulfillment-service handles tier resolution, policy enforcement, and volume inventory via a private Volume API. The osac-operator reconciles Volume CRs on the hub cluster, calling vendor CSI controllers to create and delete volumes on storage arrays. This follows the established OSAC resource lifecycle pattern (same as ComputeInstance and ClusterOrder).Key design decisions
vast.osac-csi-backend.svc.cluster.local), operator and vendor controller on the same hubvast-tenant-config-{tenant}), credentials never leave the hubRequesting Review On
cluster-storagefinalizer. See Cluster teardown cleanup section.osac-csi-backendwith service name matching the provider (e.g.,vast).How to Review
Assisted-by: Cursor/Claude
Summary by CodeRabbit