diff --git a/pkg/cloud/aws_metrics.go b/pkg/cloud/aws_metrics.go index 827f1d2298..5f48369abd 100644 --- a/pkg/cloud/aws_metrics.go +++ b/pkg/cloud/aws_metrics.go @@ -1,3 +1,4 @@ +//go:build !providerless // +build !providerless /* diff --git a/pkg/driver/mocks/mock_mount.go b/pkg/driver/mocks/mock_mount.go index eeb81d98c9..138d84f10a 100644 --- a/pkg/driver/mocks/mock_mount.go +++ b/pkg/driver/mocks/mock_mount.go @@ -104,6 +104,20 @@ func (mr *MockMounterMockRecorder) GetDeviceNameFromMount(mountPath interface{}) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetDeviceNameFromMount", reflect.TypeOf((*MockMounter)(nil).GetDeviceNameFromMount), mountPath) } +// IsCorruptedMnt mocks base method. +func (m *MockMounter) IsCorruptedMnt(err error) bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsCorruptedMnt", err) + ret0, _ := ret[0].(bool) + return ret0 +} + +// IsCorruptedMnt indicates an expected call of IsCorruptedMnt. +func (mr *MockMounterMockRecorder) IsCorruptedMnt(err interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsCorruptedMnt", reflect.TypeOf((*MockMounter)(nil).IsCorruptedMnt), err) +} + // GetMountRefs mocks base method. func (m *MockMounter) GetMountRefs(pathname string) ([]string, error) { m.ctrl.T.Helper() diff --git a/pkg/driver/mount.go b/pkg/driver/mount.go index 0e3e938a0b..83eb433918 100644 --- a/pkg/driver/mount.go +++ b/pkg/driver/mount.go @@ -32,7 +32,7 @@ type Mounter interface { mountInterface FormatAndMount(source string, target string, fstype string, options []string) error - + IsCorruptedMnt(err error) bool GetDeviceNameFromMount(mountPath string) (string, int, error) MakeFile(path string) error MakeDir(path string) error diff --git a/pkg/driver/mount_linux.go b/pkg/driver/mount_linux.go index ea4427c549..64467a4b0d 100644 --- a/pkg/driver/mount_linux.go +++ b/pkg/driver/mount_linux.go @@ -1,3 +1,4 @@ +//go:build linux // +build linux /* @@ -33,6 +34,11 @@ func (m NodeMounter) GetDeviceNameFromMount(mountPath string) (string, int, erro return mountutils.GetDeviceNameFromMount(m, mountPath) } +// IsCorruptedMnt return true if err is about corrupted mount point +func (m NodeMounter) IsCorruptedMnt(err error) bool { + return mountutils.IsCorruptedMnt(err) +} + // This function is mirrored in ./sanity_test.go to make sure sanity test covered this block of code // Please mirror the change to func MakeFile in ./sanity_test.go func (m *NodeMounter) MakeFile(path string) error { diff --git a/pkg/driver/mount_windows.go b/pkg/driver/mount_windows.go index 916672a136..3b47360b91 100644 --- a/pkg/driver/mount_windows.go +++ b/pkg/driver/mount_windows.go @@ -1,3 +1,4 @@ +//go:build windows // +build windows /* diff --git a/pkg/driver/node.go b/pkg/driver/node.go index 641e16a732..95ad9ddc8e 100644 --- a/pkg/driver/node.go +++ b/pkg/driver/node.go @@ -586,17 +586,57 @@ func (d *nodeService) nodePublishVolumeForBlock(req *csi.NodePublishVolumeReques return status.Errorf(codes.Internal, "Could not create file %q: %v", target, err) } - klog.V(4).Infof("NodePublishVolume [block]: mounting %s at %s", source, target) - if err := d.mounter.Mount(source, target, "", mountOptions); err != nil { - if removeErr := os.Remove(target); removeErr != nil { - return status.Errorf(codes.Internal, "Could not remove mount target %q: %v", target, removeErr) - } + //Checking if the target file is already mounted with a device. + mounted, err := d.isMounted(source, target) + if err != nil { return status.Errorf(codes.Internal, "Could not mount %q at %q: %v", source, target, err) } + if !mounted { + klog.V(4).Infof("NodePublishVolume [block]: mounting %s at %s", source, target) + if err := d.mounter.Mount(source, target, "", mountOptions); err != nil { + if removeErr := os.Remove(target); removeErr != nil { + return status.Errorf(codes.Internal, "Could not remove mount target %q: %v", target, removeErr) + } + return status.Errorf(codes.Internal, "Could not mount %q at %q: %v", source, target, err) + } + } else { + klog.V(4).Infof("NodePublishVolume [block]: Target path %q is already mounted", target) + } + return nil } +func (d *nodeService) isMounted(source string, target string) (bool, error) { + /* + Checking if it's a mount point using IsLikelyNotMountPoint. There are three different return values, + 1. true, err when the directory does not exist or corrupted. + 2. false, nil when the path is already mounted with a device. + 3. true, nil when the path is not mounted with any device. + */ + notMnt, err := d.mounter.IsLikelyNotMountPoint(target) + if err != nil && !os.IsNotExist(err) { + //Checking if the path exists and error is related to Corrupted Mount, in that case, the system could unmount and mount. + _, 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 { + return !notMnt, status.Errorf(codes.Internal, "Unable to unmount the target %q : %v", target, mntErr) + } + //After successful unmount, the device is ready to be mounted. + return !notMnt, nil + } + return !notMnt, status.Errorf(codes.Internal, "Could not mount %q at %q: %v", source, target, err) + } + + if !notMnt { + klog.V(4).Infof("NodePublishVolume: Target path %q is already mounted", target) + return !notMnt, nil + } + + return !notMnt, err +} + func (d *nodeService) nodePublishVolumeForFileSystem(req *csi.NodePublishVolumeRequest, mountOptions []string, mode *csi.VolumeCapability_Mount) error { target := req.GetTargetPath() source := req.GetStagingTargetPath() @@ -620,13 +660,20 @@ func (d *nodeService) nodePublishVolumeForFileSystem(req *csi.NodePublishVolumeR mountOptions = collectMountOptions(fsType, mountOptions) klog.V(4).Infof("NodePublishVolume: mounting %s at %s with option %s as fstype %s", source, target, mountOptions, fsType) - if err := d.mounter.Mount(source, target, fsType, mountOptions); err != nil { - if removeErr := os.Remove(target); removeErr != nil { - return status.Errorf(codes.Internal, "Could not remove mount target %q: %v", target, err) - } + + //Checking if the target directory is already mounted with a device. + mounted, err := d.isMounted(source, target) + + if err != nil { return status.Errorf(codes.Internal, "Could not mount %q at %q: %v", source, target, err) } + if !mounted { + if err := d.mounter.Mount(source, target, fsType, mountOptions); err != nil { + return status.Errorf(codes.Internal, "Could not mount %q at %q: %v", source, target, err) + } + } + return nil } diff --git a/pkg/driver/node_linux.go b/pkg/driver/node_linux.go index a6efa9b305..7ad539b541 100644 --- a/pkg/driver/node_linux.go +++ b/pkg/driver/node_linux.go @@ -1,3 +1,4 @@ +//go:build linux // +build linux /* diff --git a/pkg/driver/node_test.go b/pkg/driver/node_test.go index c3019b3e34..1b735fb1d6 100644 --- a/pkg/driver/node_test.go +++ b/pkg/driver/node_test.go @@ -597,6 +597,109 @@ func TestNodePublishVolume(t *testing.T) { mockMounter.EXPECT().MakeDir(gomock.Eq(targetPath)).Return(nil) mockMounter.EXPECT().Mount(gomock.Eq(stagingTargetPath), gomock.Eq(targetPath), gomock.Eq(defaultFsType), gomock.Eq([]string{"bind"})).Return(nil) + mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, nil) + + req := &csi.NodePublishVolumeRequest{ + PublishContext: map[string]string{DevicePathKey: devicePath}, + StagingTargetPath: stagingTargetPath, + TargetPath: targetPath, + VolumeCapability: stdVolCap, + VolumeId: "vol-test", + } + + _, err := awsDriver.NodePublishVolume(context.TODO(), req) + if err != nil { + t.Fatalf("Expect no error but got: %v", err) + } + }, + }, + { + name: "success filesystem mounted already", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + defer mockCtl.Finish() + + mockMetadata := mocks.NewMockMetadataService(mockCtl) + mockMounter := mocks.NewMockMounter(mockCtl) + + awsDriver := &nodeService{ + metadata: mockMetadata, + mounter: mockMounter, + inFlight: internal.NewInFlight(), + } + + mockMounter.EXPECT().MakeDir(gomock.Eq(targetPath)).Return(nil) + mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(false, nil) + + req := &csi.NodePublishVolumeRequest{ + PublishContext: map[string]string{DevicePathKey: devicePath}, + StagingTargetPath: stagingTargetPath, + TargetPath: targetPath, + VolumeCapability: stdVolCap, + VolumeId: "vol-test", + } + + _, err := awsDriver.NodePublishVolume(context.TODO(), req) + if err != nil { + t.Fatalf("Expect no error but got: %v", err) + } + }, + }, + { + name: "success filesystem mountpoint error", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + defer mockCtl.Finish() + + mockMetadata := mocks.NewMockMetadataService(mockCtl) + mockMounter := mocks.NewMockMounter(mockCtl) + + awsDriver := &nodeService{ + metadata: mockMetadata, + mounter: mockMounter, + inFlight: internal.NewInFlight(), + } + + gomock.InOrder( + mockMounter.EXPECT().PathExists(gomock.Eq(targetPath)).Return(true, nil), + ) + mockMounter.EXPECT().MakeDir(gomock.Eq(targetPath)).Return(nil) + mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, errors.New("Internal system error")) + + req := &csi.NodePublishVolumeRequest{ + PublishContext: map[string]string{DevicePathKey: devicePath}, + StagingTargetPath: stagingTargetPath, + TargetPath: targetPath, + VolumeCapability: stdVolCap, + VolumeId: "vol-test", + } + + _, err := awsDriver.NodePublishVolume(context.TODO(), req) + expectErr(t, err, codes.Internal) + }, + }, + { + name: "success filesystem corrupted mountpoint error", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + defer mockCtl.Finish() + + mockMetadata := mocks.NewMockMetadataService(mockCtl) + mockMounter := mocks.NewMockMounter(mockCtl) + + awsDriver := &nodeService{ + metadata: mockMetadata, + mounter: mockMounter, + inFlight: internal.NewInFlight(), + } + + mockMounter.EXPECT().PathExists(gomock.Eq(targetPath)).Return(true, errors.New("CorruptedMntError")) + mockMounter.EXPECT().IsCorruptedMnt(gomock.Eq(errors.New("CorruptedMntError"))).Return(true) + + 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().Mount(gomock.Eq(stagingTargetPath), gomock.Eq(targetPath), gomock.Eq(defaultFsType), gomock.Eq([]string{"bind"})).Return(nil) req := &csi.NodePublishVolumeRequest{ PublishContext: map[string]string{DevicePathKey: devicePath}, @@ -629,6 +732,7 @@ func TestNodePublishVolume(t *testing.T) { mockMounter.EXPECT().MakeDir(gomock.Eq(targetPath)).Return(nil) mockMounter.EXPECT().Mount(gomock.Eq(stagingTargetPath), gomock.Eq(targetPath), gomock.Eq(FSTypeXfs), gomock.Eq([]string{"bind", "nouuid"})).Return(nil) + mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, nil) req := &csi.NodePublishVolumeRequest{ PublishContext: map[string]string{DevicePathKey: devicePath}, @@ -670,6 +774,7 @@ func TestNodePublishVolume(t *testing.T) { mockMounter.EXPECT().MakeDir(gomock.Eq(targetPath)).Return(nil) mockMounter.EXPECT().Mount(gomock.Eq(stagingTargetPath), gomock.Eq(targetPath), gomock.Eq(defaultFsType), gomock.Eq([]string{"bind", "ro"})).Return(nil) + mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, nil) req := &csi.NodePublishVolumeRequest{ PublishContext: map[string]string{DevicePathKey: devicePath}, @@ -703,6 +808,7 @@ func TestNodePublishVolume(t *testing.T) { mockMounter.EXPECT().MakeDir(gomock.Eq(targetPath)).Return(nil) mockMounter.EXPECT().Mount(gomock.Eq(stagingTargetPath), gomock.Eq(targetPath), gomock.Eq(defaultFsType), gomock.Eq([]string{"bind", "test-flag"})).Return(nil) + mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, nil) req := &csi.NodePublishVolumeRequest{ PublishContext: map[string]string{DevicePathKey: "/dev/fake"}, @@ -753,6 +859,145 @@ func TestNodePublishVolume(t *testing.T) { mockMounter.EXPECT().MakeDir(gomock.Eq("/test")).Return(nil) mockMounter.EXPECT().MakeFile(targetPath).Return(nil) mockMounter.EXPECT().Mount(gomock.Eq(devicePath), gomock.Eq(targetPath), gomock.Eq(""), gomock.Eq([]string{"bind"})).Return(nil) + mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, nil) + + req := &csi.NodePublishVolumeRequest{ + PublishContext: map[string]string{DevicePathKey: "/dev/fake"}, + StagingTargetPath: stagingTargetPath, + TargetPath: targetPath, + VolumeCapability: &csi.VolumeCapability{ + AccessType: &csi.VolumeCapability_Block{ + Block: &csi.VolumeCapability_BlockVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER, + }, + }, + VolumeId: "vol-test", + } + + _, err := awsDriver.NodePublishVolume(context.TODO(), req) + if err != nil { + t.Fatalf("Expect no error but got: %v", err) + } + }, + }, + { + name: "success mounted already [raw block]", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + defer mockCtl.Finish() + + mockMetadata := mocks.NewMockMetadataService(mockCtl) + mockMounter := mocks.NewMockMounter(mockCtl) + + awsDriver := &nodeService{ + metadata: mockMetadata, + mounter: mockMounter, + inFlight: internal.NewInFlight(), + } + + gomock.InOrder( + mockMounter.EXPECT().PathExists(gomock.Eq(devicePath)).Return(true, nil), + mockMounter.EXPECT().PathExists(gomock.Eq("/test")).Return(false, nil), + ) + mockMounter.EXPECT().MakeDir(gomock.Eq("/test")).Return(nil) + mockMounter.EXPECT().MakeFile(targetPath).Return(nil) + mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(false, nil) + + req := &csi.NodePublishVolumeRequest{ + PublishContext: map[string]string{DevicePathKey: "/dev/fake"}, + StagingTargetPath: stagingTargetPath, + TargetPath: targetPath, + VolumeCapability: &csi.VolumeCapability{ + AccessType: &csi.VolumeCapability_Block{ + Block: &csi.VolumeCapability_BlockVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER, + }, + }, + VolumeId: "vol-test", + } + + _, err := awsDriver.NodePublishVolume(context.TODO(), req) + if err != nil { + t.Fatalf("Expect no error but got: %v", err) + } + }, + }, + { + name: "success mountpoint error [raw block]", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + defer mockCtl.Finish() + + mockMetadata := mocks.NewMockMetadataService(mockCtl) + mockMounter := mocks.NewMockMounter(mockCtl) + + awsDriver := &nodeService{ + metadata: mockMetadata, + mounter: mockMounter, + inFlight: internal.NewInFlight(), + } + + gomock.InOrder( + mockMounter.EXPECT().PathExists(gomock.Eq(devicePath)).Return(true, nil), + mockMounter.EXPECT().PathExists(gomock.Eq("/test")).Return(false, nil), + mockMounter.EXPECT().PathExists(gomock.Eq(targetPath)).Return(true, nil), + ) + + mockMounter.EXPECT().MakeDir(gomock.Eq("/test")).Return(nil) + mockMounter.EXPECT().MakeFile(targetPath).Return(nil) + mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, errors.New("Internal System Error")) + + req := &csi.NodePublishVolumeRequest{ + PublishContext: map[string]string{DevicePathKey: "/dev/fake"}, + StagingTargetPath: stagingTargetPath, + TargetPath: targetPath, + VolumeCapability: &csi.VolumeCapability{ + AccessType: &csi.VolumeCapability_Block{ + Block: &csi.VolumeCapability_BlockVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER, + }, + }, + VolumeId: "vol-test", + } + + _, err := awsDriver.NodePublishVolume(context.TODO(), req) + expectErr(t, err, codes.Internal) + }, + }, + { + name: "success corrupted mountpoint error [raw block]", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + defer mockCtl.Finish() + + mockMetadata := mocks.NewMockMetadataService(mockCtl) + mockMounter := mocks.NewMockMounter(mockCtl) + + awsDriver := &nodeService{ + metadata: mockMetadata, + mounter: mockMounter, + inFlight: internal.NewInFlight(), + } + + gomock.InOrder( + mockMounter.EXPECT().PathExists(gomock.Eq(devicePath)).Return(true, nil), + mockMounter.EXPECT().PathExists(gomock.Eq("/test")).Return(false, nil), + mockMounter.EXPECT().PathExists(gomock.Eq(targetPath)).Return(true, errors.New("CorruptedMntError")), + ) + + mockMounter.EXPECT().IsCorruptedMnt(errors.New("CorruptedMntError")).Return(true) + + mockMounter.EXPECT().MakeDir(gomock.Eq("/test")).Return(nil) + mockMounter.EXPECT().MakeFile(targetPath).Return(nil) + mockMounter.EXPECT().Unmount(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) req := &csi.NodePublishVolumeRequest{ PublishContext: map[string]string{DevicePathKey: "/dev/fake"}, @@ -797,6 +1042,7 @@ func TestNodePublishVolume(t *testing.T) { mockMounter.EXPECT().MakeDir(gomock.Eq("/test")).Return(nil) mockMounter.EXPECT().MakeFile(targetPath).Return(nil) mockMounter.EXPECT().Mount(gomock.Eq(devicePathWithPartition), gomock.Eq(targetPath), gomock.Eq(""), gomock.Eq([]string{"bind"})).Return(nil) + mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, nil) req := &csi.NodePublishVolumeRequest{ PublishContext: map[string]string{DevicePathKey: "/dev/fake"}, @@ -842,6 +1088,7 @@ func TestNodePublishVolume(t *testing.T) { mockMounter.EXPECT().MakeDir(gomock.Eq("/test")).Return(nil) mockMounter.EXPECT().MakeFile(targetPath).Return(nil) mockMounter.EXPECT().Mount(gomock.Eq(devicePath), gomock.Eq(targetPath), gomock.Eq(""), gomock.Eq([]string{"bind"})).Return(nil) + mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, nil) req := &csi.NodePublishVolumeRequest{ PublishContext: map[string]string{DevicePathKey: "/dev/fake"}, diff --git a/pkg/driver/node_windows.go b/pkg/driver/node_windows.go index 3a46c8ce76..533fd4e36e 100644 --- a/pkg/driver/node_windows.go +++ b/pkg/driver/node_windows.go @@ -1,3 +1,4 @@ +//go:build windows // +build windows /* diff --git a/pkg/driver/sanity_test.go b/pkg/driver/sanity_test.go index 91d7422050..35c15578eb 100644 --- a/pkg/driver/sanity_test.go +++ b/pkg/driver/sanity_test.go @@ -301,6 +301,10 @@ func newFakeMounter() *fakeMounter { } } +func (f *fakeMounter) IsCorruptedMnt(err error) bool { + return false +} + func (f *fakeMounter) Mount(source string, target string, fstype string, options []string) error { return nil } diff --git a/pkg/mounter/safe_mounter_unix.go b/pkg/mounter/safe_mounter_unix.go index a72b544748..215047b001 100644 --- a/pkg/mounter/safe_mounter_unix.go +++ b/pkg/mounter/safe_mounter_unix.go @@ -1,3 +1,4 @@ +//go:build linux || darwin // +build linux darwin /* diff --git a/pkg/mounter/safe_mounter_windows.go b/pkg/mounter/safe_mounter_windows.go index c1a50c55c3..968be7bfdd 100644 --- a/pkg/mounter/safe_mounter_windows.go +++ b/pkg/mounter/safe_mounter_windows.go @@ -1,3 +1,4 @@ +//go:build windows // +build windows /*