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

Use UnmountMountPoint util to clean up subpaths #71804

Merged
merged 7 commits into from Jan 4, 2019

Conversation

@msau42
Copy link
Member

msau42 commented Dec 6, 2018

What type of PR is this?
/kind bug

What this PR does / why we need it:
UnmountMountPoint is a wrapper util that handles corrupt and stale mounts. Cleaning subpaths mounts should use the util instead of directly calling Unmount.

Which issue(s) this PR fixes:

Fixes #71584

Special notes for your reviewer:
To make this small for backporting, I've tried to do the minimum changes required and will look into refactoring/cleanup afterwards.

Does this PR introduce a user-facing change?:

Fixes issue with cleaning up stale NFS subpath mounts
@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Dec 6, 2018

/assign @jsafrane

Show resolved Hide resolved pkg/util/mount/mount_linux.go Outdated
} else if IsCorruptedMnt(err) {
return true, err
} else {
// TODO: MSAU: should unknown err return pathExists=true for safety?

This comment has been minimized.

@msau42

msau42 Dec 6, 2018

Member

The original function returned false. I think it should actually return true otherwise we may return cleanup success for unknown errors.

This comment has been minimized.

@msau42

msau42 Dec 27, 2018

Member

changed it back to original for now. I see util/file/FileExists also returns false for all errors.

// PathExists returns true if the specified path exists.
// TODO: MSAU this is not a good name
func PathExists(path string, mounter Interface) (bool, error) {
_, err := mounter.ExistsPath(path)

This comment has been minimized.

@msau42

msau42 Dec 26, 2018

Member

This may not work for containerized kubelet because it would cause the path to be checked based on the host, not inside the kubelet container.

I will change it back to os.Stat(). I will need to figure out a different way to unit test this.

@msau42 msau42 force-pushed the msau42:fix-subpath-nfs branch from 1492086 to 13852f5 Dec 27, 2018

@k8s-ci-robot k8s-ci-robot added sig/storage and removed needs-sig labels Dec 27, 2018

@msau42 msau42 force-pushed the msau42:fix-subpath-nfs branch from 13852f5 to d337701 Dec 27, 2018

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Dec 27, 2018

Still todo:

  • Figure out a way to unit test this
  • Write an e2e test
  • Open up issues for refactoring/cleanup
  • Test containerized kubelet

@msau42 msau42 force-pushed the msau42:fix-subpath-nfs branch from d337701 to 4abcc08 Dec 27, 2018

@msau42 msau42 force-pushed the msau42:fix-subpath-nfs branch from 9dbb523 to 24526e8 Dec 28, 2018

@k8s-ci-robot k8s-ci-robot added size/L and removed size/XL labels Dec 28, 2018

@msau42 msau42 changed the title [WIP] Use UnmountMountPoint util to clean up subpaths Use UnmountMountPoint util to clean up subpaths Dec 28, 2018

@msau42 msau42 force-pushed the msau42:fix-subpath-nfs branch from cffda7c to 7a4f906 Jan 4, 2019

@jingxu97

This comment has been minimized.

Copy link
Contributor

jingxu97 commented Jan 4, 2019

/lgtm

@jingxu97

This comment has been minimized.

Copy link
Contributor

jingxu97 commented Jan 4, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 4, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jingxu97, 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

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Jan 4, 2019

/test pull-kubernetes-integration

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Jan 4, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 252a74d into kubernetes:master Jan 4, 2019

19 checks passed

cla/linuxfoundation msau42 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-100-performance 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-e2e-kubeadm-gce Skipped
pull-kubernetes-godeps Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big 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
tide In merge pool.
Details
klog.V(4).Infof("%q is unmounted, deleting the directory", mountPath)
return os.Remove(mountPath)
}
return fmt.Errorf("Failed to unmount path %v", mountPath)

This comment has been minimized.

@tedyu

tedyu Jan 7, 2019

The message here seems inaccurate : the path is likely mount point

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

Merge pull request #72583 from msau42/automated-cherry-pick-of-#71804…
…-upstream-release-1.13

Automated cherry pick of #71804: Move unmount volume util from pkg/volume/util to

k8s-ci-robot added a commit that referenced this pull request Jan 8, 2019

Merge pull request #72596 from msau42/automated-cherry-pick-of-#71804…
…-upstream-release-1.12

Automated cherry pick of #71804: Move unmount volume util from pkg/volume/util to

k8s-ci-robot added a commit that referenced this pull request Jan 12, 2019

Merge pull request #72601 from msau42/automated-cherry-pick-of-#71804…
…-upstream-release-1.11

Automated cherry pick of #71804: Move unmount volume util from pkg/volume/util to
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment