From 1a63f5b9088a54fa88b1fcef23076bdcb622eaef Mon Sep 17 00:00:00 2001
From: "cheolho.kang" <cheolho.kang@samsung.com>
Date: Thu, 20 Mar 2025 17:05:26 +0900
Subject: [PATCH] feat: implement `NodeUnstageVolume()` and
 `NodeUnpublishVolume()`

- Implement `NodeUnstageVolume()` to handle device dettachment from the node
- Move device dettachment logic from `NodeUnpublishVolume()` to `NodeUnstageVolume()`
- Refactor `NodeUnpublishVolume()` to only handle unmounting from target path

Signed-off-by: cheolho.kang <cheolho.kang@samsung.com>
---
 pkg/nvmf/nodeserver.go | 45 +++++++++++++++++++++++++++++++++++-------
 pkg/nvmf/nvmf.go       | 16 +++------------
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/pkg/nvmf/nodeserver.go b/pkg/nvmf/nodeserver.go
index c95a253..bea95e1 100644
--- a/pkg/nvmf/nodeserver.go
+++ b/pkg/nvmf/nodeserver.go
@@ -106,20 +106,25 @@ func (n *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublish
 }
 
 func (n *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpublishVolumeRequest) (*csi.NodeUnpublishVolumeResponse, error) {
-	klog.Infof("NodeUnpublishVolume: Starting unpublish volume, %s, %v", req.VolumeId, req)
-
 	if req.VolumeId == "" {
 		return nil, status.Error(codes.InvalidArgument, "NodeUnpublishVolume VolumeID must be provided")
 	}
 	if req.TargetPath == "" {
 		return nil, status.Error(codes.InvalidArgument, "NodeUnpublishVolume Staging TargetPath must be provided")
 	}
+
+	// Acquire lock to prevent concurrent operations on this server
+	n.mtx.Lock()
+	defer n.mtx.Unlock()
+
+	klog.V(4).Infof("NodeUnpublishVolume called for volume %s", req.VolumeId)
+
+	// Unmount the volume
 	targetPath := req.GetTargetPath()
 	unmounter := getNVMfDiskUnMounter()
-	err := DetachDisk(req.VolumeId, unmounter, targetPath)
+	err := UnmountVolume(targetPath, unmounter)
 	if err != nil {
-		klog.Errorf("NodeUnpublishVolume: VolumeID: %s detachDisk err: %v", req.VolumeId, err)
-		return nil, err
+		return nil, status.Errorf(codes.Unavailable, "NodeUnpublishVolume: failed to unmount volume. VolumeID: %s detachDisk err: %v", req.VolumeId, err)
 	}
 
 	return &csi.NodeUnpublishVolumeResponse{}, nil
@@ -199,15 +204,41 @@ func (n *NodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolu
 // NodeUnstageVolume detaches the NVMe device from the node
 func (n *NodeServer) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstageVolumeRequest) (*csi.NodeUnstageVolumeResponse, error) {
 	// Validate parameters
-	if req.VolumeId == "" {
+	volumeID := req.GetVolumeId()
+	if volumeID == "" {
 		return nil, status.Error(codes.InvalidArgument, "NodeUnstageVolume Volume ID must be provided")
 	}
-	if req.StagingTargetPath == "" {
+	if req.GetStagingTargetPath() == "" {
 		return nil, status.Error(codes.InvalidArgument, "NodeUnstageVolume Staging target path must be provided")
 	}
 
+	// Acquire lock to prevent concurrent operations on this server
+	n.mtx.Lock()
+	defer n.mtx.Unlock()
+
 	klog.V(4).Infof("NodeUnstageVolume called for volume %s", req.VolumeId)
 
+	// Unmount the volume
+	// Staging path is appended with volumeID.
+	// This was defined in NodeStageVolume to avoid conflicts.
+	stagingPath := req.GetStagingTargetPath() + "/" + volumeID
+	unmounter := getNVMfDiskUnMounter()
+	err := UnmountVolume(stagingPath, unmounter)
+	if err != nil {
+		klog.Errorf("NodeUnstageVolume: failed to unmount volume %s: %v", volumeID, err)
+		return nil, status.Errorf(codes.Internal, "failed to unmount volume: %v", err)
+	}
+
+	// Detach the volume
+	err = DetachDisk(volumeID, stagingPath)
+	if err != nil {
+		klog.Errorf("NodeUnstageVolume: failed to detach volume %s: %v", volumeID, err)
+		return nil, status.Errorf(codes.Internal, "failed to detach volume: %v", err)
+	}
+
+	// Remove the connector file
+	removeConnectorFile(stagingPath)
+
 	return &csi.NodeUnstageVolumeResponse{}, nil
 }
 
diff --git a/pkg/nvmf/nvmf.go b/pkg/nvmf/nvmf.go
index 550d404..8b935f4 100644
--- a/pkg/nvmf/nvmf.go
+++ b/pkg/nvmf/nvmf.go
@@ -129,18 +129,8 @@ func AttachDisk(volumeID string, connector *Connector) (string, error) {
 	return devicePath, nil
 }
 
-func DetachDisk(volumeID string, num *nvmfDiskUnMounter, targetPath string) error {
-	if pathExists, pathErr := mount.PathExists(targetPath); pathErr != nil {
-		return fmt.Errorf("Error checking if path exists: %v", pathErr)
-	} else if !pathExists {
-		klog.Warningf("Warning: Unmount skipped because path does not exist: %v", targetPath)
-		return nil
-	}
-	if err := num.mounter.Unmount(targetPath); err != nil {
-		klog.Errorf("iscsi detach disk: failed to unmount: %s\nError: %v", targetPath, err)
-		return err
-	}
-
+// DetachDisk disconnects an NVMe-oF disk
+func DetachDisk(volumeID string, targetPath string) error {
 	connector, err := GetConnectorFromFile(targetPath + ".json")
 	if err != nil {
 		klog.Errorf("DetachDisk: failed to get connector from path %s Error: %v", targetPath, err)
@@ -151,7 +141,7 @@ func DetachDisk(volumeID string, num *nvmfDiskUnMounter, targetPath string) erro
 		klog.Errorf("DetachDisk: VolumeID: %s failed to disconnect, Error: %v", volumeID, err)
 		return err
 	}
-	removeConnectorFile(targetPath)
+
 	return nil
 }