From 7012994a61c142b173e4a1fbe2a12e3d580b11d5 Mon Sep 17 00:00:00 2001 From: Jing Xu Date: Mon, 13 Jan 2020 13:32:04 -0800 Subject: [PATCH] Fix issue in kubelet getMountedVolumePathListFromDisk This PR fixes issue #74650. It adds the extra check for /mount dir under pod volume dir. It also adds the unit test for this function --- pkg/kubelet/BUILD | 1 + pkg/kubelet/kubelet_getters.go | 27 ++++- pkg/kubelet/kubelet_volumes_linux_test.go | 114 ++++++++++++++++++++++ pkg/volume/csi/csi_mounter.go | 2 +- pkg/volume/csi/csi_util.go | 5 + 5 files changed, 145 insertions(+), 4 deletions(-) diff --git a/pkg/kubelet/BUILD b/pkg/kubelet/BUILD index 02c938705a9..5a740261bfe 100644 --- a/pkg/kubelet/BUILD +++ b/pkg/kubelet/BUILD @@ -151,6 +151,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", diff --git a/pkg/kubelet/kubelet_getters.go b/pkg/kubelet/kubelet_getters.go index 3dfed31904d..4ab8fba85ba 100644 --- a/pkg/kubelet/kubelet_getters.go +++ b/pkg/kubelet/kubelet_getters.go @@ -27,6 +27,7 @@ import ( "k8s.io/klog" "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) diff --git a/pkg/kubelet/kubelet_volumes_linux_test.go b/pkg/kubelet/kubelet_volumes_linux_test.go index 7f08ff4d5cc..bd1c073b11f 100644 --- a/pkg/kubelet/kubelet_volumes_linux_test.go +++ b/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) + } + }) + } +} diff --git a/pkg/volume/csi/csi_mounter.go b/pkg/volume/csi/csi_mounter.go index a5e1254ff2a..dd1109cb1ea 100644 --- a/pkg/volume/csi/csi_mounter.go +++ b/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 } diff --git a/pkg/volume/csi/csi_util.go b/pkg/volume/csi/csi_util.go index 8201d144100..3cf7338153c 100644 --- a/pkg/volume/csi/csi_util.go +++ b/pkg/volume/csi/csi_util.go @@ -166,3 +166,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") +}