Skip to content
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

bugfix(mount): lstat with abs path of parent instead of '/..' #58433

Conversation

yue9944882
Copy link
Member

What this PR does / why we need it:

If a nfs volume with improper permission is mounted on a Pod, operation of deleting this Pod will fail and the pod itself will be stuck at a 'TERMINATING' status. Kubelet cannot reconcile it correctly.

This is because kubelet will try to find the mount-point with '..' file which needs x permission of dir. When it's forbidden, the nfs volume will never umount without a correct mount-point finded.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #57095

Special notes for your reviewer:

Release note:

Get parent dir via canonical absolute path when trying to judge mount-point

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 18, 2018
@@ -265,7 +265,7 @@ func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) {
if err != nil {
return true, err
}
rootStat, err := os.Lstat(file + "/..")
rootStat, err := os.Lstat(filepath.Dir(strings.TrimSuffix("/")))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether trying to remove tailing slash here is necessary

@justinsb
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 21, 2018
@yue9944882 yue9944882 force-pushed the bugfix/lstat-parent-with-abs-path branch from d7e0377 to c398269 Compare January 21, 2018 15:09
@yue9944882
Copy link
Member Author

yue9944882 commented Jan 23, 2018

/kind bug
/sig node

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 23, 2018
@yue9944882
Copy link
Member Author

@jingxu97 @saad-ali PTAL thx

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Feb 12, 2018
@dashpole
Copy link
Contributor

/sig storage

@k8s-ci-robot k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Feb 12, 2018
@msau42
Copy link
Member

msau42 commented Feb 13, 2018

/assign

@msau42
Copy link
Member

msau42 commented Feb 13, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2018
@yue9944882
Copy link
Member Author

/test pull-kubernetes-e2e-kubeadm-gce-canary

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Feb 14, 2018

@yue9944882: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kubeadm-gce-canary c398269 link /test pull-kubernetes-e2e-kubeadm-gce-canary

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@yue9944882
Copy link
Member Author

@jingxu97 @saad-ali @jsafrane How do you think about approving this plz? :-)

@jsafrane
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, msau42, yue9944882

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 OWNERS Files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2018
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 59103, 58433). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-github-robot pushed a commit that referenced this pull request Mar 15, 2018
…33-upstream-release-1.8

Automatic merge from submit-queue.

Automated cherry pick of #58433: bugfix(mount): lstat with abs path of parent instead of '/..'

Cherry pick of #58433 on release-1.8.

#58433: bugfix(mount): lstat with abs path of parent instead of '/..'

```release-note
Fix a regression that prevented cleanup of a `subPath` volume mount
```
k8s-github-robot pushed a commit that referenced this pull request Mar 15, 2018
…33-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #58433: bugfix(mount): lstat with abs path of parent instead of '/..'

Cherry pick of #58433 on release-1.9.

#58433: bugfix(mount): lstat with abs path of parent instead of '/..'
Fixes: #61178

```release-note
Fix a regression that prevented cleanup of a `subPath` volume mount
```
k8s-github-robot pushed a commit that referenced this pull request Mar 15, 2018
…33-upstream-release-1.7

Automatic merge from submit-queue.

Automated cherry pick of #58433: bugfix(mount): lstat with abs path of parent instead of '/..'

Cherry pick of #58433 on release-1.7.

#58433: bugfix(mount): lstat with abs path of parent instead of '/..'

```release-note
Fix a regression that prevented cleanup of a `subPath` volume mount
```
k8s-github-robot pushed a commit that referenced this pull request Apr 13, 2018
Automatic merge from submit-queue (batch tested with PRs 61608, 62304). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove isNotDir error check

**What this PR does / why we need it**:
This check was supposed to handle the "subpath file" scenario, but:
1. It's wrong (should have been !)
2. It's not needed anymore. `IsLikelyNotMountPoint` was fixed to handle file mounts via #58433


**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
NONE
```
wgliang pushed a commit to wgliang/utils that referenced this pull request Jun 11, 2018
Automatic merge from submit-queue (batch tested with PRs 61608, 62304). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove isNotDir error check

**What this PR does / why we need it**:
This check was supposed to handle the "subpath file" scenario, but:
1. It's wrong (should have been !)
2. It's not needed anymore. `IsLikelyNotMountPoint` was fixed to handle file mounts via kubernetes/kubernetes#58433


**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
NONE
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pods created with an NFS volume with specific ownership get stuck in "Terminating" when deleted
9 participants