From 42722ffef0273a25bbad004c79b09bcbe7cab761 Mon Sep 17 00:00:00 2001 From: Roman Bednar Date: Thu, 18 May 2023 13:25:42 +0200 Subject: [PATCH 1/4] graduate RetroactiveDefaultStorageClass feature to GA in 1.28 --- pkg/features/kube_features.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index c10ef871ac0..e012edcb861 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}, From 97a81a59f62faa846599073c6d13a8f6a9c38251 Mon Sep 17 00:00:00 2001 From: Roman Bednar Date: Fri, 19 May 2023 13:16:12 +0200 Subject: [PATCH 2/4] test: correct validation test error message --- pkg/apis/core/validation/validation_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 7474473f816..28159d4ea89 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -2736,7 +2736,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) } }) } From 6afb363ca1780af2927664c18c9b1036e8d70851 Mon Sep 17 00:00:00 2001 From: Roman Bednar Date: Wed, 7 Jun 2023 14:26:13 +0200 Subject: [PATCH 3/4] test: remove RetroactiveDefaultStorageClass feature gate Since the feature is GA and locked to true, tests can no longer set it to false. Cleaning up by removing all references to this feature gate from tests. Feature gate will be removed in v1.29. --- pkg/apis/core/validation/validation_test.go | 104 ++++++------------ .../persistentvolume/pv_controller_test.go | 2 - .../volume/persistent_volumes_test.go | 4 - 3 files changed, 32 insertions(+), 78 deletions(-) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 28159d4ea89..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, }, }, } 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/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" From ac15d697578c4eb5ca0e64aef4e75f5567ca6689 Mon Sep 17 00:00:00 2001 From: Roman Bednar Date: Wed, 7 Jun 2023 14:27:49 +0200 Subject: [PATCH 4/4] remove RetroactiveDefaultStorageClass feature gate checks --- pkg/apis/core/validation/validation.go | 23 ++++++++----------- .../volume/persistentvolume/pv_controller.go | 22 ++++++++---------- 2 files changed, 19 insertions(+), 26 deletions(-) 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/controller/volume/persistentvolume/pv_controller.go b/pkg/controller/volume/persistentvolume/pv_controller.go index 562b2e0ab80..e5b69b23bd5 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller.go +++ b/pkg/controller/volume/persistentvolume/pv_controller.go @@ -353,18 +353,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 {