Merge pull request #320 from andyzhangx/CleanupMountPoint

fix: use mount.CleanupMountPoint in NodeUnpublishVolume for handling stale nfs connections during unmount process
kubernetes-csi · Apr 25, 2022 · a42b320 · a42b320
2 parents 0726684 + 38b6ba2
commit a42b320
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 17 deletions.
diff --git a/pkg/nfs/nodeserver.go b/pkg/nfs/nodeserver.go
@@ -130,24 +130,13 @@ func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpu
 	if len(targetPath) == 0 {
 		return nil, status.Error(codes.InvalidArgument, "Target path missing in request")
 	}
-	notMnt, err := ns.mounter.IsLikelyNotMountPoint(targetPath)
 
+	klog.V(2).Infof("NodeUnpublishVolume: unmounting volume %s on %s", volumeID, targetPath)
+	err := mount.CleanupMountPoint(targetPath, ns.mounter, true /*extensiveMountPointCheck*/)
 	if err != nil {
-		if os.IsNotExist(err) {
-			return nil, status.Error(codes.NotFound, "Targetpath not found")
-		}
-		return nil, status.Error(codes.Internal, err.Error())
-	}
-	if notMnt {
-		klog.V(2).Infof("NodeUnpublishVolume: Targetpath %s of volumeID(%s) is not mounted", targetPath, volumeID)
-		return &csi.NodeUnpublishVolumeResponse{}, nil
-	}
-
-	klog.V(2).Infof("NodeUnpublishVolume: CleanupMountPoint %s on volumeID(%s)", targetPath, volumeID)
-	err = mount.CleanupMountPoint(targetPath, ns.mounter, false)
-	if err != nil {
-		return nil, status.Error(codes.Internal, err.Error())
+		return nil, status.Errorf(codes.Internal, "failed to unmount target %q: %v", targetPath, err)
 	}
+	klog.V(2).Infof("NodeUnpublishVolume: unmount volume %s on %s successfully", volumeID, targetPath)
 
 	return &csi.NodeUnpublishVolumeResponse{}, nil
 }

diff --git a/pkg/nfs/nodeserver_test.go b/pkg/nfs/nodeserver_test.go
@@ -19,8 +19,10 @@ package nfs
 import (
 	"context"
 	"errors"
+	"fmt"
 	"os"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/container-storage-interface/spec/lib/go/csi"
@@ -186,7 +188,7 @@ func TestNodeUnpublishVolume(t *testing.T) {
 		{
 			desc:        "[Error] Unmount error mocked by IsLikelyNotMountPoint",
 			req:         csi.NodeUnpublishVolumeRequest{TargetPath: errorTarget, VolumeId: "vol_1"},
-			expectedErr: status.Error(codes.Internal, "fake IsLikelyNotMountPoint: fake error"),
+			expectedErr: fmt.Errorf("fake IsLikelyNotMountPoint: fake error"),
 		},
 		{
 			desc: "[Success] Volume not mounted",
@@ -203,7 +205,9 @@ func TestNodeUnpublishVolume(t *testing.T) {
 		}
 		_, err := ns.NodeUnpublishVolume(context.Background(), &tc.req)
 		if !reflect.DeepEqual(err, tc.expectedErr) {
-			t.Errorf("Desc:%v\nUnexpected error: %v\nExpected: %v", tc.desc, err, tc.expectedErr)
+			if err == nil || tc.expectedErr == nil || !strings.Contains(err.Error(), tc.expectedErr.Error()) {
+				t.Errorf("Desc:%v\nUnexpected error: %v\nExpected: %v", tc.desc, err, tc.expectedErr)
+			}
 		}
 		if tc.cleanup != nil {
 			tc.cleanup()