From a6ffbb41f8e7e7791d40436ebc2257834b81d661 Mon Sep 17 00:00:00 2001 From: weizhichen Date: Thu, 9 Mar 2023 08:42:27 +0000 Subject: [PATCH] Squashed commit of the following: commit 1b3ae27e7af577372d5aaaf28ea401eb33d1c4df Author: weizhichen Date: Thu Mar 9 08:39:04 2023 +0000 fix commit 566e139308e3cec4c9d4765eb4ccc3a735346c2e Author: weizhichen Date: Thu Mar 9 08:36:32 2023 +0000 fix unit test commit 13a58ebd25b824dcf854a132e9ac474c8296f0bf Author: weizhichen Date: Thu Mar 2 03:32:39 2023 +0000 add unit test commit c984e36e37c41bbef8aec46fe3fe81ab1c6a2521 Author: weizhichen Date: Tue Feb 28 15:25:56 2023 +0000 fix imports commit 58ec617e0ff1fbd209ca0af3237017679c3c0ad7 Author: weizhichen Date: Tue Feb 28 15:24:21 2023 +0000 delete CheckVolumeExistenceOperation commit 0d8cf0caa78bdf1f1f84ce011c4cc0e0de0e8707 Author: weizhichen Date: Tue Feb 28 14:29:37 2023 +0000 fix 111933 --- .../reconciler/reconciler_test.go | 63 ++++++++++++++++++- .../reconciler/reconstruct_common.go | 19 +----- pkg/volume/testing/testing.go | 5 ++ .../operationexecutor/operation_executor.go | 62 ------------------ 4 files changed, 69 insertions(+), 80 deletions(-) diff --git a/pkg/kubelet/volumemanager/reconciler/reconciler_test.go b/pkg/kubelet/volumemanager/reconciler/reconciler_test.go index e331f62e4c0..d13f3ad799f 100644 --- a/pkg/kubelet/volumemanager/reconciler/reconciler_test.go +++ b/pkg/kubelet/volumemanager/reconciler/reconciler_test.go @@ -2341,9 +2341,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) { @@ -2355,6 +2355,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{ diff --git a/pkg/kubelet/volumemanager/reconciler/reconstruct_common.go b/pkg/kubelet/volumemanager/reconciler/reconstruct_common.go index 255b89410cb..d7e088803fb 100644 --- a/pkg/kubelet/volumemanager/reconciler/reconstruct_common.go +++ b/pkg/kubelet/volumemanager/reconciler/reconstruct_common.go @@ -89,7 +89,9 @@ func (gvi *globalVolumeInfo) addPodVolume(rcv *reconstructedVolume) { 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, + 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, @@ -252,8 +254,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 @@ -270,8 +270,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( @@ -287,7 +285,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 { @@ -301,16 +298,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, diff --git a/pkg/volume/testing/testing.go b/pkg/volume/testing/testing.go index 38bfc08252a..11ae577702f 100644 --- a/pkg/volume/testing/testing.go +++ b/pkg/volume/testing/testing.go @@ -94,6 +94,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 @@ -297,6 +299,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() diff --git a/pkg/volume/util/operationexecutor/operation_executor.go b/pkg/volume/util/operationexecutor/operation_executor.go index 6785f58aab4..2a06466cfcc 100644 --- a/pkg/volume/util/operationexecutor/operation_executor.go +++ b/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. @@ -1089,61 +1085,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 -}