diff --git a/pkg/controller/volume/persistentvolume/binder_test.go b/pkg/controller/volume/persistentvolume/binder_test.go index bd191a9642c..c341649a5cf 100644 --- a/pkg/controller/volume/persistentvolume/binder_test.go +++ b/pkg/controller/volume/persistentvolume/binder_test.go @@ -19,7 +19,7 @@ package persistentvolume import ( "testing" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" storage "k8s.io/api/storage/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilfeature "k8s.io/apiserver/pkg/util/feature" @@ -621,9 +621,9 @@ func TestSyncBlockVolumeDisabled(t *testing.T) { // syncVolume binds a requested block claim to a block volume "14-1 - binding to volumeMode block", withVolumeVolumeMode(&modeBlock, newVolumeArray("volume14-1", "10Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classEmpty)), - withVolumeVolumeMode(&modeBlock, newVolumeArray("volume14-1", "10Gi", "uid14-1", "claim14-1", v1.VolumeBound, v1.PersistentVolumeReclaimRetain, classEmpty, pvutil.AnnBoundByController)), + withVolumeVolumeMode(&modeBlock, newVolumeArray("volume14-1", "10Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classEmpty)), + withClaimVolumeMode(&modeBlock, newClaimArray("claim14-1", "uid14-1", "10Gi", "", v1.ClaimPending, nil)), withClaimVolumeMode(&modeBlock, newClaimArray("claim14-1", "uid14-1", "10Gi", "", v1.ClaimPending, nil)), - withClaimVolumeMode(&modeBlock, newClaimArray("claim14-1", "uid14-1", "10Gi", "volume14-1", v1.ClaimBound, nil, pvutil.AnnBoundByController, pvutil.AnnBindCompleted)), noevents, noerrors, testSyncClaim, }, { @@ -657,9 +657,9 @@ func TestSyncBlockVolumeDisabled(t *testing.T) { // syncVolume binds a requested filesystem claim to an unspecified volumeMode for volume "14-5 - binding different volumeModes should be ignored", withVolumeVolumeMode(&modeBlock, newVolumeArray("volume14-5", "10Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classEmpty)), - withVolumeVolumeMode(&modeBlock, newVolumeArray("volume14-5", "10Gi", "uid14-5", "claim14-5", v1.VolumeBound, v1.PersistentVolumeReclaimRetain, classEmpty, pvutil.AnnBoundByController)), + withVolumeVolumeMode(&modeBlock, newVolumeArray("volume14-5", "10Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classEmpty)), + withClaimVolumeMode(&modeFile, newClaimArray("claim14-5", "uid14-5", "10Gi", "", v1.ClaimPending, nil)), withClaimVolumeMode(&modeFile, newClaimArray("claim14-5", "uid14-5", "10Gi", "", v1.ClaimPending, nil)), - withClaimVolumeMode(&modeFile, newClaimArray("claim14-5", "uid14-5", "10Gi", "volume14-5", v1.ClaimBound, nil, pvutil.AnnBoundByController, pvutil.AnnBindCompleted)), noevents, noerrors, testSyncClaim, }, } diff --git a/pkg/controller/volume/persistentvolume/index_test.go b/pkg/controller/volume/persistentvolume/index_test.go index 01a01aa31c1..f69e109cc2e 100644 --- a/pkg/controller/volume/persistentvolume/index_test.go +++ b/pkg/controller/volume/persistentvolume/index_test.go @@ -20,7 +20,7 @@ import ( "sort" "testing" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilfeature "k8s.io/apiserver/pkg/util/feature" @@ -1118,19 +1118,19 @@ func TestVolumeModeCheck(t *testing.T) { enableBlock: true, }, "feature disabled - pvc block and pv filesystem": { - isExpectedMismatch: false, + isExpectedMismatch: true, vol: createVolumeModeFilesystemTestVolume(), pvc: makeVolumeModePVC("8G", &blockMode, nil), enableBlock: false, }, "feature disabled - pvc filesystem and pv block": { - isExpectedMismatch: false, + isExpectedMismatch: true, vol: createVolumeModeBlockTestVolume(), pvc: makeVolumeModePVC("8G", &filesystemMode, nil), enableBlock: false, }, "feature disabled - pvc block and pv block": { - isExpectedMismatch: false, + isExpectedMismatch: true, vol: createVolumeModeBlockTestVolume(), pvc: makeVolumeModePVC("8G", &blockMode, nil), enableBlock: false, @@ -1146,10 +1146,7 @@ func TestVolumeModeCheck(t *testing.T) { for name, scenario := range scenarios { t.Run(name, func(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, scenario.enableBlock)() - expectedMismatch, err := pvutil.CheckVolumeModeMismatches(&scenario.pvc.Spec, &scenario.vol.Spec) - if err != nil { - t.Errorf("Unexpected failure for checkVolumeModeMismatches: %v", err) - } + expectedMismatch := pvutil.CheckVolumeModeMismatches(&scenario.pvc.Spec, &scenario.vol.Spec) // expected to match but either got an error or no returned pvmatch if expectedMismatch && !scenario.isExpectedMismatch { t.Errorf("Unexpected failure for scenario, expected not to mismatch on modes but did: %s", name) @@ -1222,7 +1219,7 @@ func TestFilteringVolumeModes(t *testing.T) { enableBlock: false, }, "2-2 feature disabled - pvc mode is block and pv mode is block - fields should be dropped by api and not analyzed with gate disabled": { - isExpectedMatch: true, + isExpectedMatch: false, vol: createTestVolOrderedIndex(createVolumeModeBlockTestVolume()), pvc: makeVolumeModePVC("8G", &blockMode, nil), enableBlock: false, diff --git a/pkg/controller/volume/persistentvolume/pv_controller.go b/pkg/controller/volume/persistentvolume/pv_controller.go index a6012339fbd..3d64ac00488 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller.go +++ b/pkg/controller/volume/persistentvolume/pv_controller.go @@ -268,11 +268,7 @@ func checkVolumeSatisfyClaim(volume *v1.PersistentVolume, claim *v1.PersistentVo return fmt.Errorf("storageClassName does not match") } - isMismatch, err := pvutil.CheckVolumeModeMismatches(&claim.Spec, &volume.Spec) - if err != nil { - return fmt.Errorf("error checking volumeMode: %v", err) - } - if isMismatch { + if pvutil.CheckVolumeModeMismatches(&claim.Spec, &volume.Spec) { return fmt.Errorf("incompatible volumeMode") } @@ -589,7 +585,7 @@ func (ctrl *PersistentVolumeController) syncVolume(volume *v1.PersistentVolume) } return nil } else if claim.Spec.VolumeName == "" { - if isMismatch, err := pvutil.CheckVolumeModeMismatches(&claim.Spec, &volume.Spec); err != nil || isMismatch { + if pvutil.CheckVolumeModeMismatches(&claim.Spec, &volume.Spec) { // Binding for the volume won't be called in syncUnboundClaim, // because findBestMatchForClaim won't return the volume due to volumeMode mismatch. volumeMsg := fmt.Sprintf("Cannot bind PersistentVolume to requested PersistentVolumeClaim %q due to incompatible volumeMode.", claim.Name) diff --git a/pkg/controller/volume/persistentvolume/util/util.go b/pkg/controller/volume/persistentvolume/util/util.go index 8eaedad7a1d..c9227484414 100644 --- a/pkg/controller/volume/persistentvolume/util/util.go +++ b/pkg/controller/volume/persistentvolume/util/util.go @@ -19,7 +19,7 @@ package persistentvolume import ( "fmt" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" storage "k8s.io/api/storage/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" @@ -207,13 +207,8 @@ func FindMatchingVolume( volumeQty := volume.Spec.Capacity[v1.ResourceStorage] - // check if volumeModes do not match (feature gate protected) - isMismatch, err := CheckVolumeModeMismatches(&claim.Spec, &volume.Spec) - if err != nil { - return nil, fmt.Errorf("error checking if volumeMode was a mismatch: %v", err) - } // filter out mismatching volumeModes - if isMismatch { + if CheckVolumeModeMismatches(&claim.Spec, &volume.Spec) { continue } @@ -309,9 +304,22 @@ func FindMatchingVolume( // CheckVolumeModeMismatches is a convenience method that checks volumeMode for PersistentVolume // and PersistentVolumeClaims -func CheckVolumeModeMismatches(pvcSpec *v1.PersistentVolumeClaimSpec, pvSpec *v1.PersistentVolumeSpec) (bool, error) { +func CheckVolumeModeMismatches(pvcSpec *v1.PersistentVolumeClaimSpec, pvSpec *v1.PersistentVolumeSpec) bool { if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) { - return false, nil + if pvcSpec.VolumeMode != nil && *pvcSpec.VolumeMode == v1.PersistentVolumeBlock { + // Block PVC does not match anything when the feature is off. We explicitly want + // to prevent binding block PVC to filesystem PV. + // The PVC should be ignored by PV controller. + return true + } + if pvSpec.VolumeMode != nil && *pvSpec.VolumeMode == v1.PersistentVolumeBlock { + // Block PV does not match anything when the feature is off. We explicitly want + // to prevent binding block PV to filesystem PVC. + // The PV should be ignored by PV controller. + return true + } + // Both PV + PVC are not block. + return false } // In HA upgrades, we cannot guarantee that the apiserver is on a version >= controller-manager. @@ -324,7 +332,7 @@ func CheckVolumeModeMismatches(pvcSpec *v1.PersistentVolumeClaimSpec, pvSpec *v1 if pvSpec.VolumeMode != nil { pvVolumeMode = *pvSpec.VolumeMode } - return requestedVolumeMode != pvVolumeMode, nil + return requestedVolumeMode != pvVolumeMode } // CheckAccessModes returns true if PV satisfies all the PVC's requested AccessModes