Skip to content

Commit

Permalink
Merge pull request kubernetes#111467 from RomanBednar/retro-sc-assign…
Browse files Browse the repository at this point in the history 
…ment

Allow retroactive storage class assigment to PVCs
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Aug 2, 2022
2 parents 236fd8e + 2f533cd commit 90f9a52
Show file tree
Hide file tree
Showing 8 changed files with 412 additions and 61 deletions.
32 changes: 28 additions & 4 deletions pkg/apis/core/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2027,12 +2027,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
}

func ValidationOptionsForPersistentVolumeClaim(pvc, oldPvc *core.PersistentVolumeClaim) PersistentVolumeClaimSpecValidationOptions {
opts := PersistentVolumeClaimSpecValidationOptions{
AllowReadWriteOncePod: utilfeature.DefaultFeatureGate.Enabled(features.ReadWriteOncePod),
EnableRecoverFromExpansionFailure: utilfeature.DefaultFeatureGate.Enabled(features.RecoverVolumeExpansionFailure),
AllowReadWriteOncePod: utilfeature.DefaultFeatureGate.Enabled(features.ReadWriteOncePod),
EnableRecoverFromExpansionFailure: utilfeature.DefaultFeatureGate.Enabled(features.RecoverVolumeExpansionFailure),
EnableRetroactiveDefaultStorageClass: utilfeature.DefaultFeatureGate.Enabled(features.RetroactiveDefaultStorageClass),
}
if oldPvc == nil {
// If there's no old PVC, use the options based solely on feature enablement
Expand Down Expand Up @@ -2168,14 +2171,21 @@ func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeCl
oldPvcClone.Spec.VolumeName = newPvcClone.Spec.VolumeName // +k8s:verify-mutation:reason=clone
}

if validateStorageClassUpgrade(oldPvcClone.Annotations, newPvcClone.Annotations,
if validateStorageClassUpgradeFromAnnotation(oldPvcClone.Annotations, newPvcClone.Annotations,
oldPvcClone.Spec.StorageClassName, newPvcClone.Spec.StorageClassName) {
newPvcClone.Spec.StorageClassName = nil
metav1.SetMetaDataAnnotation(&newPvcClone.ObjectMeta, core.BetaStorageClassAnnotation, oldPvcClone.Annotations[core.BetaStorageClassAnnotation])
} else {
// storageclass annotation should be immutable after creation
// TODO: remove Beta when no longer needed
allErrs = append(allErrs, ValidateImmutableAnnotation(newPvc.ObjectMeta.Annotations[v1.BetaStorageClassAnnotation], oldPvc.ObjectMeta.Annotations[v1.BetaStorageClassAnnotation], v1.BetaStorageClassAnnotation, field.NewPath("metadata"))...)

// If update from annotation to attribute failed we can attempt try to validate update from nil value.
if validateStorageClassUpgradeFromNil(oldPvc.Annotations, oldPvc.Spec.StorageClassName, newPvc.Spec.StorageClassName, opts) {
newPvcClone.Spec.StorageClassName = oldPvcClone.Spec.StorageClassName // +k8s:verify-mutation:reason=clone
}
// TODO: add a specific error with a hint that storage class name can not be changed
// (instead of letting spec comparison below return generic field forbidden error)
}

// lets make sure storage values are same.
Expand Down Expand Up @@ -2216,7 +2226,7 @@ func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeCl
// 2. The old pvc's StorageClassName is not set
// 3. The new pvc's StorageClassName is set and equal to the old value in annotation
// 4. If the new pvc's StorageClassAnnotation is set,it must be equal to the old pv/pvc's StorageClassAnnotation
func validateStorageClassUpgrade(oldAnnotations, newAnnotations map[string]string, oldScName, newScName *string) bool {
func validateStorageClassUpgradeFromAnnotation(oldAnnotations, newAnnotations map[string]string, oldScName, newScName *string) bool {
oldSc, oldAnnotationExist := oldAnnotations[core.BetaStorageClassAnnotation]
newScInAnnotation, newAnnotationExist := newAnnotations[core.BetaStorageClassAnnotation]
return oldAnnotationExist /* condition 1 */ &&
Expand All @@ -2225,6 +2235,20 @@ func validateStorageClassUpgrade(oldAnnotations, newAnnotations map[string]strin
(!newAnnotationExist || newScInAnnotation == oldSc) /* condition 4 */
}

// 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
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 */
}

var resizeStatusSet = sets.NewString(string(core.PersistentVolumeClaimNoExpansionInProgress),
string(core.PersistentVolumeClaimControllerExpansionInProgress),
string(core.PersistentVolumeClaimControllerExpansionFailed),
Expand Down
130 changes: 126 additions & 4 deletions pkg/apis/core/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1192,6 +1192,17 @@ func testVolumeClaimStorageClassInSpec(name, namespace, scName string, spec core
}
}

func testVolumeClaimStorageClassNilInSpec(name, namespace string, spec core.PersistentVolumeClaimSpec) *core.PersistentVolumeClaim {
spec.StorageClassName = nil
return &core.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
},
Spec: spec,
}
}

func testVolumeSnapshotDataSourceInSpec(name string, kind string, apiGroup string) *core.PersistentVolumeClaimSpec {
scName := "csi-plugin"
dataSourceInSpec := core.PersistentVolumeClaimSpec{
Expand Down Expand Up @@ -1249,6 +1260,18 @@ func testVolumeClaimStorageClassInAnnotationAndSpec(name, namespace, scNameInAnn
}
}

func testVolumeClaimStorageClassInAnnotationAndNilInSpec(name, namespace, scNameInAnn string, spec core.PersistentVolumeClaimSpec) *core.PersistentVolumeClaim {
spec.StorageClassName = nil
return &core.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
Annotations: map[string]string{v1.BetaStorageClassAnnotation: scNameInAnn},
},
Spec: spec,
}
}

func testValidatePVC(t *testing.T, ephemeral bool) {
invalidClassName := "-invalid-"
validClassName := "valid"
Expand Down Expand Up @@ -1972,6 +1995,28 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) {
},
})

validClaimStorageClassInSpecChanged := testVolumeClaimStorageClassInSpec("foo", "ns", "fast2", core.PersistentVolumeClaimSpec{
AccessModes: []core.PersistentVolumeAccessMode{
core.ReadOnlyMany,
},
Resources: core.ResourceRequirements{
Requests: core.ResourceList{
core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
},
},
})

validClaimStorageClassNil := testVolumeClaimStorageClassNilInSpec("foo", "ns", core.PersistentVolumeClaimSpec{
AccessModes: []core.PersistentVolumeAccessMode{
core.ReadOnlyMany,
},
Resources: core.ResourceRequirements{
Requests: core.ResourceList{
core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
},
},
})

invalidClaimStorageClassInSpec := testVolumeClaimStorageClassInSpec("foo", "ns", "fast2", core.PersistentVolumeClaimSpec{
AccessModes: []core.PersistentVolumeAccessMode{
core.ReadOnlyMany,
Expand All @@ -1995,6 +2040,18 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) {
},
})

validClaimStorageClassInAnnotationAndNilInSpec := testVolumeClaimStorageClassInAnnotationAndNilInSpec(
"foo", "ns", "fast", core.PersistentVolumeClaimSpec{
AccessModes: []core.PersistentVolumeAccessMode{
core.ReadOnlyMany,
},
Resources: core.ResourceRequirements{
Requests: core.ResourceList{
core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
},
},
})

invalidClaimStorageClassInAnnotationAndSpec := testVolumeClaimStorageClassInAnnotationAndSpec(
"foo", "ns", "fast2", "fast", core.PersistentVolumeClaimSpec{
AccessModes: []core.PersistentVolumeAccessMode{
Expand Down Expand Up @@ -2116,10 +2173,11 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) {
})

scenarios := map[string]struct {
isExpectedFailure bool
oldClaim *core.PersistentVolumeClaim
newClaim *core.PersistentVolumeClaim
enableRecoverFromExpansion bool
isExpectedFailure bool
oldClaim *core.PersistentVolumeClaim
newClaim *core.PersistentVolumeClaim
enableRecoverFromExpansion bool
enableRetroactiveDefaultStorageClass bool
}{
"valid-update-volumeName-only": {
isExpectedFailure: false,
Expand Down Expand Up @@ -2230,6 +2288,69 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) {
oldClaim: validClaimStorageClass,
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.
},
"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.
},
"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.
},
"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.
},
"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.
},
"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.
},
"invalid-upgrade-storage-class-annotation-to-spec": {
isExpectedFailure: true,
oldClaim: validClaimStorageClass,
Expand Down Expand Up @@ -2294,6 +2415,7 @@ 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)
Expand Down
37 changes: 37 additions & 0 deletions pkg/controller/volume/persistentvolume/pv_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,14 @@ func (ctrl *PersistentVolumeController) syncUnboundClaim(ctx context.Context, cl
klog.V(4).Infof("synchronizing unbound PersistentVolumeClaim[%s]: no volume found", claimToClaimKey(claim))
// No PV could be found
// OBSERVATION: pvc is "Pending", will retry

if utilfeature.DefaultFeatureGate.Enabled(features.RetroactiveDefaultStorageClass) {
klog.V(4).Infof("FeatureGate[%s] is enabled, attempting to assign storage class to unbound PersistentVolumeClaim[%s]", features.RetroactiveDefaultStorageClass, claimToClaimKey(claim))
if claim, err = ctrl.assignDefaultStorageClass(claim); err != nil {
return fmt.Errorf("can't update PersistentVolumeClaim[%q]: %w", claimToClaimKey(claim), err)
}
}

switch {
case delayBinding && !storagehelpers.IsDelayBindingProvisioning(claim):
if err = ctrl.emitEventForUnboundDelayBindingClaim(claim); err != nil {
Expand Down Expand Up @@ -918,6 +926,35 @@ func (ctrl *PersistentVolumeController) updateVolumePhaseWithEvent(volume *v1.Pe
return newVol, nil
}

// assignDefaultStorageClass updates the claim storage class if there is any, the claim is updated to the API server.
// Ignores claims that already have a storage class.
// TODO: if resync is ever changed to a larger period, we might need to change how we set the default class on existing unbound claims
func (ctrl *PersistentVolumeController) assignDefaultStorageClass(claim *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) {
if claim.Spec.StorageClassName != nil {
return claim, nil
}

class, err := util.GetDefaultClass(ctrl.classLister)
if err != nil {
// It is safe to ignore errors here because it means we either could not list SCs or there is more than one default.
// TODO: do not ignore errors after this PR is merged: https://github.com/kubernetes/kubernetes/pull/110559
klog.V(4).Infof("failed to get default storage class: %v", err)
return claim, nil
} else if class == nil {
klog.V(4).Infof("can not assign storage class to PersistentVolumeClaim[%s]: default storage class not found", claimToClaimKey(claim))
return claim, nil
}

klog.V(4).Infof("assigning StorageClass[%s] to PersistentVolumeClaim[%s]", class.Name, claimToClaimKey(claim))
claim.Spec.StorageClassName = &class.Name
newClaim, err := ctrl.kubeClient.CoreV1().PersistentVolumeClaims(claim.GetNamespace()).Update(context.TODO(), claim, metav1.UpdateOptions{})
if err != nil {
return claim, err
}

return newClaim, nil
}

// bindVolumeToClaim modifies given volume to be bound to a claim and saves it to
// API server. The claim is not modified in this method!
func (ctrl *PersistentVolumeController) bindVolumeToClaim(volume *v1.PersistentVolume, claim *v1.PersistentVolumeClaim) (*v1.PersistentVolume, error) {
Expand Down

0 comments on commit 90f9a52

Please sign in to comment.