kubelet: do not call RemoveAll on volumes directory for orphaned pods #102576

dobsonj · 2021-06-03T21:27:47Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

See issue #101911 for the full story. It is possible in some situations for the volume of a terminating pod to be added back to the DSW after being removed, because pods are currently added and removed based on two different caches. As a result, the orphaned volume cleanup code can call RemoveAll on the pod dir, which deletes vol_data.json, and causes NodeUnstageVolume to fail when attempting to unmount the volume.

To avoid this issue, this PR instead uses rmdir on the volumes dir during orphaned volume cleanup which will fail if any files are left in the volumes directory. The other subpaths under the pod directory CAN have files that need to be removed during orphan cleanup, so those continue to use RemoveAll.

Here is an example pod directory that needs to be cleaned up:

/var/lib/kubelet/pods/de33b3cd-9542-4245-b20b-892204fa49f5:
containers  etc-hosts  plugins  volumes

This PR recursively calls rmdir on the volumes directory which will be successful only if there are no files or mounts left behind. Then it calls RemoveAll on the other subpaths (containers, etc-hosts, plugins). And if both of those are successful, it calls rmdir on the pod directory itself.

Which issue(s) this PR fixes:

Fixes #101911

Special notes for your reviewer:

cc: @msau42 @gnufied @jingxu97

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2021-06-03T21:27:55Z

Hi @dobsonj. 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.

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.

msau42 · 2021-06-03T21:29:44Z

/ok-to-test
/assign @gnufied @jingxu97

dobsonj · 2021-06-04T14:07:24Z

/test all
They passed once, and then didn't run again after I removed the merge commit.

dobsonj · 2021-06-04T17:29:13Z

/retest

gnufied · 2021-06-04T17:56:25Z

pkg/util/removeall/removeall.go

nit: I would probably move fd.Close in a defer after opening the file and and assumning no errors were thrown.

This looks like a deliberate decision in the original implementation. The comment right above that (line 96) says:

// Close directory, because windows won't remove opened directory.

if we were to defer instead, the fd would be closed after the remove() call, which could introduce issues on Windows systems.

gnufied

Is there a way we can add some unit tests for this?

dobsonj · 2021-06-07T18:30:47Z

Is there a way we can add some unit tests for this?

Yes, I added one new test case for kubelet_volumes (where cleanupOrphanedPodDirs is tested), along with a new set of tests for RemoveDirsOneFilesystem.

dobsonj · 2021-06-07T19:44:43Z

/test all

dobsonj · 2021-06-21T16:10:21Z

@liggitt @ehashman any other feedback on this PR? I'd like to move it forward if I've addressed your comments.

gnufied · 2021-06-21T17:15:15Z

/lgtm

liggitt · 2021-06-23T04:24:47Z

I'll defer to node/storage approvers on this one. Once this has approval from them, I can ack if needed to get the new OWNERS file in place.

gnufied · 2021-06-23T04:36:19Z

/approve

ehashman

I'm not very familiar with this code but the description, test, and implementation in pkg/kubelet/kubelet_volumes.go LGTM with SIG Storage's 👍

ehashman · 2021-06-28T23:27:46Z

pkg/kubelet/kubelet_volumes.go

Error condition looks good. See also https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#change-log-functions-to-structured-equivalent for there not being "WarningS" equivalents.

mrunalp · 2021-06-29T03:57:31Z

/approve

liggitt · 2021-06-29T04:16:43Z

/approve

k8s-ci-robot · 2021-06-29T04:17:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dobsonj, gnufied, liggitt, mrunalp

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/OWNERS~~ [liggitt]
~~test/e2e/storage/OWNERS~~ [gnufied,liggitt]

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

fejta-bot · 2021-06-29T07:39:22Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2021-06-29T11:19:23Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

…576-upstream-release-1.20 Automated cherry pick of #102576: kubelet: do not call RemoveAll on volumes directory for

…576-upstream-release-1.21 Automated cherry pick of #102576: kubelet: do not call RemoveAll on volumes directory for

…576-upstream-release-1.19 Automated cherry pick of #102576: kubelet: do not call RemoveAll on volumes directory for

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. labels Jun 3, 2021

k8s-ci-robot requested review from Random-Liu and brendandburns June 3, 2021 21:28

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 3, 2021

k8s-ci-robot assigned gnufied and jingxu97 Jun 3, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 3, 2021

dobsonj force-pushed the 101911 branch from ab0130e to d0c7bb2 Compare June 3, 2021 22:44

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Jun 3, 2021

gnufied reviewed Jun 4, 2021

View reviewed changes

dobsonj marked this pull request as ready for review June 7, 2021 19:47

update comments and owners file for pkg/util/removeall

01bb0f8

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2021

dobsonj mentioned this pull request Jun 21, 2021

REQUEST: New membership for dobsonj kubernetes/org#2799

Closed

6 tasks

ehashman reviewed Jun 28, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2021

k8s-ci-robot merged commit 756203f into kubernetes:master Jun 29, 2021

k8s-ci-robot added this to the v1.22 milestone Jun 29, 2021

dobsonj deleted the 101911 branch July 22, 2021 22:56

msau42 mentioned this pull request Aug 2, 2021

kubelet fails to unmount CSI volumes, floods log with errors and has to be restarted #101378

Closed

k8s-ci-robot added a commit that referenced this pull request Aug 6, 2021

Merge pull request #103828 from dobsonj/automated-cherry-pick-of-#102…

1faeb9d

…576-upstream-release-1.20 Automated cherry pick of #102576: kubelet: do not call RemoveAll on volumes directory for

k8s-ci-robot added a commit that referenced this pull request Aug 6, 2021

Merge pull request #103826 from dobsonj/automated-cherry-pick-of-#102…

801fefe

…576-upstream-release-1.21 Automated cherry pick of #102576: kubelet: do not call RemoveAll on volumes directory for

k8s-ci-robot added a commit that referenced this pull request Aug 6, 2021

Merge pull request #103859 from dobsonj/automated-cherry-pick-of-#102…

f889896

…576-upstream-release-1.19 Automated cherry pick of #102576: kubelet: do not call RemoveAll on volumes directory for

fgimenez mentioned this pull request Aug 12, 2021

Bump k8s providers kubevirt/kubevirtci#661

Merged

mmerkes mentioned this pull request Oct 11, 2021

Updates 1.19+ kubernetes build date in Makefile awslabs/amazon-eks-ami#781

Merged

Praveenrajmani mentioned this pull request Mar 21, 2022

livenessprobe Connection to unix:///csi/csi.sock minio/directpv#550

Closed

nikhiljha mentioned this pull request Aug 16, 2022

recursively clean up orphaned volumes #111862

Closed

vadasambar mentioned this pull request Mar 22, 2023

kubelet fails with UnmountVolume.NewUnmounter failed for volume and vol_data.json: no such file or directory for CSI volumes and floods the logs #116847

Closed

kubelet: do not call RemoveAll on volumes directory for orphaned pods #102576

kubelet: do not call RemoveAll on volumes directory for orphaned pods #102576

Uh oh!

Conversation

dobsonj commented Jun 3, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

k8s-ci-robot commented Jun 3, 2021

Uh oh!

msau42 commented Jun 3, 2021

Uh oh!

dobsonj commented Jun 4, 2021

Uh oh!

dobsonj commented Jun 4, 2021

Uh oh!

gnufied Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

dobsonj Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

gnufied Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

gnufied left a comment

Choose a reason for hiding this comment

Uh oh!

dobsonj commented Jun 7, 2021

Uh oh!

dobsonj commented Jun 7, 2021

Uh oh!

dobsonj commented Jun 21, 2021

Uh oh!

gnufied commented Jun 21, 2021

Uh oh!

liggitt commented Jun 23, 2021

Uh oh!

gnufied commented Jun 23, 2021

Uh oh!

ehashman left a comment

Choose a reason for hiding this comment

Uh oh!

ehashman Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

mrunalp commented Jun 29, 2021

Uh oh!

liggitt commented Jun 29, 2021

Uh oh!

k8s-ci-robot commented Jun 29, 2021

Uh oh!

fejta-bot commented Jun 29, 2021

Uh oh!

fejta-bot commented Jun 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants