Use force umount for nfs volumes #96844

gnufied · 2020-11-25T02:34:54Z

/sig storage

Use force unmount for NFS volumes if regular mount fails after 1 minute timeout

gnufied · 2020-11-25T16:04:46Z

/retest

jingxu97 · 2020-11-25T17:26:10Z

One concern is unmount timeout might caused by different reasons, e.g.,

NFS server is down (lost network connection with server)
Some other process are using this dir

In case the 1, the recommended way is to use unmount with force. But how about the second case?

Normally the volume is only used by associated Pods. The workflow is to kill the containers which are using the volume, and then cleanup volumes. So in this aspect, maybe case 2 should not happen?

jsafrane · 2020-11-26T17:55:29Z

Some other process are using this dir

It is not possible to unmount a directory if something uses it, even with -f. You still get device is busy.

jingxu97 · 2020-11-27T19:37:17Z

pkg/volume/nfs/nfs.go

@@ -61,7 +62,8 @@ var _ volume.PersistentVolumePlugin = &nfsPlugin{}
 var _ volume.RecyclableVolumePlugin = &nfsPlugin{}

 const (
-	nfsPluginName = "kubernetes.io/nfs"
+	nfsPluginName  = "kubernetes.io/nfs"
+	unMountTimeout = time.Minute


just want to double check whether this is optimal timeout setting in this case..

there is no 100% sure way to know the answer. We have deployed some CSI driver with this value and it appears to be working fine. Unlike block devices - NFS volumes don't have to fsync on umount, so I think it should be fine.

jingxu97 · 2020-11-27T19:44:25Z

staging/src/k8s.io/mount-utils/mount_helper_common.go

+		klog.Warningf("Warning: Unmount skipped because path does not exist: %v", mountPath)
+		return nil
+	}
+	corruptedMnt := IsCorruptedMnt(pathErr)


At this point, pathExists == true. Based on the logic of PathExists, only two cases

pathExists and error == nil

if error != nil, then is must be corruptedMnt == true

So I think there is no need to check IsCorruptedMnt again here ..

hmm, good point. but I think there is a logic error in this code.

If pathExists is false and pathErr is nil. then and then only it should skip unmounting.

If pathExists is false but pathErr is somehow non-nil, then unmounting should not succeed and should result in error and I think we have had this bug in the code for awhile now. :(

I have pushed an update that fixes this PTAL.

Still feel there are some cleanup can be done related to the checking before and after actual unmount execution and path removal. Especially the checking logic before actually executing unmount or dir deletion has some issues. We have seen issues around this with some kind of corrupted dir (even though dir still exist, but PathExists show it does not..). Also since pathExists has redundant check on IsCorruptedMnt.

But this further cleanup can be done in a different PR. For this one, let's focus on adding Force option.

Why do you say - PathExists has a redundant check of IsCorruptedMnt? But if it was - then I think it was always there and I am not changing any of that code. If anything - this PR I think fixes the bug with ignoring errors from PathExists check.

got it. for this change it is good for me.

gnufied · 2020-12-15T15:37:54Z

/triage accepted

gnufied · 2020-12-15T17:19:56Z

/kind bug

gnufied · 2020-12-15T17:22:37Z

/assign @jingxu97

jingxu97 · 2020-11-27T20:08:01Z

staging/src/k8s.io/mount-utils/mount_helper_common.go

+	var notMnt bool
+	var err error
+	if !corruptedMnt {
+		_, err = removePathIfNotMountPoint(mountPath, mounter, extensiveMountPointCheck)


here as long as pathErr != nil (previously already check pathExists == true) I think you can use removePathIfNotMountPoint

Can you update this comment. You have commented on a outdated diff and I have changed this code.

gnufied · 2020-12-15T20:06:53Z

/priority important-soon

jingxu97 · 2020-12-16T04:48:00Z

CleanupMountWithForce and CleanupMountPoint should have the exact logic except the former uses mounter.UnmountWithForce and latter uses mounter.Unmount() function. So do you think passing a parameter option e.g. "bool useForceUnmount" passed to CleanupMountPoint() function is easier?

gnufied · 2020-12-16T15:36:59Z

CleanupMountPoint function is part of public interface of mount utils package. It is used in many cases in k8s and potentially outside core k8s. I did not wanted to break any existing usage if I could help. But also it is just one boolean flag, we also have to potentially supply timeout. I think it is bit more cleaner to have separate function for this, so as we don't break existing usage.

gnufied · 2020-12-17T20:56:45Z

/retest

jingxu97 · 2020-12-18T04:22:54Z

staging/src/k8s.io/mount-utils/mount_linux.go

+}
+
+func forceUmount(path string) error {
+	cmd := exec.Command("umount", "-f", path)


so should we consider to have a timeout even for force unmount?

the same question for normal Unmount function

the same question for normal Unmount function.

For normal unmount it is bit problematic to change behaviour for all existing drivers. As we know that some storage types flush pending writes when a volume is unmounted - and hence unmount time could vary and it is not safe to kill unmount process when this happens. So I think we never want to use a universal timeout for normal unmount function for all drivers.

so should we consider to have a timeout even for force unmount?

force works best for unreachable NFS system. It is documented directly in the man page. But it is not guaranteed to not hang and as far as I know if it hangs in other cases, it could be hanging for a legitimate reason (such as volume being used or something else) and in those cases using a timeout will basically kill the unmount process and it could be problematic. So, I think I would not want to use a timeout for force unmount. It could result in a leaked goroutine but that is okay (it is okay because we are not going to retry unmount if previous unmount is hung), we always leaked goroutines in this code path.

jingxu97 · 2020-12-18T04:24:40Z

just a coupe of questions which can be followed up later.

This change is good for me.
/lgtm

jingxu97 · 2020-12-18T16:41:15Z

/approve

k8s-ci-robot · 2020-12-18T16:41:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, jingxu97

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:

~~pkg/volume/nfs/OWNERS~~ [gnufied,jingxu97]
~~staging/src/k8s.io/mount-utils/OWNERS~~ [jingxu97]

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

gnufied · 2020-12-18T19:08:14Z

/retest

k8s-ci-robot requested review from jingxu97 and msau42 November 25, 2020 02:35

gnufied force-pushed the use-force-unmount branch from b3eea65 to 4f3cf33 Compare November 25, 2020 04:26

jingxu97 reviewed Nov 27, 2020

View reviewed changes

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 15, 2020

gnufied added 2 commits December 15, 2020 11:50

Use force umount for nfs volumes

379daa6

Only skip unmount if path does not exist and there is no error

95852d7

gnufied force-pushed the use-force-unmount branch from 3fbc7f5 to 95852d7 Compare December 15, 2020 17:04

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 15, 2020

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Dec 15, 2020

k8s-ci-robot assigned jingxu97 Dec 15, 2020

jingxu97 reviewed Dec 15, 2020

View reviewed changes

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 15, 2020

jingxu97 reviewed Dec 18, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 18, 2020

k8s-ci-robot merged commit b538d23 into kubernetes:master Dec 18, 2020

k8s-ci-robot added this to the v1.21 milestone Dec 18, 2020

github-actions bot mentioned this pull request Jan 5, 2021

Week Ending January 3, 2021 dev-obs/actus#313

Open

This was referenced Apr 14, 2021

Automated cherry pick of #96844: Use force umount for nfs volumes #101121

Closed

Automated cherry pick of #96844: Use force umount for nfs volumes #101122

Closed

This was referenced Dec 29, 2022

fix: use force unmount to fix unmount NFS volume stuck issue kubernetes-csi/csi-driver-nfs#394

Merged

fix: tryUnmount should respect mounter.withSafeNotMountedBehavior #114736

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use force umount for nfs volumes #96844

Use force umount for nfs volumes #96844

gnufied commented Nov 25, 2020 •

edited

gnufied commented Nov 25, 2020

jingxu97 commented Nov 25, 2020

jsafrane commented Nov 26, 2020

jingxu97 Nov 27, 2020

gnufied Dec 15, 2020

jingxu97 Nov 27, 2020

gnufied Dec 15, 2020 •

edited

jingxu97 Dec 16, 2020

gnufied Dec 17, 2020

jingxu97 Dec 18, 2020

gnufied commented Dec 15, 2020

gnufied commented Dec 15, 2020

gnufied commented Dec 15, 2020

jingxu97 Nov 27, 2020

gnufied Dec 15, 2020

gnufied commented Dec 15, 2020

jingxu97 commented Dec 16, 2020

gnufied commented Dec 16, 2020

gnufied commented Dec 17, 2020

jingxu97 Dec 18, 2020

jingxu97 Dec 18, 2020

gnufied Dec 18, 2020 •

edited

jingxu97 commented Dec 18, 2020

jingxu97 commented Dec 18, 2020

k8s-ci-robot commented Dec 18, 2020

gnufied commented Dec 18, 2020

Use force umount for nfs volumes #96844

Use force umount for nfs volumes #96844

Conversation

gnufied commented Nov 25, 2020 • edited

gnufied commented Nov 25, 2020

jingxu97 commented Nov 25, 2020

jsafrane commented Nov 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied Dec 15, 2020 • edited

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

gnufied commented Dec 15, 2020

gnufied commented Dec 15, 2020

gnufied commented Dec 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied commented Dec 15, 2020

jingxu97 commented Dec 16, 2020

gnufied commented Dec 16, 2020

gnufied commented Dec 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied Dec 18, 2020 • edited

Choose a reason for hiding this comment

jingxu97 commented Dec 18, 2020

jingxu97 commented Dec 18, 2020

k8s-ci-robot commented Dec 18, 2020

gnufied commented Dec 18, 2020

gnufied commented Nov 25, 2020 •

edited

gnufied Dec 15, 2020 •

edited

gnufied Dec 18, 2020 •

edited