Merge pull request #87166 from jingxu97/Jan/mountcheckfix

Fix issue in kubelet getMountedVolumePathListFromDisk

pkg/kubelet/BUILD

@@ -152,6 +152,7 @@ go_library(
         "//vendor/k8s.io/utils/mount:go_default_library",
         "//vendor/k8s.io/utils/net:go_default_library",
         "//vendor/k8s.io/utils/path:go_default_library",
+        "//vendor/k8s.io/utils/strings:go_default_library",
     ] + select({
         "@io_bazel_rules_go//go/platform:windows": [
             "//pkg/kubelet/winstats:go_default_library",

pkg/kubelet/kubelet_getters.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/klog/v2"
 	"k8s.io/utils/mount"
 	utilpath "k8s.io/utils/path"
+	utilstrings "k8s.io/utils/strings"
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -35,6 +36,7 @@ import (
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
 	kubelettypes "k8s.io/kubernetes/pkg/kubelet/types"
 	utilnode "k8s.io/kubernetes/pkg/util/node"
+	"k8s.io/kubernetes/pkg/volume/csi"
 )
 
 // getRootDir returns the full path to the directory under which kubelet can
@@ -310,8 +312,22 @@ func (kl *Kubelet) getPodVolumePathListFromDisk(podUID types.UID) ([]string, err
 		if err != nil {
 			return volumes, fmt.Errorf("could not read directory %s: %v", volumePluginPath, err)
 		}
-		for _, volumeDir := range volumeDirs {
-			volumes = append(volumes, filepath.Join(volumePluginPath, volumeDir))
+		unescapePluginName := utilstrings.UnescapeQualifiedName(volumePluginName)
+
+		if unescapePluginName != csi.CSIPluginName {
+			for _, volumeDir := range volumeDirs {
+				volumes = append(volumes, filepath.Join(volumePluginPath, volumeDir))
+			}
+		} else {
+			// For CSI volumes, the mounted volume path has an extra sub path "/mount", so also add it
+			// to the list if the mounted path exists.
+			for _, volumeDir := range volumeDirs {
+				path := filepath.Join(volumePluginPath, volumeDir)
+				csimountpath := csi.GetCSIMounterPath(path)
+				if pathExists, _ := mount.PathExists(csimountpath); pathExists {
+					volumes = append(volumes, csimountpath)
+				}
+			}
 		}
 	}
 	return volumes, nil
@@ -323,10 +339,15 @@ func (kl *Kubelet) getMountedVolumePathListFromDisk(podUID types.UID) ([]string,
 	if err != nil {
 		return mountedVolumes, err
 	}
+	// Only use IsLikelyNotMountPoint to check might not cover all cases. For CSI volumes that
+	// either: 1) don't mount or 2) bind mount in the rootfs, the mount check will not work as expected.
+	// We plan to remove this mountpoint check as a condition before deleting pods since it is
+	// not reliable and the condition might be different for different types of volumes. But it requires
+	// a reliable way to clean up unused volume dir to avoid problems during pod deletion. See discussion in issue #74650
 	for _, volumePath := range volumePaths {
 		isNotMount, err := kl.mounter.IsLikelyNotMountPoint(volumePath)
 		if err != nil {
-			return mountedVolumes, err
+			return mountedVolumes, fmt.Errorf("fail to check mount point %q: %v", volumePath, err)
 		}
 		if !isNotMount {
 			mountedVolumes = append(mountedVolumes, volumePath)

pkg/kubelet/kubelet_volumes_linux_test.go

@@ -28,7 +28,9 @@ import (
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
+	"k8s.io/apimachinery/pkg/types"
 	_ "k8s.io/kubernetes/pkg/apis/core/install"
+	"k8s.io/utils/mount"
 )
 
 func validateDirExists(dir string) error {
@@ -154,3 +156,115 @@ func TestCleanupOrphanedPodDirs(t *testing.T) {
 		})
 	}
 }
+
+func TestPodVolumesExistWithMount(t *testing.T) {
+	poduid := types.UID("poduid")
+	testCases := map[string]struct {
+		prepareFunc func(kubelet *Kubelet) error
+		expected    bool
+	}{
+		"noncsivolume-dir-not-exist": {
+			prepareFunc: func(kubelet *Kubelet) error {
+				return nil
+			},
+			expected: false,
+		},
+		"noncsivolume-dir-exist-noplugins": {
+			prepareFunc: func(kubelet *Kubelet) error {
+				podDir := kubelet.getPodDir(poduid)
+				return os.MkdirAll(filepath.Join(podDir, "volumes/"), 0750)
+			},
+			expected: false,
+		},
+		"noncsivolume-dir-exist-nomount": {
+			prepareFunc: func(kubelet *Kubelet) error {
+				podDir := kubelet.getPodDir(poduid)
+				return os.MkdirAll(filepath.Join(podDir, "volumes/plugin/name"), 0750)
+			},
+			expected: false,
+		},
+		"noncsivolume-dir-exist-with-mount": {
+			prepareFunc: func(kubelet *Kubelet) error {
+				podDir := kubelet.getPodDir(poduid)
+				volumePath := filepath.Join(podDir, "volumes/plugin/name")
+				if err := os.MkdirAll(volumePath, 0750); err != nil {
+					return err
+				}
+				fm := mount.NewFakeMounter(
+					[]mount.MountPoint{
+						{Device: "/dev/sdb", Path: volumePath},
+					})
+				kubelet.mounter = fm
+				return nil
+			},
+			expected: true,
+		},
+		"noncsivolume-dir-exist-nomount-withcsimountpath": {
+			prepareFunc: func(kubelet *Kubelet) error {
+				podDir := kubelet.getPodDir(poduid)
+				volumePath := filepath.Join(podDir, "volumes/plugin/name/mount")
+				if err := os.MkdirAll(volumePath, 0750); err != nil {
+					return err
+				}
+				fm := mount.NewFakeMounter(
+					[]mount.MountPoint{
+						{Device: "/dev/sdb", Path: volumePath},
+					})
+				kubelet.mounter = fm
+				return nil
+			},
+			expected: false,
+		},
+		"csivolume-dir-exist-nomount": {
+			prepareFunc: func(kubelet *Kubelet) error {
+				podDir := kubelet.getPodDir(poduid)
+				volumePath := filepath.Join(podDir, "volumes/kubernetes.io~csi/name")
+				return os.MkdirAll(volumePath, 0750)
+			},
+			expected: false,
+		},
+		"csivolume-dir-exist-mount-nocsimountpath": {
+			prepareFunc: func(kubelet *Kubelet) error {
+				podDir := kubelet.getPodDir(poduid)
+				volumePath := filepath.Join(podDir, "volumes/kubernetes.io~csi/name/mount")
+				return os.MkdirAll(volumePath, 0750)
+			},
+			expected: false,
+		},
+		"csivolume-dir-exist-withcsimountpath": {
+			prepareFunc: func(kubelet *Kubelet) error {
+				podDir := kubelet.getPodDir(poduid)
+				volumePath := filepath.Join(podDir, "volumes/kubernetes.io~csi/name/mount")
+				if err := os.MkdirAll(volumePath, 0750); err != nil {
+					return err
+				}
+				fm := mount.NewFakeMounter(
+					[]mount.MountPoint{
+						{Device: "/dev/sdb", Path: volumePath},
+					})
+				kubelet.mounter = fm
+				return nil
+			},
+			expected: true,
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			testKubelet := newTestKubelet(t, false /* controllerAttachDetachEnabled */)
+			defer testKubelet.Cleanup()
+			kubelet := testKubelet.kubelet
+
+			if tc.prepareFunc != nil {
+				if err := tc.prepareFunc(kubelet); err != nil {
+					t.Fatalf("%s failed preparation: %v", name, err)
+				}
+			}
+
+			exist := kubelet.podVolumesExist(poduid)
+			if tc.expected != exist {
+				t.Errorf("%s failed: expected %t, got %t", name, tc.expected, exist)
+			}
+		})
+	}
+}

pkg/volume/csi/csi_mounter.go

@@ -82,7 +82,7 @@ type csiMountMgr struct {
 var _ volume.Volume = &csiMountMgr{}
 
 func (c *csiMountMgr) GetPath() string {
-	dir := filepath.Join(getTargetPath(c.podUID, c.specVolumeID, c.plugin.host), "/mount")
+	dir := GetCSIMounterPath(filepath.Join(getTargetPath(c.podUID, c.specVolumeID, c.plugin.host)))
 	klog.V(4).Info(log("mounter.GetPath generated [%s]", dir))
 	return dir
 }

pkg/volume/csi/csi_util.go

@@ -171,3 +171,8 @@ func getPVSourceFromSpec(spec *volume.Spec) (*api.CSIPersistentVolumeSource, err
 	}
 	return pvSrc, nil
 }
+
+// GetCSIMounterPath returns the mounter path given the base path.
+func GetCSIMounterPath(path string) string {
+	return filepath.Join(path, "/mount")
+}