Merge pull request #116134 from cvvz/fix-111933

fix: After a Node is down and take some time to get back to up again, the mount point of the evicted Pods cannot be cleaned up successfully.

pkg/kubelet/volumemanager/reconciler/reconciler_test.go

@@ -2342,9 +2342,9 @@ func TestSyncStates(t *testing.T) {
 		{
 			name: "when reconstruction fails for a volume, volumes should be cleaned up",
 			volumePaths: []string{
-				filepath.Join("pod1", "volumes", "fake-plugin", "pvc-abcdef"),
+				filepath.Join("pod1", "volumes", "fake-plugin", volumetesting.FailNewMounter),
 			},
-			createMountPoint: false,
+			createMountPoint: true,
 			podInfos:         []podInfo{},
 			verifyFunc: func(rcInstance *reconciler, fakePlugin *volumetesting.FakeVolumePlugin) error {
 				return retryWithExponentialBackOff(reconcilerSyncWaitDuration, func() (bool, error) {
@@ -2356,6 +2356,65 @@ func TestSyncStates(t *testing.T) {
 				})
 			},
 		},
+		{
+			name: "when mount point does not exist, reconstruction should not fail, volumes should be added in asw",
+			volumePaths: []string{
+				filepath.Join("pod1", "volumes", "fake-plugin", "pvc-abcdef"),
+			},
+			createMountPoint: false,
+			podInfos:         []podInfo{},
+			verifyFunc: func(rcInstance *reconciler, fakePlugin *volumetesting.FakeVolumePlugin) error {
+				mountedPods := rcInstance.actualStateOfWorld.GetMountedVolumes()
+				if len(mountedPods) != 1 {
+					return fmt.Errorf("expected 1 pods to in asw got %d", len(mountedPods))
+				}
+				return nil
+			},
+		},
+		{
+			name: "when mount point does not exist, reconstruction should not fail, if volume exists in dsw, volume should be recorded in skipped during reconstruction",
+			volumePaths: []string{
+				filepath.Join("pod1uid", "volumes", "fake-plugin", "volume-name"),
+			},
+			createMountPoint: false,
+			podInfos:         []podInfo{defaultPodInfo},
+			postSyncStatCallback: func(rcInstance *reconciler, fakePlugin *volumetesting.FakeVolumePlugin) error {
+				skippedVolumes := rcInstance.skippedDuringReconstruction
+				if len(skippedVolumes) != 1 {
+					return fmt.Errorf("expected 1 pods to in skippedDuringReconstruction got %d", len(skippedVolumes))
+				}
+				rcInstance.processReconstructedVolumes()
+				return nil
+			},
+			verifyFunc: func(rcInstance *reconciler, fakePlugin *volumetesting.FakeVolumePlugin) error {
+				mountedPods := rcInstance.actualStateOfWorld.GetAllMountedVolumes()
+				if len(mountedPods) != 1 {
+					return fmt.Errorf("expected 1 pods to in mounted volume list got %d", len(mountedPods))
+				}
+				mountedPodVolume := mountedPods[0]
+				addedViaReconstruction := rcInstance.actualStateOfWorld.IsVolumeReconstructed(mountedPodVolume.VolumeName, mountedPodVolume.PodName)
+				if !addedViaReconstruction {
+					return fmt.Errorf("expected volume %s to be marked as added via reconstruction", mountedPodVolume.VolumeName)
+				}
+
+				// check device mount state
+				attachedVolumes := rcInstance.actualStateOfWorld.GetAttachedVolumes()
+				if len(attachedVolumes) != 1 {
+					return fmt.Errorf("expected 1 volume to be unmounted, got %d", len(attachedVolumes))
+				}
+				firstAttachedVolume := attachedVolumes[0]
+				if !firstAttachedVolume.DeviceMayBeMounted() {
+					return fmt.Errorf("expected %s volume to be mounted in uncertain state", firstAttachedVolume.VolumeName)
+				}
+
+				// also skippedVolumes map should be empty
+				skippedVolumes := rcInstance.skippedDuringReconstruction
+				if len(skippedVolumes) > 0 {
+					return fmt.Errorf("expected 0 pods in skipped volumes found %d", len(skippedVolumes))
+				}
+				return nil
+			},
+		},
 		{
 			name: "when volume exists in dsow, volume should be recorded in skipped during reconstruction",
 			volumePaths: []string{

pkg/kubelet/volumemanager/reconciler/reconstruct_common.go

@@ -94,6 +94,8 @@ func (rc *reconciler) cleanupMounts(volume podVolume) {
 	klog.V(2).InfoS("Reconciler sync states: could not find volume information in desired state, clean up the mount points", "podName", volume.podName, "volumeSpecName", volume.volumeSpecName)
 	mountedVolume := operationexecutor.MountedVolume{
 		PodName: volume.podName,
+		// VolumeName should be generated by `GetUniqueVolumeNameFromSpec` or `GetUniqueVolumeNameFromSpecWithPod`.
+		// However, since we don't have the volume information in asw when cleanup mounts, it doesn't matter what we put here.
 		VolumeName:          v1.UniqueVolumeName(volume.volumeSpecName),
 		InnerVolumeSpecName: volume.volumeSpecName,
 		PluginName:          volume.pluginName,
@@ -256,8 +258,6 @@ func (rc *reconciler) reconstructVolume(volume podVolume) (rvolume *reconstructe
 	var volumeMapper volumepkg.BlockVolumeMapper
 	var volumeMounter volumepkg.Mounter
 	var deviceMounter volumepkg.DeviceMounter
-	// Path to the mount or block device to check
-	var checkPath string
 
 	if volume.volumeMode == v1.PersistentVolumeBlock {
 		var newMapperErr error
@@ -274,8 +274,6 @@ func (rc *reconciler) reconstructVolume(volume podVolume) (rvolume *reconstructe
 				pod.UID,
 				newMapperErr)
 		}
-		mapDir, linkName := volumeMapper.GetPodDeviceMapPath()
-		checkPath = filepath.Join(mapDir, linkName)
 	} else {
 		var err error
 		volumeMounter, err = plugin.NewMounter(
@@ -291,7 +289,6 @@ func (rc *reconciler) reconstructVolume(volume podVolume) (rvolume *reconstructe
 				pod.UID,
 				err)
 		}
-		checkPath = volumeMounter.GetPath()
 		if deviceMountablePlugin != nil {
 			deviceMounter, err = deviceMountablePlugin.NewDeviceMounter()
 			if err != nil {
@@ -305,16 +302,6 @@ func (rc *reconciler) reconstructVolume(volume podVolume) (rvolume *reconstructe
 		}
 	}
 
-	// Check existence of mount point for filesystem volume or symbolic link for block volume
-	isExist, checkErr := rc.operationExecutor.CheckVolumeExistenceOperation(volumeSpec, checkPath, volumeSpec.Name(), rc.mounter, uniqueVolumeName, volume.podName, pod.UID, attachablePlugin)
-	if checkErr != nil {
-		return nil, checkErr
-	}
-	// If mount or symlink doesn't exist, volume reconstruction should be failed
-	if !isExist {
-		return nil, fmt.Errorf("volume: %q is not mounted", uniqueVolumeName)
-	}
-
 	reconstructedVolume := &reconstructedVolume{
 		volumeName: uniqueVolumeName,
 		podName:    volume.podName,

pkg/volume/testing/testing.go

@@ -95,6 +95,8 @@ const (
 	volumeNotMounted     = "volumeNotMounted"
 	volumeMountUncertain = "volumeMountUncertain"
 	volumeMounted        = "volumeMounted"
+
+	FailNewMounter = "fail-new-mounter"
 )
 
 // CommandScript is used to pre-configure a command that will be executed and
@@ -298,6 +300,9 @@ func (plugin *FakeVolumePlugin) SupportsSELinuxContextMount(spec *volume.Spec) (
 func (plugin *FakeVolumePlugin) NewMounter(spec *volume.Spec, pod *v1.Pod, opts volume.VolumeOptions) (volume.Mounter, error) {
 	plugin.Lock()
 	defer plugin.Unlock()
+	if spec.Name() == FailNewMounter {
+		return nil, fmt.Errorf("AlwaysFailNewMounter")
+	}
 	fakeVolume := plugin.getFakeVolume(&plugin.Mounters)
 	fakeVolume.Lock()
 	defer fakeVolume.Unlock()

pkg/volume/util/operationexecutor/operation_executor.go

@@ -28,7 +28,6 @@ import (
 	"github.com/go-logr/logr"
 
 	"k8s.io/klog/v2"
-	"k8s.io/mount-utils"
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -38,7 +37,6 @@ import (
 	"k8s.io/kubernetes/pkg/volume/util/hostutil"
 	"k8s.io/kubernetes/pkg/volume/util/nestedpendingoperations"
 	volumetypes "k8s.io/kubernetes/pkg/volume/util/types"
-	"k8s.io/kubernetes/pkg/volume/util/volumepathhandler"
 )
 
 // OperationExecutor defines a set of operations for attaching, detaching,
@@ -151,8 +149,6 @@ type OperationExecutor interface {
 	ExpandInUseVolume(volumeToMount VolumeToMount, actualStateOfWorld ActualStateOfWorldMounterUpdater, currentSize resource.Quantity) error
 	// ReconstructVolumeOperation construct a new volumeSpec and returns it created by plugin
 	ReconstructVolumeOperation(volumeMode v1.PersistentVolumeMode, plugin volume.VolumePlugin, mapperPlugin volume.BlockVolumePlugin, uid types.UID, podName volumetypes.UniquePodName, volumeSpecName string, volumePath string, pluginName string) (volume.ReconstructedVolume, error)
-	// CheckVolumeExistenceOperation checks volume existence
-	CheckVolumeExistenceOperation(volumeSpec *volume.Spec, mountPath, volumeName string, mounter mount.Interface, uniqueVolumeName v1.UniqueVolumeName, podName volumetypes.UniquePodName, podUID types.UID, attachable volume.AttachableVolumePlugin) (bool, error)
 }
 
 // NewOperationExecutor returns a new instance of OperationExecutor.
@@ -1092,61 +1088,3 @@ func (oe *operationExecutor) ReconstructVolumeOperation(
 		Spec: volumeSpec,
 	}, nil
 }
-
-// CheckVolumeExistenceOperation checks mount path directory if volume still exists
-func (oe *operationExecutor) CheckVolumeExistenceOperation(
-	volumeSpec *volume.Spec,
-	mountPath, volumeName string,
-	mounter mount.Interface,
-	uniqueVolumeName v1.UniqueVolumeName,
-	podName volumetypes.UniquePodName,
-	podUID types.UID,
-	attachable volume.AttachableVolumePlugin) (bool, error) {
-	fsVolume, err := util.CheckVolumeModeFilesystem(volumeSpec)
-	if err != nil {
-		return false, err
-	}
-
-	// Filesystem Volume case
-	// For attachable volume case, check mount path directory if volume is still existing and mounted.
-	// Return true if volume is mounted.
-	if fsVolume {
-		if attachable != nil {
-			var isNotMount bool
-			var mountCheckErr error
-			if mounter == nil {
-				return false, fmt.Errorf("mounter was not set for a filesystem volume")
-			}
-			if isNotMount, mountCheckErr = mount.IsNotMountPoint(mounter, mountPath); mountCheckErr != nil {
-				return false, fmt.Errorf("could not check whether the volume %q (spec.Name: %q) pod %q (UID: %q) is mounted with: %v",
-					uniqueVolumeName,
-					volumeName,
-					podName,
-					podUID,
-					mountCheckErr)
-			}
-			return !isNotMount, nil
-		}
-		// For non-attachable volume case, skip check and return true without mount point check
-		// since plugins may not have volume mount point.
-		return true, nil
-	}
-
-	// Block Volume case
-	// Check mount path directory if volume still exists, then return true if volume
-	// is there. Either plugin is attachable or non-attachable, the plugin should
-	// have symbolic link associated to raw block device under pod device map
-	// if volume exists.
-	blkutil := volumepathhandler.NewBlockVolumePathHandler()
-	var islinkExist bool
-	var checkErr error
-	if islinkExist, checkErr = blkutil.IsSymlinkExist(mountPath); checkErr != nil {
-		return false, fmt.Errorf("could not check whether the block volume %q (spec.Name: %q) pod %q (UID: %q) is mapped to: %v",
-			uniqueVolumeName,
-			volumeName,
-			podName,
-			podUID,
-			checkErr)
-	}
-	return islinkExist, nil
-}