fix smb unmount issue on Windows #75087

andyzhangx · 2019-03-07T05:46:58Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR fix following issue:
pod with smb(cifs) mount could not be terminated on Windows when that smb account is invalid, the pod will be stuck forever

This PR is another windows fix related to #74625

Which issue(s) this PR fixes:

Fixes #75025

Special notes for your reviewer:
This PR implement IsCorruptedMnt func on Windows, unix code is actully unchanged.
The warnning logs in this PR would be like following in kubelet which is expected:

W0308 05:53:34.344557    2392 mount_helper_windows.go:52] IsCorruptedMnt failed with ERROR_LOGON_FAILURE error: CreateFile c:\var\lib\kubelet\pods\4fe70e4a-4166-11e9-bf93-000d3af95268\volumes\kubernetes.io~azure-file\pvc-4fe1243c-4166-11e9-bf93-000d3af95268: The user name or password is incorrect.
W0308 05:53:34.347757    2392 mount_helper_windows.go:52] IsCorruptedMnt failed with ERROR_LOGON_FAILURE error: CreateFile c:\var\lib\kubelet\pods\4fe70e4a-4166-11e9-bf93-000d3af95268\volumes\kubernetes.io~azure-file\pvc-4fe1243c-4166-11e9-bf93-000d3af95268: The user name or password is incorrect.
I0308 05:53:34.347757    2392 mount_helper_common.go:74] "c:\\var\\lib\\kubelet\\pods\\4fe70e4a-4166-11e9-bf93-000d3af95268\\volumes\\kubernetes.io~azure-file\\pvc-4fe1243c-4166-11e9-bf93-000d3af95268" is a mountpoint, unmounting
I0308 05:53:34.347757    2392 mount_windows.go:149] azureMount: Unmount target ("c:\\var\\lib\\kubelet\\pods\\4fe70e4a-4166-11e9-bf93-000d3af95268\\volumes\\kubernetes.io~azure-file\\pvc-4fe1243c-4166-11e9-bf93-000d3af95268")

Does this PR introduce a user-facing change?:

fix smb unmount issue on Windows

/sig windows
/priority important-soon
/assign @jingxu97 @davidz627 @msau42

cc @feiskyer could you mark milestone of this PR, thanks.

andyzhangx · 2019-03-07T05:47:27Z

/test pull-kubernetes-cross

feiskyer · 2019-03-07T05:50:08Z

/milestone v1.14

feiskyer · 2019-03-07T05:51:06Z

pkg/util/mount/mount_helper_windows.go

Is "password is incorrect" the only case for corrupt? Are there any other possibilities?

good question, there must be other possiblities, while I don't know.
another fix is write like this which may have drawbacks, it depends on whether we use whiltelist or blacklist:

func IsCorruptedMnt(err error) bool { return err != nil }

I think there are some errors still missing on Linux implementation since only following 4 errors caught in IsCorruptedMnt func:

kubernetes/pkg/util/mount/mount_helper.go

Lines 107 to 123 in a8492d7

func IsCorruptedMnt(err error) bool {

if err == nil {

return false

}

var underlyingError error

switch pe := err.(type) {

case nil:

return false

case *os.PathError:

underlyingError = pe.Err

case *os.LinkError:

underlyingError = pe.Err

case *os.SyscallError:

underlyingError = pe.Err

}

return underlyingError == syscall.ENOTCONN || underlyingError == syscall.ESTALE || underlyingError == syscall.EIO || underlyingError == syscall.EACCES

@jingxu97 @msau42 @davidz627 what do you think about this? why way do you prefer on Windows?

invalid password seems like a strange scenario to consider as a "corrupted" mount. Is that a normal error that users may encounter? Is it recoverable?

I think these could also be returned

ERROR_NETWORK_ACCESS_DENIED

ERROR_BAD_NET_NAME

ERROR_NETNAME_DELETED

(reference list: https://docs.microsoft.com/en-us/windows/desktop/Debug/system-error-codes--0-499-)

@andyzhangx

I think there are some errors still missing on Linux implementation since only following 4 errors caught in IsCorruptedMnt func

this command uses Stat() -> fstatat() on *unix
and i think the list of existing errors is odd.

here is what fstatat returns:
http://man7.org/linux/man-pages/man3/fstatat.3p.html

on Windows Stat() uses the stat() function here:
https://golang.org/src/os/stat_windows.go

so it will throw a PathError at least. i don't know where SMBConnectionError comes from.

in terms of Windows error codes @PatrickLang already listed some, i don't know the codes of a corrupted mklink / CreateSymbolicLink, but here is what you can do to check against them:

package main import ( "fmt" "golang.org/x/sys/windows" "syscall" ) func main() { ptr, err := windows.UTF16PtrFromString("./some-missing-path") if err != nil { fmt.Println(err) return } attrib, err := windows.GetFileAttributes(ptr) if err != nil { fmt.Println(err.(syscall.Errno) == windows.ERROR_FILE_NOT_FOUND) // or any error code... return } fmt.Println(attrib) }

and then error info The user name or password is incorrect does not exists in types_windows.go

we can still verify against the integer (ERROR_LOGON_FAILURE, 1326)
https://docs.microsoft.com/en-us/windows/desktop/debug/system-error-codes--1300-1699-

e.g.

const ERROR_LOGON_FAILURE = 1326 ... err.(syscall.Errno) == ERROR_LOGON_FAILURE

@neolit123 thanks, it's 1040, not in the list of https://docs.microsoft.com/en-us/windows/desktop/Debug/system-error-codes--1000-1299-, so shall I go with 1040 error checking?

hm, i don't know what 1040 is, but probably best not to check for it.
golang just calls native functions and those follow only the existing error codes.

@neolit123 sorry, I was wrong, it's this error:

ERROR_LOGON_FAILURE 1326 (0x52E) The user name or password is incorrect.

I think I have the correct solution now, I have this error check in the code first, thanks!

that's great that it matches the real error codes from windows

andyzhangx · 2019-03-07T06:08:52Z

/test pull-kubernetes-cross

andyzhangx · 2019-03-07T12:06:48Z

/test pull-kubernetes-e2e-gce
/test pull-kubernetes-integration

michmike · 2019-03-07T19:02:05Z

adding a hold until we identify all the proper mount failures on windows
/hold
@neolit123 can you also please review?

michmike · 2019-03-07T19:06:00Z

cc: @benmoss for review as well

PatrickLang · 2019-03-07T20:09:10Z

Just like #74625, this would also need backport to 1.11-1.13.

pkg/util/mount/mount_helper_windows.go

PatrickLang · 2019-03-07T20:21:58Z

cc @KnicKnic - any feedback on identifying failed mounts?

andyzhangx · 2019-03-08T06:06:46Z

thanks guys, I used errorno check as @neolit123 suggested, pushed a new commit: d0d49da

BTW, this issue is a common failure in SMB user scenario, for some reason(e.g. security consideration), user changed the password of SMB server, while the original SMB mount could not be terminated forever, on Linux, it has been fixed by adding one more check:

underlyingError == syscall.EACCES

kubernetes/pkg/util/mount/mount_helper.go

Line 123 in 788f245

    
           return underlyingError == syscall.ENOTCONN || underlyingError == syscall.ESTALE || underlyingError == syscall.EIO || underlyingError == syscall.EACCES

While it is not fixed on Windows yet.

It looks like it's very close to code freeze date, can we make it into 1.14? @michmike

michmike · 2019-03-08T06:17:40Z

thanks guys, I used errorno check as @neolit123 suggested, pushed a new commit: d0d49da

BTW, this issue is a common failure in SMB user scenario, for some reason(e.g. security consideration), user changed the password of SMB server, while the original SMB mount could not be terminated forever, on Linux, it has been fixed by adding one more check:
underlyingError == syscall.EACCES
kubernetes/pkg/util/mount/mount_helper.go

Line 123 in 788f245

return underlyingError == syscall.ENOTCONN || underlyingError == syscall.ESTALE || underlyingError == syscall.EIO || underlyingError == syscall.EACCES
While it is not fixed on Windows yet.

It looks like it's very close to code freeze date, can we make it into 1.14?

don't worry about code freeze. we need to fix this properly , test it and then we can see about 1.14

michmike · 2019-03-08T06:18:49Z

i would like @adelina-t, @BCLAU to also have the fix run in the same environment where it failed before

andyzhangx · 2019-03-08T06:24:54Z

i would like @adelina-t, @BCLAU to also have the fix run in the same environment where it failed before

This fix only applies for SMB unmount issue on Windows, happens when password changed, do we have any test failure on this before? @michmike

pkg/util/mount/mount_helper_windows.go

michmike · 2019-03-09T22:49:41Z

I have added more error code check and squashed all commits, PTAL again.
In the code, I used an array to collect all known errors.

Below are the errors we need to handle per windows team:

For SMB mounts, the typical errors for connectivity are 53 and 64.

53 ERROR_BAD_NETPATH <--> 0xc00000be STATUS_BAD_NETWORK_PATH
64 ERROR_NETNAME_DELETED <--> 0xc00000c9 STATUS_NETWORK_NAME_DELETED
64 ERROR_NETNAME_DELETED <--> 0xc000013b STATUS_LOCAL_DISCONNECT
64 ERROR_NETNAME_DELETED <--> 0xc000013c STATUS_REMOTE_DISCONNECT
64 ERROR_NETNAME_DELETED <--> 0xc000020b STATUS_ADDRESS_CLOSED
64 ERROR_NETNAME_DELETED <--> 0xc000020c STATUS_CONNECTION_DISCONNECTED
64 ERROR_NETNAME_DELETED <--> 0xc000020d STATUS_CONNECTION_RESET
64 ERROR_NETNAME_DELETED <--> 0xc0000466 STATUS_SERVER_UNAVAILABLE
64 ERROR_NETNAME_DELETED <--> 0xc0000480 STATUS_SHARE_UNAVAILABLE

For incorrect password/logon failure:
1326 ERROR_LOGON_FAILURE <--> c000006d STATUS_LOGON_FAILURE

This is an error if there is another existing mapping using a different set of credentials:
1219 ERROR_SESSION_CREDENTIAL_CONFLICT <--> c0000195 STATUS_NETWORK_CREDENTIAL_CONFLICT

This indicates the share does not exist:
67 ERROR_BAD_NET_NAME <--> c00000cc STATUS_BAD_NETWORK_NAME

do all those errors map to the same one with 64 as the error code?

pkg/util/mount/mount_windows.go

michmike · 2019-03-09T22:53:07Z

i added some comments which you should resolve before removing the hold
/hold
/lgtm

michmike · 2019-03-09T22:53:46Z

pinging the sig-storage team to take a look at this as well

msau42 · 2019-03-09T23:47:07Z

Are these all errors that when if encountered, the mount point could still be successfully unmounted and cleaned up?

To give some context, the unmount sequence is like:

Check if directory is a mount point <-- this is where the IsCorruptedMount check is done
If it is a mount, then call Unmount
If unmount was successful and the directory is no longer a mount point, remove the directory.

fix log warning use IsCorruptedMnt in GetMountRefs on Windows use errorno in IsCorruptedMnt check fix comments: add more error code add more error no checking change year fix comments

andyzhangx · 2019-03-10T02:18:49Z

do all those errors map to the same one with 64 as the error code?

Partly correct, a lot of errors are mapping to 64, others are different error codes. @michmike

Are these all errors that when if encountered, the mount point could still be successfully unmounted and cleaned up?
To give some context, the unmount sequence is like:
Check if directory is a mount point <-- this is where the IsCorruptedMount check is done
If it is a mount, then call Unmount
If unmount was successful and the directory is no longer a mount point, remove the directory.

Yes, on Windows, all Unmount would be simply rmdir opertion, it will always succeed for those conditions(@msau42 ):

kubernetes/pkg/util/mount/mount_windows.go

Lines 148 to 153 in 01c9edf

    
           func (mounter *Mounter) Unmount(target string) error { 
        
           	klog.V(4).Infof("azureMount: Unmount target (%q)", target) 
        
           	target = normalizeWindowsPath(target) 
        
           	if output, err := exec.Command("cmd", "/c", "rmdir", target).CombinedOutput(); err != nil { 
        
           		klog.Errorf("rmdir failed: %v, output: %q", err, string(output)) 
        
           		return err

/hold cancel
/test pull-kubernetes-cross

michmike · 2019-03-10T18:06:18Z

/approve

michmike · 2019-03-10T18:06:23Z

/lgtm

michmike · 2019-03-10T18:06:54Z

@msau42 if this looks good to you as well, please approve and we will go through the motions of getting this approved for 1.14

michmike · 2019-03-10T18:07:07Z

/assign @jingxu97

jingxu97 · 2019-03-11T22:17:42Z

I have a questions of what is considered as corrupted mount point? In what conditions we should clean them up? For example, for error STATUS_SERVER_UNAVAILABLE, is that possible that server is just temporally unavailable, do we want to clean up the mount in this situation?

jingxu97 · 2019-03-11T23:36:06Z

Talked with @msau42 and @saad-ali, for this PR, adding more error codes should be safe. If it is considered corrected mount point, it will move to the next step trying to unmount and then check mountpoint before removing files and directories.

But the whole process of checking correpted mount point before trying to unmount and remove files could be simplified. I propose to call umount directly, if it fails, checks whether the path is a mount point or not, if not, delete the files and directories. Please see #75273

jingxu97 · 2019-03-11T23:36:16Z

/approve

k8s-ci-robot · 2019-03-11T23:36:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, jingxu97, michmike

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/util/mount/OWNERS~~ [jingxu97]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…5087-upstream-release-1.12 Automated cherry pick of #75087: fix smb unmount issue on Windows

…5087-upstream-release-1.13 Automated cherry pick of #75087: fix smb unmount issue on Windows

k8s-ci-robot assigned davidz627 Mar 7, 2019

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 7, 2019

k8s-ci-robot assigned jingxu97 and msau42 Mar 7, 2019

k8s-ci-robot requested review from jingxu97 and msau42 March 7, 2019 05:47

k8s-ci-robot added this to the v1.14 milestone Mar 7, 2019

feiskyer reviewed Mar 7, 2019

View reviewed changes

andyzhangx force-pushed the unmount-issue-windows branch from 251340f to 48aad50 Compare March 7, 2019 06:07

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2019

PatrickLang reviewed Mar 7, 2019

View reviewed changes

pkg/util/mount/mount_helper_windows.go Outdated Show resolved Hide resolved

michmike mentioned this pull request Mar 8, 2019

cherry pick fix for smb unmount issue on Windows #75154

Closed

michmike reviewed Mar 8, 2019

View reviewed changes

pkg/util/mount/mount_helper_windows.go Outdated Show resolved Hide resolved

michmike reviewed Mar 9, 2019

View reviewed changes

pkg/util/mount/mount_windows.go Outdated Show resolved Hide resolved

michmike approved these changes Mar 9, 2019

View reviewed changes

k8s-ci-robot assigned michmike Mar 9, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2019

fix smb unmount issue on Windows

720a5e2

fix log warning use IsCorruptedMnt in GetMountRefs on Windows use errorno in IsCorruptedMnt check fix comments: add more error code add more error no checking change year fix comments

andyzhangx force-pushed the unmount-issue-windows branch from 92469f2 to 720a5e2 Compare March 10, 2019 02:13

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 10, 2019

michmike approved these changes Mar 10, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2019

jingxu97 mentioned this pull request Mar 11, 2019

Simplify CleanupMountPoint workflow by removing IsCorruptedMnt and IsNotMountPoint #75273

Closed

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2019

k8s-ci-robot merged commit 43b6ddf into kubernetes:master Mar 12, 2019

This was referenced Mar 14, 2019

Automated cherry pick of #75087: fix smb unmount issue on Windows #75358

Merged

Automated cherry pick of #75087: fix smb unmount issue on Windows #75359

Merged

k8s-ci-robot added a commit that referenced this pull request Apr 18, 2019

Merge pull request #75359 from andyzhangx/automated-cherry-pick-of-#7…

5696842

…5087-upstream-release-1.12 Automated cherry pick of #75087: fix smb unmount issue on Windows

k8s-ci-robot added a commit that referenced this pull request May 7, 2019

Merge pull request #75358 from andyzhangx/automated-cherry-pick-of-#7…

37c4776

…5087-upstream-release-1.13 Automated cherry pick of #75087: fix smb unmount issue on Windows

	func IsCorruptedMnt(err error) bool {
	if err == nil {
	return false
	}
	var underlyingError error
	switch pe := err.(type) {
	case nil:
	return false
	case *os.PathError:
	underlyingError = pe.Err
	case *os.LinkError:
	underlyingError = pe.Err
	case *os.SyscallError:
	underlyingError = pe.Err
	}

	return underlyingError == syscall.ENOTCONN \|\| underlyingError == syscall.ESTALE \|\| underlyingError == syscall.EIO \|\| underlyingError == syscall.EACCES

fix smb unmount issue on Windows #75087

fix smb unmount issue on Windows #75087

Conversation

andyzhangx commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andyzhangx commented Mar 7, 2019

Uh oh!

feiskyer commented Mar 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyzhangx Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neolit123 Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyzhangx commented Mar 7, 2019

Uh oh!

andyzhangx commented Mar 7, 2019

Uh oh!

michmike commented Mar 7, 2019

Uh oh!

michmike commented Mar 7, 2019

Uh oh!

PatrickLang commented Mar 7, 2019

Uh oh!

Uh oh!

PatrickLang commented Mar 7, 2019

Uh oh!

andyzhangx commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michmike commented Mar 8, 2019

Uh oh!

michmike commented Mar 8, 2019

Uh oh!

andyzhangx commented Mar 8, 2019

Uh oh!

Uh oh!

michmike commented Mar 9, 2019

Uh oh!

Uh oh!

michmike commented Mar 9, 2019

Uh oh!

michmike commented Mar 9, 2019

Uh oh!

msau42 commented Mar 9, 2019

Uh oh!

andyzhangx commented Mar 10, 2019

Uh oh!

michmike commented Mar 10, 2019

Uh oh!

michmike commented Mar 10, 2019

Uh oh!

michmike commented Mar 10, 2019

Uh oh!

michmike commented Mar 10, 2019

Uh oh!

jingxu97 commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jingxu97 commented Mar 11, 2019

andyzhangx commented Mar 7, 2019 •

edited

Loading

andyzhangx Mar 7, 2019 •

edited

Loading

neolit123 Mar 7, 2019 •

edited

Loading

andyzhangx commented Mar 8, 2019 •

edited

Loading

jingxu97 commented Mar 11, 2019 •

edited

Loading