Skip to content
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

Jump to bottom

Automated cherry pick of #84049: Do not bind block PV/PVCs when block feature gate is off #84177

Merged
merged 1 commit into from
Oct 29, 2019
Merged

Automated cherry pick of #84049: Do not bind block PV/PVCs when block feature gate is off #84177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions pkg/controller/volume/persistentvolume/binder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -609,9 +609,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, 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, annBoundByController, annBindCompleted)),
noevents, noerrors, testSyncClaim,
},
{
Expand Down Expand Up @@ -645,9 +645,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, 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, annBoundByController, annBindCompleted)),
noevents, noerrors, testSyncClaim,
},
}
Expand Down
28 changes: 18 additions & 10 deletions pkg/controller/volume/persistentvolume/index.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
"fmt"
"sort"

"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"
"k8s.io/apimachinery/pkg/labels"
Expand Down Expand Up @@ -158,13 +158,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
}

Expand Down Expand Up @@ -260,9 +255,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.
Expand All @@ -275,7 +283,7 @@ func checkVolumeModeMismatches(pvcSpec *v1.PersistentVolumeClaimSpec, pvSpec *v1
if pvSpec.VolumeMode != nil {
pvVolumeMode = *pvSpec.VolumeMode
}
return requestedVolumeMode != pvVolumeMode, nil
return requestedVolumeMode != pvVolumeMode
}

// findBestMatchForClaim is a convenience method that finds a volume by the claim's AccessModes and requests for Storage
Expand Down
15 changes: 6 additions & 9 deletions pkg/controller/volume/persistentvolume/index_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -1135,19 +1135,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,
Expand All @@ -1163,10 +1163,7 @@ func TestVolumeModeCheck(t *testing.T) {
for name, scenario := range scenarios {
t.Run(name, func(t *testing.T) {
defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, scenario.enableBlock)()
expectedMismatch, err := checkVolumeModeMismatches(&scenario.pvc.Spec, &scenario.vol.Spec)
if err != nil {
t.Errorf("Unexpected failure for checkVolumeModeMismatches: %v", err)
}
expectedMismatch := 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)
Expand Down Expand Up @@ -1239,7 +1236,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,
Expand Down
10 changes: 3 additions & 7 deletions pkg/controller/volume/persistentvolume/pv_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (
"strings"
"time"

"k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
storage "k8s.io/api/storage/v1"
apierrs "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -276,11 +276,7 @@ func checkVolumeSatisfyClaim(volume *v1.PersistentVolume, claim *v1.PersistentVo
return fmt.Errorf("storageClassName does not match")
}

isMismatch, err := checkVolumeModeMismatches(&claim.Spec, &volume.Spec)
if err != nil {
return fmt.Errorf("error checking volumeMode: %v", err)
}
if isMismatch {
if checkVolumeModeMismatches(&claim.Spec, &volume.Spec) {
return fmt.Errorf("incompatible volumeMode")
}

Expand Down Expand Up @@ -610,7 +606,7 @@ func (ctrl *PersistentVolumeController) syncVolume(volume *v1.PersistentVolume)
}
return nil
} else if claim.Spec.VolumeName == "" {
if isMismatch, err := checkVolumeModeMismatches(&claim.Spec, &volume.Spec); err != nil || isMismatch {
if 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)
Expand Down