Fix subpath issues with orphaned pod cleanup #72291

msau42 · 2018-12-22T01:11:00Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Check for existence of volume-subpaths directory before cleaning up the pod directory

Which issue(s) this PR fixes:

Fixes #72257

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fixes issue where subpath volume content was deleted during orphaned pod cleanup for Local volumes that are directories (and not mount points) on the root filesystem.

msau42 · 2018-12-22T01:11:18Z

/assign @jingxu97 @saad-ali

msau42 · 2018-12-22T01:11:32Z

/priority critical-urgent

msau42 · 2018-12-22T01:12:04Z

pkg/util/mount/mount_linux.go

I opened up #72290 for refactoring and fixing these inter-pkg dependency issues

Shnatsel · 2018-12-24T12:24:14Z

pull-kubernetes-verify is flaky, which is tracked as #71394

/retest

jingxu97 · 2018-12-24T19:48:23Z

pkg/kubelet/kubelet_getters.go

modify the comments?

jingxu97 · 2018-12-24T19:58:20Z

pkg/kubelet/kubelet_volumes.go

In the above podVolumesExist() checking, it calls getMountedVolumePathListFromDisk() to check whether there are mounted volumes exist. And then it calls getPodVolumePathListFromDisk to check whether there are still volume paths exist. I think there are some redundancy here.

it might be redundant, but I don't want to remove it as part of this bug fix, as this code is not very well tested.

I'll leave it comment that this may be able to be cleaned up

Opened up #72346

saad-ali · 2018-12-26T17:59:33Z

pkg/kubelet/kubelet_getters.go

Per documentation:

// IsNotMountPoint is more expensive than IsLikelyNotMountPoint.

How frequently is getPodVolumeSubpathsDir(...) called? Want to be careful we don't introduce a performance regression.

I'm going to remove this commit. Changing to IsNotMountPoint is not strictly necessary to fix this particular bug.

But we should investigate other callers of these functions to see if this may be wrong in other contexts.

Opened up #72347 to investigate all uses of IsLikelyNotMountPoint

why is it necessary to call IsLikelyNotMountPoint function at all then? There's no protection that it won't end up removing data if mount is from the same device anyway (which was the case for the bug we hit), so if you are ok with that, it seems like calling the function just gives false sense of confidence.

IsLikelyNotMountPoint still works for most other volume types that are actually different devices.

The reason why I didn't want to change this function here is because it's being called in other paths as well, and we only wanted to do a targeted fix for this particular issue. We can do a more holistic investigation in #72346 and #72347 for further areas to cleanup.

saad-ali · 2018-12-26T19:11:20Z

/lgtm
/approve

saad-ali · 2018-12-26T19:14:09Z

/test pull-kubernetes-local-e2e-containerized

msau42 · 2018-12-26T21:17:40Z

/retest

msau42 · 2018-12-26T21:43:25Z

I manually tested this by:

Create a pod with a subpath volume
Create a directory and random file under /var/lib/kubelet/pods/poduid/volume-subpaths
Delete the pod
Verify the directories and files under 2) were not deleted, and that this log message is printed:

kubelet_volumes.go:154] Orphaned pod "5799cca2-0956-11e9-b2f6-42010a800002" found, but volume subpaths are still present on disk

msau42 · 2018-12-26T22:17:34Z

/retest

k8s-ci-robot · 2018-12-26T22:37:44Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-local-e2e-containerized	`3ebbbbd`	link	`/test pull-kubernetes-local-e2e-containerized`

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.

Details

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.

msau42 · 2018-12-26T23:59:47Z

/assign @Random-Liu
for kubelet review

Random-Liu · 2018-12-27T02:21:26Z

/approve

k8s-ci-robot · 2018-12-27T02:21:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, Random-Liu, saad-ali

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/kubelet/OWNERS~~ [Random-Liu]
~~pkg/util/mount/OWNERS~~ [saad-ali]

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

krmayankk · 2018-12-27T03:43:58Z

pkg/kubelet/kubelet_volumes.go

 			continue
 		}
+
+		// If there are any volume-subpaths, do not cleanup directories


i do not fully follow why we should not cleanup directories if volume-subpaths exists ? May be add some additional comments to explain that

It means that all the volumes have not been cleanly unmounted yet

…-upstream-release-1.12 Automated cherry pick of #72291: Check for volume-subpaths directory in orpahaned pod

…-upstream-release-1.13 Automated cherry pick of #72291: Check for volume-subpaths directory in orpahaned pod

…-upstream-release-1.11 Automated cherry pick of #72291: Check for volume-subpaths directory in orpahaned pod

k8s-ci-robot assigned jingxu97 and saad-ali Dec 22, 2018

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 22, 2018

k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 22, 2018

k8s-ci-robot requested review from saad-ali and smarterclayton December 22, 2018 01:11

msau42 commented Dec 22, 2018

View reviewed changes

pkg/util/mount/mount_linux.go Outdated

Copy link

Member Author

msau42 Dec 22, 2018

Choose a reason for hiding this comment

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

I opened up #72290 for refactoring and fixing these inter-pkg dependency issues

jingxu97 reviewed Dec 24, 2018

View reviewed changes

msau42 force-pushed the fix-subpath-orphan branch from 6bfc578 to 639278a Compare December 26, 2018 18:00

saad-ali reviewed Dec 26, 2018

View reviewed changes

msau42 mentioned this pull request Dec 26, 2018

Investigate cleanup of orphaned pod volumes #72346

Open

msau42 added 2 commits December 26, 2018 10:49

Check for volume-subpaths directory in orpahaned pod cleanup

8724b46

Leave refactoring TODO

3ebbbbd

msau42 force-pushed the fix-subpath-orphan branch from 639278a to 3ebbbbd Compare December 26, 2018 18:50

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

k8s-ci-robot assigned Random-Liu Dec 26, 2018

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

krmayankk reviewed Dec 27, 2018

View reviewed changes

k8s-ci-robot merged commit 68451f3 into kubernetes:master Dec 27, 2018

k8s-ci-robot added a commit that referenced this pull request Jan 5, 2019

Merge pull request #72380 from msau42/automated-cherry-pick-of-#72291…

8e672bc

…-upstream-release-1.12 Automated cherry pick of #72291: Check for volume-subpaths directory in orpahaned pod

k8s-ci-robot added a commit that referenced this pull request Jan 5, 2019

Merge pull request #72379 from msau42/automated-cherry-pick-of-#72291…

31c32d7

…-upstream-release-1.13 Automated cherry pick of #72291: Check for volume-subpaths directory in orpahaned pod

k8s-ci-robot added a commit that referenced this pull request Jan 17, 2019

Merge pull request #72381 from msau42/automated-cherry-pick-of-#72291…

97adb20

…-upstream-release-1.11 Automated cherry pick of #72291: Check for volume-subpaths directory in orpahaned pod

msau42 deleted the fix-subpath-orphan branch March 4, 2019 04:14

Fix subpath issues with orphaned pod cleanup #72291

Fix subpath issues with orphaned pod cleanup #72291

Uh oh!

Conversation

msau42 commented Dec 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msau42 commented Dec 22, 2018

Uh oh!

msau42 commented Dec 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shnatsel commented Dec 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saad-ali commented Dec 26, 2018

Uh oh!

saad-ali commented Dec 26, 2018

Uh oh!

msau42 commented Dec 26, 2018

Uh oh!

msau42 commented Dec 26, 2018

Uh oh!

msau42 commented Dec 26, 2018

Uh oh!

k8s-ci-robot commented Dec 26, 2018

Uh oh!

msau42 commented Dec 26, 2018

Uh oh!

Random-Liu commented Dec 27, 2018

Uh oh!

k8s-ci-robot commented Dec 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

msau42 commented Dec 22, 2018 •

edited

Loading