From 6748570724495bd6220a21632f67ba1f75634660 Mon Sep 17 00:00:00 2001 From: Mucahit Kurt Date: Tue, 22 Oct 2019 23:48:22 +0300 Subject: [PATCH] Change the logic of pod volumes existence check during kubelet cleanupOrphanedPodDirs, 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 --- pkg/kubelet/kubelet_pods.go | 4 +-- pkg/kubelet/kubelet_volumes.go | 44 ++++++++++++++--------- pkg/kubelet/kubelet_volumes_linux_test.go | 2 +- pkg/kubelet/kubelet_volumes_test.go | 22 +++++++++++- 4 files changed, 51 insertions(+), 21 deletions(-) diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index dfd60bd85a1..4f8517c62e4 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -976,7 +976,7 @@ func (kl *Kubelet) PodResourcesAreReclaimed(pod *v1.Pod, status v1.PodStatus) bo return false } - if kl.podVolumesExist(pod.UID) && !kl.keepTerminatedPodVolumes { + if kl.podVolumePathsExistInCacheOrDisk(pod.UID) && !kl.keepTerminatedPodVolumes { // We shouldn't delete pods whose volumes have not been cleaned up if we are not keeping terminated pod volumes klog.V(3).Infof("Pod %q is terminated, but some volumes have not been cleaned up", format.Pod(pod)) return false @@ -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.podVolumePathsExistInCacheOrDisk(uid); podVolumesExist && !kl.keepTerminatedPodVolumes { klog.V(3).Infof("Orphaned pod %q found, but volumes not yet removed. Reducing cpu to minimum", uid) if err := pcm.ReduceCPULimits(val); err != nil { klog.Warningf("Failed to reduce cpu time for pod %q pending volume cleanup due to %v", uid, err) diff --git a/pkg/kubelet/kubelet_volumes.go b/pkg/kubelet/kubelet_volumes.go index ac8e3e2baee..dc330376387 100644 --- a/pkg/kubelet/kubelet_volumes.go +++ b/pkg/kubelet/kubelet_volumes.go @@ -49,15 +49,15 @@ func (kl *Kubelet) ListVolumesForPod(podUID types.UID) (map[string]volume.Volume return volumesToReturn, len(volumesToReturn) > 0 } -// podVolumesExist checks with the volume manager and returns true any of the +// podMountedVolumesExistInCacheOrDisk checks with the volume manager and returns true any of the // pods for the specified volume are mounted. -func (kl *Kubelet) podVolumesExist(podUID types.UID) bool { +func (kl *Kubelet) podMountedVolumesExistInCacheOrDisk(podUID types.UID) bool { if mountedVolumes := kl.volumeManager.GetMountedVolumesForPod( volumetypes.UniquePodName(podUID)); len(mountedVolumes) > 0 { return true } - // TODO: This checks pod volume paths and whether they are mounted. If checking returns error, podVolumesExist will return true + // TODO: This checks pod volume paths and whether they are mounted. If checking returns error, podMountedVolumesExistInCacheOrDisk will return true // which means we consider volumes might exist and requires further checking. // There are some volume plugins such as flexvolume might not have mounts. See issue #61229 volumePaths, err := kl.getMountedVolumePathListFromDisk(podUID) @@ -73,6 +73,27 @@ func (kl *Kubelet) podVolumesExist(podUID types.UID) bool { return false } +// podVolumePathsExistInCacheOrDisk checks with the volume manager and returns true any of the +// volumes for the specified pod are mounted or any of the volume paths of the specified pod exist. +func (kl *Kubelet) podVolumePathsExistInCacheOrDisk(podUID types.UID) bool { + if mountedVolumes := + kl.volumeManager.GetMountedVolumesForPod( + volumetypes.UniquePodName(podUID)); len(mountedVolumes) > 0 { + return true + } + + volumePaths, err := kl.getPodVolumePathListFromDisk(podUID) + if err != nil { + klog.Errorf("pod %q found, but error %v occurred during checking volume dirs from disk", podUID, err) + return true + } + if len(volumePaths) > 0 { + klog.V(4).Infof("pod %q found, but volume paths are still present on disk %v", podUID, volumePaths) + return true + } + return false +} + // newVolumeMounterFromPlugins attempts to find a plugin by volume spec, pod // and volume options and then creates a Mounter. // Returns a valid mounter or an error. @@ -113,21 +134,10 @@ func (kl *Kubelet) cleanupOrphanedPodDirs(pods []*v1.Pod, runningPods []*kubecon continue } // If volumes have not been unmounted/detached, do not delete directory. - // Doing so may result in corruption of data. - // TODO: getMountedVolumePathListFromDisk() call may be redundant with - // kl.getPodVolumePathListFromDisk(). Can this be cleaned up? - if podVolumesExist := kl.podVolumesExist(uid); podVolumesExist { - klog.V(3).Infof("Orphaned pod %q found, but volumes are not cleaned up", uid) - continue - } // If there are still volume directories, do not delete directory - volumePaths, err := kl.getPodVolumePathListFromDisk(uid) - if err != nil { - orphanVolumeErrors = append(orphanVolumeErrors, fmt.Errorf("orphaned pod %q found, but error %v occurred during reading volume dir from disk", uid, err)) - continue - } - if len(volumePaths) > 0 { - orphanVolumeErrors = append(orphanVolumeErrors, fmt.Errorf("orphaned pod %q found, but volume paths are still present on disk", uid)) + // Doing so may result in corruption of data. + if kl.podVolumePathsExistInCacheOrDisk(uid) { + klog.V(3).Infof("Orphaned pod %q found, but volumes are not cleaned up", uid) continue } diff --git a/pkg/kubelet/kubelet_volumes_linux_test.go b/pkg/kubelet/kubelet_volumes_linux_test.go index bb2be4f0913..a31922e2c8f 100644 --- a/pkg/kubelet/kubelet_volumes_linux_test.go +++ b/pkg/kubelet/kubelet_volumes_linux_test.go @@ -261,7 +261,7 @@ func TestPodVolumesExistWithMount(t *testing.T) { } } - exist := kubelet.podVolumesExist(poduid) + exist := kubelet.podMountedVolumesExistInCacheOrDisk(poduid) if tc.expected != exist { t.Errorf("%s failed: expected %t, got %t", name, tc.expected, exist) } diff --git a/pkg/kubelet/kubelet_volumes_test.go b/pkg/kubelet/kubelet_volumes_test.go index bfc5c8b7c22..53035614a5b 100644 --- a/pkg/kubelet/kubelet_volumes_test.go +++ b/pkg/kubelet/kubelet_volumes_test.go @@ -25,10 +25,14 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/uuid" core "k8s.io/client-go/testing" "k8s.io/kubernetes/pkg/volume" volumetest "k8s.io/kubernetes/pkg/volume/testing" "k8s.io/kubernetes/pkg/volume/util" + "os" + "path" + "path/filepath" ) func TestListVolumesForPod(t *testing.T) { @@ -196,11 +200,27 @@ func TestPodVolumesExist(t *testing.T) { } for _, pod := range pods { - podVolumesExist := kubelet.podVolumesExist(pod.UID) + podVolumesExist := kubelet.podMountedVolumesExistInCacheOrDisk(pod.UID) assert.True(t, podVolumesExist, "pod %q", pod.UID) } } +func TestPodVolumePathsExist(t *testing.T) { + testKubelet := newTestKubelet(t, false /* controllerAttachDetachEnabled */) + defer testKubelet.Cleanup() + kubelet := testKubelet.kubelet + pod := podWithUIDNameNs(uuid.NewUUID(), "pod1", "ns") + + // To pass volumeManager GetMountedVolumesForPod control create pod volume data directory directly + podVolumeDir := kubelet.getPodVolumeDir(pod.UID, "volumePlugin1", "volume1") + podDataDir := filepath.Join(podVolumeDir, "data1") + os.MkdirAll(path.Dir(podDataDir), 0755) + defer os.RemoveAll(path.Dir(kubelet.getPodDir(pod.UID))) + + podVolumePathsExist := kubelet.podVolumePathsExistInCacheOrDisk(pod.UID) + assert.True(t, podVolumePathsExist, "pod %q", pod.UID) +} + func TestVolumeAttachAndMountControllerDisabled(t *testing.T) { testKubelet := newTestKubelet(t, false /* controllerAttachDetachEnabled */) defer testKubelet.Cleanup()