From 98f2212197f3067de8d19c7d1baae8880fc6cdd9 Mon Sep 17 00:00:00 2001 From: Eddie Torres Date: Fri, 25 Mar 2022 16:30:27 +0000 Subject: [PATCH] Platform agnostic device removal Signed-off-by: Eddie Torres --- pkg/driver/mock_mount.go | 28 ++++++++++++++++ pkg/driver/mount.go | 2 ++ pkg/driver/mount_linux.go | 11 ++++++- pkg/driver/mount_windows.go | 36 ++++++++++++++++++++ pkg/driver/node.go | 6 ++-- pkg/driver/node_test.go | 14 ++++---- pkg/driver/sanity_test.go | 8 +++++ pkg/mounter/README | 3 -- pkg/mounter/safe_mounter_windows.go | 51 ++++++++++++++++++++++++----- 9 files changed, 137 insertions(+), 22 deletions(-) delete mode 100644 pkg/mounter/README diff --git a/pkg/driver/mock_mount.go b/pkg/driver/mock_mount.go index d90ebec107..e86cf46474 100644 --- a/pkg/driver/mock_mount.go +++ b/pkg/driver/mock_mount.go @@ -238,6 +238,34 @@ func (mr *MockMounterMockRecorder) Unmount(target interface{}) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Unmount", reflect.TypeOf((*MockMounter)(nil).Unmount), target) } +// Unpublish mocks base method. +func (m *MockMounter) Unpublish(path string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Unpublish", path) + ret0, _ := ret[0].(error) + return ret0 +} + +// Unpublish indicates an expected call of Unpublish. +func (mr *MockMounterMockRecorder) Unpublish(path interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Unpublish", reflect.TypeOf((*MockMounter)(nil).Unpublish), path) +} + +// Unstage mocks base method. +func (m *MockMounter) Unstage(path string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Unstage", path) + ret0, _ := ret[0].(error) + return ret0 +} + +// Unstage indicates an expected call of Unstage. +func (mr *MockMounterMockRecorder) Unstage(path interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Unstage", reflect.TypeOf((*MockMounter)(nil).Unstage), path) +} + // MockDeviceIdentifier is a mock of DeviceIdentifier interface. type MockDeviceIdentifier struct { ctrl *gomock.Controller diff --git a/pkg/driver/mount.go b/pkg/driver/mount.go index 9a22dbf599..62ef2716ad 100644 --- a/pkg/driver/mount.go +++ b/pkg/driver/mount.go @@ -41,6 +41,8 @@ type Mounter interface { MakeDir(path string) error PathExists(path string) (bool, error) NeedResize(devicePath string, deviceMountPath string) (bool, error) + Unpublish(path string) error + Unstage(path string) error } // NodeMounter implements Mounter. diff --git a/pkg/driver/mount_linux.go b/pkg/driver/mount_linux.go index 64467a4b0d..7ea6f66d81 100644 --- a/pkg/driver/mount_linux.go +++ b/pkg/driver/mount_linux.go @@ -21,11 +21,12 @@ package driver import ( "fmt" - "k8s.io/klog" "os" "strconv" "strings" + "k8s.io/klog" + mountutils "k8s.io/mount-utils" ) @@ -188,3 +189,11 @@ func (m *NodeMounter) parseFsInfoOutput(cmdOutput string, spliter string, blockS } return blockSize, blockCount, err } + +func (m *NodeMounter) Unpublish(path string) error { + return m.Unmount(path) +} + +func (m *NodeMounter) Unstage(path string) error { + return m.Unmount(path) +} diff --git a/pkg/driver/mount_windows.go b/pkg/driver/mount_windows.go index 024682a149..8e66a2be1e 100644 --- a/pkg/driver/mount_windows.go +++ b/pkg/driver/mount_windows.go @@ -104,3 +104,39 @@ func (m *NodeMounter) NeedResize(devicePath string, deviceMountPath string) (boo // Implement it to respect spec v1.4.0 https://github.com/container-storage-interface/spec/pull/452 return false, nil } + +// Unmount volume from target path +func (m *NodeMounter) Unpublish(target string) error { + proxyMounter, ok := m.SafeFormatAndMount.Interface.(*mounter.CSIProxyMounter) + if !ok { + return fmt.Errorf("failed to cast mounter to csi proxy mounter") + } + // WriteVolumeCache before unmount + proxyMounter.WriteVolumeCache(target) + // Remove symlink + err := proxyMounter.Rmdir(target) + if err != nil { + return err + } + return nil +} + +// Unmount volume from staging path +// usually this staging path is a global directory on the node +func (m *NodeMounter) Unstage(target string) error { + proxyMounter, ok := m.SafeFormatAndMount.Interface.(*mounter.CSIProxyMounter) + if !ok { + return fmt.Errorf("failed to cast mounter to csi proxy mounter") + } + // Unmounts and offlines the disk via the CSI Proxy API + err := proxyMounter.Unmount(target) + if err != nil { + return err + } + // Cleanup stage path + err = proxyMounter.Rmdir(target) + if err != nil { + return err + } + return nil +} \ No newline at end of file diff --git a/pkg/driver/node.go b/pkg/driver/node.go index 58141a7a12..4d95d40f03 100644 --- a/pkg/driver/node.go +++ b/pkg/driver/node.go @@ -269,7 +269,7 @@ func (d *nodeService) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstag } klog.V(4).Infof("NodeUnstageVolume: unmounting %s", target) - err = d.mounter.Unmount(target) + err = d.mounter.Unstage(target) if err != nil { return nil, status.Errorf(codes.Internal, "Could not unmount target %q: %v", target, err) } @@ -416,7 +416,7 @@ func (d *nodeService) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpu }() klog.V(4).Infof("NodeUnpublishVolume: unmounting %s", target) - err := d.mounter.Unmount(target) + err := d.mounter.Unpublish(target) if err != nil { return nil, status.Errorf(codes.Internal, "Could not unmount %q: %v", target, err) } @@ -620,7 +620,7 @@ func (d *nodeService) isMounted(source string, target string) (bool, error) { _, pathErr := d.mounter.PathExists(target) if pathErr != nil && d.mounter.IsCorruptedMnt(pathErr) { klog.V(4).Infof("NodePublishVolume: Target path %q is a corrupted mount. Trying to unmount.", target) - if mntErr := d.mounter.Unmount(target); mntErr != nil { + if mntErr := d.mounter.Unpublish(target); mntErr != nil { return false, status.Errorf(codes.Internal, "Unable to unmount the target %q : %v", target, mntErr) } //After successful unmount, the device is ready to be mounted. diff --git a/pkg/driver/node_test.go b/pkg/driver/node_test.go index 4ed6a6e425..d093fd6b42 100644 --- a/pkg/driver/node_test.go +++ b/pkg/driver/node_test.go @@ -408,7 +408,7 @@ func TestNodeUnstageVolume(t *testing.T) { } mockMounter.EXPECT().GetDeviceNameFromMount(gomock.Eq(targetPath)).Return(devicePath, 1, nil) - mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(nil) + mockMounter.EXPECT().Unstage(gomock.Eq(targetPath)).Return(nil) req := &csi.NodeUnstageVolumeRequest{ StagingTargetPath: targetPath, @@ -468,7 +468,7 @@ func TestNodeUnstageVolume(t *testing.T) { } mockMounter.EXPECT().GetDeviceNameFromMount(gomock.Eq(targetPath)).Return(devicePath, 2, nil) - mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(nil) + mockMounter.EXPECT().Unstage(gomock.Eq(targetPath)).Return(nil) req := &csi.NodeUnstageVolumeRequest{ StagingTargetPath: targetPath, @@ -737,7 +737,7 @@ func TestNodePublishVolume(t *testing.T) { mockMounter.EXPECT().MakeDir(gomock.Eq(targetPath)).Return(nil) mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, errors.New("internal system error")) - mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(nil) + mockMounter.EXPECT().Unpublish(gomock.Eq(targetPath)).Return(nil) mockMounter.EXPECT().Mount(gomock.Eq(stagingTargetPath), gomock.Eq(targetPath), gomock.Eq(defaultFsType), gomock.Eq([]string{"bind"})).Return(nil) req := &csi.NodePublishVolumeRequest{ @@ -1052,7 +1052,7 @@ func TestNodePublishVolume(t *testing.T) { mockMounter.EXPECT().MakeDir(gomock.Eq("/test")).Return(nil) mockMounter.EXPECT().MakeFile(targetPath).Return(nil) - mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(nil) + mockMounter.EXPECT().Unpublish(gomock.Eq(targetPath)).Return(nil) mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, errors.New("Internal System Error")) mockMounter.EXPECT().Mount(gomock.Eq(devicePath), gomock.Eq(targetPath), gomock.Any(), gomock.Any()).Return(nil) @@ -1598,7 +1598,7 @@ func TestNodeUnpublishVolume(t *testing.T) { VolumeId: volumeID, } - mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(nil) + mockMounter.EXPECT().Unpublish(gomock.Eq(targetPath)).Return(nil) _, err := awsDriver.NodeUnpublishVolume(context.TODO(), req) if err != nil { t.Fatalf("Expect no error but got: %v", err) @@ -1656,7 +1656,7 @@ func TestNodeUnpublishVolume(t *testing.T) { }, }, { - name: "fail error on unmount", + name: "fail error on unpublish", testFunc: func(t *testing.T) { mockCtl := gomock.NewController(t) defer mockCtl.Finish() @@ -1677,7 +1677,7 @@ func TestNodeUnpublishVolume(t *testing.T) { VolumeId: volumeID, } - mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(errors.New("test Unmount error")) + mockMounter.EXPECT().Unpublish(gomock.Eq(targetPath)).Return(errors.New("test Unpublish error")) _, err := awsDriver.NodeUnpublishVolume(context.TODO(), req) expectErr(t, err, codes.Internal) }, diff --git a/pkg/driver/sanity_test.go b/pkg/driver/sanity_test.go index ce661d7b24..4d5c773646 100644 --- a/pkg/driver/sanity_test.go +++ b/pkg/driver/sanity_test.go @@ -322,6 +322,14 @@ func (f *fakeMounter) Unmount(target string) error { return nil } +func (f *fakeMounter) Unstage(target string) error { + return nil +} + +func (f *fakeMounter) Unpublish(target string) error { + return nil +} + func (f *fakeMounter) List() ([]mount.MountPoint, error) { return []mount.MountPoint{}, nil } diff --git a/pkg/mounter/README b/pkg/mounter/README deleted file mode 100644 index 45e6bd9f23..0000000000 --- a/pkg/mounter/README +++ /dev/null @@ -1,3 +0,0 @@ -This CSIProxyMounter code is copied from https://github.com/kubernetes-sigs/azuredisk-csi-driver/tree/18cf57775b5dcb4eac435feac5fbe13bb06216d1/pkg/mounter. - -TODO consume this as a standalone module instead. This can't be included in the k8s.io/utils module because, by depending on kubernetes-csi/csi-proxy, it violates the criterion "No dependencies on any other Kubernetes repository." diff --git a/pkg/mounter/safe_mounter_windows.go b/pkg/mounter/safe_mounter_windows.go index ab33df1794..e569716be3 100644 --- a/pkg/mounter/safe_mounter_windows.go +++ b/pkg/mounter/safe_mounter_windows.go @@ -71,10 +71,47 @@ func (mounter *CSIProxyMounter) Mount(source string, target string, fstype strin return nil } +func (mounter *CSIProxyMounter) Unmount(target string) error { + // Find the volume id + getVolumeIdRequest := &volume.GetVolumeIDFromTargetPathRequest{ + TargetPath: normalizeWindowsPath(target), + } + volumeIdResponse, err := mounter.VolumeClient.GetVolumeIDFromTargetPath(context.Background(), getVolumeIdRequest) + volumeId := volumeIdResponse.GetVolumeId() + + // Call UnmountVolume CSI proxy function which flushes data cache to disk and removes the global staging path + unmountVolumeRequest := &volume.UnmountVolumeRequest{ + VolumeId: volumeId, + TargetPath: normalizeWindowsPath(target), + } + _, err = mounter.VolumeClient.UnmountVolume(context.Background(), unmountVolumeRequest) + if err != nil { + return err + } + + // Get disk number + getDiskNumberRequest := &volume.GetDiskNumberFromVolumeIDRequest{ + VolumeId: volumeId, + } + getDiskNumberResponse, err := mounter.VolumeClient.GetDiskNumberFromVolumeID(context.Background(), getDiskNumberRequest) + diskNumber := getDiskNumberResponse.GetDiskNumber() + if err != nil { + return err + } + + // Offline the disk + setDiskStateRequest := &disk.SetDiskStateRequest{ + DiskNumber: diskNumber, + IsOnline: false, + } + _, err = mounter.DiskClient.SetDiskState(context.Background(), setDiskStateRequest) + if err != nil { + return err + } + return nil +} + // Rmdir - delete the given directory -// TODO: Call separate rmdir for pod context and plugin context. v1alpha1 for CSI -// proxy does a relaxed check for prefix as c:\var\lib\kubelet, so we can do -// rmdir with either pod or plugin context. func (mounter *CSIProxyMounter) Rmdir(path string) error { rmdirRequest := &fs.RmdirRequest{ Path: normalizeWindowsPath(path), @@ -87,10 +124,9 @@ func (mounter *CSIProxyMounter) Rmdir(path string) error { return nil } -// Unmount - Removes the directory - equivalent to unmount on Linux. -func (mounter *CSIProxyMounter) Unmount(target string) error { - // WriteVolumeCache before unmount - response, err := mounter.VolumeClient.GetVolumeIDFromTargetPath(context.Background(), &volume.GetVolumeIDFromTargetPathRequest{TargetPath: target}) +func (mounter *CSIProxyMounter) WriteVolumeCache(target string) { + request := &volume.GetVolumeIDFromTargetPathRequest{TargetPath: normalizeWindowsPath(target)} + response, err := mounter.VolumeClient.GetVolumeIDFromTargetPath(context.Background(), request) if err != nil || response == nil { klog.Warningf("GetVolumeIDFromTargetPath(%s) failed with error: %v, response: %v", target, err, response) } else { @@ -101,7 +137,6 @@ func (mounter *CSIProxyMounter) Unmount(target string) error { klog.Warningf("WriteVolumeCache(%s) failed with error: %v, response: %v", response.VolumeId, err, res) } } - return mounter.Rmdir(target) } func (mounter *CSIProxyMounter) List() ([]mount.MountPoint, error) {