Local storage teardown fix #48402

ianchakeres · 2017-07-02T18:43:56Z

What this PR does / why we need it: Local storage uses bindmounts and the method IsLikelyNotMountPoint does not detect these as mountpoints. Therefore, local PVs are not properly unmounted when they are deleted.

Which issue this PR fixes: fixes #48331

Special notes for your reviewer:

You can use these e2e tests to reproduce the issue and validate the fix works appropriately #47999

The existing method IsLikelyNotMountPoint purposely does not check mountpoints reliability (

kubernetes/pkg/util/mount/mount_linux.go

Line 161 in 4c5b22d

// IsLikelyNotMountPoint determines if a directory is not a mountpoint.

), since the number of mountpoints can be large.

kubernetes/pkg/util/mount/mount.go

Line 46 in 4c5b22d

List() ([]MountPoint, error)

This implementation changes the behavior for local storage to detect mountpoints reliably, and avoids changing the behavior for any other callers to a UnmountPath.

Release note:

Fixes bind-mount teardown failure with non-mount point Local volumes (issue https://github.com/kubernetes/kubernetes/issues/48331).

ianchakeres · 2017-07-02T18:44:09Z

/sig storage

ianchakeres · 2017-07-02T18:44:20Z

@k8s-ci-robot ok to test

ianchakeres · 2017-07-02T18:46:24Z

@CallMeFoxie @msau42 @saad-ali - Here is the PR to fix #48331. Please take a look and let me know if you have any questions or comments.

CallMeFoxie · 2017-07-03T06:10:36Z

I0703 08:07:44.466131 22258 operation_generator.go:523] UnmountVolume.TearDown succeeded for volume "kubernetes.io/secret/ae9f9214-5fb5-11e7-a549-180373f4ecc0-default-token-mhn97" (OuterVolumeSpecName: "default-token-mhn97") pod "ae9f9214-5fb5-11e7-a549-180373f4ecc0" (UID: "ae9f9214-5fb5-11e7-a549-180373f4ecc0"). InnerVolumeSpecName "default-token-mhn97". PluginName "kubernetes.io/secret", VolumeGidValue ""
I0703 08:07:44.467018 22258 operation_generator.go:523] UnmountVolume.TearDown succeeded for volume "kubernetes.io/local-volume/ae9f9214-5fb5-11e7-a549-180373f4ecc0-test-affinity-test-1" (OuterVolumeSpecName: "test") pod "ae9f9214-5fb5-11e7-a549-180373f4ecc0" (UID: "ae9f9214-5fb5-11e7-a549-180373f4ecc0"). InnerVolumeSpecName "test-affinity-test-1". PluginName "kubernetes.io/local-volume", VolumeGidValue ""

looks good to me even with local self hosted storage, unmounts the volumes as it should back and forth.
cc @msau42 @saad-ali @jsafrane

jsafrane · 2017-07-03T10:11:22Z

pkg/volume/util/util.go

 	if notMnt {
 		glog.Warningf("Warning: %q is not a mountpoint, deleting", mountPath)
 		return os.Remove(mountPath)
 	}

 	// Unmount the mount path
+	glog.Warningf("Warning: %q is a mountpoint, unmounting", mountPath)


I think that lower log level should be used, it's part of a normal operation.

@jsafrane - would you like me to lower the log level for the previous statement L107 too?

No, at 107 something else happened - something unmounted a fs and left a stray directory there, e.g. kubelet crashed after unmount and before os.Remove. It's quite exceptional event.

Here on line 112 it's just normal operation, nothing suspicious.

jsafrane · 2017-07-03T15:21:50Z

lgtm to me, however I let @msau42 or @rootfs say the final word.

msau42 · 2017-07-04T22:45:54Z

pkg/util/mount/mount.go

+
+// IsNotMountPoint determines if a directory is a mountpoint.
+// It should return ErrNotExist when the directory does not exist.
+// This method uses the List() of all mountpoints


Add a comment that in particular this is can detect bind mounts of directories.

msau42 · 2017-07-04T22:47:31Z

pkg/util/mount/mount.go

+	if mountPointsErr != nil {
+		return notMnt, mountPointsErr
+	}
+	deletedFile := fmt.Sprintf("%s\\040(deleted)", file)


This parsing is implementation specific. It should be done in the the mounter implementation instead.

msau42 · 2017-07-04T22:50:47Z

pkg/volume/local/local.go

@@ -269,5 +269,5 @@ func (u *localVolumeUnmounter) TearDown() error {
 // TearDownAt unmounts the bind mount
 func (u *localVolumeUnmounter) TearDownAt(dir string) error {
 	glog.V(4).Infof("Unmounting volume %q at path %q\n", u.volName, dir)
-	return util.UnmountPath(dir, u.mounter)
+	return util.UnmountMountPoint(dir, u.mounter, true) /* extensiveMountPointCheck = true */


During mount we also call IsLikelyNotMountPoint. Need to also evaluate if that call needs to be updated too.

I modified all references of IsLikelyNotMountPoint to use IsNotMountPoint inside local.go.

ianchakeres · 2017-07-05T15:31:17Z

/retest

saad-ali · 2017-07-05T16:52:52Z

pkg/util/mount/mount.go

+// It should return ErrNotExist when the directory does not exist.
+// This method uses the List() of all mountpoints
+// It is more extensive than IsLikelyNotMountPoint
+// and it detects bind mounts in linux


Also add a comment indicating that compared to IsLikelyNotMountPoint(...), this will have a higher perf impact if the machine has a lot of mounts (since it lists all the mounts), and should not be used in critical paths unless absolutely necessary.

saad-ali · 2017-07-05T16:54:59Z

pkg/util/mount/mount.go

+	// IsNotMountPoint is more extensive than IsLikelyNotMountPoint
+	// It uses IsMountPointMatch to detect matching dir in List()
+	// It detects bind mounts in linux
+	IsNotMountPoint(file string) (bool, error)
 	// IsLikelyNotMountPoint determines if a directory is a mountpoint.
 	// It should return ErrNotExist when the directory does not exist.


The comment on the existing method sucks. Now that we're adding a similar method, let's update both to make it very crisp and clear 1) what each does, 2) what the differences between them are, and 3) when a caller should use one vs the other, and 4) what the pitfalls of each are.

saad-ali · 2017-07-05T17:07:53Z

pkg/volume/util/util.go

+
+// UnmountMountPoint is a common unmount routine that unmounts the given path and
+// deletes the remaining directory if successful.
+// if extensiveMountPointCheck is true


Also please add comment about perf implications about enabling this option.

saad-ali · 2017-07-05T17:09:27Z

pkg/volume/util/util.go

@@ -75,23 +75,41 @@ func SetReady(dir string) {
 // UnmountPath is a common unmount routine that unmounts the given path and
 // deletes the remaining directory if successful.
 func UnmountPath(mountPath string, mounter mount.Interface) error {
+	return UnmountMountPoint(mountPath, mounter, false) /* extensiveMountPointCheck = false */


Super nit:

I prefer

UnmountMountPoint(mountPath, mounter, false /* extensiveMountPointCheck */)

since it's less verbose and doesn't require you to update the value and comment if you change the value.

saad-ali · 2017-07-05T17:10:58Z

Paging @pmorie since he's familiar with this some of the mounter code (nsenter_mount in particular).

msau42 · 2017-07-10T18:29:38Z

/lgtm

saad-ali · 2017-07-10T18:30:43Z

Also, please add more descriptive release note (explain what exact bug this is fixing).

Updated release note on behalf of @ianchakeres with link to bug being fixed.

saad-ali · 2017-07-10T21:56:21Z

/test pull-kubernetes-e2e-kops-aws

wojtek-t · 2017-07-11T06:38:52Z

@saad-ali - thanks! Will create a cherrypick PR once this one is merged (but I don't have any more concerns).

ianchakeres · 2017-07-11T11:32:08Z

/test pull-kubernetes-e2e-kops-aws

ianchakeres · 2017-07-11T11:34:25Z

@saad-ali - I'm hoping that #48403 will fix pull-kubernetes-e2e-kops-aws errors we're seeing.

ianchakeres · 2017-07-11T14:59:32Z

This PR #48762 should help with docker timeout in e2e-kops-aws.

fejta · 2017-07-11T20:06:50Z

/test pull-kubernetes-e2e-kops-aws

Added IsNotMountPoint method to mount utils (pkg/util/mount/mount.go) Added UnmountMountPoint method to volume utils (pkg/volume/util/util.go) Call UnmountMountPoint method from local storage (pkg/volume/local/local.go) IsLikelyNotMountPoint behavior was not modified, so the logic/behavior for UnmountPath is not modified

ianchakeres · 2017-07-11T21:25:06Z

@saad-ali @msau42 - I just squashed the commits.

msau42 · 2017-07-11T21:29:08Z

/lgtm

k8s-github-robot · 2017-07-11T21:29:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ianchakeres, msau42, saad-ali
We suggest the following additional approver: smarterclayton

Assign the PR to them by writing /assign @smarterclayton in a comment when ready.

Associated issue: 48331

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

Needs approval from an approver in each of these OWNERS Files:

pkg/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-07-12T03:35:29Z

Automatic merge from submit-queue

wojtek-t · 2017-07-12T12:38:35Z

#48809 constains a cherrypick of it

#48524-upstream-release-1.7 Automatic merge from submit-queue Automated cherry pick of #48402 #48524 upstream release 1.7 Cherry pick of #48524 and #48402 on release-1.7. #48524 : fix udp service blackhole problem when number of backends changes from 0 to non-0 #48402 : Local storage teardown fix

k8s-cherrypick-bot · 2017-07-18T08:13:51Z

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 2, 2017

k8s-github-robot assigned msau42 and rootfs Jul 2, 2017

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 2, 2017

k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 2, 2017

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jul 2, 2017

jsafrane reviewed Jul 3, 2017

View reviewed changes

jsafrane added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 3, 2017

ianchakeres mentioned this pull request Jul 3, 2017

LocalPV e2e test with two pods writing/reading to a shared file on one PVC #47999

Merged

msau42 reviewed Jul 4, 2017

View reviewed changes

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 5, 2017

ianchakeres force-pushed the local-storage-teardown-fix branch from e8447ad to 748db25 Compare July 5, 2017 15:15

ianchakeres force-pushed the local-storage-teardown-fix branch 5 times, most recently from cd8f0b9 to 6a246da Compare July 5, 2017 16:38

saad-ali reviewed Jul 5, 2017

View reviewed changes

saad-ali requested a review from pmorie July 5, 2017 17:11

saad-ali added the cherrypick-candidate label Jul 5, 2017

saad-ali added this to the v1.7 milestone Jul 5, 2017

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

saad-ali added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2017

ianchakeres force-pushed the local-storage-teardown-fix branch from 9a39ee1 to 2b18d3b Compare July 11, 2017 21:24

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 11, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 11, 2017

k8s-github-robot merged commit 03360d7 into kubernetes:master Jul 12, 2017

wojtek-t added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jul 12, 2017

wojtek-t mentioned this pull request Jul 12, 2017

Automated cherry pick of #48402 #48524 upstream release 1.7 #48809

Merged

k8s-cherrypick-bot removed the cherrypick-candidate label Jul 18, 2017

msau42 mentioned this pull request Jul 20, 2017

Allow unmounting bind-mounted directories. #49118

Merged

This was referenced Jul 25, 2017

isLikelyNotMountPoint() function needs refactored #26996

Closed

Have Cinder mount tasks use new kubelet functions #49786

Closed

msau42 mentioned this pull request Jul 28, 2017

Cache mounts on the host #49793

Open

ianchakeres mentioned this pull request Sep 19, 2017

Fix volume remount on reboot #51985

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local storage teardown fix #48402

Local storage teardown fix #48402

ianchakeres commented Jul 2, 2017 •

edited by saad-ali

ianchakeres commented Jul 2, 2017

ianchakeres commented Jul 2, 2017

ianchakeres commented Jul 2, 2017

CallMeFoxie commented Jul 3, 2017

jsafrane Jul 3, 2017

ianchakeres Jul 3, 2017

jsafrane Jul 3, 2017

jsafrane commented Jul 3, 2017

msau42 Jul 4, 2017

msau42 Jul 4, 2017

msau42 Jul 4, 2017

ianchakeres Jul 5, 2017

ianchakeres commented Jul 5, 2017

saad-ali Jul 5, 2017

saad-ali Jul 5, 2017

saad-ali Jul 5, 2017

saad-ali Jul 5, 2017

saad-ali commented Jul 5, 2017 •

edited

msau42 commented Jul 10, 2017

saad-ali commented Jul 10, 2017

saad-ali commented Jul 10, 2017

wojtek-t commented Jul 11, 2017

ianchakeres commented Jul 11, 2017

ianchakeres commented Jul 11, 2017

ianchakeres commented Jul 11, 2017

fejta commented Jul 11, 2017

ianchakeres commented Jul 11, 2017

msau42 commented Jul 11, 2017

k8s-github-robot commented Jul 11, 2017

k8s-github-robot commented Jul 12, 2017

wojtek-t commented Jul 12, 2017

k8s-cherrypick-bot commented Jul 18, 2017

Local storage teardown fix #48402

Local storage teardown fix #48402

Conversation

ianchakeres commented Jul 2, 2017 • edited by saad-ali

ianchakeres commented Jul 2, 2017

ianchakeres commented Jul 2, 2017

ianchakeres commented Jul 2, 2017

CallMeFoxie commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianchakeres commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Jul 5, 2017 • edited

msau42 commented Jul 10, 2017

saad-ali commented Jul 10, 2017

saad-ali commented Jul 10, 2017

wojtek-t commented Jul 11, 2017

ianchakeres commented Jul 11, 2017

ianchakeres commented Jul 11, 2017

ianchakeres commented Jul 11, 2017

fejta commented Jul 11, 2017

ianchakeres commented Jul 11, 2017

msau42 commented Jul 11, 2017

k8s-github-robot commented Jul 11, 2017

k8s-github-robot commented Jul 12, 2017

wojtek-t commented Jul 12, 2017

k8s-cherrypick-bot commented Jul 18, 2017

ianchakeres commented Jul 2, 2017 •

edited by saad-ali

saad-ali commented Jul 5, 2017 •

edited