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.

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

fix mount unmount failure for a Windows pod #63272

Merged
merged 2 commits into from May 15, 2018

Conversation

Projects
None yet
5 participants
@andyzhangx
Copy link
Member

andyzhangx commented Apr 28, 2018

What this PR does / why we need it:
IsLikelyNotMountPoint func does not return correctly, for invalid symlink, it should return true(not a mount point), now it will always return false:

func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) {
stat, err := os.Lstat(file)
if err != nil {
return true, err
}
// If current file is a symlink, then it is a mountpoint.
if stat.Mode()&os.ModeSymlink != 0 {
return false, nil

func doUnmountMountPoint(mountPath string, mounter mount.Interface, extensiveMountPointCheck bool, corruptedMnt bool) error {
if !corruptedMnt {
var notMnt bool
var err error
if extensiveMountPointCheck {
notMnt, err = mount.IsNotMountPoint(mounter, mountPath)
} else {
notMnt, err = mounter.IsLikelyNotMountPoint(mountPath)
}
if err != nil {
return err
}
if notMnt {
glog.Warningf("Warning: %q is not a mountpoint, deleting", mountPath)
return os.Remove(mountPath)

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #63249

Special notes for your reviewer:

Release note:

fix mount unmount failure for a Windows pod

/sig windows
/assign @msau42

@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented Apr 28, 2018

/test pull-kubernetes-cross

@andyzhangx andyzhangx force-pushed the andyzhangx:IsLikelyNotMountPoint-windows-fix branch from 1c1dfda to 4e92e60 Apr 28, 2018


for _, test := range tests {
filePath := filepath.Join(base, test.fileName)
if test.fileValid {

This comment has been minimized.

@msau42

msau42 Apr 30, 2018

Member

You could potentially pass in a setup method per test case, instead of combining all the scenarios here, to make it easier to comprehend each test case separately.

This comment has been minimized.

@andyzhangx

andyzhangx May 1, 2018

Author Member

@msau42 thanks for the suggestion, I have made a new commit with setUp func, the test code is more clear now

@andyzhangx andyzhangx force-pushed the andyzhangx:IsLikelyNotMountPoint-windows-fix branch from 4e92e60 to f3698b0 May 1, 2018

@andyzhangx
Copy link
Member Author

andyzhangx left a comment

@msau42 thanks for the suggestion, I have made a new commit with setUp func, the test code is more clear now


for _, test := range tests {
filePath := filepath.Join(base, test.fileName)
if test.fileValid {

This comment has been minimized.

@andyzhangx

andyzhangx May 1, 2018

Author Member

@msau42 thanks for the suggestion, I have made a new commit with setUp func, the test code is more clear now

@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented May 1, 2018

/test pull-kubernetes-cross

@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented May 1, 2018

/test pull-kubernetes-verify

2 similar comments
@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented May 2, 2018

/test pull-kubernetes-verify

@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented May 3, 2018

/test pull-kubernetes-verify

@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented May 3, 2018

@msau42 PTAL, thanks.


func TestIsLikelyNotMountPoint(t *testing.T) {
mounter := Mounter{"fake/path"}
base, err := ioutil.TempDir("", "TestIsLikelyNotMountPoint")

This comment has been minimized.

@msau42

msau42 May 7, 2018

Member

Should you make the TempDir per test case so that you won't run into name collision issues?

This comment has been minimized.

@andyzhangx

andyzhangx May 8, 2018

Author Member

@msau42 I have already merged a PR: #63399 which fixes this issue, and also rebased on the latest master branch, now every test case in mount_windows_test.go will make TempDir

This comment has been minimized.

@msau42

msau42 May 8, 2018

Member

I actually mean each test case in tests uses a new tmpdir. That way you don't need to rely on each test case filename to be unique

This comment has been minimized.

@andyzhangx

andyzhangx May 9, 2018

Author Member

Hi @msau42 actually ioutil.TempDir("", "") will create a new tmpdir with a unique name, add a prefix like "TestIsLikelyNotMountPoint" is for debugging purpose, so when it fails, I could comment out defer os.RemoveAll(base) and find the tmp directory easily. Anyway, whether I set prefix or not, ioutil.TempDir will still create a unique dir.

This comment has been minimized.

@msau42

msau42 May 9, 2018

Member

Sorry I think I'm not being clear. I was suggesting a new tempdir for each iteration in the testcase loop

This comment has been minimized.

@andyzhangx

andyzhangx May 9, 2018

Author Member

@msau42 good point, I have changed the test case, create a new tempdir for every single test case, PTAL

@andyzhangx andyzhangx force-pushed the andyzhangx:IsLikelyNotMountPoint-windows-fix branch from f3698b0 to 51b0649 May 8, 2018

add IsLikelyNotMountPoint test on Windows
fix comments

fix comments on unit test

fix comments

@andyzhangx andyzhangx force-pushed the andyzhangx:IsLikelyNotMountPoint-windows-fix branch from 51b0649 to 1d24f48 May 9, 2018

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented May 9, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 9, 2018

@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented May 9, 2018

/test pull-kubernetes-e2e-gce

@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented May 9, 2018

/assign @jsafrane PTAL, thanks

@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented May 13, 2018

/test pull-kubernetes-bazel-test
/test pull-kubernetes-e2e-kops-aws

@jsafrane

This comment has been minimized.

Copy link
Member

jsafrane commented May 15, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 15, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, jsafrane, msau42

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 15, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented May 15, 2018

/test pull-kubernetes-integration

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 15, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 89b747b into kubernetes:master May 15, 2018

15 of 16 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-integration
Details
cla/linuxfoundation andyzhangx authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

andyzhangx added a commit to andyzhangx/kubernetes that referenced this pull request Jun 8, 2018

k8s-github-robot pushed a commit that referenced this pull request Jun 26, 2018

Kubernetes Submit Queue
Merge pull request #64679 from andyzhangx/automated-cherry-pick-of-#6…
…3272-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #63272: fix IsLikelyNotMountPoint func on Windows

Cherry pick of #63272 on release-1.9.

#63272: fix IsLikelyNotMountPoint func on Windows

k8s-github-robot pushed a commit that referenced this pull request Jul 20, 2018

Kubernetes Submit Queue
Merge pull request #64900 from andyzhangx/automated-cherry-pick-of-#6…
…3272-upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #63272: fix IsLikelyNotMountPoint func on Windows

Cherry pick of #63272 on release-1.10.

#63272: fix IsLikelyNotMountPoint func on Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.