From 091316040463777dc809b29f69e274c446f23edd Mon Sep 17 00:00:00 2001 From: huweiwen Date: Sun, 17 Dec 2023 11:23:22 +0800 Subject: [PATCH] kubelet/volumeManager: verifyVolumesMountedFunc checks both desired and actual This is intended to remove dependency on mountedVolume.OuterVolumeSpecNames --- .../cache/actual_state_of_world.go | 19 ++-- .../cache/desired_state_of_world.go | 17 ++++ .../cache/desired_state_of_world_test.go | 87 +++++++++++++++++++ pkg/kubelet/volumemanager/volume_manager.go | 36 +++----- 4 files changed, 126 insertions(+), 33 deletions(-) diff --git a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go b/pkg/kubelet/volumemanager/cache/actual_state_of_world.go index ea5cf573da3..55c42665bbf 100644 --- a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go +++ b/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 } } diff --git a/pkg/kubelet/volumemanager/cache/desired_state_of_world.go b/pkg/kubelet/volumemanager/cache/desired_state_of_world.go index f09c4bec21b..d61de903ed9 100644 --- a/pkg/kubelet/volumemanager/cache/desired_state_of_world.go +++ b/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() diff --git a/pkg/kubelet/volumemanager/cache/desired_state_of_world_test.go b/pkg/kubelet/volumemanager/cache/desired_state_of_world_test.go index 33b21e7a503..fe3f98e9dfc 100644 --- a/pkg/kubelet/volumemanager/cache/desired_state_of_world_test.go +++ b/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: 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) diff --git a/pkg/kubelet/volumemanager/volume_manager.go b/pkg/kubelet/volumemanager/volume_manager.go index b23a840ae2d..b1401b64172 100644 --- a/pkg/kubelet/volumemanager/volume_manager.go +++ b/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)