nvmeof: change the return error code - #6535
Conversation
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 <gadi.didi@ibm.com>
There was a problem hiding this comment.
🟡 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
CreateVolumeandDeleteVolumedirectly to preserve their gRPC codes. - Map parameter/validation failures to
InvalidArgument, lock/contention cases toAborted, and operational failures toInternalinside 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.
|
This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏 |
|
@gadididi , this now has a conflict, can you please address it? |
Signed-off-by: Gadi <58472718+gadididi@users.noreply.github.com>
Pull request has been modified.
|
@mergify rebase |
☑️ Nothing to do, the required conditions are not metDetails
|
|
/queue |
|
/test ci/centos/k8s-e2e-external-storage/1.35 |
|
/test ci/centos/k8s-e2e-external-storage/1.34 |
|
/test ci/centos/mini-e2e-helm/k8s-1.35 |
|
/test ci/centos/upgrade-tests-cephfs |
|
/test ci/centos/mini-e2e-helm/k8s-1.34 |
|
/test ci/centos/mini-e2e/k8s-1.35 |
|
/test ci/centos/upgrade-tests-rbd |
|
/test ci/centos/k8s-e2e-external-storage/1.36 |
|
/test ci/centos/mini-e2e/k8s-1.34 |
|
/test ci/centos/mini-e2e-helm/k8s-1.36 |
|
/test ci/centos/mini-e2e/k8s-1.36 |
ceph-csi-bot
added
ci/in-progress/e2e
mergify
Bot
removed
the
ci/in-progress/e2e
|
/retest ci/centos/k8s-e2e-external-storage/1.34 |
|
@nixpanic ,I addressed the conflict, need to reapprove, thanks! |
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:
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!)