diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 6502d9c210a..2002ae1be92 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -2019,18 +2019,15 @@ type PersistentVolumeClaimSpecValidationOptions struct { AllowReadWriteOncePod bool // Allow users to recover from previously failing expansion operation EnableRecoverFromExpansionFailure bool - // Allow assigning StorageClass to unbound PVCs retroactively - EnableRetroactiveDefaultStorageClass bool // Allow to validate the label value of the label selector AllowInvalidLabelValueInSelector bool } func ValidationOptionsForPersistentVolumeClaim(pvc, oldPvc *core.PersistentVolumeClaim) PersistentVolumeClaimSpecValidationOptions { opts := PersistentVolumeClaimSpecValidationOptions{ - AllowReadWriteOncePod: utilfeature.DefaultFeatureGate.Enabled(features.ReadWriteOncePod), - EnableRecoverFromExpansionFailure: utilfeature.DefaultFeatureGate.Enabled(features.RecoverVolumeExpansionFailure), - EnableRetroactiveDefaultStorageClass: utilfeature.DefaultFeatureGate.Enabled(features.RetroactiveDefaultStorageClass), - AllowInvalidLabelValueInSelector: false, + AllowReadWriteOncePod: utilfeature.DefaultFeatureGate.Enabled(features.ReadWriteOncePod), + EnableRecoverFromExpansionFailure: utilfeature.DefaultFeatureGate.Enabled(features.RecoverVolumeExpansionFailure), + AllowInvalidLabelValueInSelector: false, } if oldPvc == nil { // If there's no old PVC, use the options based solely on feature enablement @@ -2286,16 +2283,14 @@ func validateStorageClassUpgradeFromAnnotation(oldAnnotations, newAnnotations ma // Provide an upgrade path from PVC with nil storage class. We allow update of // StorageClassName only if following four conditions are met at the same time: -// 1. RetroactiveDefaultStorageClass FeatureGate is enabled -// 2. The new pvc's StorageClassName is not nil -// 3. The old pvc's StorageClassName is nil -// 4. The old pvc either does not have beta annotation set, or the beta annotation matches new pvc's StorageClassName +// 1. The new pvc's StorageClassName is not nil +// 2. The old pvc's StorageClassName is nil +// 3. The old pvc either does not have beta annotation set, or the beta annotation matches new pvc's StorageClassName func validateStorageClassUpgradeFromNil(oldAnnotations map[string]string, oldScName, newScName *string, opts PersistentVolumeClaimSpecValidationOptions) bool { oldAnnotation, oldAnnotationExist := oldAnnotations[core.BetaStorageClassAnnotation] - return opts.EnableRetroactiveDefaultStorageClass /* condition 1 */ && - newScName != nil /* condition 2 */ && - oldScName == nil /* condition 3 */ && - (!oldAnnotationExist || *newScName == oldAnnotation) /* condition 4 */ + return newScName != nil /* condition 1 */ && + oldScName == nil /* condition 2 */ && + (!oldAnnotationExist || *newScName == oldAnnotation) /* condition 3 */ } var resizeStatusSet = sets.NewString(string(core.PersistentVolumeClaimNoExpansionInProgress), diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 7474473f816..30b66d19e5a 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -2420,11 +2420,10 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) { }) scenarios := map[string]struct { - isExpectedFailure bool - oldClaim *core.PersistentVolumeClaim - newClaim *core.PersistentVolumeClaim - enableRecoverFromExpansion bool - enableRetroactiveDefaultStorageClass bool + isExpectedFailure bool + oldClaim *core.PersistentVolumeClaim + newClaim *core.PersistentVolumeClaim + enableRecoverFromExpansion bool }{ "valid-update-volumeName-only": { isExpectedFailure: false, @@ -2536,67 +2535,34 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) { newClaim: validClaimStorageClassInSpec, }, "valid-upgrade-nil-storage-class-spec-to-spec": { - isExpectedFailure: false, - oldClaim: validClaimStorageClassNil, - newClaim: validClaimStorageClassInSpec, - enableRetroactiveDefaultStorageClass: true, - // Feature enabled - change from nil sc name is valid if there is no beta annotation. - }, - "invalid-upgrade-nil-storage-class-spec-to-spec": { - isExpectedFailure: true, - oldClaim: validClaimStorageClassNil, - newClaim: validClaimStorageClassInSpec, - enableRetroactiveDefaultStorageClass: false, - // Feature disabled - change from nil sc name is invalid if there is no beta annotation. + isExpectedFailure: false, + oldClaim: validClaimStorageClassNil, + newClaim: validClaimStorageClassInSpec, }, "invalid-upgrade-not-nil-storage-class-spec-to-spec": { - isExpectedFailure: true, - oldClaim: validClaimStorageClassInSpec, - newClaim: validClaimStorageClassInSpecChanged, - enableRetroactiveDefaultStorageClass: true, - // Feature enablement must not allow non nil value change. + isExpectedFailure: true, + oldClaim: validClaimStorageClassInSpec, + newClaim: validClaimStorageClassInSpecChanged, }, "invalid-upgrade-to-nil-storage-class-spec-to-spec": { - isExpectedFailure: true, - oldClaim: validClaimStorageClassInSpec, - newClaim: validClaimStorageClassNil, - enableRetroactiveDefaultStorageClass: true, - // Feature enablement must not allow change to nil value change. - }, - "valid-upgrade-storage-class-annotation-and-nil-spec-to-spec": { - isExpectedFailure: false, - oldClaim: validClaimStorageClassInAnnotationAndNilInSpec, - newClaim: validClaimStorageClassInAnnotationAndSpec, - enableRetroactiveDefaultStorageClass: false, - // Change from nil sc name is valid if annotations match. + isExpectedFailure: true, + oldClaim: validClaimStorageClassInSpec, + newClaim: validClaimStorageClassNil, }, "valid-upgrade-storage-class-annotation-and-nil-spec-to-spec-retro": { - isExpectedFailure: false, - oldClaim: validClaimStorageClassInAnnotationAndNilInSpec, - newClaim: validClaimStorageClassInAnnotationAndSpec, - enableRetroactiveDefaultStorageClass: true, - // Change from nil sc name is valid if annotations match, feature enablement must not break this old behavior. - }, - "invalid-upgrade-storage-class-annotation-and-spec-to-spec": { - isExpectedFailure: true, - oldClaim: validClaimStorageClassInAnnotationAndSpec, - newClaim: validClaimStorageClassInSpecChanged, - enableRetroactiveDefaultStorageClass: false, - // Change from non nil sc name is invalid if annotations don't match. + isExpectedFailure: false, + oldClaim: validClaimStorageClassInAnnotationAndNilInSpec, + newClaim: validClaimStorageClassInAnnotationAndSpec, }, "invalid-upgrade-storage-class-annotation-and-spec-to-spec-retro": { - isExpectedFailure: true, - oldClaim: validClaimStorageClassInAnnotationAndSpec, - newClaim: validClaimStorageClassInSpecChanged, - enableRetroactiveDefaultStorageClass: true, - // Change from non nil sc name is invalid if annotations don't match, feature enablement must not break this old behavior. + isExpectedFailure: true, + oldClaim: validClaimStorageClassInAnnotationAndSpec, + newClaim: validClaimStorageClassInSpecChanged, }, "invalid-upgrade-storage-class-annotation-and-no-spec": { - isExpectedFailure: true, - oldClaim: validClaimStorageClassInAnnotationAndNilInSpec, - newClaim: validClaimStorageClassInSpecChanged, - enableRetroactiveDefaultStorageClass: true, - // Change from nil sc name is invalid if annotations don't match, feature enablement must not break this old behavior. + isExpectedFailure: true, + oldClaim: validClaimStorageClassInAnnotationAndNilInSpec, + newClaim: validClaimStorageClassInSpecChanged, }, "invalid-upgrade-storage-class-annotation-to-spec": { isExpectedFailure: true, @@ -2662,7 +2628,6 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) { for name, scenario := range scenarios { t.Run(name, func(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RecoverVolumeExpansionFailure, scenario.enableRecoverFromExpansion)() - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RetroactiveDefaultStorageClass, scenario.enableRetroactiveDefaultStorageClass)() scenario.oldClaim.ResourceVersion = "1" scenario.newClaim.ResourceVersion = "1" opts := ValidationOptionsForPersistentVolumeClaim(scenario.newClaim, scenario.oldClaim) @@ -2687,45 +2652,40 @@ func TestValidationOptionsForPersistentVolumeClaim(t *testing.T) { oldPvc: nil, enableReadWriteOncePod: true, expectValidationOpts: PersistentVolumeClaimSpecValidationOptions{ - AllowReadWriteOncePod: true, - EnableRecoverFromExpansionFailure: false, - EnableRetroactiveDefaultStorageClass: true, + AllowReadWriteOncePod: true, + EnableRecoverFromExpansionFailure: false, }, }, "rwop allowed because feature enabled": { oldPvc: pvcWithAccessModes([]core.PersistentVolumeAccessMode{core.ReadWriteOnce}), enableReadWriteOncePod: true, expectValidationOpts: PersistentVolumeClaimSpecValidationOptions{ - AllowReadWriteOncePod: true, - EnableRecoverFromExpansionFailure: false, - EnableRetroactiveDefaultStorageClass: true, + AllowReadWriteOncePod: true, + EnableRecoverFromExpansionFailure: false, }, }, "rwop not allowed because not used and feature disabled": { oldPvc: pvcWithAccessModes([]core.PersistentVolumeAccessMode{core.ReadWriteOnce}), enableReadWriteOncePod: false, expectValidationOpts: PersistentVolumeClaimSpecValidationOptions{ - AllowReadWriteOncePod: false, - EnableRecoverFromExpansionFailure: false, - EnableRetroactiveDefaultStorageClass: true, + AllowReadWriteOncePod: false, + EnableRecoverFromExpansionFailure: false, }, }, "rwop allowed because used and feature enabled": { oldPvc: pvcWithAccessModes([]core.PersistentVolumeAccessMode{core.ReadWriteOncePod}), enableReadWriteOncePod: true, expectValidationOpts: PersistentVolumeClaimSpecValidationOptions{ - AllowReadWriteOncePod: true, - EnableRecoverFromExpansionFailure: false, - EnableRetroactiveDefaultStorageClass: true, + AllowReadWriteOncePod: true, + EnableRecoverFromExpansionFailure: false, }, }, "rwop allowed because used and feature disabled": { oldPvc: pvcWithAccessModes([]core.PersistentVolumeAccessMode{core.ReadWriteOncePod}), enableReadWriteOncePod: false, expectValidationOpts: PersistentVolumeClaimSpecValidationOptions{ - AllowReadWriteOncePod: true, - EnableRecoverFromExpansionFailure: false, - EnableRetroactiveDefaultStorageClass: true, + AllowReadWriteOncePod: true, + EnableRecoverFromExpansionFailure: false, }, }, } @@ -2736,7 +2696,7 @@ func TestValidationOptionsForPersistentVolumeClaim(t *testing.T) { opts := ValidationOptionsForPersistentVolumeClaim(nil, tc.oldPvc) if opts != tc.expectValidationOpts { - t.Errorf("Expected opts: %+v, received: %+v", opts, tc.expectValidationOpts) + t.Errorf("Expected opts: %+v, received: %+v", tc.expectValidationOpts, opts) } }) } diff --git a/pkg/controller/volume/persistentvolume/pv_controller.go b/pkg/controller/volume/persistentvolume/pv_controller.go index 523f89197f5..2002c62db4f 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller.go +++ b/pkg/controller/volume/persistentvolume/pv_controller.go @@ -349,18 +349,16 @@ func (ctrl *PersistentVolumeController) syncUnboundClaim(ctx context.Context, cl // No PV could be found // OBSERVATION: pvc is "Pending", will retry - if utilfeature.DefaultFeatureGate.Enabled(features.RetroactiveDefaultStorageClass) { - logger.V(4).Info("FeatureGate is enabled, attempting to assign storage class to unbound PersistentVolumeClaim", "featureGate", features.RetroactiveDefaultStorageClass, "PVC", klog.KObj(claim)) - updated, err := ctrl.assignDefaultStorageClass(ctx, claim) - if err != nil { - metrics.RecordRetroactiveStorageClassMetric(false) - return fmt.Errorf("can't update PersistentVolumeClaim[%q]: %w", claimToClaimKey(claim), err) - } - if updated { - logger.V(4).Info("PersistentVolumeClaim update successful, restarting claim sync", "PVC", klog.KObj(claim)) - metrics.RecordRetroactiveStorageClassMetric(true) - return nil - } + logger.V(4).Info("Attempting to assign storage class to unbound PersistentVolumeClaim", "PVC", klog.KObj(claim)) + updated, err := ctrl.assignDefaultStorageClass(ctx, claim) + if err != nil { + metrics.RecordRetroactiveStorageClassMetric(false) + return fmt.Errorf("can't update PersistentVolumeClaim[%q]: %w", claimToClaimKey(claim), err) + } + if updated { + logger.V(4).Info("PersistentVolumeClaim update successful, restarting claim sync", "PVC", klog.KObj(claim)) + metrics.RecordRetroactiveStorageClassMetric(true) + return nil } switch { diff --git a/pkg/controller/volume/persistentvolume/pv_controller_test.go b/pkg/controller/volume/persistentvolume/pv_controller_test.go index 481d84579d9..d2014721517 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_test.go +++ b/pkg/controller/volume/persistentvolume/pv_controller_test.go @@ -754,8 +754,6 @@ func TestModifyDeletionFinalizers(t *testing.T) { } func TestRetroactiveStorageClassAssignment(t *testing.T) { - // Enable RetroactiveDefaultStorageClass feature gate. - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RetroactiveDefaultStorageClass, true)() tests := []struct { storageClasses []*storagev1.StorageClass tests []controllerTest diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 05280321a6c..b6db55cb8d9 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -668,6 +668,8 @@ const ( // owner: @RomanBednar // kep: https://kep.k8s.io/3333 // alpha: v1.25 + // beta: 1.26 + // stable: v1.28 // // Allow assigning StorageClass to unbound PVCs retroactively RetroactiveDefaultStorageClass featuregate.Feature = "RetroactiveDefaultStorageClass" @@ -1026,7 +1028,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS RecoverVolumeExpansionFailure: {Default: false, PreRelease: featuregate.Alpha}, - RetroactiveDefaultStorageClass: {Default: true, PreRelease: featuregate.Beta}, + RetroactiveDefaultStorageClass: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.29 RotateKubeletServerCertificate: {Default: true, PreRelease: featuregate.Beta}, diff --git a/test/integration/volume/persistent_volumes_test.go b/test/integration/volume/persistent_volumes_test.go index fcc24e4ffd5..ce597dc0cb8 100644 --- a/test/integration/volume/persistent_volumes_test.go +++ b/test/integration/volume/persistent_volumes_test.go @@ -19,9 +19,6 @@ package volume import ( "context" "fmt" - utilfeature "k8s.io/apiserver/pkg/util/feature" - featuregatetesting "k8s.io/component-base/featuregate/testing" - "k8s.io/kubernetes/pkg/features" "math/rand" "os" "strconv" @@ -1045,7 +1042,6 @@ func TestPersistentVolumeMultiPVsDiffAccessModes(t *testing.T) { // assignment and binding of PVCs with storage class name set to nil or "" with // and without presence of a default SC. func TestRetroactiveStorageClassAssignment(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RetroactiveDefaultStorageClass, true)() s := kubeapiservertesting.StartTestServerOrDie(t, nil, []string{"--disable-admission-plugins=DefaultStorageClass"}, framework.SharedEtcd()) defer s.TearDownFn() namespaceName := "retro-pvc-sc"