From ed0facacfa4a2d9dacf85da7bfa53688af384f84 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Tue, 5 Dec 2023 15:49:51 -0500 Subject: [PATCH 1/3] Fix device uncertain errors on reboot --- pkg/kubelet/volumemanager/cache/actual_state_of_world.go | 5 +++++ pkg/kubelet/volumemanager/reconciler/reconstruct_new_test.go | 4 ++++ pkg/volume/util/operationexecutor/operation_executor.go | 5 +++++ pkg/volume/util/operationexecutor/operation_generator.go | 5 +++++ 4 files changed, 19 insertions(+) diff --git a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go b/pkg/kubelet/volumemanager/cache/actual_state_of_world.go index ada8c4415e4..3d449dcc0ee 100644 --- a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go +++ b/pkg/kubelet/volumemanager/cache/actual_state_of_world.go @@ -414,6 +414,10 @@ func (asw *actualStateOfWorld) IsVolumeReconstructed(volumeName v1.UniqueVolumeN if !ok { return false } + + if podName == operationexecutor.EmptyUniquePodName { + return true + } _, foundPod := podMap[podName] return foundPod } @@ -810,6 +814,7 @@ func (asw *actualStateOfWorld) SetDeviceMountState( volumeObj.seLinuxMountContext = &seLinuxMountContext } } + asw.attachedVolumes[volumeName] = volumeObj return nil } diff --git a/pkg/kubelet/volumemanager/reconciler/reconstruct_new_test.go b/pkg/kubelet/volumemanager/reconciler/reconstruct_new_test.go index 2189d34991b..9968a09a24e 100644 --- a/pkg/kubelet/volumemanager/reconciler/reconstruct_new_test.go +++ b/pkg/kubelet/volumemanager/reconciler/reconstruct_new_test.go @@ -281,6 +281,10 @@ func TestReconstructVolumesMount(t *testing.T) { volumePath: filepath.Join("pod1uid", "volumes", "fake-plugin", volumetesting.FailOnSetupVolumeName), expectMount: false, }, + { + name: "reconstructed volume device map fails", + volumePath: filepath.Join("pod1uid", "volumeDevices", "fake-plugin", volumetesting.FailOnSetupVolumeName), + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/pkg/volume/util/operationexecutor/operation_executor.go b/pkg/volume/util/operationexecutor/operation_executor.go index f4d257b005a..b4887357719 100644 --- a/pkg/volume/util/operationexecutor/operation_executor.go +++ b/pkg/volume/util/operationexecutor/operation_executor.go @@ -39,6 +39,11 @@ import ( volumetypes "k8s.io/kubernetes/pkg/volume/util/types" ) +const ( + // EmptyUniquePodName is a UniquePodName for empty string. + EmptyUniquePodName volumetypes.UniquePodName = volumetypes.UniquePodName("") +) + // OperationExecutor defines a set of operations for attaching, detaching, // mounting, or unmounting a volume that are executed with a NewNestedPendingOperations which // prevents more than one operation from being triggered on the same volume. diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index a8183572564..df05542e7a2 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -780,6 +780,11 @@ func (og *operationGenerator) checkForFailedMount(volumeToMount VolumeToMount, m func (og *operationGenerator) markDeviceErrorState(volumeToMount VolumeToMount, devicePath, deviceMountPath string, mountError error, actualStateOfWorld ActualStateOfWorldMounterUpdater) { if volumetypes.IsOperationFinishedError(mountError) && actualStateOfWorld.GetDeviceMountState(volumeToMount.VolumeName) == DeviceMountUncertain { + if actualStateOfWorld.IsVolumeReconstructed(volumeToMount.VolumeName, EmptyUniquePodName) { + klog.V(2).InfoS("MountVolume.markDeviceErrorState uncertainDeviceFix leaving volume uncertain", "volumeName", volumeToMount.VolumeName) + return + } + // Only devices which were uncertain can be marked as unmounted markDeviceUnmountError := actualStateOfWorld.MarkDeviceAsUnmounted(volumeToMount.VolumeName) if markDeviceUnmountError != nil { From 56dd5ab10ff19aa9ebe438122a00472c4135de23 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 11 Dec 2023 17:15:16 -0500 Subject: [PATCH 2/3] Add tests for checking of uncertain device paths --- .../cache/actual_state_of_world.go | 2 +- .../reconciler/reconciler_test.go | 9 +- .../reconciler/reconstruct_new_test.go | 99 +++++++++++++++---- pkg/volume/testing/testing.go | 4 +- .../operationexecutor/operation_generator.go | 1 + 5 files changed, 92 insertions(+), 23 deletions(-) diff --git a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go b/pkg/kubelet/volumemanager/cache/actual_state_of_world.go index 3d449dcc0ee..ebc7869fd1f 100644 --- a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go +++ b/pkg/kubelet/volumemanager/cache/actual_state_of_world.go @@ -404,7 +404,7 @@ func (asw *actualStateOfWorld) IsVolumeReconstructed(volumeName v1.UniqueVolumeN volumeState := asw.GetVolumeMountState(volumeName, podName) // only uncertain volumes are reconstructed - if volumeState != operationexecutor.VolumeMountUncertain { + if volumeState != operationexecutor.VolumeMountUncertain && podName != operationexecutor.EmptyUniquePodName { return false } diff --git a/pkg/kubelet/volumemanager/reconciler/reconciler_test.go b/pkg/kubelet/volumemanager/reconciler/reconciler_test.go index 0aa8456a23a..2f03c5b5737 100644 --- a/pkg/kubelet/volumemanager/reconciler/reconciler_test.go +++ b/pkg/kubelet/volumemanager/reconciler/reconciler_test.go @@ -2245,7 +2245,7 @@ func getInlineFakePod(podName, podUUID, outerName, innerName string) *v1.Pod { return pod } -func getReconciler(kubeletDir string, t *testing.T, volumePaths []string) (Reconciler, *volumetesting.FakeVolumePlugin) { +func getReconciler(kubeletDir string, t *testing.T, volumePaths []string, kubeClient *fake.Clientset) (Reconciler, *volumetesting.FakeVolumePlugin) { node := getFakeNode() volumePluginMgr, fakePlugin := volumetesting.GetTestKubeletVolumePluginMgrWithNodeAndRoot(t, node, kubeletDir) tmpKubeletPodDir := filepath.Join(kubeletDir, "pods") @@ -2253,7 +2253,10 @@ func getReconciler(kubeletDir string, t *testing.T, volumePaths []string) (Recon dsw := cache.NewDesiredStateOfWorld(volumePluginMgr, seLinuxTranslator) asw := cache.NewActualStateOfWorld(nodeName, volumePluginMgr) - kubeClient := createTestClient() + if kubeClient == nil { + kubeClient = createTestClient() + } + fakeRecorder := &record.FakeRecorder{} fakeHandler := volumetesting.NewBlockVolumePathHandler() oex := operationexecutor.NewOperationExecutor(operationexecutor.NewOperationGenerator( @@ -2483,7 +2486,7 @@ func TestSyncStates(t *testing.T) { os.MkdirAll(vp, 0755) } - rc, fakePlugin := getReconciler(tmpKubeletDir, t, mountPaths) + rc, fakePlugin := getReconciler(tmpKubeletDir, t, mountPaths, nil /*custom kubeclient*/) rcInstance, _ := rc.(*reconciler) logger, _ := ktesting.NewTestContext(t) for _, tpodInfo := range tc.podInfos { diff --git a/pkg/kubelet/volumemanager/reconciler/reconstruct_new_test.go b/pkg/kubelet/volumemanager/reconciler/reconstruct_new_test.go index 9968a09a24e..c33ce87228a 100644 --- a/pkg/kubelet/volumemanager/reconciler/reconstruct_new_test.go +++ b/pkg/kubelet/volumemanager/reconciler/reconstruct_new_test.go @@ -24,6 +24,7 @@ import ( "testing" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" utilfeature "k8s.io/apiserver/pkg/util/feature" featuregatetesting "k8s.io/component-base/featuregate/testing" @@ -105,7 +106,7 @@ func TestReconstructVolumes(t *testing.T) { os.MkdirAll(vp, 0755) } - rc, fakePlugin := getReconciler(tmpKubeletDir, t, mountPaths) + rc, fakePlugin := getReconciler(tmpKubeletDir, t, mountPaths, nil /*custom kubeclient*/) rcInstance, _ := rc.(*reconciler) // Act @@ -203,7 +204,7 @@ func TestCleanOrphanVolumes(t *testing.T) { mountPaths := []string{} - rc, fakePlugin := getReconciler(tmpKubeletDir, t, mountPaths) + rc, fakePlugin := getReconciler(tmpKubeletDir, t, mountPaths, nil /*custom kubeclient*/) rcInstance, _ := rc.(*reconciler) rcInstance.volumesFailedReconstruction = tc.volumesFailedReconstruction logger, _ := ktesting.NewTestContext(t) @@ -265,25 +266,31 @@ func TestReconstructVolumesMount(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.NewVolumeManagerReconstruction, true)() tests := []struct { - name string - volumePath string - expectMount bool + name string + volumePath string + expectMount bool + volumeMode v1.PersistentVolumeMode + deviceMountPath string }{ { name: "reconstructed volume is mounted", volumePath: filepath.Join("pod1uid", "volumes", "fake-plugin", "volumename"), expectMount: true, + volumeMode: v1.PersistentVolumeFilesystem, }, { name: "reconstructed volume fails to mount", // FailOnSetupVolumeName: MountDevice succeeds, SetUp fails volumePath: filepath.Join("pod1uid", "volumes", "fake-plugin", volumetesting.FailOnSetupVolumeName), expectMount: false, + volumeMode: v1.PersistentVolumeFilesystem, }, { - name: "reconstructed volume device map fails", - volumePath: filepath.Join("pod1uid", "volumeDevices", "fake-plugin", volumetesting.FailOnSetupVolumeName), + name: "reconstructed volume device map fails", + volumePath: filepath.Join("pod1uid", "volumeDevices", "fake-plugin", volumetesting.FailMountDeviceVolumeName), + volumeMode: v1.PersistentVolumeBlock, + deviceMountPath: filepath.Join("plugins", "fake-plugin", "volumeDevices", "pluginDependentPath"), }, } for _, tc := range tests { @@ -303,7 +310,16 @@ func TestReconstructVolumesMount(t *testing.T) { mountPaths := []string{vp} os.MkdirAll(vp, 0755) - rc, fakePlugin := getReconciler(tmpKubeletDir, t, mountPaths) + // Arrange 2 - populate DSW + outerName := filepath.Base(tc.volumePath) + pod, pv, pvc := getPodPVCAndPV(tc.volumeMode, "pod1", outerName, "pvc1") + volumeSpec := &volume.Spec{PersistentVolume: pv} + kubeClient := createtestClientWithPVPVC(pv, pvc, v1.AttachedVolume{ + Name: v1.UniqueVolumeName(fmt.Sprintf("fake-plugin/%s", outerName)), + DevicePath: "fake/path", + }) + + rc, fakePlugin := getReconciler(tmpKubeletDir, t, mountPaths, kubeClient /*custom kubeclient*/) rcInstance, _ := rc.(*reconciler) // Act 1 - reconstruction @@ -319,10 +335,6 @@ func TestReconstructVolumesMount(t *testing.T) { t.Errorf("expected 1 uncertain volume in asw, got %+v", allPods) } - // Arrange 2 - populate DSW - outerName := filepath.Base(tc.volumePath) - pod := getInlineFakePod("pod1", "pod1uid", outerName, outerName) - volumeSpec := &volume.Spec{Volume: &pod.Spec.Volumes[0]} podName := util.GetUniquePodName(pod) volumeName, err := rcInstance.desiredStateOfWorld.AddPodToVolume( podName, pod, volumeSpec, volumeSpec.Name(), "" /* volumeGidValue */, nil /* SELinuxContext */) @@ -346,12 +358,15 @@ func TestReconstructVolumesMount(t *testing.T) { // MountDevice was attempted var lastErr error err = retryWithExponentialBackOff(testOperationBackOffDuration, func() (bool, error) { - // MountDevice should always be called and succeed - if err := volumetesting.VerifyMountDeviceCallCount(1, fakePlugin); err != nil { - lastErr = err - return false, nil + if tc.volumeMode == v1.PersistentVolumeFilesystem { + if err := volumetesting.VerifyMountDeviceCallCount(1, fakePlugin); err != nil { + lastErr = err + return false, nil + } + return true, nil + } else { + return true, nil } - return true, nil }) if err != nil { t.Errorf("Error waiting for volumes to get mounted: %s: %s", err, lastErr) @@ -381,6 +396,14 @@ func TestReconstructVolumesMount(t *testing.T) { if len(allPods) != 1 { t.Errorf("expected 1 mounted or uncertain volumes after reconcile, got %+v", allPods) } + if tc.deviceMountPath != "" { + expectedDeviceMountPath := filepath.Join(tmpKubeletDir, tc.deviceMountPath) + deviceMountPath := allPods[0].DeviceMountPath + if expectedDeviceMountPath != deviceMountPath { + t.Errorf("expected deviceMountPath to be %s, got %s", expectedDeviceMountPath, deviceMountPath) + } + } + } // Unmount was *not* attempted in any case @@ -388,3 +411,45 @@ func TestReconstructVolumesMount(t *testing.T) { }) } } + +func getPodPVCAndPV(volumeMode v1.PersistentVolumeMode, podName, pvName, pvcName string) (*v1.Pod, *v1.PersistentVolume, *v1.PersistentVolumeClaim) { + pv := &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvName, + UID: "pvuid", + }, + Spec: v1.PersistentVolumeSpec{ + ClaimRef: &v1.ObjectReference{Name: pvcName}, + VolumeMode: &volumeMode, + }, + } + pvc := &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvcName, + UID: "pvcuid", + }, + Spec: v1.PersistentVolumeClaimSpec{ + VolumeName: pvName, + VolumeMode: &volumeMode, + }, + } + pod := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: podName, + UID: "pod1uid", + }, + Spec: v1.PodSpec{ + Volumes: []v1.Volume{ + { + Name: "volume-name", + VolumeSource: v1.VolumeSource{ + PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ + ClaimName: pvc.Name, + }, + }, + }, + }, + }, + } + return pod, pv, pvc +} diff --git a/pkg/volume/testing/testing.go b/pkg/volume/testing/testing.go index 5eb82b68ea6..f0d4cc48175 100644 --- a/pkg/volume/testing/testing.go +++ b/pkg/volume/testing/testing.go @@ -869,8 +869,8 @@ func (fv *FakeVolume) GetSetUpDeviceCallCount() int { // Block volume support func (fv *FakeVolume) GetGlobalMapPath(spec *volume.Spec) (string, error) { - fv.RLock() - defer fv.RUnlock() + fv.Lock() + defer fv.Unlock() fv.GlobalMapPathCallCount++ return fv.getGlobalMapPath() } diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index df05542e7a2..68082507312 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -780,6 +780,7 @@ func (og *operationGenerator) checkForFailedMount(volumeToMount VolumeToMount, m func (og *operationGenerator) markDeviceErrorState(volumeToMount VolumeToMount, devicePath, deviceMountPath string, mountError error, actualStateOfWorld ActualStateOfWorldMounterUpdater) { if volumetypes.IsOperationFinishedError(mountError) && actualStateOfWorld.GetDeviceMountState(volumeToMount.VolumeName) == DeviceMountUncertain { + if actualStateOfWorld.IsVolumeReconstructed(volumeToMount.VolumeName, EmptyUniquePodName) { klog.V(2).InfoS("MountVolume.markDeviceErrorState uncertainDeviceFix leaving volume uncertain", "volumeName", volumeToMount.VolumeName) return From e706b6ba14b5460e6ddc0c2bedc1cdb3639d8666 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Wed, 13 Dec 2023 10:18:57 -0500 Subject: [PATCH 3/3] Use a separate function for checking if device was reconstructed --- .../volumemanager/cache/actual_state_of_world.go | 13 ++++++++----- .../util/operationexecutor/operation_executor.go | 9 ++++----- .../util/operationexecutor/operation_generator.go | 4 ++-- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go b/pkg/kubelet/volumemanager/cache/actual_state_of_world.go index ebc7869fd1f..f0e53131499 100644 --- a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go +++ b/pkg/kubelet/volumemanager/cache/actual_state_of_world.go @@ -404,7 +404,7 @@ func (asw *actualStateOfWorld) IsVolumeReconstructed(volumeName v1.UniqueVolumeN volumeState := asw.GetVolumeMountState(volumeName, podName) // only uncertain volumes are reconstructed - if volumeState != operationexecutor.VolumeMountUncertain && podName != operationexecutor.EmptyUniquePodName { + if volumeState != operationexecutor.VolumeMountUncertain { return false } @@ -414,14 +414,17 @@ func (asw *actualStateOfWorld) IsVolumeReconstructed(volumeName v1.UniqueVolumeN if !ok { return false } - - if podName == operationexecutor.EmptyUniquePodName { - return true - } _, foundPod := podMap[podName] return foundPod } +func (asw *actualStateOfWorld) IsVolumeDeviceReconstructed(volumeName v1.UniqueVolumeName) bool { + asw.RLock() + defer asw.RUnlock() + _, ok := asw.foundDuringReconstruction[volumeName] + return ok +} + func (asw *actualStateOfWorld) CheckAndMarkVolumeAsUncertainViaReconstruction(opts operationexecutor.MarkVolumeOpts) (bool, error) { asw.Lock() defer asw.Unlock() diff --git a/pkg/volume/util/operationexecutor/operation_executor.go b/pkg/volume/util/operationexecutor/operation_executor.go index b4887357719..0fa04e427e4 100644 --- a/pkg/volume/util/operationexecutor/operation_executor.go +++ b/pkg/volume/util/operationexecutor/operation_executor.go @@ -39,11 +39,6 @@ import ( volumetypes "k8s.io/kubernetes/pkg/volume/util/types" ) -const ( - // EmptyUniquePodName is a UniquePodName for empty string. - EmptyUniquePodName volumetypes.UniquePodName = volumetypes.UniquePodName("") -) - // OperationExecutor defines a set of operations for attaching, detaching, // mounting, or unmounting a volume that are executed with a NewNestedPendingOperations which // prevents more than one operation from being triggered on the same volume. @@ -234,6 +229,10 @@ type ActualStateOfWorldMounterUpdater interface { // IsVolumeReconstructed returns true if volume currently added to actual state of the world // was found during reconstruction. IsVolumeReconstructed(volumeName v1.UniqueVolumeName, podName volumetypes.UniquePodName) bool + + // IsVolumeDeviceReconstructed returns true if volume device identified by volumeName has been + // found during reconstruction. + IsVolumeDeviceReconstructed(volumeName v1.UniqueVolumeName) bool } // ActualStateOfWorldAttacherUpdater defines a set of operations updating the diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 68082507312..c880aa54cda 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -781,8 +781,8 @@ func (og *operationGenerator) markDeviceErrorState(volumeToMount VolumeToMount, if volumetypes.IsOperationFinishedError(mountError) && actualStateOfWorld.GetDeviceMountState(volumeToMount.VolumeName) == DeviceMountUncertain { - if actualStateOfWorld.IsVolumeReconstructed(volumeToMount.VolumeName, EmptyUniquePodName) { - klog.V(2).InfoS("MountVolume.markDeviceErrorState uncertainDeviceFix leaving volume uncertain", "volumeName", volumeToMount.VolumeName) + if actualStateOfWorld.IsVolumeDeviceReconstructed(volumeToMount.VolumeName) { + klog.V(2).InfoS("MountVolume.markDeviceErrorState leaving volume uncertain", "volumeName", volumeToMount.VolumeName) return }