Merge pull request #122211 from gnufied/fix-uncertain-raw-block-devices

Fix device uncertain errors on reboot

pkg/kubelet/volumemanager/cache/actual_state_of_world.go

@@ -418,6 +418,13 @@ func (asw *actualStateOfWorld) IsVolumeReconstructed(volumeName v1.UniqueVolumeN
 	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()
@@ -810,6 +817,7 @@ func (asw *actualStateOfWorld) SetDeviceMountState(
 			volumeObj.seLinuxMountContext = &seLinuxMountContext
 		}
 	}
+
 	asw.attachedVolumes[volumeName] = volumeObj
 	return nil
 }

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(
@@ -2499,7 +2502,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 {

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)
@@ -268,18 +269,28 @@ func TestReconstructVolumesMount(t *testing.T) {
 		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.FailMountDeviceVolumeName),
+			volumeMode:      v1.PersistentVolumeBlock,
+			deviceMountPath: filepath.Join("plugins", "fake-plugin", "volumeDevices", "pluginDependentPath"),
 		},
 	}
 	for _, tc := range tests {
@@ -299,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
@@ -315,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 */)
@@ -342,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 tc.volumeMode == v1.PersistentVolumeFilesystem {
 					if err := volumetesting.VerifyMountDeviceCallCount(1, fakePlugin); err != nil {
 						lastErr = err
 						return false, nil
 					}
 					return true, nil
+				} else {
+					return true, nil
+				}
 			})
 			if err != nil {
 				t.Errorf("Error waiting for volumes to get mounted: %s: %s", err, lastErr)
@@ -377,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
@@ -384,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
+}

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()
+	fv.Lock()
-	defer fv.RUnlock()
+	defer fv.Unlock()
 	fv.GlobalMapPathCallCount++
 	return fv.getGlobalMapPath()
 }

pkg/volume/util/operationexecutor/operation_executor.go

@@ -229,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

pkg/volume/util/operationexecutor/operation_generator.go

@@ -780,6 +780,12 @@ 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.IsVolumeDeviceReconstructed(volumeToMount.VolumeName) {
+			klog.V(2).InfoS("MountVolume.markDeviceErrorState 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 {