Feat/nfs export names - #6497
Conversation
|
This doesn't address the degenerate export names issue. I'll re-submit. |
nixpanic
left a comment
There was a problem hiding this comment.
Looks reasonable, thanks for the PR.
Can you please create a clean series of commits, instead of fixups that tidy the code after it was added? Note that we do want Co-authored-by referencing your AI agent on each commit where it assisted.
It would be good to add a test-case in e2e/nfs.go for the functionality too.
|
Both of those CI tests failed on downloading from the repo(https://download.ceph.com/rpm-${CEPH_VERSION}/el10/noarch/ceph-release-1-0.el10.noarch.rpm). I don't know that I can do anything about that. |
c1fbc81 to
8d96c63
Compare
Resubmitted. Thanks for the guidance! |
nixpanic
left a comment
There was a problem hiding this comment.
Thanks, this looks complete to me. Unfortunately we do not have any documentation specific to NFS (or other backends), otherwise it would have been nice to add this option and an example there.
|
@Mergifyio rebase This will include the CI fixes. |
🛑 The pull request rule doesn't match anymoreDetailsThis action has been cancelled. |
8d96c63 to
6dd8906
Compare
|
@Mergifyio rebase GitHub Workflows should work again. |
🛑 The pull request rule doesn't match anymoreDetailsThis action has been cancelled. |
6dd8906 to
e5d903a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Validation can orphan backend volumes, and the new E2E test lacks the required provisioner metadata flag.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds opt-in human-readable NFS export paths derived from PVC metadata.
Changes:
- Persists friendly export names for create, delete, and client updates.
- Detects export-name conflicts and documents the new StorageClass parameter.
- Adds unit and end-to-end coverage.
File summaries
| File | Description |
|---|---|
PendingReleaseNotes.md |
Documents the feature and conflict behavior. |
internal/util/k8s/parameters.go |
Exposes PVC-name metadata. |
internal/nfs/types/volume.go |
Persists and resolves friendly export paths. |
internal/nfs/types/volume_test.go |
Tests export-path selection. |
internal/nfs/types/errors.go |
Defines the export-name conflict error. |
internal/nfs/controller/controllerserver.go |
Parses the option and maps conflicts. |
internal/nfs/controller/controllerserver_test.go |
Tests friendly-name derivation. |
examples/nfs/storageclass.yaml |
Documents StorageClass configuration. |
e2e/utils.go |
Adds export pseudo-path lookup support. |
e2e/nfs.go |
Adds a friendly-export-name scenario. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e5d903a to
bb171b7
Compare
Pull request has been modified.
iPraveenParihar
left a comment
There was a problem hiding this comment.
Thanks! LGTM :)
have more info on the error logs
Pull request has been modified.
4a3388e to
280cf40
Compare
iPraveenParihar
left a comment
There was a problem hiding this comment.
@IanEff squash commits into one?
280cf40 to
792cfac
Compare
Currently, NFS-export names are derived from ComposeCSIID(), a long alphanumeric string that isn't meaningful to users. Add an opt-in "<namespace>/<pvc-name>" naming scheme, gated behind a new friendlyExportNames StorageClass parameter, sourced from PVC metadata supplied by the external-provisioner (--extra-create-metadata=true). The friendly name is persisted in the CephFS journal so DeleteExport and SetClients, which only have the CSI volume ID to work from, can resolve the same export path later. Gated behind an explicit StorageClass parameter rather than being automatic whenever --extra-create-metadata is set, since that flag is already used unconditionally by the CephFS backend for per-tenant KMS scoping (store.Owner, cephfs/store/volumeoptions.go). Keying export naming purely off metadata presence would silently rename exports for any existing encrypted CephFS-backed NFS deployment. Fixes: ceph#4414 Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Ian Furst <ian.furst@gmail.com>
Unlike a volume-ID derived export name, "<namespace>/<pvc-name>" is not guaranteed unique over time (e.g. a PVC recreated under the same name before its old export was cleaned up). CreateExport treated any "Export already exists" response as a safe idempotent retry, which would silently reuse another volume's export and serve its data. Compare the existing export's subvolume path before accepting the collision as our own retry; otherwise fail with ErrExportNameConflict, surfaced to CSI callers as codes.AlreadyExists. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Ian Furst <ian.furst@gmail.com>
792cfac to
2a544db
Compare
|
@iPraveenParihar Squashed! |
|
Wait for #6538 to be merged before starting CI jobs in this PR. |
Merge Protections🔴 1 of 2 protections blocking · waiting on ⛓️ dependency
🔴 ⛓️ Depends-On RequirementsWaiting for
This rule is failing.Requirement based on the presence of
Show 1 satisfied protection🟢 🚦 Auto-queueWhen all merge protections are satisfied, this pull request will be queued automatically. |

Describe what this PR does
NFS export names are currently derived from
ComposeCSIID(), a long alphanumeric string that's meaningless to humans. This PR adds an opt-infriendlyExportNamesStorageClass parameter that names exports<namespace>/<pvc-name>instead.The name is sourced from PVC metadata supplied by the external provisioner, enabled with
--extra-create-metadata=true, and persisted in the CephFS journal so thatDeleteExportandSetClientscan resolve the volume path later on.Is there anything that requires special attention
This feature is gated behind an explicit StorageClass parameter rather than firing automatically whenever
--extra-create-metadatais set because that flag is already used unconditionally by the CephFS backend for per-tenant KMS scoping.It's entirely backwards compatible, as the parameter is set to off by default. No existing exports are affected
Related issues
Fixes: #4414
Future concerns
None.
Checklist:
guidelines in the developer
guide.
Request
notes
updated with breaking and/or notable changes for the next major release.
Show available bot commands
These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:
/retest ci/centos/<job-name>: retest the<job-name>after unrelatedfailure (please report the failure too!)
CI job ordering.
Depends-on: #6538