Skip to content

kubelet: multiple volumes reference one PVC in one Pod #122140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

kubelet: multiple volumes reference one PVC in one Pod #122140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0913160
kubelet/volumeManager: verifyVolumesMountedFunc checks both desired a…
huww98 Dec 17, 2023
c20b105
kubelet/volumeManager: GetMountedVolumesForPod() returns desired and …
huww98 Dec 17, 2023
50fec6f
kubelet/volumeManager: (Get -> Has)PossiblyMountedVolumesForPod
huww98 Dec 17, 2023
1d792b0
move timeout to only cover the line under test
huww98 Aug 4, 2025
c1f881b
remove MountedVolume.OuterVolumeSpecNames from logs
huww98 Dec 17, 2023
bee5826
kubelet/volumeManager: remove outerVolumeSpecName from ASW
huww98 Dec 17, 2023
5437577
kubelet: multiple volumes reference one PVC in one Pod
huww98 Nov 28, 2023
5aab30c
test: add assertion to check OuterVolumeSpecNames
huww98 Dec 12, 2023
dd8e00f
e2e: case for multiple volumes reference one PVC
huww98 Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions pkg/kubelet/kubelet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3049,25 +3049,19 @@ func TestSyncLabels(t *testing.T) {
func waitForVolumeUnmount(
volumeManager kubeletvolume.VolumeManager,
pod *v1.Pod) error {
var podVolumes kubecontainer.VolumeMap
err := retryWithExponentialBackOff(
time.Duration(50*time.Millisecond),
func() (bool, error) {
// Verify volumes detached
podVolumes = volumeManager.GetMountedVolumesForPod(
hasVolumes := volumeManager.HasPossiblyMountedVolumesForPod(
util.GetUniquePodName(pod))

if len(podVolumes) != 0 {
return false, nil
}

return true, nil
return !hasVolumes, nil
},
)

if err != nil {
return fmt.Errorf(
"Expected volumes to be unmounted. But some volumes are still mounted: %#v", podVolumes)
"Expected volumes to be unmounted. But some volumes are still mounted")
}

return nil
Expand Down
4 changes: 1 addition & 3 deletions pkg/kubelet/kubelet_volumes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,7 @@ func (kl *Kubelet) ListBlockVolumesForPod(podUID types.UID) (map[string]volume.B
// podVolumesExist checks with the volume manager and returns true any of the
// pods for the specified volume are mounted or are uncertain.
func (kl *Kubelet) podVolumesExist(podUID types.UID) bool {
if mountedVolumes :=
kl.volumeManager.GetPossiblyMountedVolumesForPod(
volumetypes.UniquePodName(podUID)); len(mountedVolumes) > 0 {
if kl.volumeManager.HasPossiblyMountedVolumesForPod(volumetypes.UniquePodName(podUID)) {
return true
}
// TODO: This checks pod volume paths and whether they are mounted. If checking returns error, podVolumesExist will return true
Expand Down
20 changes: 8 additions & 12 deletions pkg/kubelet/kubelet_volumes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,16 +470,12 @@ func TestVolumeUnmountAndDetachControllerDisabled(t *testing.T) {
kubelet.podManager.SetPods([]*v1.Pod{})

assert.NoError(t, kubelet.volumeManager.WaitForUnmount(context.Background(), pod))
if actual := kubelet.volumeManager.GetMountedVolumesForPod(util.GetUniquePodName(pod)); len(actual) > 0 {
t.Fatalf("expected volume unmount to wait for no volumes: %v", actual)
}

// Verify volumes unmounted
podVolumes = kubelet.volumeManager.GetMountedVolumesForPod(
hasMountedVolumes := kubelet.volumeManager.HasPossiblyMountedVolumesForPod(
util.GetUniquePodName(pod))

assert.Empty(t, podVolumes,
"Expected volumes to be unmounted and detached. But some volumes are still mounted: %#v", podVolumes)
assert.False(t, hasMountedVolumes, "Expected volumes to be unmounted and detached. But some volumes are still mounted")

assert.NoError(t, volumetest.VerifyTearDownCallCount(
1 /* expectedTearDownCallCount */, testKubelet.volumePlugin))
Expand Down Expand Up @@ -557,9 +553,9 @@ func TestVolumeAttachAndMountControllerEnabled(t *testing.T) {

podVolumes := kubelet.volumeManager.GetMountedVolumesForPod(
util.GetUniquePodName(pod))
allPodVolumes := kubelet.volumeManager.GetPossiblyMountedVolumesForPod(
hasVolumes := kubelet.volumeManager.HasPossiblyMountedVolumesForPod(
util.GetUniquePodName(pod))
assert.Equal(t, podVolumes, allPodVolumes, "GetMountedVolumesForPod and GetPossiblyMountedVolumesForPod should return the same volumes")
assert.True(t, hasVolumes, "HasPossiblyMountedVolumesForPod should return true")

expectedPodVolumes := []string{"vol1"}
assert.Len(t, podVolumes, len(expectedPodVolumes), "Volumes for pod %+v", pod)
Expand Down Expand Up @@ -644,9 +640,9 @@ func TestVolumeUnmountAndDetachControllerEnabled(t *testing.T) {

podVolumes := kubelet.volumeManager.GetMountedVolumesForPod(
util.GetUniquePodName(pod))
allPodVolumes := kubelet.volumeManager.GetPossiblyMountedVolumesForPod(
hasVolumes := kubelet.volumeManager.HasPossiblyMountedVolumesForPod(
util.GetUniquePodName(pod))
assert.Equal(t, podVolumes, allPodVolumes, "GetMountedVolumesForPod and GetPossiblyMountedVolumesForPod should return the same volumes")
assert.True(t, hasVolumes, "HasPossiblyMountedVolumesForPod should return true")

expectedPodVolumes := []string{"vol1"}
assert.Len(t, podVolumes, len(expectedPodVolumes), "Volumes for pod %+v", pod)
Expand All @@ -672,9 +668,9 @@ func TestVolumeUnmountAndDetachControllerEnabled(t *testing.T) {
// Verify volumes unmounted
podVolumes = kubelet.volumeManager.GetMountedVolumesForPod(
util.GetUniquePodName(pod))
allPodVolumes = kubelet.volumeManager.GetPossiblyMountedVolumesForPod(
hasVolumes = kubelet.volumeManager.HasPossiblyMountedVolumesForPod(
util.GetUniquePodName(pod))
assert.Equal(t, podVolumes, allPodVolumes, "GetMountedVolumesForPod and GetPossiblyMountedVolumesForPod should return the same volumes")
assert.False(t, hasVolumes, "HasPossiblyMountedVolumesForPod should return false")

assert.Empty(t, podVolumes,
"Expected volumes to be unmounted and detached. But some volumes are still mounted")
Expand Down
30 changes: 9 additions & 21 deletions pkg/kubelet/volumemanager/cache/actual_state_of_world.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -356,12 +356,6 @@ type mountedPod struct {
// /var/lib/kubelet/pods/{podUID}/volumes/{escapeQualifiedPluginName}/{volumeSpecName}/
volumeSpec *volume.Spec

// outerVolumeSpecName is the volume.Spec.Name() of the volume as referenced
// directly in the pod. If the volume was referenced through a persistent
// volume claim, this contains the volume.Spec.Name() of the persistent
// volume claim
outerVolumeSpecName string

// remountRequired indicates the underlying volume has been successfully
// mounted to this pod but it should be remounted to reflect changes in the
// referencing pod.
Expand Down Expand Up @@ -484,7 +478,6 @@ func (asw *actualStateOfWorld) CheckAndMarkVolumeAsUncertainViaReconstruction(op
volumeName := opts.VolumeName
mounter := opts.Mounter
blockVolumeMapper := opts.BlockVolumeMapper
outerVolumeSpecName := opts.OuterVolumeSpecName
volumeGIDValue := opts.VolumeGIDVolume
volumeSpec := opts.VolumeSpec

Expand All @@ -493,7 +486,6 @@ func (asw *actualStateOfWorld) CheckAndMarkVolumeAsUncertainViaReconstruction(op
podUID: podUID,
mounter: mounter,
blockVolumeMapper: blockVolumeMapper,
outerVolumeSpecName: outerVolumeSpecName,
volumeGIDValue: volumeGIDValue,
volumeSpec: volumeSpec,
remountRequired: false,
Expand Down Expand Up @@ -731,7 +723,6 @@ func (asw *actualStateOfWorld) AddPodToVolume(markVolumeOpts operationexecutor.M
volumeName := markVolumeOpts.VolumeName
mounter := markVolumeOpts.Mounter
blockVolumeMapper := markVolumeOpts.BlockVolumeMapper
outerVolumeSpecName := markVolumeOpts.OuterVolumeSpecName
volumeGIDValue := markVolumeOpts.VolumeGIDVolume
volumeSpec := markVolumeOpts.VolumeSpec
asw.Lock()
Expand Down Expand Up @@ -760,7 +751,6 @@ func (asw *actualStateOfWorld) AddPodToVolume(markVolumeOpts operationexecutor.M
podUID: podUID,
mounter: mounter,
blockVolumeMapper: blockVolumeMapper,
outerVolumeSpecName: outerVolumeSpecName,
volumeGIDValue: volumeGIDValue,
volumeSpec: volumeSpec,
volumeMountStateForPod: markVolumeOpts.VolumeMountState,
Expand Down Expand Up @@ -1102,15 +1092,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
}
}

Expand Down Expand Up @@ -1308,7 +1297,6 @@ func getMountedVolume(
PodName: mountedPod.podName,
VolumeName: attachedVolume.volumeName,
InnerVolumeSpecName: mountedPod.volumeSpec.Name(),
OuterVolumeSpecName: mountedPod.outerVolumeSpecName,
PluginName: attachedVolume.pluginName,
PodUID: mountedPod.podUID,
Mounter: mountedPod.mounter,
Expand Down
107 changes: 49 additions & 58 deletions pkg/kubelet/volumemanager/cache/actual_state_of_world_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,13 +232,12 @@ func Test_AddPodToVolume_Positive_ExistingVolumeNewNode(t *testing.T) {

// Act
markVolumeOpts := operationexecutor.MarkVolumeOpts{
PodName: podName,
PodUID: pod.UID,
VolumeName: generatedVolumeName,
Mounter: mounter,
BlockVolumeMapper: mapper,
OuterVolumeSpecName: volumeSpec.Name(),
VolumeSpec: volumeSpec,
PodName: podName,
PodUID: pod.UID,
VolumeName: generatedVolumeName,
Mounter: mounter,
BlockVolumeMapper: mapper,
VolumeSpec: volumeSpec,
}
err = asw.AddPodToVolume(markVolumeOpts)
// Assert
Expand Down Expand Up @@ -307,13 +306,12 @@ func Test_AddPodToVolume_Positive_ExistingVolumeExistingNode(t *testing.T) {
}

markVolumeOpts := operationexecutor.MarkVolumeOpts{
PodName: podName,
PodUID: pod.UID,
VolumeName: generatedVolumeName,
Mounter: mounter,
BlockVolumeMapper: mapper,
OuterVolumeSpecName: volumeSpec.Name(),
VolumeSpec: volumeSpec,
PodName: podName,
PodUID: pod.UID,
VolumeName: generatedVolumeName,
Mounter: mounter,
BlockVolumeMapper: mapper,
VolumeSpec: volumeSpec,
}
err = asw.AddPodToVolume(markVolumeOpts)
if err != nil {
Expand Down Expand Up @@ -415,13 +413,12 @@ func Test_AddTwoPodsToVolume_Positive(t *testing.T) {
}

markVolumeOpts1 := operationexecutor.MarkVolumeOpts{
PodName: podName1,
PodUID: pod1.UID,
VolumeName: generatedVolumeName1,
Mounter: mounter1,
BlockVolumeMapper: mapper1,
OuterVolumeSpecName: volumeSpec1.Name(),
VolumeSpec: volumeSpec1,
PodName: podName1,
PodUID: pod1.UID,
VolumeName: generatedVolumeName1,
Mounter: mounter1,
BlockVolumeMapper: mapper1,
VolumeSpec: volumeSpec1,
}
err = asw.AddPodToVolume(markVolumeOpts1)
if err != nil {
Expand All @@ -441,13 +438,12 @@ func Test_AddTwoPodsToVolume_Positive(t *testing.T) {
}

markVolumeOpts2 := operationexecutor.MarkVolumeOpts{
PodName: podName2,
PodUID: pod2.UID,
VolumeName: generatedVolumeName1,
Mounter: mounter2,
BlockVolumeMapper: mapper2,
OuterVolumeSpecName: volumeSpec2.Name(),
VolumeSpec: volumeSpec2,
PodName: podName2,
PodUID: pod2.UID,
VolumeName: generatedVolumeName1,
Mounter: mounter2,
BlockVolumeMapper: mapper2,
VolumeSpec: volumeSpec2,
}
err = asw.AddPodToVolume(markVolumeOpts2)
if err != nil {
Expand Down Expand Up @@ -603,14 +599,13 @@ func TestActualStateOfWorld_FoundDuringReconstruction(t *testing.T) {
}

markVolumeOpts1 := operationexecutor.MarkVolumeOpts{
PodName: podName1,
PodUID: pod1.UID,
VolumeName: generatedVolumeName1,
Mounter: mounter1,
BlockVolumeMapper: mapper1,
OuterVolumeSpecName: volumeSpec1.Name(),
VolumeSpec: volumeSpec1,
VolumeMountState: operationexecutor.VolumeMountUncertain,
PodName: podName1,
PodUID: pod1.UID,
VolumeName: generatedVolumeName1,
Mounter: mounter1,
BlockVolumeMapper: mapper1,
VolumeSpec: volumeSpec1,
VolumeMountState: operationexecutor.VolumeMountUncertain,
}
_, err = asw.CheckAndMarkVolumeAsUncertainViaReconstruction(markVolumeOpts1)
if err != nil {
Expand Down Expand Up @@ -687,13 +682,12 @@ func Test_MarkVolumeAsDetached_Negative_PodInVolume(t *testing.T) {
}

markVolumeOpts := operationexecutor.MarkVolumeOpts{
PodName: podName,
PodUID: pod.UID,
VolumeName: generatedVolumeName,
Mounter: mounter,
BlockVolumeMapper: mapper,
OuterVolumeSpecName: volumeSpec.Name(),
VolumeSpec: volumeSpec,
PodName: podName,
PodUID: pod.UID,
VolumeName: generatedVolumeName,
Mounter: mounter,
BlockVolumeMapper: mapper,
VolumeSpec: volumeSpec,
}
err = asw.AddPodToVolume(markVolumeOpts)
if err != nil {
Expand Down Expand Up @@ -794,13 +788,12 @@ func Test_AddPodToVolume_Negative_VolumeDoesntExist(t *testing.T) {

// Act
markVolumeOpts := operationexecutor.MarkVolumeOpts{
PodName: podName,
PodUID: pod.UID,
VolumeName: volumeName,
Mounter: mounter,
BlockVolumeMapper: mapper,
OuterVolumeSpecName: volumeSpec.Name(),
VolumeSpec: volumeSpec,
PodName: podName,
PodUID: pod.UID,
VolumeName: volumeName,
Mounter: mounter,
BlockVolumeMapper: mapper,
VolumeSpec: volumeSpec,
}
err = asw.AddPodToVolume(markVolumeOpts)
// Assert
Expand Down Expand Up @@ -930,7 +923,6 @@ func Test_AddPodToVolume_Positive_SELinux(t *testing.T) {
VolumeName: generatedVolumeName,
Mounter: mounter,
BlockVolumeMapper: mapper,
OuterVolumeSpecName: volumeSpec.Name(),
VolumeSpec: volumeSpec,
SELinuxMountContext: "system_u:object_r:container_file_t:s0:c0,c1",
VolumeMountState: operationexecutor.VolumeMounted,
Expand Down Expand Up @@ -1044,13 +1036,12 @@ func TestUncertainVolumeMounts(t *testing.T) {
}

markVolumeOpts1 := operationexecutor.MarkVolumeOpts{
PodName: podName1,
PodUID: pod1.UID,
VolumeName: generatedVolumeName1,
Mounter: mounter1,
OuterVolumeSpecName: volumeSpec1.Name(),
VolumeSpec: volumeSpec1,
VolumeMountState: operationexecutor.VolumeMountUncertain,
PodName: podName1,
PodUID: pod1.UID,
VolumeName: generatedVolumeName1,
Mounter: mounter1,
VolumeSpec: volumeSpec1,
VolumeMountState: operationexecutor.VolumeMountUncertain,
}
err = asw.AddPodToVolume(markVolumeOpts1)
if err != nil {
Expand Down
Loading