kubelet/volumeManager: verifyVolumesMountedFunc checks both desired and actual

This is intended to remove dependency on mountedVolume.OuterVolumeSpecNames

pkg/kubelet/volumemanager/cache/actual_state_of_world.go

@@ -149,9 +149,9 @@ type ActualStateOfWorld interface {
 	// current actual state of the world.
 	GetMountedVolumesForPod(podName volumetypes.UniquePodName) []MountedVolume
 
-	// GetMountedVolumeForPodByOuterVolumeSpecName returns the volume and true if
-	// the given outerVolumeSpecName is mounted on the given pod.
-	GetMountedVolumeForPodByOuterVolumeSpecName(podName volumetypes.UniquePodName, outerVolumeSpecName string) (MountedVolume, bool)
+	// GetMountedVolumeForPod returns the volume and true if
+	// the given name is mounted on the given pod.
+	GetMountedVolumeForPod(podName volumetypes.UniquePodName, volumeName v1.UniqueVolumeName) (MountedVolume, bool)
 
 	// GetPossiblyMountedVolumesForPod generates and returns a list of volumes for
 	// the specified pod that either are attached and mounted or are "uncertain",
@@ -1102,15 +1102,14 @@ func (asw *actualStateOfWorld) GetMountedVolumesForPod(
 	return mountedVolume
 }
 
-func (asw *actualStateOfWorld) GetMountedVolumeForPodByOuterVolumeSpecName(
-	podName volumetypes.UniquePodName, outerVolumeSpecName string) (MountedVolume, bool) {
+func (asw *actualStateOfWorld) GetMountedVolumeForPod(
+	podName volumetypes.UniquePodName, volumeName v1.UniqueVolumeName) (MountedVolume, bool) {
 	asw.RLock()
 	defer asw.RUnlock()
-	for _, volumeObj := range asw.attachedVolumes {
-		if podObj, hasPod := volumeObj.mountedPods[podName]; hasPod {
-			if podObj.volumeMountStateForPod == operationexecutor.VolumeMounted && podObj.outerVolumeSpecName == outerVolumeSpecName {
-				return getMountedVolume(&podObj, &volumeObj), true
-			}
+	volumeObj := asw.attachedVolumes[volumeName]
+	if podObj, hasPod := volumeObj.mountedPods[podName]; hasPod {
+		if podObj.volumeMountStateForPod == operationexecutor.VolumeMounted {
+			return getMountedVolume(&podObj, &volumeObj), true
 		}
 	}
 

pkg/kubelet/volumemanager/cache/desired_state_of_world.go

@@ -99,6 +99,9 @@ type DesiredStateOfWorld interface {
 	// attached volumes, false is returned.
 	PodExistsInVolume(podName types.UniquePodName, volumeName v1.UniqueVolumeName, seLinuxMountContext string) bool
 
+	// GetVolumeName returns the UniqueVolumeName for the given pod, indexed by outerVolumeSpecName.
+	GetVolumeNamesForPod(podName types.UniquePodName) map[string]v1.UniqueVolumeName
+
 	// GetVolumesToMount generates and returns a list of volumes that should be
 	// attached to this node and the pods they should be mounted to based on the
 	// current desired state of the world.
@@ -562,6 +565,20 @@ func (dsw *desiredStateOfWorld) GetPods() map[types.UniquePodName]bool {
 	return podList
 }
 
+func (dsw *desiredStateOfWorld) GetVolumeNamesForPod(podName types.UniquePodName) map[string]v1.UniqueVolumeName {
+	dsw.RLock()
+	defer dsw.RUnlock()
+
+	volumeNames := make(map[string]v1.UniqueVolumeName)
+	for volumeName, volumeObj := range dsw.volumesToMount {
+		podObj, ok := volumeObj.podsToMount[podName]
+		if ok {
+			volumeNames[podObj.outerVolumeSpecName] = volumeName
+		}
+	}
+	return volumeNames
+}
+
 func (dsw *desiredStateOfWorld) GetVolumesToMount() []VolumeToMount {
 	dsw.RLock()
 	defer dsw.RUnlock()

pkg/kubelet/volumemanager/cache/desired_state_of_world_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package cache
 
 import (
+	"maps"
 	"testing"
 
 	v1 "k8s.io/api/core/v1"
@@ -1275,6 +1276,92 @@ func Test_AddPodToVolume_SELinux_MultiplePods(t *testing.T) {
 	}
 }
 
+func TestGetVolumeNamesForPod(t *testing.T) {
+	logger, _ := ktesting.NewTestContext(t)
+	volumePluginMgr, _ := volumetesting.GetTestKubeletVolumePluginMgr(t)
+	seLinuxTranslator := util.NewFakeSELinuxLabelTranslator()
+	dsw := NewDesiredStateOfWorld(volumePluginMgr, seLinuxTranslator)
+
+	pod1 := &v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "pod1",
+			UID:  "pod1-uid",
+		},
+		Spec: v1.PodSpec{
+			Volumes: []v1.Volume{
+				{
+					Name: "volume1-name",
+					VolumeSource: v1.VolumeSource{
+						GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{
+							PDName: "fake-device1",
+						},
+					},
+				},
+				{
+					Name: "volume3-name",
+					VolumeSource: v1.VolumeSource{
+						GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{
+							PDName: "fake-device3",
+						},
+					},
+				},
+			},
+		},
+	}
+	pod2 := &v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "pod2",
+			UID:  "pod2-uid",
+		},
+		Spec: v1.PodSpec{
+			Volumes: []v1.Volume{
+				{
+					Name: "volume2-name",
+					VolumeSource: v1.VolumeSource{
+						GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{
+							PDName: "fake-device2",
+						},
+					},
+				},
+			},
+		},
+	}
+
+	addVolume := func(pod *v1.Pod, index int) {
+		volumeSpec := &volume.Spec{
+			Volume: &pod.Spec.Volumes[index],
+		}
+		podName := util.GetUniquePodName(pod)
+		_, err := dsw.AddPodToVolume(
+			logger, podName, pod, volumeSpec, volumeSpec.Name(), "" /* volumeGIDValue */, nil /* seLinuxContainerContexts */)
+		if err != nil {
+			t.Fatalf("AddPodToVolume failed. Expected: <no error> Actual: <%v>", err)
+		}
+	}
+
+	addVolume(pod1, 0)
+	addVolume(pod1, 1)
+	addVolume(pod2, 0)
+
+	verifyVolumeNames := func(pod *v1.Pod, expectedVolumeNames map[string]v1.UniqueVolumeName) {
+		t.Run(pod.Name, func(t *testing.T) {
+			podName := util.GetUniquePodName(pod)
+			actualVolumeNames := dsw.GetVolumeNamesForPod(podName)
+			if !maps.Equal(expectedVolumeNames, actualVolumeNames) {
+				t.Errorf("GetVolumeNamesForPod returned incorrect value. Expected: <%v> Actual: <%v>",
+					expectedVolumeNames, actualVolumeNames)
+			}
+		})
+	}
+	verifyVolumeNames(pod1, map[string]v1.UniqueVolumeName{
+		"volume1-name": "fake-plugin/fake-device1",
+		"volume3-name": "fake-plugin/fake-device3",
+	})
+	verifyVolumeNames(pod2, map[string]v1.UniqueVolumeName{
+		"volume2-name": "fake-plugin/fake-device2",
+	})
+}
+
 func verifyVolumeExistsDsw(
 	t *testing.T, expectedVolumeName v1.UniqueVolumeName, expectedSELinuxContext string, dsw DesiredStateOfWorld) {
 	volumeExists := dsw.VolumeExists(expectedVolumeName, expectedSELinuxContext)

pkg/kubelet/volumemanager/volume_manager.go

@@ -572,13 +572,7 @@ func (vm *volumeManager) verifyVolumesMountedFunc(podName types.UniquePodName, e
 		if errs := vm.desiredStateOfWorld.PopPodErrors(podName); len(errs) > 0 {
 			return true, errors.New(strings.Join(errs, "; "))
 		}
-		for _, expectedVolume := range expectedVolumes {
-			_, found := vm.actualStateOfWorld.GetMountedVolumeForPodByOuterVolumeSpecName(podName, expectedVolume)
-			if !found {
-				return false, nil
-			}
-		}
-		return true, nil
+		return len(vm.getUnmountedVolumes(podName, expectedVolumes)) == 0, nil
 	}
 }
 
@@ -593,25 +587,21 @@ func (vm *volumeManager) verifyVolumesUnmountedFunc(podName types.UniquePodName)
 	}
 }
 
-// getUnmountedVolumes fetches the current list of mounted volumes from
-// the actual state of the world, and uses it to process the list of
-// expectedVolumes. It returns a list of unmounted volumes.
+// getUnmountedVolumes returns a list of unmounted volumes.
+// This includes the volumes in expectedVolumes, but not in one of DSW/ASW.
 // The list also includes volume that may be mounted in uncertain state.
 func (vm *volumeManager) getUnmountedVolumes(podName types.UniquePodName, expectedVolumes []string) []string {
-	mountedVolumes := sets.New[string]()
-	for _, mountedVolume := range vm.actualStateOfWorld.GetMountedVolumesForPod(podName) {
-		mountedVolumes.Insert(mountedVolume.OuterVolumeSpecName)
-	}
-	return filterUnmountedVolumes(mountedVolumes, expectedVolumes)
-}
-
-// filterUnmountedVolumes adds each element of expectedVolumes that is not in
-// mountedVolumes to a list of unmountedVolumes and returns it.
-func filterUnmountedVolumes(mountedVolumes sets.Set[string], expectedVolumes []string) []string {
 	unmountedVolumes := []string{}
-	for _, expectedVolume := range expectedVolumes {
-		if !mountedVolumes.Has(expectedVolume) {
-			unmountedVolumes = append(unmountedVolumes, expectedVolume)
+	volumeNames := vm.desiredStateOfWorld.GetVolumeNamesForPod(podName)
+	for _, outerName := range expectedVolumes {
+		volumeName, ok := volumeNames[outerName]
+		if !ok {
+			unmountedVolumes = append(unmountedVolumes, outerName)
+			continue
+		}
+		_, ok = vm.actualStateOfWorld.GetMountedVolumeForPod(podName, volumeName)
+		if !ok {
+			unmountedVolumes = append(unmountedVolumes, outerName)
 		}
 	}
 	slices.Sort(unmountedVolumes)