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

Jump to bottom

GetMountRefs fixed to handle corrupted mounts by treating it like an unmounted volume #74625

Merged
merged 1 commit into from
Mar 7, 2019
Merged

GetMountRefs fixed to handle corrupted mounts by treating it like an unmounted volume #74625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/util/mount/mount_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,5 +120,5 @@ func IsCorruptedMnt(err error) bool {
underlyingError = pe.Err
}

return underlyingError == syscall.ENOTCONN || underlyingError == syscall.ESTALE || underlyingError == syscall.EIO
return underlyingError == syscall.ENOTCONN || underlyingError == syscall.ESTALE || underlyingError == syscall.EIO || underlyingError == syscall.EACCES
}
10 changes: 7 additions & 3 deletions pkg/util/mount/mount_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1003,10 +1003,14 @@ func (mounter *Mounter) SafeMakeDir(subdir string, base string, perm os.FileMode
}

func (mounter *Mounter) GetMountRefs(pathname string) ([]string, error) {
if _, err := os.Stat(pathname); os.IsNotExist(err) {
Copy link
Member

Choose a reason for hiding this comment

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

does windows also need fixing since the original change also added a stat check for mount_windows? cc @jingxu97 @andyzhangx

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like PathExists in the mount_helper does not consider windows OS. We could add a windows version of PathExists or add windows handling inside of PathExists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure which windows errors are because of corrupted mount that correspond with unix errors ENOTCONN, ESTALE, EIO.

Windows error list here: https://github.com/golang/sys/blob/master/windows/types_windows.go#L9

Copy link
Member

Choose a reason for hiding this comment

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

Note that pkg syscall and sys are different

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, pkg sys seems to imply that the same syscall.Errno codes mean different things in windows vs. unix.
And IsCorruptedMnt seems to be using the unix interpretation of the error codes so I would think we need a Windows specific version of PathExists and IsCorruptedMnt.

Copy link
Member

Choose a reason for hiding this comment

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

looks like PathExists in the mount_helper does not consider windows OS. We could add a windows version of PathExists or add windows handling inside of PathExists?

whether PathExists works on windows os depends on whether IsCorruptedMnt works on windows, it at least could compile on windows. Since the original issue is related to XFS mount issue, and windows does not support that, I think use same fix on windows would be ok.
@davidz627 could you do same fix on windows_mount.go?
BTW, as I implemented GetMountRefs func in the beginning, you could see it's not reliable since there is no place to query all mount points on Windows, different from Linux.

kubernetes/pkg/util/mount/mount_windows.go

Lines 497 to 505 in 4fa5ece

// GetMountRefs : empty implementation here since there is no place to query all mount points on Windows
func (mounter *Mounter) GetMountRefs(pathname string) ([]string, error) {
if _, err := os.Stat(normalizeWindowsPath(pathname)); os.IsNotExist(err) {
return []string{}, nil
} else if err != nil {
return nil, err
}
return []string{pathname}, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've updated the windows GetMountRefs as well. I believe the errcodes are different for windows though. Therefore I have removed the IsCorruptedMnt check as I believe it would be just checking "random" error codes and may mask real issues.

Since XFS mount issue doesn't exist on windows anyway this is not a necessary addition.

pathExists, pathErr := PathExists(pathname)
if !pathExists {
return []string{}, nil
} else if err != nil {
return nil, err
} else if IsCorruptedMnt(pathErr) {
klog.Warningf("GetMountRefs found corrupted mount at %s, treating as unmounted path", pathname)
return []string{}, nil
} else if pathErr != nil {
return nil, fmt.Errorf("error checking path %s: %v", pathname, pathErr)
}
realpath, err := filepath.EvalSymlinks(pathname)
if err != nil {
Expand Down
10 changes: 7 additions & 3 deletions pkg/util/mount/mount_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,10 +496,14 @@ func getAllParentLinks(path string) ([]string, error) {

// GetMountRefs : empty implementation here since there is no place to query all mount points on Windows
func (mounter *Mounter) GetMountRefs(pathname string) ([]string, error) {
if _, err := os.Stat(normalizeWindowsPath(pathname)); os.IsNotExist(err) {
pathExists, pathErr := PathExists(normalizeWindowsPath(pathname))
// TODO(#75012): Need a Windows specific IsCorruptedMnt function that checks against whatever errno's
// Windows emits when we try to Stat a corrupted mount
// https://golang.org/pkg/syscall/?GOOS=windows&GOARCH=amd64#Errno
if !pathExists {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about checking IsCorruptedMnt? This function should work for both windows and linux?

Copy link
Contributor Author

@davidz627 davidz627 Mar 5, 2019 •
edited

Choose a reason for hiding this comment

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

#74625 (comment)

technically it "works" as in it compiles, but I think it is checking the incorrect error codes. Since there is no XFS on windows anyway I'm not sure we need to check for those types of corrupted mounts anyway

return []string{}, nil
} else if err != nil {
return nil, err
} else if pathErr != nil {
return nil, fmt.Errorf("error checking path %s: %v", normalizeWindowsPath(pathname), pathErr)
}
return []string{pathname}, nil
}
Expand Down
9 changes: 4 additions & 5 deletions pkg/util/mount/nsenter_mount.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,12 +344,11 @@ func (mounter *NsenterMounter) SafeMakeDir(subdir string, base string, perm os.F
}

func (mounter *NsenterMounter) GetMountRefs(pathname string) ([]string, error) {
exists, err := mounter.ExistsPath(pathname)
if err != nil {
return nil, err
}
if !exists {
pathExists, pathErr := PathExists(pathname)
if !pathExists || IsCorruptedMnt(pathErr) {
return []string{}, nil
} else if pathErr != nil {
return nil, fmt.Errorf("Error checking path %s: %v", pathname, pathErr)
}
hostpath, err := mounter.ne.EvalSymlinks(pathname, true /* mustExist */)
if err != nil {
Expand Down