Skip to content

Commit a5d5a17

Browse files
sbezverksbezverk
committed
VolumeMode Validation and tests
Signed-off-by: Serguei Bezverkhi <[email protected]>
1 parent b2a0315 commit a5d5a17

File tree

10 files changed

+192
-175
lines changed

10 files changed

+192
-175
lines changed

‎pkg/api/persistentvolume/util.go‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,8 @@ import (
2525
// DropDisabledFields removes disabled fields from the pv spec.
2626
// This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a pv spec.
2727
func DropDisabledFields(pvSpec *api.PersistentVolumeSpec, oldPVSpec *api.PersistentVolumeSpec) {
28-
if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) {
29-
// TODO(liggitt): change this to only drop pvSpec.VolumeMode if (oldPVSpec == nil || oldPVSpec.VolumeMode == nil)
30-
// Requires more coordinated changes to validation
28+
if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) && !volumeModeInUse(oldPVSpec) {
3129
pvSpec.VolumeMode = nil
32-
if oldPVSpec != nil {
33-
oldPVSpec.VolumeMode = nil
34-
}
3530
}
3631

3732
if !utilfeature.DefaultFeatureGate.Enabled(features.CSIPersistentVolume) {
@@ -41,3 +36,13 @@ func DropDisabledFields(pvSpec *api.PersistentVolumeSpec, oldPVSpec *api.Persist
4136
}
4237
}
4338
}
39+
40+
func volumeModeInUse(oldPVSpec *api.PersistentVolumeSpec) bool {
41+
if oldPVSpec == nil {
42+
return false
43+
}
44+
if oldPVSpec.VolumeMode != nil {
45+
return true
46+
}
47+
return false
48+
}

‎pkg/api/persistentvolume/util_test.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -106,13 +106,12 @@ func TestDropDisabledFields(t *testing.T) {
106106
oldSpec: specWithMode(nil),
107107
expectOldSpec: specWithMode(nil),
108108
},
109-
// TODO: consider changing this case to preserve
110-
"disabled block clears old and new on update when old pv did use block": {
109+
"disabled block does not clear new on update when old pv did use block": {
111110
blockEnabled: false,
112111
newSpec: specWithMode(&modeBlock),
113-
expectNewSpec: specWithMode(nil),
112+
expectNewSpec: specWithMode(&modeBlock),
114113
oldSpec: specWithMode(&modeBlock),
115-
expectOldSpec: specWithMode(nil),
114+
expectOldSpec: specWithMode(&modeBlock),
116115
},
117116

118117
"enabled block preserves new": {

‎pkg/api/persistentvolumeclaim/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ go_test(
3737
deps = [
3838
"//pkg/apis/core:go_default_library",
3939
"//pkg/features:go_default_library",
40+
"//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
4041
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
4142
"//staging/src/k8s.io/apiserver/pkg/util/feature/testing:go_default_library",
4243
],

‎pkg/api/persistentvolumeclaim/util.go‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,17 @@ import (
2525
// DropDisabledFields removes disabled fields from the pvc spec.
2626
// This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a pvc spec.
2727
func DropDisabledFields(pvcSpec, oldPVCSpec *core.PersistentVolumeClaimSpec) {
28-
if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) {
28+
if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) && !volumeModeInUse(oldPVCSpec) {
2929
pvcSpec.VolumeMode = nil
30-
if oldPVCSpec != nil {
31-
oldPVCSpec.VolumeMode = nil
32-
}
3330
}
3431
}
32+
33+
func volumeModeInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) bool {
34+
if oldPVCSpec == nil {
35+
return false
36+
}
37+
if oldPVCSpec.VolumeMode != nil {
38+
return true
39+
}
40+
return false
41+
}

‎pkg/api/persistentvolumeclaim/util_test.go‎

Lines changed: 81 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,11 @@ limitations under the License.
1717
package persistentvolumeclaim
1818

1919
import (
20+
"fmt"
21+
"reflect"
2022
"testing"
2123

24+
"k8s.io/apimachinery/pkg/util/diff"
2225
utilfeature "k8s.io/apiserver/pkg/util/feature"
2326
utilfeaturetesting "k8s.io/apiserver/pkg/util/feature/testing"
2427
"k8s.io/kubernetes/pkg/apis/core"
@@ -28,33 +31,89 @@ import (
2831
func TestDropAlphaPVCVolumeMode(t *testing.T) {
2932
vmode := core.PersistentVolumeFilesystem
3033

31-
// PersistentVolume with VolumeMode set
32-
pvc := core.PersistentVolumeClaim{
33-
Spec: core.PersistentVolumeClaimSpec{
34-
AccessModes: []core.PersistentVolumeAccessMode{core.ReadWriteOnce},
35-
VolumeMode: &vmode,
34+
pvcWithoutVolumeMode := func() *core.PersistentVolumeClaim {
35+
return &core.PersistentVolumeClaim{
36+
Spec: core.PersistentVolumeClaimSpec{
37+
VolumeMode: nil,
38+
},
39+
}
40+
}
41+
pvcWithVolumeMode := func() *core.PersistentVolumeClaim {
42+
return &core.PersistentVolumeClaim{
43+
Spec: core.PersistentVolumeClaimSpec{
44+
VolumeMode: &vmode,
45+
},
46+
}
47+
}
48+
49+
pvcInfo := []struct {
50+
description string
51+
hasVolumeMode bool
52+
pvc func() *core.PersistentVolumeClaim
53+
}{
54+
{
55+
description: "pvc without VolumeMode",
56+
hasVolumeMode: false,
57+
pvc: pvcWithoutVolumeMode,
58+
},
59+
{
60+
description: "pvc with Filesystem VolumeMode",
61+
hasVolumeMode: true,
62+
pvc: pvcWithVolumeMode,
63+
},
64+
{
65+
description: "is nil",
66+
hasVolumeMode: false,
67+
pvc: func() *core.PersistentVolumeClaim { return nil },
3668
},
3769
}
3870

39-
// Enable alpha feature BlockVolume
40-
defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, true)()
41-
// now test dropping the fields - should not be dropped
42-
DropDisabledFields(&pvc.Spec, nil)
71+
for _, enabled := range []bool{true, false} {
72+
for _, oldpvcInfo := range pvcInfo {
73+
for _, newpvcInfo := range pvcInfo {
74+
oldpvcHasVolumeMode, oldpvc := oldpvcInfo.hasVolumeMode, oldpvcInfo.pvc()
75+
newpvcHasVolumeMode, newpvc := newpvcInfo.hasVolumeMode, newpvcInfo.pvc()
76+
if newpvc == nil {
77+
continue
78+
}
4379

44-
// check to make sure VolumeDevices is still present
45-
// if featureset is set to true
46-
if pvc.Spec.VolumeMode == nil {
47-
t.Error("VolumeMode in pvc.Spec should not have been dropped based on feature-gate")
48-
}
80+
t.Run(fmt.Sprintf("feature enabled=%v, old pvc %v, new pvc %v", enabled, oldpvcInfo.description, newpvcInfo.description), func(t *testing.T) {
81+
defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, enabled)()
82+
83+
var oldpvcSpec *core.PersistentVolumeClaimSpec
84+
if oldpvc != nil {
85+
oldpvcSpec = &oldpvc.Spec
86+
}
87+
DropDisabledFields(&newpvc.Spec, oldpvcSpec)
4988

50-
// Disable alpha feature BlockVolume
51-
defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, false)()
52-
// now test dropping the fields
53-
DropDisabledFields(&pvc.Spec, nil)
89+
// old pvc should never be changed
90+
if !reflect.DeepEqual(oldpvc, oldpvcInfo.pvc()) {
91+
t.Errorf("old pvc changed: %v", diff.ObjectReflectDiff(oldpvc, oldpvcInfo.pvc()))
92+
}
5493

55-
// check to make sure VolumeDevices is nil
56-
// if featureset is set to false
57-
if pvc.Spec.VolumeMode != nil {
58-
t.Error("DropDisabledFields VolumeMode for pvc.Spec failed")
94+
switch {
95+
case enabled || oldpvcHasVolumeMode:
96+
// new pvc should not be changed if the feature is enabled, or if the old pvc had BlockVolume
97+
if !reflect.DeepEqual(newpvc, newpvcInfo.pvc()) {
98+
t.Errorf("new pvc changed: %v", diff.ObjectReflectDiff(newpvc, newpvcInfo.pvc()))
99+
}
100+
case newpvcHasVolumeMode:
101+
// new pvc should be changed
102+
if reflect.DeepEqual(newpvc, newpvcInfo.pvc()) {
103+
t.Errorf("new pvc was not changed")
104+
}
105+
// new pvc should not have BlockVolume
106+
if !reflect.DeepEqual(newpvc, pvcWithoutVolumeMode()) {
107+
t.Errorf("new pvc had pvcBlockVolume: %v", diff.ObjectReflectDiff(newpvc, pvcWithoutVolumeMode()))
108+
}
109+
default:
110+
// new pvc should not need to be changed
111+
if !reflect.DeepEqual(newpvc, newpvcInfo.pvc()) {
112+
t.Errorf("new pvc changed: %v", diff.ObjectReflectDiff(newpvc, newpvcInfo.pvc()))
113+
}
114+
}
115+
})
116+
}
117+
}
59118
}
60119
}

‎pkg/apis/core/v1/BUILD‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ go_library(
1515
deps = [
1616
"//pkg/apis/apps:go_default_library",
1717
"//pkg/apis/core:go_default_library",
18-
"//pkg/features:go_default_library",
1918
"//pkg/util/parsers:go_default_library",
2019
"//staging/src/k8s.io/api/core/v1:go_default_library",
2120
"//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library",
@@ -26,7 +25,6 @@ go_library(
2625
"//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
2726
"//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library",
2827
"//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
29-
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
3028
"//vendor/k8s.io/utils/pointer:go_default_library",
3129
],
3230
)
@@ -44,7 +42,6 @@ go_test(
4442
"//pkg/apis/apps:go_default_library",
4543
"//pkg/apis/core:go_default_library",
4644
"//pkg/apis/core/fuzzer:go_default_library",
47-
"//pkg/features:go_default_library",
4845
"//staging/src/k8s.io/api/apps/v1:go_default_library",
4946
"//staging/src/k8s.io/api/core/v1:go_default_library",
5047
"//staging/src/k8s.io/apimachinery/pkg/api/apitesting/fuzzer:go_default_library",
@@ -55,8 +52,6 @@ go_test(
5552
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
5653
"//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
5754
"//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library",
58-
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
59-
"//staging/src/k8s.io/apiserver/pkg/util/feature/testing:go_default_library",
6055
"//vendor/k8s.io/utils/pointer:go_default_library",
6156
],
6257
)

‎pkg/apis/core/v1/defaults.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,6 @@ import (
2222
"k8s.io/api/core/v1"
2323
"k8s.io/apimachinery/pkg/runtime"
2424
"k8s.io/apimachinery/pkg/util/intstr"
25-
utilfeature "k8s.io/apiserver/pkg/util/feature"
26-
"k8s.io/kubernetes/pkg/features"
2725
"k8s.io/kubernetes/pkg/util/parsers"
2826
utilpointer "k8s.io/utils/pointer"
2927
)
@@ -247,7 +245,7 @@ func SetDefaults_PersistentVolume(obj *v1.PersistentVolume) {
247245
if obj.Spec.PersistentVolumeReclaimPolicy == "" {
248246
obj.Spec.PersistentVolumeReclaimPolicy = v1.PersistentVolumeReclaimRetain
249247
}
250-
if obj.Spec.VolumeMode == nil && utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) {
248+
if obj.Spec.VolumeMode == nil {
251249
obj.Spec.VolumeMode = new(v1.PersistentVolumeMode)
252250
*obj.Spec.VolumeMode = v1.PersistentVolumeFilesystem
253251
}
@@ -256,7 +254,7 @@ func SetDefaults_PersistentVolumeClaim(obj *v1.PersistentVolumeClaim) {
256254
if obj.Status.Phase == "" {
257255
obj.Status.Phase = v1.ClaimPending
258256
}
259-
if obj.Spec.VolumeMode == nil && utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) {
257+
if obj.Spec.VolumeMode == nil {
260258
obj.Spec.VolumeMode = new(v1.PersistentVolumeMode)
261259
*obj.Spec.VolumeMode = v1.PersistentVolumeFilesystem
262260
}

0 commit comments

Comments
 (0)