-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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
Local storage teardown fix #48402
Local storage teardown fix #48402
Conversation
/sig storage |
@k8s-ci-robot ok to test |
@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. |
looks good to me even with local self hosted storage, unmounts the volumes as it should back and forth. |
pkg/volume/util/util.go
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that lower log level should be used, it's part of a normal operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsafrane - would you like me to lower the log level for the previous statement L107 too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment that in particular this is can detect bind mounts of directories.
pkg/util/mount/mount.go
Outdated
if mountPointsErr != nil { | ||
return notMnt, mountPointsErr | ||
} | ||
deletedFile := fmt.Sprintf("%s\\040(deleted)", file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parsing is implementation specific. It should be done in the the mounter implementation instead.
@@ -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 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During mount we also call IsLikelyNotMountPoint. Need to also evaluate if that call needs to be updated too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified all references of IsLikelyNotMountPoint to use IsNotMountPoint inside local.go.
e8447ad
to
748db25
Compare
/retest |
cd8f0b9
to
6a246da
Compare
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pkg/util/mount/mount.go
Outdated
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
// UnmountMountPoint is a common unmount routine that unmounts the given path and | ||
// deletes the remaining directory if successful. | ||
// if extensiveMountPointCheck is true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please add comment about perf implications about enabling this option.
pkg/volume/util/util.go
Outdated
@@ -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 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Paging @pmorie since he's familiar with this some of the mounter code ( |
/lgtm |
Updated release note on behalf of @ianchakeres with link to bug being fixed. |
/test pull-kubernetes-e2e-kops-aws |
@saad-ali - thanks! Will create a cherrypick PR once this one is merged (but I don't have any more concerns). |
/test pull-kubernetes-e2e-kops-aws |
This PR #48762 should help with docker timeout in e2e-kops-aws. |
/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
9a39ee1
to
2b18d3b
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ianchakeres, msau42, saad-ali Assign the PR to them by writing 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:
You can indicate your approval by writing |
Automatic merge from submit-queue |
#48809 constains a cherrypick of it |
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. |
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
kubernetes/pkg/util/mount/mount.go
Line 46 in 4c5b22d
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: