Repository navigation
nvmeof: simplify subsystem deletion - #6533
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
DeleteSubsystem currently hides EBUSY, changing its contract for all callers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Simplifies NVMe-oF subsystem cleanup by relying on gateway deletion semantics.
Changes:
- Removes namespace/listener preflight calls.
- Treats
ENOENTandEBUSYas expected outcomes.
File summaries
| File | Review |
|---|---|
internal/nvmeof/nvmeof.go |
Must preserve EBUSY as a typed/sentinel error and handle it only in cleanup. |
internal/nvmeof/controller/controllerserver.go |
Cleanup is simplified; update the inaccurate error-handling comment. |
Review details
Suppressed comments (1)
internal/nvmeof/nvmeof.go:425
- Please add an automated unit test for the new status handling, especially that
EBUSYfollows the intended path while unrelated nonzero statuses still fail. This errno branch is the core behavior of the simplification, but the existing tests do not exerciseDeleteSubsystemresponse statuses.
case status.GetStatus() == int32(syscall.EBUSY):
log.DebugLog(ctx, "subsystem %s has namespaces and cannot be deleted: %s", subsystemNQN, status.GetErrorMessage())
return nil // Treat as success for our use case, cannot delete subsystem because it is busy
case status.GetStatus() != 0:
- Files reviewed: 2/2 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.
| case status.GetStatus() == int32(syscall.EBUSY): | ||
| log.DebugLog(ctx, "subsystem %s has namespaces and cannot be deleted: %s", subsystemNQN, status.GetErrorMessage()) | ||
|
|
||
| return nil // Treat as success for our use case, cannot delete subsystem because it is busy |
There was a problem hiding this comment.
the only one calls for this function is nvmeof-csi driver, and we want to handle as no-error the case there are more ns in this subsystem
| log.DebugLog(ctx, "Subsystem %s already deleted (not found)", subsystemNQN) | ||
|
|
||
| return nil | ||
| case status.GetStatus() == int32(syscall.EBUSY): |
There was a problem hiding this comment.
This is success becase we have a namespace in it but it will get deleted successfully when no namespace exists? can you please add more comment for this case?
There was a problem hiding this comment.
I will add.
this case when delete the subsystem failed from the GW side because there is more namespace in this subsystem (not the one we just removed in the previous call deleteNamespace, other)
I added this log.DebugLog(ctx, "subsystem %s has namespaces and cannot be deleted: %s", subsystemNQN, status.GetErrorMessage())
I will add also comment explanation
There was a problem hiding this comment.
I changed to return new error, and in the nvmeof controller I handle it if errors.Is(err, nvmeoferrors.ErrSubsystemHasNamespaces)
3d53ac6 to
e584014
Compare
| case status.GetStatus() == int32(syscall.EBUSY): | ||
| log.DebugLog(ctx, "subsystem %s has namespaces and cannot be deleted: %s", subsystemNQN, status.GetErrorMessage()) | ||
|
|
||
| return fmt.Errorf("%w: %s", nvmeoferrors.ErrSubsystemHasNamespaces, status.GetErrorMessage()) |
There was a problem hiding this comment.
IMO this is not required we should follow same pattern we are following for ENOENT,
There was a problem hiding this comment.
I think returning an error is nice. The function did not do what was requested (DeleteSubsystem()), and this gives a good way to handle the error in different ways.
|
This is a great performance enhancement, the listing of namespaces takes quite long, and is very visible when deleting a volume. Without the listing, |
|
/queue |
|
@Mergifyio rebase |
🛑 The pull request rule doesn't match anymoreDetailsThis action has been cancelled. |
e584014 to
f4ee275
Compare
|
/queue |
|
@Mergifyio rebase |
🛑 The pull request rule doesn't match anymoreDetailsThis action has been cancelled. |
instead of call 4 GRPc calls to the nvmeof gw: 1. SubsystemExists 2. ListNamespaces - this call takes really more time. 3. then it delete per listener DeleteListener 4. then finally delete the subsystem - DeleteSubsystem jsut call DeleteSubsystem, if subsystem not empty will retrun `EBUSY`. if subsystem does not exist, return `ENOENT`. the GW automatically deletes the listeners. Signed-off-by: gadi-didi <[email protected]>
736ee4f to
d919a05
Compare
ceph-csi-bot
added
ok-to-test
|
/test ci/centos/upgrade-tests-cephfs |
|
/test ci/centos/k8s-e2e-external-storage/1.36 |
|
/test ci/centos/upgrade-tests-rbd |
|
/test ci/centos/k8s-e2e-external-storage/1.35 |
|
/test ci/centos/mini-e2e-helm/k8s-1.35 |
|
/test ci/centos/mini-e2e-helm/k8s-1.36 |
|
/test ci/centos/k8s-e2e-external-storage/1.34 |
|
/test ci/centos/mini-e2e/k8s-1.35 |
|
/test ci/centos/mini-e2e/k8s-1.36 |
|
/test ci/centos/mini-e2e-helm/k8s-1.34 |
|
/test ci/centos/mini-e2e/k8s-1.34 |
ceph-csi-bot
added
the
ci/in-progress/e2e
mergify
Bot
removed
the
ci/in-progress/e2e
|
/retest ci/centos/mini-e2e/k8s-1.34 |
Merge Queue Status
This pull request spent 21 seconds in the queue, including 2 seconds running CI. Required conditions to merge
|
instead of call 4 GRPc calls to the nvmeof gw:
jsut call DeleteSubsystem, if subsystem not empty
will retrun
EBUSY.if subsystem does not exist, return
ENOENT.the GW automatically deletes the
listeners.
UPDATE:
tested manually on ODF cluster with 2 PVCs.
we handle it by
EBUSYGet CI jobs structured, wait for others to finish before starting more.
Depends-on: #6526 #6515