Merge pull request #106 from leakingtapan/unpublish

Fix bug when unpublishing already unmounted file system

go.mod

@@ -62,3 +62,5 @@ replace k8s.io/sample-apiserver => k8s.io/sample-apiserver v0.0.0-20191003001538
 replace k8s.io/sample-cli-plugin => k8s.io/sample-cli-plugin v0.0.0-20191003002540-40951731b79f
 
 replace k8s.io/sample-controller => k8s.io/sample-controller v0.0.0-20191003001734-27680fba8268
+
+go 1.13

pkg/driver/mounter.go

@@ -23,6 +23,7 @@ import (
 type Mounter interface {
 	mount.Interface
 	MakeDir(pathname string) error
+	GetDeviceName(mountPath string) (string, int, error)
 }
 
 type NodeMounter struct {
@@ -44,3 +45,7 @@ func (m *NodeMounter) MakeDir(pathname string) error {
 	}
 	return nil
 }
+
+func (m *NodeMounter) GetDeviceName(mountPath string) (string, int, error) {
+	return mount.GetDeviceNameFromMount(m, mountPath)
+}

pkg/driver/node.go

@@ -138,8 +138,25 @@ func (d *Driver) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpublish
 		return nil, status.Error(codes.InvalidArgument, "Target path not provided")
 	}
 
+	// Check if target directory is a mount point. GetDeviceNameFromMount
+	// given a mnt point, finds the device from /proc/mounts
+	// returns the device name, reference count, and error code
+	_, refCount, err := d.mounter.GetDeviceName(target)
+	if err != nil {
+		msg := fmt.Sprintf("failed to check if volume is mounted: %v", err)
+		return nil, status.Error(codes.Internal, msg)
+	}
+
+	// From the spec: If the volume corresponding to the volume_id
+	// is not staged to the staging_target_path, the Plugin MUST
+	// reply 0 OK.
+	if refCount == 0 {
+		klog.V(5).Infof("NodeUnpublishVolume: %s target not mounted", target)
+		return &csi.NodeUnpublishVolumeResponse{}, nil
+	}
+
 	klog.V(5).Infof("NodeUnpublishVolume: unmounting %s", target)
-	err := d.mounter.Unmount(target)
+	err = d.mounter.Unmount(target)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "Could not unmount %q: %v", target, err)
 	}

pkg/driver/node_test.go

@@ -463,6 +463,33 @@ func TestNodeUnpublishVolume(t *testing.T) {
 					TargetPath: targetPath,
 				}
 
+				mockMounter.EXPECT().GetDeviceName(gomock.Eq(targetPath)).Return("", 1, nil)
+				mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(nil)
+
+				_, err := driver.NodeUnpublishVolume(ctx, req)
+				if err != nil {
+					t.Fatalf("NodeUnpublishVolume is failed: %v", err)
+				}
+			},
+		},
+		{
+			name: "success: unpublish with already unmounted target",
+			testFunc: func(t *testing.T) {
+				mockCtrl := gomock.NewController(t)
+				mockMounter := mocks.NewMockMounter(mockCtrl)
+				driver := &Driver{
+					endpoint: endpoint,
+					nodeID:   nodeID,
+					mounter:  mockMounter,
+				}
+
+				ctx := context.Background()
+				req := &csi.NodeUnpublishVolumeRequest{
+					VolumeId:   volumeId,
+					TargetPath: targetPath,
+				}
+
+				mockMounter.EXPECT().GetDeviceName(gomock.Eq(targetPath)).Return("", 0, nil)
 				mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(nil)
 
 				_, err := driver.NodeUnpublishVolume(ctx, req)
@@ -489,7 +516,7 @@ func TestNodeUnpublishVolume(t *testing.T) {
 
 				_, err := driver.NodeUnpublishVolume(ctx, req)
 				if err == nil {
-					t.Fatalf("NodeUnpublishVolume is not failed: %v", err)
+					t.Fatalf("NodeUnpublishVolume is not failed")
 				}
 			},
 		},
@@ -511,11 +538,39 @@ func TestNodeUnpublishVolume(t *testing.T) {
 				}
 
 				mountErr := fmt.Errorf("Unmount failed")
+				mockMounter.EXPECT().GetDeviceName(gomock.Eq(targetPath)).Return("", 1, nil)
 				mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(mountErr)
 
 				_, err := driver.NodeUnpublishVolume(ctx, req)
 				if err == nil {
-					t.Fatalf("NodeUnpublishVolume is not failed: %v", err)
+					t.Fatalf("NodeUnpublishVolume is not failed")
+				}
+			},
+		},
+		{
+			name: "fail: mounter failed to GetDeviceName",
+			testFunc: func(t *testing.T) {
+				mockCtrl := gomock.NewController(t)
+				mockMounter := mocks.NewMockMounter(mockCtrl)
+				driver := &Driver{
+					endpoint: endpoint,
+					nodeID:   nodeID,
+					mounter:  mockMounter,
+				}
+
+				ctx := context.Background()
+				req := &csi.NodeUnpublishVolumeRequest{
+					VolumeId:   volumeId,
+					TargetPath: targetPath,
+				}
+
+				mounterErr := fmt.Errorf("Unmount failed")
+				mockMounter.EXPECT().GetDeviceName(gomock.Eq(targetPath)).Return("", 1, mounterErr)
+				mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(nil)
+
+				_, err := driver.NodeUnpublishVolume(ctx, req)
+				if err == nil {
+					t.Fatalf("NodeUnpublishVolume is not failed")
 				}
 			},
 		},