Fix: Pod terminating stuck because of trying to umount not actual mounted dir

[mochizuki875]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Fix the problem Pod terminating stuck because of trying to umount not actual mounted dir.
In addition, I refactored releted methods and functions.

Which issue(s) this PR fixes:

Fixes #114546

Special notes for your reviewer:

NONE

Does this PR introduce a user-facing change?

Fix the problem Pod terminating stuck because of trying to umount not actual mounted dir.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

[k8s-ci-robot]

Welcome @mochizuki875!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @mochizuki875. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mochizuki875]

/sig storage

[msau42]

/ok-to-test

[mochizuki875]

Hi @andyzhangx , thank you for fixing the issue #114736.
Actually, I've come accross the same issue using In-Tree NFS Volume Plugin(#114546) and try to fix it in this PR.
This PR has the same fixes as your PR, with the addition of using common variable names and logic for Unmount, tryUnmount, and forceUmount.
So could you please check this PR?

[mochizuki875]

/sig node

[andyzhangx]

Hi @andyzhangx , thank you for fixing the issue #114736. Actually, I've come accross the same issue using In-Tree NFS Volume Plugin(#114546) and try to fix it in this PR. This PR has the same fixes as your PR, with the addition of using common variable names and logic for Unmount, tryUnmount, and forceUmount. So could you please check this PR?

@mochizuki875 could you point out which line of code fix the original nfs unmount stuck issue in this PR? thanks.

[mochizuki875]

@andyzhangx
Thank you for your reply!
When Pod is terminated, NFS VolumePlugin calls CleanupMountWithForce.

kubernetes/pkg/volume/nfs/nfs.go

Line 294 in 0b2e541

return mount.CleanupMountWithForce(dir, forceUnmounter, true /* extensiveMountPointCheck */, unMountTimeout)

And then, calls UnmountWithForce.

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

Line 58 in 0b2e541

if err := mounter.UnmountWithForce(mountPath, umountTimeout); err != nil {

Finally, tryUnmount and forceUmount are called.
https://github.com/kubernetes/kubernetes/blob/891eb75b8956c94c59dbfab29e78f12863202088/staging/src/k8s.io/mount-utils/mount_linux.go#L373-L383

Before your PR(#114736), there was not the check logic of umount result in tryUnmount and forceUmount, so an error occured and kubelet could not finish the terminating proccess of Pod if NFS volume is not mounted to Pod.(In this case, umount command returns "not mounted".)
So I added checkUmountError function as a common logic to check a result of umount command.
https://github.com/kubernetes/kubernetes/blob/891eb75b8956c94c59dbfab29e78f12863202088/staging/src/k8s.io/mount-utils/mount_linux.go#L796-L811

And I implemented checkUmountError to Unmount, tryUnmount, and forceUmount because they need same logic to check a result of umount command.

https://github.com/kubernetes/kubernetes/blob/891eb75b8956c94c59dbfab29e78f12863202088/staging/src/k8s.io/mount-utils/mount_linux.go#L361-L369
https://github.com/kubernetes/kubernetes/blob/891eb75b8956c94c59dbfab29e78f12863202088/staging/src/k8s.io/mount-utils/mount_linux.go#L766-L794

Also, although it is a small detail, the names of variables that indicate the same meaning were different between Unmount, tryUnmount, and forceUmount(ex. target and path, command and cmd), so I corrected to common names.

[bart0sh]

/priority important-soon
/triage accepted

[liggitt]

This breaks running unit tests without superuser mode

Running unit tests downstream, this is the only failing unit test:

{Failed  === RUN   TestNotMountedBehaviorOfUnmount
    mount_linux_test.go:719: Expect complete Unmount(), but it dose not: unmount failed: exit status 32
        Unmounting arguments: /tmp/kubelet-umount2687109515
        Output: umount: /tmp/kubelet-umount2687109515: must be superuser to unmount.
    mount_linux_test.go:723: Expect complete tryUnmount(), but it does not: unmount failed: exit status 32
        Unmounting arguments: /tmp/kubelet-umount2687109515
        Output: umount: /tmp/kubelet-umount2687109515: must be superuser to unmount.
    mount_linux_test.go:729: Expect complete forceUnmount(), but it does not: unmount failed: exit status 32
        Unmounting arguments: /tmp/kubelet-umount2687109515
        Output: umount: /tmp/kubelet-umount2687109515: must be superuser to unmount.
--- FAIL: TestNotMountedBehaviorOfUnmount (0.03s)

[jsafrane]

Opened a revert: #115732

[saurav-agarwalla]

Are there any plans to backport this to older Kubernetes versions? This seems like a critical fix.

[humblec]

@ljosyula considering you have filed the PRs for 1.25 and 1.26, can you file one for 1.24 as well?