nvmeof: change the return error code by gadididi · Pull Request #6535 · ceph/ceph-csi

gadididi · 2026-09-03T12:10:43Z

Return specific gRPC status codes from NVMe-oF resource creation and cleanup.
Preserve validation errors as InvalidArgument, duplicate resources as Aborted, and operational failures as Internal.
Propagate these errors through CreateVolume and DeleteVolume instead of wrapping them as generic internal errors.

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

CI job ordering.

Depends-on: #6540

Copilot

🟡 Changes recommended

Client-visible error semantics changed without targeted unit tests, and there are misleading/less-informative error/log messages in the updated paths.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the NVMe-oF CSI controller to return more specific gRPC status codes from NVMe-oF resource creation and cleanup, and to propagate those errors through CreateVolume/DeleteVolume rather than re-wrapping them as generic Internal errors.

Changes:

Propagate NVMe-oF setup/cleanup errors from CreateVolume and DeleteVolume directly to preserve their gRPC codes.
Map parameter/validation failures to InvalidArgument, lock/contention cases to Aborted, and operational failures to Internal inside NVMe-oF resource create/cleanup paths.

File summaries

File	Description
internal/nvmeof/controller/controllerserver.go	Adjusts NVMe-oF create/cleanup error returns to use/propagate specific gRPC status codes.

Review details

Suppressed comments (1)

internal/nvmeof/controller/controllerserver.go:987

This cleanup path returns an error that will be surfaced directly to the CSI client; the new message "gateway connection failed" loses the earlier "for cleanup" context and can be confused with CreateVolume failures. Include cleanup context in the status message to make troubleshooting clearer.

	gateway, err := connectGateway(ctx, &nvmeof.GatewayConfig{
		Address: nvmeofData.GatewayManagementInfo.Address,
		Port:    nvmeofData.GatewayManagementInfo.Port,
	})
	if err != nil {
		return status.Errorf(codes.Internal, "gateway connection failed: %v", err)
	}

Files reviewed: 1/1 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

nixpanic

Thanks for the cleanup!

mergify · 2026-09-08T15:40:46Z

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

nixpanic · 2026-09-08T16:05:47Z

@gadididi , this now has a conflict, can you please address it?

Pull request has been modified.

gadididi · 2026-09-09T08:14:13Z

@mergify rebase

mergify · 2026-09-09T08:14:33Z

rebase

☑️ Nothing to do, the required conditions are not met

Details

-conflict [📌 rebase requirement]
-closed [📌 rebase requirement]
queue-position = -1 [📌 rebase requirement]
any of:
- -linear-history [📌 rebase requirement]
- #commits-behind > 0 [📌 rebase requirement]

gadididi · 2026-09-09T08:15:33Z

/queue

ceph-csi-bot · 2026-09-09T08:16:01Z

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

ceph-csi-bot · 2026-09-09T08:16:01Z

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

ceph-csi-bot · 2026-09-09T08:16:02Z

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

ceph-csi-bot · 2026-09-09T08:16:02Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2026-09-09T08:16:02Z

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

ceph-csi-bot · 2026-09-09T08:16:02Z

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

ceph-csi-bot · 2026-09-09T08:16:03Z

/test ci/centos/upgrade-tests-rbd

nixpanic · 2026-09-11T10:11:28Z

Wait for #6540 to be merged before /queue'ing this PR.

gadididi · 2026-09-14T07:29:29Z

@mergify rebase

mergify · 2026-09-14T07:29:35Z

rebase

❌ This pull request comes from a fork and cannot be rebased

Details

GitHub refuses an OAuth token on its rebase API for a fork, so rebasing one means impersonating a GitHub user to force-push the contributor's branch. Mergify does not do that.

Use the update action or the @mergifyio update command instead: it brings the pull request up to date by merging the base branch into it, and needs no impersonation. It only has something to do when the pull request is behind its base branch, so if what the branch needs is a linear history, its author has to rebase it themselves.

nixpanic · 2026-09-14T07:33:31Z

@mergify rebase

There is no need to do this, it'll get done when CI jobs are triggered just before merging. Doing it earlier will just cause additional CI jobs to run, which will be re-run later again too.

Also, @mergifyio rebase doesn't work anymore, we're looking into that 😞

nixpanic · 2026-09-14T07:54:14Z

@Mergifyio update

mergify · 2026-09-14T07:54:24Z

update

❌ Pull request can't be updated with latest base branch changes

Details

This pull request seems to come from a fork, and Mergify needs the author's permission to update its branch.
The author needs to enable "Allow edits from maintainers" on this pull request, or update the branch manually.

gadididi · 2026-09-14T08:00:25Z

/test ci/centos/mini-e2e/k8s-1.36/nvmeof

This failed with

nvmeof Test NVMe CSI [It] test volumeGroupSnapshot
/go/src/github.com/ceph/ceph-csi/e2e/nvmeof.go:582

  [FAILED] failed to validate omap count for rados ls --pool=nvmeofpool | grep -v default | grep -v csi.volume.group. |  grep -c ^csi.volume.: expected omap object count 6, got 7

(logs)

Might be a race while deleting volumes?

@nixpanic
I will take a look later today 🙂

fix the return error code from `createNVMeoFResources()` and `cleanupNVMeoFResources` to be more accuratly. return GRPc error code instead of they returned just general internal error. Signed-off-by: gadi-didi <[email protected]>

ceph-csi-bot · 2026-09-14T08:26:49Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2026-09-14T08:26:49Z

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

ceph-csi-bot · 2026-09-14T08:26:50Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2026-09-14T08:26:50Z

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

ceph-csi-bot · 2026-09-14T08:26:50Z

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

ceph-csi-bot · 2026-09-14T08:26:50Z

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

ceph-csi-bot · 2026-09-14T08:26:50Z

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

ceph-csi-bot · 2026-09-14T08:26:50Z

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

Pull request has been modified.

gadididi · 2026-09-14T12:20:53Z

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

gadididi · 2026-09-14T13:13:26Z

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

mergify · 2026-09-15T04:18:00Z

Merge Queue Status

✅ Entered queue — 2026-09-15 04:17 UTC · Rule: default · triggered by merge protections
✅ Checks skipped · PR is already up-to-date
✅ Merged — 2026-09-15 04:18 UTC · at 2e808b77a2316227da5ea470f1fdf08db58ae88b · rebase

This pull request spent 10 seconds in the queue, including 2 seconds running CI.

Required conditions to merge

depends-on = ceph/ceph-csi#6540 [⛓️ util: make health-check error message static #6540]
- nvmeof: change the return error code #6535
github-review-approved [🛡 GitHub branch protection]
- nvmeof: change the return error code #6535

gadididi requested a review from nixpanic September 3, 2026 12:10

gadididi self-assigned this Sep 3, 2026

Copilot AI lite review requested due to automatic review settings September 3, 2026 12:10

gadididi requested a review from a team as a code owner September 3, 2026 12:10

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

gadididi requested a review from a team as a code owner September 3, 2026 12:10

gadididi removed request for a team September 3, 2026 12:10

Copilot started reviewing on behalf of gadididi September 3, 2026 12:11 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Comment thread internal/nvmeof/controller/controllerserver.go

Comment thread internal/nvmeof/controller/controllerserver.go

nixpanic previously approved these changes Sep 3, 2026

View reviewed changes

nixpanic added the cleanup label Sep 3, 2026

nixpanic requested a review from a team September 3, 2026 15:11

Madhu-1 previously approved these changes Sep 4, 2026

View reviewed changes

nixpanic mentioned this pull request Sep 8, 2026

cleanup: use read-only image opens #6536

Merged

2 of 6 tasks

ceph-csi-bot added the ok-to-test Label to trigger E2E tests label Sep 9, 2026

nvmeof: change the return error code

4eda169

fix the return error code from `createNVMeoFResources()` and `cleanupNVMeoFResources` to be more accuratly. return GRPc error code instead of they returned just general internal error. Signed-off-by: gadi-didi <[email protected]>

nixpanic force-pushed the nvmeof/fix_error_code branch from d7594ed to 4eda169 Compare September 14, 2026 08:26

nixpanic added the ok-to-test Label to trigger E2E tests label Sep 14, 2026

nixpanic approved these changes Sep 14, 2026

View reviewed changes

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 Sep 14, 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 Sep 14, 2026

gadididi requested a review from Madhu-1 September 14, 2026 14:08

Madhu-1 approved these changes Sep 15, 2026

View reviewed changes

mergify Bot merged commit 2e808b7 into ceph:devel Sep 15, 2026
40 of 41 checks passed

gadididi deleted the nvmeof/fix_error_code branch September 15, 2026 06:49

Conversation

gadididi commented Sep 3, 2026 • edited by nixpanic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Sep 8, 2026

Uh oh!

nixpanic commented Sep 8, 2026

Uh oh!

gadididi commented Sep 9, 2026

Uh oh!

mergify Bot commented Sep 9, 2026

☑️ Nothing to do, the required conditions are not met

Uh oh!

gadididi commented Sep 9, 2026

Uh oh!

ceph-csi-bot commented Sep 9, 2026

Uh oh!

ceph-csi-bot commented Sep 9, 2026

Uh oh!

ceph-csi-bot commented Sep 9, 2026

Uh oh!

ceph-csi-bot commented Sep 9, 2026

Uh oh!

ceph-csi-bot commented Sep 9, 2026

Uh oh!

ceph-csi-bot commented Sep 9, 2026

Uh oh!

ceph-csi-bot commented Sep 9, 2026

Uh oh!

nixpanic commented Sep 11, 2026

Uh oh!

gadididi commented Sep 14, 2026

Uh oh!

mergify Bot commented Sep 14, 2026

❌ This pull request comes from a fork and cannot be rebased

Uh oh!

nixpanic commented Sep 14, 2026

Uh oh!

nixpanic commented Sep 14, 2026

Uh oh!

mergify Bot commented Sep 14, 2026

❌ Pull request can't be updated with latest base branch changes

Uh oh!

gadididi commented Sep 14, 2026

Uh oh!

ceph-csi-bot commented Sep 14, 2026

Uh oh!

ceph-csi-bot commented Sep 14, 2026

Uh oh!

ceph-csi-bot commented Sep 14, 2026

Uh oh!

ceph-csi-bot commented Sep 14, 2026

Uh oh!

ceph-csi-bot commented Sep 14, 2026

Uh oh!

ceph-csi-bot commented Sep 14, 2026

Uh oh!

ceph-csi-bot commented Sep 14, 2026

Uh oh!

ceph-csi-bot commented Sep 14, 2026

Uh oh!

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

Uh oh!

Uh oh!

gadididi commented Sep 14, 2026

Uh oh!

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

Uh oh!

Merge Queue Status

Uh oh!

gadididi commented Sep 3, 2026 •

edited by nixpanic

Loading

gadididi commented Sep 14, 2026 •

edited

Loading

mergify Bot commented Sep 15, 2026 •

edited

Loading