nvmeof: rearrangement create nvmeof resources by gadididi · Pull Request #6237 · ceph/ceph-csi

gadididi · 2026-04-16T14:58:08Z

Clean up NVMe-oF controller code

Moved two chunks of NVMe-oF setup logic out of the controller and into the nvmeof package where they belong.

First commit: Moved listener parsing from controller into nvmeof package. The controller was doing JSON parsing and validation for listeners, which doesn't make sense—that's nvmeof-specific logic. Now the controller just calls nvmeof.SetUpListeners() and gets back the parsed data.

Second commit: Moved volume data initialization into its own method. The controller had a lot of lines of code setting up the NVMeoFVolumeData struct inline. Pulled all that into a new SetNvmeofVolumeData() method on the struct. Controller now just creates an empty struct and calls the setup method.

Both changes make the controller code shorter and keep nvmeof-specific stuff in the nvmeof package. No behavior changes, just moving code around.

need to test and check nothing broke.

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!)

Copilot

Pull request overview

This PR refactors NVMe-oF controller setup by moving NVMe-oF–specific parsing/initialization logic into the internal/nvmeof package, reducing controller responsibilities while preserving behavior.

Changes:

Moved listeners JSON parsing/validation into nvmeof.SetUpListeners().
Extracted NVMe-oF volume data initialization into (*NVMeoFVolumeData).SetNvmeofVolumeData(...).
Simplified controller-side validation and resource creation to call the new helpers.

Reviewed changes

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

File	Description
internal/nvmeof/volume.go	Adds listener parsing/validation helper and a volume-data initialization method.
internal/nvmeof/controller/controllerserver.go	Replaces inline listener parsing and struct setup with calls into the nvmeof package.

nixpanic

The Copilot doc changes are indeed nice improvements.

Left a few other comments as well.

nixpanic · 2026-04-17T10:44:03Z

+func validateListenersParameter(listenersJSON string) (uint, error) {
+	listeners, err := nvmeof.SetUpListeners(listenersJSON)
+	if err != nil {
+		return 0, err


have the function return an int and return -1 in case of an error? An int should be large enough to count all listeners.

nixpanic · 2026-04-17T10:44:44Z

+// SetUpListeners parses the listeners JSON and validates the required fields.
+// It returns a slice of ListenerDetails or an error if the JSON is invalid
+// or if required fields are missing.
+func SetUpListeners(listenersJSON string) ([]ListenerDetails, error) {


Setup as one word.

This function can be tested with a unit-test too.

nixpanic · 2026-04-17T10:48:57Z

 	return listeners, nil
 }
+
+func (v *NVMeoFVolumeData) SetNvmeofVolumeData(parameters map[string]string) error {


maybe call the function SetFromParameters? The double NVMeoF in the struct and function name should be prevented.

nixpanic · 2026-04-17T10:49:46Z

+
+func (v *NVMeoFVolumeData) SetNvmeofVolumeData(parameters map[string]string) error {
+	// set subsystem NQN
+	v.SubsystemNQN = parameters["subsystemNQN"]


Have you considered returning an error for required parameters?

I thought about it, but at the beginning of CreateVolume() it checks if required parameters exist (in validateCreateVolumeRequest()). So, I thought maybe it is redundant to validate twice.

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

internal/nvmeof/controller/controllerserver.go:495

Comment grammar: "it possible" / "Also it possible" should be corrected (e.g., "it is possible" / "It is also possible") to keep inline docs clear and professional.

// validateListenersParameter parses the listeners JSON parameter and validates its contents.
// it possible to have zero listeners if networkMask is provided,
// because in that case the gateway will automatically create listeners
// for all interfaces in the specified network mask.
// Also it possible to have listeners with empty address or port,
// in that case the gateway will listen on all

gadididi · 2026-04-23T10:35:08Z

@mergify rebase

move `SetUpListeners()` to be in nvmeof package. there is no good reason for the controller to do the parsing and assigning. Signed-off-by: gadi-didi <gadi.didi@ibm.com>

Move NVMeoFVolumeData initialization logic from createNVMeoFResources() into a new SetNvmeofVolumeData() method on the struct. This makes createNVMeoFResources() shorter and more readable and maintains separation of concerns between controller operations and data structure initialization. No functional changes.. Signed-off-by: gadi-didi <gadi.didi@ibm.com>

added TestSetupListeners and TestSetFromParameters. Signed-off-by: gadi-didi <gadi.didi@ibm.com>

mergify · 2026-04-23T10:35:27Z

Deprecation notice: This pull request comes from a fork and was rebased using bot_account impersonation. This capability will be removed on July 1, 2026. After this date, the rebase action will no longer be able to rebase fork pull requests with this configuration. Please switch to the update action/command to ensure compatibility going forward.

mergify · 2026-04-23T10:35:28Z

rebase

✅ Branch has been successfully rebased

gadididi · 2026-04-23T10:36:26Z

@mergify queue

mergify · 2026-04-23T10:36:34Z

Merge Queue Status

🛑 Queue command has been cancelled

ceph-csi-bot · 2026-04-23T11:36:29Z

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

ceph-csi-bot · 2026-04-23T11:36:29Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2026-04-23T11:36:30Z

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

ceph-csi-bot · 2026-04-23T11:36:30Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2026-04-23T11:36:30Z

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

ceph-csi-bot · 2026-04-23T11:36:31Z

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

ceph-csi-bot · 2026-04-23T11:36:31Z

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

ceph-csi-bot · 2026-04-23T11:36:31Z

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

ceph-csi-bot · 2026-04-23T11:36:31Z

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

ceph-csi-bot · 2026-04-23T11:36:32Z

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

ceph-csi-bot · 2026-04-23T11:36:32Z

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

nixpanic · 2026-04-23T14:46:00Z

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

nixpanic · 2026-04-23T14:47:11Z

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

Failed in an unrelated test logs:

RBD Test RBD CSI [It] restore snapshot to a bigger size PVC

  [FAILED] failed to validate rbd images in pool replicapool trash: failed to list images in trash 2026-04-23T13:13:07.027+0000 7fb6ab342640 -1 monclient: get_auth_request but no auth handler is set up

nixpanic · 2026-04-24T09:12:45Z

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

nixpanic · 2026-04-24T09:13:11Z

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

Jenkins Slave was restarted 😞

mergify · 2026-04-24T12:13:17Z

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-04-24T12:13:40Z

Merge Queue Status

✅ Entered queue — 2026-04-24 12:13 UTC · Rule: default
✅ Checks skipped · PR is already up-to-date
✅ Merged — 2026-04-24 12:14 UTC · at 145f5ffee56eae5ca6d7f325676652b747ff8226 · rebase

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

Required conditions to merge

#approved-reviews-by >= 2 [🛡 GitHub branch protection]
- nvmeof: rearrangement create nvmeof resources #6237
#changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
- nvmeof: rearrangement create nvmeof resources #6237

gadididi requested a review from Copilot April 16, 2026 14:58

gadididi self-assigned this Apr 16, 2026

gadididi added the component/nvme-of Issues and PRs related to NVMe-oF. label Apr 16, 2026

gadididi requested a review from nixpanic April 16, 2026 14:58

Copilot started reviewing on behalf of gadididi April 16, 2026 14:58 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

nixpanic changed the title ~~nNvmeof: rearrangement create nvmeof resources~~ nvmeof: rearrangement create nvmeof resources Apr 17, 2026

nixpanic reviewed Apr 17, 2026

View reviewed changes

gadididi force-pushed the nvmeof/rearrangement_createNVMeoFResources branch from b059aa5 to f80b032 Compare April 19, 2026 06:24

gadididi requested review from Copilot and nixpanic April 19, 2026 10:14

Copilot started reviewing on behalf of gadididi April 19, 2026 10:15 View session

Copilot AI reviewed Apr 19, 2026

View reviewed changes

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

Comment thread internal/nvmeof/volume_test.go

Comment thread internal/nvmeof/volume.go

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

gadididi requested a review from a team April 19, 2026 10:53

gadididi force-pushed the nvmeof/rearrangement_createNVMeoFResources branch from 8b521b5 to eccf79a Compare April 19, 2026 11:02

nixpanic approved these changes Apr 22, 2026

View reviewed changes

nixpanic requested a review from a team April 22, 2026 16:14

Madhu-1 approved these changes Apr 23, 2026

View reviewed changes

gadididi added 3 commits April 23, 2026 10:35

nvmeof: move setup listener into nvmeof package

f0587f9

move `SetUpListeners()` to be in nvmeof package. there is no good reason for the controller to do the parsing and assigning. Signed-off-by: gadi-didi <gadi.didi@ibm.com>

nvmeof: add unit test for new functions

145f5ff

added TestSetupListeners and TestSetFromParameters. Signed-off-by: gadi-didi <gadi.didi@ibm.com>

ceph-csi-bot force-pushed the nvmeof/rearrangement_createNVMeoFResources branch from eccf79a to 145f5ff Compare April 23, 2026 10:35

nixpanic added the ok-to-test Label to trigger E2E tests label Apr 23, 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 Apr 23, 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 Apr 23, 2026

mergify Bot added the queued label Apr 24, 2026

mergify Bot merged commit e6ca34a into ceph:devel Apr 24, 2026
45 checks passed

mergify Bot removed the queued label Apr 24, 2026

gadididi deleted the nvmeof/rearrangement_createNVMeoFResources branch April 26, 2026 05:24

Uh oh!

Conversation

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

Uh oh!

Clean up NVMe-oF controller code

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

nixpanic Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

gadididi Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

nixpanic Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

nixpanic Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

gadididi Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

nixpanic Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

gadididi Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gadididi commented Apr 23, 2026

Uh oh!

mergify Bot commented Apr 23, 2026

Uh oh!

mergify Bot commented Apr 23, 2026

✅ Branch has been successfully rebased

Uh oh!

gadididi commented Apr 23, 2026

Uh oh!

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

Uh oh!

Merge Queue Status

Uh oh!

ceph-csi-bot commented Apr 23, 2026

Uh oh!

ceph-csi-bot commented Apr 23, 2026

Uh oh!

ceph-csi-bot commented Apr 23, 2026

Uh oh!

ceph-csi-bot commented Apr 23, 2026

Uh oh!

ceph-csi-bot commented Apr 23, 2026

Uh oh!

ceph-csi-bot commented Apr 23, 2026

Uh oh!

ceph-csi-bot commented Apr 23, 2026

Uh oh!

ceph-csi-bot commented Apr 23, 2026

Uh oh!

ceph-csi-bot commented Apr 23, 2026

Uh oh!

gadididi commented Apr 16, 2026 •

edited

Loading

mergify Bot commented Apr 23, 2026 •

edited

Loading

mergify Bot commented Apr 24, 2026 •

edited

Loading