nvmeof: Add external client support by gadididi · Pull Request #6251 · ceph/ceph-csi

gadididi · 2026-04-26T16:26:39Z

Add external client support

Support allowHostNQNs in VAC for external (non-K8s) clients. Hosts can be managed via mutable parameters.
Add host reconciliation logic (add/remove) for NVMe-oF subsystems.
auto-generate subsystem NQN if not provided (for external Client- they have dedicated StorageClass, without SubsystemNQN label) .

In this design, each PVC gets its own NVMe-oF subsystem. That means:

One subsystem per namespace (PVC): When a new PVC is created, a unique NVMe-oF subsystem is also created for it (if no provided SubsystemNQN in the SC) .
Isolation: Each volume is isolated at the NVMe-oF layer - no sharing of subsystems between volumes by default - prevent external hosts access storage to each other, we dont want to leave the responsibly to the client!
Access control: Only hosts explicitly added to that subsystem (via allowHostNQNs in VAC) can access the volume.

issue: #6240

Checklist:

Commit Message Formatting: Commit titles and messages follow
guidelines in the developer
guide.
Reviewed the developer guide on Submitting a Pull
Request
Pending release
notes
updated with breaking and/or notable changes for the next major release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

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 unrelated
failure (please report the failure too!)

gadididi · 2026-04-30T07:29:36Z

This PR is not tested yet. There is an issue in ODF Jenkins. Once it will resolve I will test this PR.

UPDATE:
I tested this PR. all goes smoothly.

Copilot

Pull request overview

This PR extends the NVMe-oF controller path to better support external (non-Kubernetes) clients by making subsystem host access mutable via VolumeAttributesClass parameters and by auto-generating a subsystem NQN when one is not provided.

Changes:

Auto-generate a default NVMe-oF subsystem NQN during CreateVolume when subsystemNQN is not provided.
Add gateway-side host listing and host reconciliation (add/remove) to match a desired allowHostNQNs list.
Extend ControllerModifyVolume() to handle both QoS updates and host allow-list updates, using a shared gateway-connection helper.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
internal/nvmeof/volume.go	Makes `subsystemNQN` optional by generating a default based on `volumeID`.
internal/nvmeof/volume_test.go	Adds coverage for default subsystem NQN generation when missing.
internal/nvmeof/nvmeof.go	Adds `ListHosts()` and `UpdateHostsForSubsystem()` reconciliation logic.
internal/nvmeof/controller/controllerserver.go	Parses `allowHostNQNs`, wires host/QoS updates into `ControllerModifyVolume`, and passes `volumeID` for default NQN generation.

Comments suppressed due to low confidence (1)

internal/nvmeof/controller/controllerserver.go:1121

The comment for AllowHostNQNs documents that users can set the value to "" to allow any host, but parseHostsParameters only accepts a YAML list of strings and will fail to parse "". Either implement explicit support for the wildcard value (and define how it maps to gateway behavior), or update the documentation/validation to disallow it.

// AllowHostNQNs is the VolumeAttributesClass mutable parameter key for specifying
// a YAML list of host NQNs to allow access to a volume. Use "*" to allow any host.
// Example:
//
//	allowHostNQNs: |
//	  - nqn.2014-08.org.nvmexpress:host1
//	  - nqn.2014-08.org.nvmexpress:host2
const AllowHostNQNs = "allowHostNQNs"

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

internal/nvmeof/controller/controllerserver.go:561

parseHostsParameters returns a nil slice when allowHostNQNs is present but empty. Downstream callers use hostsList != nil to decide whether to reconcile, so setting allowHostNQNs: "" cannot remove all currently-allowed hosts (it is treated like the parameter is absent). Return a non-nil empty slice (or return an explicit present boolean) when the key exists but is empty so ControllerModifyVolume can reconcile to an empty allow-list.

func parseHostsParameters(params map[string]string) ([]string, error) {
	allowHostNQNs, exists := params[AllowHostNQNs]
	if !exists {
		return nil, nil
	}
	var allowHostsList []string
	if allowHostNQNs == "" {
		return allowHostsList, nil
	}

internal/nvmeof/controller/mutable_params_test.go:175

The test case for empty allowHostNQNs currently expects a nil slice, which matches the current parsing but also hides the need to distinguish “parameter present but empty list” from “parameter absent”. Update this test to expect a non-nil empty slice (or otherwise assert the presence semantics) so regressions in host reconciliation (clearing all hosts) are caught.

nixpanic

looks good, just one unit-test is missing.

nixpanic · 2026-06-03T08:20:03Z

Wait for #6314 to be merged, then @mergifyio rebase and after rebasing is complete, add the ok-to-test label for automatic merging.

iPraveenParihar · 2026-06-03T10:04:28Z

@Mergifyio rebase

Extracted common gateway connection logic into `withGatewayConnection()` helper to avoid code duplication between QoS and host management operations (in the next commit). The helper manages: - Secret retrieval - NVMe-oF metadata extraction from volume - Gateway connection with automatic cleanup - Operation execution via callback function Refactored modifyNVMeoFQoS to use the new helper. Signed-off-by: gadi-didi <gadi.didi@ibm.com>

Implements dynamic host access control for NVMe-oF volumes to support external(non-Kubernetes) clients through VolumeAttributesClass. Changes: - Add `parseHostsParameters()` to parse YAML host list from VAC - Implement `modifyNVMeoFHosts()` for runtime host updates - Add `ListHosts()` and `UpdateHostsForSubsystem()` for host updateing - Support allowHostNQNs parameter in CreateVolume and ControllerModifyVolume - Auto-generate subsystem NQN from volumeID if not provided Users can now specify external hosts in VAC mutable parameters: allowHostNQNs: | - nqn.2014-08.org.nvmexpress:host1 - nqn.2014-08.org.nvmexpress:host2 Signed-off-by: gadi-didi <gadi.didi@ibm.com>

mergify · 2026-06-03T10:05:24Z

rebase

✅ Branch has been successfully rebased

ceph-csi-bot · 2026-06-03T10:15:58Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2026-06-03T10:15:58Z

/test ci/centos/k8s-e2e-external-storage/1.34

ceph-csi-bot · 2026-06-03T10:15:59Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2026-06-03T10:15:59Z

/test ci/centos/mini-e2e-helm/k8s-1.34

ceph-csi-bot · 2026-06-03T10:16:00Z

/test ci/centos/k8s-e2e-external-storage/1.33

ceph-csi-bot · 2026-06-03T10:16:00Z

/test ci/centos/k8s-e2e-external-storage/1.35

ceph-csi-bot · 2026-06-03T10:16:00Z

/test ci/centos/mini-e2e/k8s-1.34

ceph-csi-bot · 2026-06-03T10:16:00Z

/test ci/centos/mini-e2e-helm/k8s-1.33

ceph-csi-bot · 2026-06-03T10:16:01Z

/test ci/centos/mini-e2e-helm/k8s-1.35

ceph-csi-bot · 2026-06-03T10:16:01Z

/test ci/centos/mini-e2e/k8s-1.33

ceph-csi-bot · 2026-06-03T10:16:02Z

/test ci/centos/mini-e2e/k8s-1.35

mergify · 2026-06-03T13:39:43Z

Deprecation notice: This pull request comes from a fork and was queued with update_method=rebase and update_bot_account impersonation. This capability will be removed on July 1, 2026. After this date, the merge queue will no longer be able to rebase fork pull requests with this configuration. To avoid disruption, switch to update_method=merge in your queue rule.

mergify · 2026-06-03T13:40:10Z

Merge Queue Status

✅ Entered queue — 2026-06-03 13:39 UTC · Rule: default
✅ Checks skipped · PR is already up-to-date
✅ Merged — 2026-06-03 13:40 UTC · at c754bd636f18170f18dd9d48edd7c7d1f6250836 · rebase

This pull request spent 58 seconds in the queue, including 9 seconds running CI.

Required conditions to merge

#approved-reviews-by >= 2 [🛡 GitHub branch protection]
- nvmeof: Add external client support #6251

mergify Bot added the component/nvme-of Issues and PRs related to NVMe-oF. label Apr 26, 2026

gadididi force-pushed the nvmeof/external_clients branch 2 times, most recently from ba8d811 to 9e8560e Compare April 27, 2026 08:01

gadididi changed the title ~~nvmeof: external clients [WIP]~~ nvmeof: Add external client support Apr 27, 2026

gadididi requested a review from nixpanic April 27, 2026 09:15

gadididi marked this pull request as ready for review April 27, 2026 09:20

gadididi force-pushed the nvmeof/external_clients branch from 9e8560e to b22de20 Compare April 27, 2026 10:14

gadididi self-assigned this Apr 27, 2026

nixpanic reviewed Apr 28, 2026

View reviewed changes

gadididi force-pushed the nvmeof/external_clients branch from b22de20 to d9bc798 Compare April 29, 2026 08:25

gadididi requested review from Rakshith-R and nixpanic April 29, 2026 10:44

gadididi force-pushed the nvmeof/external_clients branch from d9bc798 to 6f8aef8 Compare April 30, 2026 09:52

nixpanic reviewed May 1, 2026

View reviewed changes

Comment thread internal/nvmeof/controller/controllerserver.go

gadididi requested a review from nixpanic May 10, 2026 06:47

gadididi added the keepalive This label can be used to disable stale bot activiity in the repo label May 10, 2026

gadididi force-pushed the nvmeof/external_clients branch from 6f8aef8 to d8185d1 Compare May 13, 2026 11:40

gadididi requested a review from Copilot May 13, 2026 11:40

Copilot started reviewing on behalf of gadididi May 13, 2026 11:41 View session

Copilot AI reviewed May 13, 2026

View reviewed changes

Comment thread internal/nvmeof/controller/controllerserver.go Outdated

Comment thread internal/nvmeof/controller/controllerserver.go

gadididi force-pushed the nvmeof/external_clients branch from d8185d1 to 852fd6c Compare May 13, 2026 13:36

gadididi requested a review from Copilot May 13, 2026 13:37

Copilot started reviewing on behalf of gadididi May 13, 2026 13:38 View session

Copilot AI reviewed May 13, 2026

View reviewed changes

Comment thread internal/nvmeof/controller/controllerserver.go

Comment thread internal/nvmeof/nvmeof.go

gadididi force-pushed the nvmeof/external_clients branch 2 times, most recently from 852fd6c to b16bc7d Compare May 13, 2026 13:56

gadididi mentioned this pull request May 14, 2026

e2e: add nvmeof vac test #6276

Merged

gadididi requested a review from a team May 14, 2026 07:14

nixpanic requested changes May 28, 2026

View reviewed changes

Comment thread internal/nvmeof/controller/controllerserver.go

gadididi added 2 commits June 3, 2026 10:05

ceph-csi-bot force-pushed the nvmeof/external_clients branch from c686822 to c754bd6 Compare June 3, 2026 10:05

gadididi added the ok-to-test Label to trigger E2E tests label Jun 3, 2026

ceph-csi-bot added ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. and removed ok-to-test Label to trigger E2E tests labels Jun 3, 2026

mergify Bot removed the ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. label Jun 3, 2026

nixpanic mentioned this pull request Jun 3, 2026

rebase: bump golang.org/x/net from 0.54.0 to 0.55.0 in the golang-dependencies group #6312

Merged

mergify Bot added the queued label Jun 3, 2026

mergify Bot merged commit 734c5b1 into ceph:devel Jun 3, 2026
44 checks passed

gadididi deleted the nvmeof/external_clients branch June 3, 2026 13:41

mergify Bot removed the queued label Jun 3, 2026

gadididi mentioned this pull request Jun 14, 2026

nvmeof: Add support for external client via ControllerModifyVolume() and using ns masking #6240

Closed

gadididi mentioned this pull request Jul 8, 2026

nvmeof: move QoS and host parsing into dedicated types\files. #6391

Open

Uh oh!

Conversation

gadididi commented Apr 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gadididi commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nixpanic commented Jun 3, 2026

Uh oh!

iPraveenParihar commented Jun 3, 2026

Uh oh!

mergify Bot commented Jun 3, 2026

✅ Branch has been successfully rebased

Uh oh!

ceph-csi-bot commented Jun 3, 2026

Uh oh!

ceph-csi-bot commented Jun 3, 2026

Uh oh!

ceph-csi-bot commented Jun 3, 2026

Uh oh!

ceph-csi-bot commented Jun 3, 2026

Uh oh!

ceph-csi-bot commented Jun 3, 2026

Uh oh!

ceph-csi-bot commented Jun 3, 2026

Uh oh!

ceph-csi-bot commented Jun 3, 2026

Uh oh!

ceph-csi-bot commented Jun 3, 2026

Uh oh!

ceph-csi-bot commented Jun 3, 2026

Uh oh!

ceph-csi-bot commented Jun 3, 2026

Uh oh!

ceph-csi-bot commented Jun 3, 2026

Uh oh!

mergify Bot commented Jun 3, 2026

Uh oh!

mergify Bot commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gadididi commented Apr 26, 2026 •

edited

Loading

gadididi commented Apr 30, 2026 •

edited

Loading

mergify Bot commented Jun 3, 2026 •

edited

Loading