diff --git a/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go b/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go index c6ec3e9d5ac..ac83f3c0168 100644 --- a/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go +++ b/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go @@ -505,19 +505,24 @@ func (dswp *desiredStateOfWorldPopulator) createVolumeSpec( pvcSource.ClaimName, pvcUID) - // TODO: remove feature gate check after no longer needed + // TODO: replace this with util.GetVolumeMode() when features.BlockVolume is removed. + // The function will return the right value then. + volumeMode := v1.PersistentVolumeFilesystem + if volumeSpec.PersistentVolume != nil && volumeSpec.PersistentVolume.Spec.VolumeMode != nil { + volumeMode = *volumeSpec.PersistentVolume.Spec.VolumeMode + } + + // TODO: remove features.BlockVolume checks / comments after no longer needed + // Error if a container has volumeMounts but the volumeMode of PVC isn't Filesystem. + // Do not check feature gate here to make sure even when the feature is disabled in kubelet, + // because controller-manager / API server can already contain block PVs / PVCs. + if mounts.Has(podVolume.Name) && volumeMode != v1.PersistentVolumeFilesystem { + return nil, nil, "", fmt.Errorf( + "volume %s has volumeMode %s, but is specified in volumeMounts", + podVolume.Name, + volumeMode) + } if utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) { - volumeMode, err := util.GetVolumeMode(volumeSpec) - if err != nil { - return nil, nil, "", err - } - // Error if a container has volumeMounts but the volumeMode of PVC isn't Filesystem - if mounts.Has(podVolume.Name) && volumeMode != v1.PersistentVolumeFilesystem { - return nil, nil, "", fmt.Errorf( - "volume %s has volumeMode %s, but is specified in volumeMounts", - podVolume.Name, - volumeMode) - } // Error if a container has volumeDevices but the volumeMode of PVC isn't Block if devices.Has(podVolume.Name) && volumeMode != v1.PersistentVolumeBlock { return nil, nil, "", fmt.Errorf( diff --git a/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator_test.go b/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator_test.go index 23727b18bda..c339545c932 100644 --- a/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator_test.go +++ b/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator_test.go @@ -389,6 +389,53 @@ func TestCreateVolumeSpec_Valid_File_VolumeMounts(t *testing.T) { pvc := &v1.PersistentVolumeClaim{ Spec: v1.PersistentVolumeClaimSpec{ VolumeName: "dswp-test-volume-name", + VolumeMode: &mode, + }, + Status: v1.PersistentVolumeClaimStatus{ + Phase: v1.ClaimBound, + }, + } + dswp, fakePodManager, _ := createDswpWithVolume(t, pv, pvc) + + // create pod + containers := []v1.Container{ + { + VolumeMounts: []v1.VolumeMount{ + { + Name: "dswp-test-volume-name", + MountPath: "/mnt", + }, + }, + }, + } + pod := createPodWithVolume("dswp-test-pod", "dswp-test-volume-name", "file-bound", containers) + + fakePodManager.AddPod(pod) + mountsMap, devicesMap := util.GetPodVolumeNames(pod) + _, volumeSpec, _, err := + dswp.createVolumeSpec(pod.Spec.Volumes[0], pod.Name, pod.Namespace, mountsMap, devicesMap) + + // Assert + if volumeSpec == nil || err != nil { + t.Fatalf("Failed to create volumeSpec with combination of filesystem mode and volumeMounts. err: %v", err) + } +} + +func TestCreateVolumeSpec_Valid_Nil_VolumeMounts(t *testing.T) { + // create dswp + pv := &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dswp-test-volume-name", + }, + Spec: v1.PersistentVolumeSpec{ + ClaimRef: &v1.ObjectReference{Namespace: "ns", Name: "file-bound"}, + VolumeMode: nil, + }, + } + pvc := &v1.PersistentVolumeClaim{ + Spec: v1.PersistentVolumeClaimSpec{ + VolumeName: "dswp-test-volume-name", + VolumeMode: nil, }, Status: v1.PersistentVolumeClaimStatus{ Phase: v1.ClaimBound, @@ -567,6 +614,56 @@ func TestCreateVolumeSpec_Invalid_Block_VolumeMounts(t *testing.T) { } } +func TestCreateVolumeSpec_Invalid_Block_VolumeMounts_Disabled(t *testing.T) { + // Enable BlockVolume feature gate + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, false)() + + // create dswp + mode := v1.PersistentVolumeBlock + pv := &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dswp-test-volume-name", + }, + Spec: v1.PersistentVolumeSpec{ + ClaimRef: &v1.ObjectReference{Namespace: "ns", Name: "block-bound"}, + VolumeMode: &mode, + }, + } + pvc := &v1.PersistentVolumeClaim{ + Spec: v1.PersistentVolumeClaimSpec{ + VolumeName: "dswp-test-volume-name", + VolumeMode: &mode, + }, + Status: v1.PersistentVolumeClaimStatus{ + Phase: v1.ClaimBound, + }, + } + dswp, fakePodManager, _ := createDswpWithVolume(t, pv, pvc) + + // create pod + containers := []v1.Container{ + { + VolumeMounts: []v1.VolumeMount{ + { + Name: "dswp-test-volume-name", + MountPath: "/mnt", + }, + }, + }, + } + pod := createPodWithVolume("dswp-test-pod", "dswp-test-volume-name", "block-bound", containers) + + fakePodManager.AddPod(pod) + mountsMap, devicesMap := util.GetPodVolumeNames(pod) + _, volumeSpec, _, err := + dswp.createVolumeSpec(pod.Spec.Volumes[0], pod.Name, pod.Namespace, mountsMap, devicesMap) + + // Assert + if volumeSpec != nil || err == nil { + t.Fatalf("Unexpected volumeMode and volumeMounts/volumeDevices combination is accepted") + } +} + func TestCheckVolumeFSResize(t *testing.T) { setCapacity := func(pv *v1.PersistentVolume, pvc *v1.PersistentVolumeClaim, capacity int) { pv.Spec.Capacity = volumeCapacity(capacity)