check volume directories instead of mounts for cleanupOrphanedPodDirs #84206

mucahitkurt · 2019-10-22T21:04:16Z

What type of PR is this?
/kind cleanup
/sig storage

What this PR does / why we need it:
Related comment; #72346 (comment)

Change podVolumesExist to check volume directories instead of mounted volumes when it's called from Kubelet.cleanupOrphanedPodDirs. Because getMountedVolumePathListFromDisk does not always produce correct results for mounted volumes and Kubelet.cleanupOrphanedPodDirs already used getPodVolumePathListFromDisk.

Which issue(s) this PR fixes:
Related issue #72346

Related issue is quite evolved to fix all podVolumesExist method usage, but we may think this PR a fix for the issue #72346 and all podVolumesExist usage may be fixed within the scope of issue #72347

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

cc @msau42 @cofyc

msau42 · 2019-10-23T17:26:14Z

/assign @msau42 @jsafrane

mucahitkurt · 2019-10-23T19:29:41Z

I'm looking into build error, when I run ./hack/update-bazel.sh it doesn't update any build file...

mattjmcnaughton

A small readability comment from me - otherwise, defer to @jsafrane and the folks who work with volumes more :)

mattjmcnaughton · 2019-10-23T23:31:26Z

pkg/kubelet/kubelet_pods.go

@@ -917,7 +917,7 @@ func (kl *Kubelet) PodResourcesAreReclaimed(pod *v1.Pod, status v1.PodStatus) bo
 		klog.V(3).Infof("Pod %q is terminated, but some containers have not been cleaned up: %s", format.Pod(pod), statusStr)
 		return false
 	}
-	if kl.podVolumesExist(pod.UID) && !kl.keepTerminatedPodVolumes {
+	if kl.podVolumesExist(pod.UID, false) && !kl.keepTerminatedPodVolumes {


Nit: Can we do

checkVolumePaths := false kl.podVolumesExist(pod.UID, checkVolumePaths)

otherwise, it can be difficult to know what the false means here.

I added parameter name as a comment for podVolumesExist call. Thanks!

Thanks for working with me on this small nit :)

This may just be my opinion, but I feel like the comment offers less advantages than the variable name (i.e. is less explicit). Are there advantages to using a comment over a variable that I'm missing?

My IDE(goland) uses similar approach to show the parameter names of the function variables and I see similar usages in kubernetes code base, using an additional variable seems little noisy to me, so I choose adding comment.

But I agree, using variable can be more explicit, I'm neutral, I'll add additional variable instead of comment.

Ah gotcha - maybe part of this is that I'm using vim haha :) Anyway, thanks!+

Will let @jsafrane or someone else on sig-storage sign off :)

mucahitkurt · 2019-10-24T02:41:03Z

/test pull-kubernetes-node-e2e-containerd
/test pull-kubernetes-kubemark-e2e-gce-big

cofyc · 2019-10-25T21:25:46Z

/cc @cofyc

mucahitkurt · 2019-10-26T07:04:32Z

/test pull-kubernetes-verify

jsafrane · 2019-10-31T12:36:31Z

I am not sure I understand the PR. If we expect that all volume plugins remove their directories in their TearDown (that's what cleanupOrphanedPodDirs assumes now), why we are checking mounts in all cleanupOrphanedPodDirs(..., false)` calls? Shouldn't be the check the same everywhere?

mucahitkurt · 2019-10-31T21:13:03Z

I am not sure I understand the PR. If we expect that all volume plugins remove their directories in their TearDown (that's what cleanupOrphanedPodDirs assumes now), why we are checking mounts in all cleanupOrphanedPodDirs(..., false)` calls? Shouldn't be the check the same everywhere?

Other callers of podExists method, cleanupOrphanedPodCgroups, PodResourcesAreReclaimed, assume that all plugins unmount their directories but don't assume that all plugins remove their directories. So we are not sure to change this behavior because it can cause not deleting a pod.

But cleanupOrphanedPodDirs was checking both mounts(via podVolumeExist call) and directory existence(via getPodVolumePathListFromDisk) before this PR, but now it's only checking directory existence, so I removed a redundant check and the behavior of the method doesn't change.

jsafrane · 2019-11-01T13:52:31Z

cleanupOrphanedPodCgroups,PodResourcesAreReclaimed, assume that all plugins unmount their directories but don't assume that all plugins remove their directories.

Now we assume (in cleanupOrphanedPodDirs), that volume plugins do remove their directories.

I see you want be on the safe side and it's probably correct at this stage of 1.17, but should we have the same assumption everywhere in the end?

/lgtm
/approve

mucahitkurt · 2019-11-01T18:55:08Z

Now we assume (in cleanupOrphanedPodDirs), that volume plugins do remove their directories.

I see you want be on the safe side and it's probably correct at this stage of 1.17, but should we have the same assumption everywhere in the end?

/lgtm
/approve

I think so, this's first suggestion of @cofyc. I think we are not confident about to depending the cleanup behavior of plugins in terms of removing directories but currently these methods already depend the cleanup behavior of plugins in terms of unmounting directories. May be @msau42 or @cofyc can express better than me.

mucahitkurt · 2019-11-01T18:55:27Z

/assign @vishh

mucahitkurt · 2020-09-20T06:33:11Z

@mucahitkurt sorry for the delay. Could you pls rebase your code? I will review it now.

Hey @jingxu97 I have rebased this PR, PTAL.

mucahitkurt · 2020-09-20T08:12:15Z

/test pull-kubernetes-bazel-test

jingxu97 · 2020-10-10T14:44:00Z

@mucahitkurt sorry for the delay. I think the main logic here is changing the ordering of checking things. There are three types of checking for pod volumes existence, check in-memory cache, check disk directories, and check whether each dir is a mount point or not.

kl.volumeManager.GetMountedVolumesForPod(podUid)
This will check in-memory cache whether volumes are still mounted. This check is fast, but might not be completely reliable when kubelet restarts and lost its cache (there is a reconstruction phase for cache from scanning dirs on disk, but I need to check whether there is guarantee when cleanupOrphanedPodDirs is called, cache is already constructed)
kl.getPodVolumePathListFromDisk(podUid)
This function directly scans the disk directory and returns a list of the volume paths for the given pod (e.g., /var/lib/kubelet/pods/pod_uid/volumes/...)
kl.getMountedVolumePathListFromDisk(podUID)
This function first calls getPodVolumePathListFromDisk to get the list of volume paths for the given pod, and check each path whether is a mount point or not using IsLikelyNotMountPoint.
There are two things worth noting about this function
- this check might not work in certain cases that mount point is in the root file system).
- this check uses os.Stats which might cause process hung in case of NFS mount point when server is down. (Please check this very old and long discussion Hung volumes can wedge the kubelet #31272)
- there is a similar function mount.IsNotMountPoint(), this function checks mount table for further verification about mount point to solve the issue with IsLikelyNotMountPoint, but it might take longer time to go through mount table.
kl.podVolumesExist(podUID)
This function is a combination of function 1 and 3 listed here, it first calls function GetMountedVolumesForPod and getMountedVolumePathListFromDisk to verify volume existence.

All these checking are used in a number of difference places. Here clearupOrphanedPodDirs, this function is being called by HandlePodCleanups, and its comment mentioned

// NOTE: This function is executed by the main sync loop, so it
// should not contain any blocking calls.

You might check the issue #31272 to see how we removed the check 3 getMountedVolumePathListFromDisk due to hung issue caused by nfs. However, I think I accidentally introduced this back again here by adding the check of mounted points in podVolumesExist. So the fix here I suggest, instead of calling podVolumesExist, call the following checks

call check 1 kl.volumeManager.GetMountedVolumesForPod(podUid) for check internal cache,
call check 2 kl.getPodVolumePathListFromDisk(podUid)

Pls let me know if you have questions about the comments. Thanks!

mucahitkurt · 2020-11-09T06:32:27Z

@jingxu97, @msau42 Sorry for the delay, I'll try to check the latest comments and try to make the fix in a few days (hope before 12th Nov.)

mucahitkurt · 2020-11-09T15:23:36Z

@mucahitkurt sorry for the delay. I think the main logic here is changing the ordering of checking things. There are three types of checking for pod volumes existence, check in-memory cache, check disk directories, and check whether each dir is a mount point or not.

kl.volumeManager.GetMountedVolumesForPod(podUid)
This will check in-memory cache whether volumes are still mounted. This check is fast, but might not be completely reliable when kubelet restarts and lost its cache (there is a reconstruction phase for cache from scanning dirs on disk, but I need to check whether there is guarantee when cleanupOrphanedPodDirs is called, cache is already constructed)

kl.getPodVolumePathListFromDisk(podUid)
This function directly scans the disk directory and returns a list of the volume paths for the given pod (e.g., /var/lib/kubelet/pods/pod_uid/volumes/...)

kl.getMountedVolumePathListFromDisk(podUID)
This function first calls getPodVolumePathListFromDisk to get the list of volume paths for the given pod, and check each path whether is a mount point or not using IsLikelyNotMountPoint.
There are two things worth noting about this function

this check might not work in certain cases that mount point is in the root file system).

this check uses os.Stats which might cause process hung in case of NFS mount point when server is down. (Please check this very old and long discussion Hung volumes can wedge the kubelet #31272)

there is a similar function mount.IsNotMountPoint(), this function checks mount table for further verification about mount point to solve the issue with IsLikelyNotMountPoint, but it might take longer time to go through mount table.

kl.podVolumesExist(podUID)
This function is a combination of function 1 and 3 listed here, it first calls function GetMountedVolumesForPod and getMountedVolumePathListFromDisk to verify volume existence.

All these checking are used in a number of difference places. Here clearupOrphanedPodDirs, this function is being called by HandlePodCleanups, and its comment mentioned
// NOTE: This function is executed by the main sync loop, so it
// should not contain any blocking calls.
You might check the issue #31272 to see how we removed the check 3 getMountedVolumePathListFromDisk due to hung issue caused by nfs. However, I think I accidentally introduced this back again here by adding the check of mounted points in podVolumesExist. So the fix here I suggest, instead of calling podVolumesExist, call the following checks

call check 1 kl.volumeManager.GetMountedVolumesForPod(podUid) for check internal cache,

call check 2 kl.getPodVolumePathListFromDisk(podUid)

Pls let me know if you have questions about the comments. Thanks!

@jingxu97 ;

I keep the new function kl.podVolumePathsExist(podUID types.UID) because it uses the check 1 and check 2.

I replaced the kl.podVolumesExist(podUID types.UID) with kl.podVolumePathsExist(podUID types.UID) in cleanupOrphanedPodDirs and PodResourcesAreReclaimed.

Do you have any concern with the new function?

kl.podVolumesExist(podUID types.UID) is used in Kubelet.cleanupOrphanedPodCgroups, I keep it as is.

jingxu97 · 2020-11-10T01:15:59Z

@mucahitkurt Thanks for the update. The function looks good to me. The only thing is the naming, podVolumePathsExist and podVolumesExist, they are too close so it is a little bit hard to understand what's the difference. I am not good at naming either, just a suggestion, e.g.,

podVolumePathsExist --> podVolumePathsExistInCacheOrDisk
podVolumesExist --> podMountedVolumesExistInCacheOrDisk

mucahitkurt · 2020-11-10T05:23:03Z

@mucahitkurt Thanks for the update. The function looks good to me. The only thing is the naming, podVolumePathsExist and podVolumesExist, they are too close so it is a little bit hard to understand what's the difference. I am not good at naming either, just a suggestion, e.g.,

podVolumePathsExist --> podVolumePathsExistInCacheOrDisk
podVolumesExist --> podMountedVolumesExistInCacheOrDisk

@jingxu97 I renamed with your suggestions, I don't have better alternatives.

mucahitkurt · 2020-11-10T06:51:58Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

jingxu97 · 2020-11-10T07:26:27Z

pkg/kubelet/kubelet_pods.go

@@ -1962,7 +1962,7 @@ func (kl *Kubelet) cleanupOrphanedPodCgroups(pcm cm.PodContainerManager, cgroupP
 		// parent croup.  If the volumes still exist, reduce the cpu shares for any
 		// process in the cgroup to the minimum value while we wait.  if the kubelet
 		// is configured to keep terminated volumes, we will delete the cgroup and not block.
-		if podVolumesExist := kl.podVolumesExist(uid); podVolumesExist && !kl.keepTerminatedPodVolumes {
+		if podVolumesExist := kl.podMountedVolumesExistInCacheOrDisk(uid); podVolumesExist && !kl.keepTerminatedPodVolumes {


actually here cleanupOrphanedPodCgroups,I think it should be similar to cleanupOrphanedPod, so using podVolumePathsExistInCacheOrDisk. ?

I changed this one to kl. kl.podVolumePathsExistInCacheOrDisk.

Right now no usage remain for the function kl.podVolumesExist or with the new name kl.podMountedVolumesExistInCacheOrDisk. Should I delete this function and all related test codes?

…pOrphanedPodDirs, cleanupOrphanedPodCgroups and PodResourcesAreReclaimed check in-memory cache whether volumes are still mounted and check disk directory for the volume paths instead of mounted volumes check Signed-off-by: Mucahit Kurt <mucahitkurt@gmail.com>

mucahitkurt · 2020-11-10T14:37:42Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

jingxu97 · 2020-11-11T20:19:19Z

/lgtm

derekwaynecarr · 2020-11-11T21:28:31Z

/approve

k8s-ci-robot · 2020-11-11T21:29:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, jsafrane, mucahitkurt

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/kubelet/OWNERS~~ [derekwaynecarr]

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

msau42 · 2020-11-12T01:18:15Z

/retest

msau42 · 2020-11-12T02:21:05Z

/retest

k8s-ci-robot requested review from feiskyer and mattjmcnaughton October 22, 2019 21:06

k8s-ci-robot assigned jsafrane and msau42 Oct 23, 2019

mucahitkurt force-pushed the refactor/remove-mount-volume-check-orphaned-pod-cleanup branch from 3f0cd3c to c7dd1a9 Compare October 23, 2019 20:10

mattjmcnaughton reviewed Oct 23, 2019

View reviewed changes

mucahitkurt force-pushed the refactor/remove-mount-volume-check-orphaned-pod-cleanup branch from c7dd1a9 to 2c1769b Compare October 24, 2019 19:40

k8s-ci-robot requested a review from cofyc October 25, 2019 21:25

mucahitkurt force-pushed the refactor/remove-mount-volume-check-orphaned-pod-cleanup branch from 2c1769b to 66edee8 Compare October 26, 2019 02:49

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2019

k8s-ci-robot assigned vishh Nov 1, 2019

mucahitkurt force-pushed the refactor/remove-mount-volume-check-orphaned-pod-cleanup branch from 66edee8 to 42fa439 Compare November 14, 2019 19:39

jingxu97 mentioned this pull request Oct 26, 2020

Automatically remove orphaned pod's dangling volumes #95301

Merged

msau42 mentioned this pull request Nov 5, 2020

WIP: Promote "CSI volume limit information using mock driver should report attach limit when limit is bigger than 0" e2e test to Conformance #85164

Closed

mucahitkurt force-pushed the refactor/remove-mount-volume-check-orphaned-pod-cleanup branch from 0c7105d to 01deec5 Compare November 9, 2020 15:12

mucahitkurt force-pushed the refactor/remove-mount-volume-check-orphaned-pod-cleanup branch from 01deec5 to d190c9c Compare November 10, 2020 05:22

jingxu97 reviewed Nov 10, 2020

View reviewed changes

mucahitkurt force-pushed the refactor/remove-mount-volume-check-orphaned-pod-cleanup branch from d190c9c to 6748570 Compare November 10, 2020 14:35

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

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

k8s-ci-robot merged commit bdac36b into kubernetes:master Nov 12, 2020

k8s-ci-robot added this to the v1.20 milestone Nov 12, 2020

SergeyKanzhelev mentioned this pull request Nov 20, 2020

[Failing Test] [sig-node] Pods Extended [k8s.io] Pod Container Status should never report success for a pending container (ci-kubernetes-e2e-ubuntu-gce) #96565

Closed

This was referenced Nov 20, 2020

Revert "check volume directories instead of mounts for cleanupOrphanedPodDirs" #96758

Merged

Secret volume directories sometimes don't get cleaned up #96759

Closed

msau42 mentioned this pull request Feb 17, 2021

Test flake: [sig-storage] CSI mock volume CSI volume limit information using mock driver should report attach limit when limit is bigger than 0 [ #91001

Closed

tnqn mentioned this pull request Apr 29, 2021

kubelet SyncLoop hangs on "os.Stat" forever if there is an unresponsive NFS volume #101622

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check volume directories instead of mounts for cleanupOrphanedPodDirs #84206

check volume directories instead of mounts for cleanupOrphanedPodDirs #84206

mucahitkurt commented Oct 22, 2019

msau42 commented Oct 23, 2019

mucahitkurt commented Oct 23, 2019

mattjmcnaughton left a comment

mattjmcnaughton Oct 23, 2019

mucahitkurt Oct 24, 2019

mattjmcnaughton Oct 25, 2019

mucahitkurt Oct 25, 2019

mucahitkurt Oct 26, 2019

mattjmcnaughton Oct 26, 2019

mattjmcnaughton Oct 26, 2019

mucahitkurt commented Oct 24, 2019

cofyc commented Oct 25, 2019

mucahitkurt commented Oct 26, 2019

jsafrane commented Oct 31, 2019

mucahitkurt commented Oct 31, 2019

jsafrane commented Nov 1, 2019

mucahitkurt commented Nov 1, 2019

mucahitkurt commented Nov 1, 2019

mucahitkurt commented Sep 20, 2020

mucahitkurt commented Sep 20, 2020

jingxu97 commented Oct 10, 2020 •

edited

mucahitkurt commented Nov 9, 2020

mucahitkurt commented Nov 9, 2020

jingxu97 commented Nov 10, 2020

mucahitkurt commented Nov 10, 2020

mucahitkurt commented Nov 10, 2020

jingxu97 Nov 10, 2020

mucahitkurt Nov 10, 2020

mucahitkurt commented Nov 10, 2020

jingxu97 commented Nov 11, 2020

derekwaynecarr commented Nov 11, 2020

k8s-ci-robot commented Nov 11, 2020

msau42 commented Nov 12, 2020

msau42 commented Nov 12, 2020

check volume directories instead of mounts for cleanupOrphanedPodDirs #84206

check volume directories instead of mounts for cleanupOrphanedPodDirs #84206

Conversation

mucahitkurt commented Oct 22, 2019

msau42 commented Oct 23, 2019

mucahitkurt commented Oct 23, 2019

mattjmcnaughton left a comment

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

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

mucahitkurt commented Oct 24, 2019

cofyc commented Oct 25, 2019

mucahitkurt commented Oct 26, 2019

jsafrane commented Oct 31, 2019

mucahitkurt commented Oct 31, 2019

jsafrane commented Nov 1, 2019

mucahitkurt commented Nov 1, 2019

mucahitkurt commented Nov 1, 2019

mucahitkurt commented Sep 20, 2020

mucahitkurt commented Sep 20, 2020

jingxu97 commented Oct 10, 2020 • edited

mucahitkurt commented Nov 9, 2020

mucahitkurt commented Nov 9, 2020

jingxu97 commented Nov 10, 2020

mucahitkurt commented Nov 10, 2020

mucahitkurt commented Nov 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mucahitkurt commented Nov 10, 2020

jingxu97 commented Nov 11, 2020

derekwaynecarr commented Nov 11, 2020

k8s-ci-robot commented Nov 11, 2020

msau42 commented Nov 12, 2020

msau42 commented Nov 12, 2020

jingxu97 commented Oct 10, 2020 •

edited