Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node Publish Mount Idempotent #1019

Merged
merged 5 commits into from
Aug 27, 2021
Merged

Node Publish Mount Idempotent #1019

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3bafcd1
Added Mount Idempotency
nirmalaagash Aug 10, 2021
36ffbfe
fixed:g-format error
nirmalaagash Aug 11, 2021
ac4a32e
Added Mount Idempotency for block device
nirmalaagash Aug 18, 2021
da8dbf3
Merge branch 'kubernetes-sigs:master' into master
nirmalaagash Aug 25, 2021
38d60d9
go formatt error fixed using go v1.17
nirmalaagash Aug 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions pkg/driver/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,13 +620,41 @@ 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 it's a mount point. There are three cases,
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An easy way to cover block devices as well is to move the check right below the in flight check.

You may want to take a look at the solutions both GCP PD and Azure Disk implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bertinatto Thanks. New commit covering block devices is pushed.

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 := mountutils.PathExists(target)
if pathErr != nil && mountutils.IsCorruptedMnt(pathErr) {
klog.V(4).Infof("Target path %q is a corrupted directory", target)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we not returning an error if target path is a corrupted directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chances are that the device mounted or the mount point is corrupted because of improper unmounting. Hence in the next phase we try to read the directory and unmount it.

} else {
return status.Errorf(codes.Internal, "Could not mount %q at %q: %v", source, target, err)
}
return status.Errorf(codes.Internal, "Could not mount %q at %q: %v", source, target, err)
}

if !notMnt {
//Return error when any of the directory inside is not readable which means that a device could be mounted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you clarify what this comment means? not sure i'm following

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if a device is already mounted in the target directory, the directories inside the directory would not be accessible until the device is unmounted from the target directory. Hence in that case, it would return an error.

_, err = os.ReadDir(target)
if err != nil {
klog.V(4).Infof("Error occurred at reading directory %q. Trying to Unmount.", target)
//Reading the directory failed and trying to unmount and remount
if mntErr := d.mounter.Unmount(target); mntErr != nil {
return status.Errorf(codes.Internal, "Unable to unmount the target %q : %v", target, err)
}
} else {
klog.V(4).Infof("Target path %q is already mounted", target)
return nil
}
}

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
}

Expand Down
8 changes: 8 additions & 0 deletions pkg/driver/node_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,8 @@ 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(false, nil)
mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(nil)

req := &csi.NodePublishVolumeRequest{
PublishContext: map[string]string{DevicePathKey: devicePath},
Expand Down Expand Up @@ -629,6 +631,8 @@ 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(false, nil)
mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(nil)

req := &csi.NodePublishVolumeRequest{
PublishContext: map[string]string{DevicePathKey: devicePath},
Expand Down Expand Up @@ -670,6 +674,8 @@ 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(false, nil)
mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(nil)

req := &csi.NodePublishVolumeRequest{
PublishContext: map[string]string{DevicePathKey: devicePath},
Expand Down Expand Up @@ -703,6 +709,8 @@ 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(false, nil)
mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(nil)

req := &csi.NodePublishVolumeRequest{
PublishContext: map[string]string{DevicePathKey: "/dev/fake"},
Expand Down