Allow retroactive storage class assigment to PVCs #111467

RomanBednar · 2022-07-27T14:55:09Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

See linked KEP for details.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

PersistentVolumeClaim objects are no longer left with storage class set to `nil` forever, but will be updated retroactively once any StorageClass is set or created as default.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/pull/3337/commits/c6d69a727ebf56681a7d615c4f6234e4de86a5ec#diff-54da445e206aec7fe7962396edc49ec525ba1d3034aa800fd3a4e93d28a739e7

kubernetes/enhancements#3333

jsafrane

Why is the PR marked as WIP?
Please fix gofmt.

pkg/apis/core/validation/validation.go

pkg/controller/volume/persistentvolume/pv_controller.go

k8s-triage-robot · 2022-07-29T08:55:40Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

jsafrane · 2022-07-29T11:51:12Z

/triage accepted
/priority important-soon

jsafrane · 2022-07-29T11:59:06Z

pkg/controller/volume/persistentvolume/pv_controller.go

+	class, err := util.GetDefaultClass(ctrl.classLister)
+	if err != nil {
+		return claim, err


I think errors from GetDefaultClass can be ignored (just log it here and return nil).

jsafrane · 2022-07-29T11:59:29Z

pkg/controller/volume/persistentvolume/pv_controller.go

+	} else if class == nil {
+		return claim, fmt.Errorf("can not assign storage class to PersistentVolumeClaim[%s]: default storage class not found", claimToClaimKey(claim))


Again, just log this at very low log level (4) and return.

jsafrane · 2022-07-29T11:59:58Z

pkg/controller/volume/persistentvolume/pv_controller.go

+	if err != nil {
+		return nil, err
+	}


This error cannot be ignored. Caller should return it and not just log it!

jsafrane · 2022-07-29T12:00:44Z

pkg/controller/volume/persistentvolume/pv_controller.go

+					// Intentionally ignoring errors - unsuccessful storage class assignment is expected if there is no default class or PVC already has a storage class
+					klog.V(2).Infof("can't update PersistentVolumeClaim[%s]: %v", claimToClaimKey(claim), err)


I should have spotted it earlier, claim update error should not be ignored.

Done, removing the comment and returning error.

jsafrane · 2022-07-29T12:06:46Z

pkg/controller/volume/persistentvolume/pv_controller.go

+func (ctrl *PersistentVolumeController) assignDefaultStorageClass(claim *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) {
+	// [Unit test set 15]
+	if claim.Spec.StorageClassName != nil {
+		return claim, fmt.Errorf("can not assign storage class to PersistentVolumeClaim[%s]: pvc already has StorageClass[%s]", claimToClaimKey(claim), *claim.Spec.StorageClassName)


don't return error here

thockin · 2022-08-02T17:38:01Z

We should consider whther this needs to go beta or just go straight to GA in 1.26. I'm not sure what we would get out of being beta. Honestly, even being alpha is minimal value here, but let's not change that now :)

/lgtm
/approve

k8s-ci-robot · 2022-08-02T17:38:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RomanBednar, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/apis/OWNERS~~ [thockin]
~~pkg/controller/volume/persistentvolume/OWNERS~~ [thockin]
~~pkg/features/OWNERS~~ [thockin]
~~pkg/volume/OWNERS~~ [thockin]
~~plugin/pkg/admission/OWNERS~~ [thockin]
~~test/e2e/framework/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

thockin · 2022-08-02T19:23:41Z

/lgtm

liggitt · 2022-10-14T13:22:21Z

pkg/controller/volume/persistentvolume/pv_controller.go

+
+			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 {


if this updated the storage class, don't we need to restart syncUnboundClaim so we recheck things like IsDelayBindingMode and findBestMatchForClaim?

@liggitt PR opened here: #113137

jingxu97 · 2023-05-02T06:00:10Z

pkg/controller/volume/persistentvolume/pv_controller.go

+
+	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{})


PVC storageclassName field is not allowed to be updated after creation, right?

IIRC we relaxed PVC validation as part of this feature, it can be updated from nil to something.

Exactly, the exact conditions for update from nil are set here: https://github.com/kubernetes/kubernetes/pull/111467/files#diff-c713e8919642d873fdf48fe8fb6d43e5cb2f53fd601066ff53580ea655948f0dR2238-R2251

k8s-ci-robot requested review from dchen1107 and deads2k July 27, 2022 14:56

jsafrane reviewed Jul 28, 2022

View reviewed changes

RomanBednar force-pushed the retro-sc-assignment branch from c1e667e to dc5ed8d Compare July 29, 2022 07:22

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jul 29, 2022

RomanBednar changed the title ~~WIP - Allow retroactive storage class assigment to PVCs~~ Allow retroactive storage class assigment to PVCs Jul 29, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2022

RomanBednar force-pushed the retro-sc-assignment branch from dc5ed8d to e5602bb Compare July 29, 2022 11:56

jsafrane reviewed Jul 29, 2022

View reviewed changes

RomanBednar force-pushed the retro-sc-assignment branch from e5602bb to 37ef371 Compare July 29, 2022 14:28

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 29, 2022

RomanBednar force-pushed the retro-sc-assignment branch from 12758cb to 056b4f9 Compare August 2, 2022 09:49

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 2, 2022

RomanBednar force-pushed the retro-sc-assignment branch from 056b4f9 to 15b5b7c Compare August 2, 2022 17:17

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2022

RomanBednar added 7 commits August 2, 2022 20:52

add RetroactiveDefaultStorageClass feature gate

d19f6fb

pass down feature gate to PVC validation opts

0f0d61f

allow pvc spec StorageClass name mutation if the feature is enabled

f051cc7

add tests for pvc update validation

caf2f41

move storage class helpers to utils

42b24b7

allow retroactive storage class assignment in pv controller

a0a5aa3

add tests for pv controller

2f533cd

RomanBednar force-pushed the retro-sc-assignment branch from 15b5b7c to 2f533cd Compare August 2, 2022 18:52

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2022

k8s-ci-robot merged commit 90f9a52 into kubernetes:master Aug 2, 2022

k8s-ci-robot added this to the v1.25 milestone Aug 2, 2022

RomanBednar deleted the retro-sc-assignment branch August 3, 2022 07:17

This was referenced Aug 3, 2022

PV controller tests should have all struct fields named #111659

Closed

Add article about Retroactive default StorageClass assignement kubernetes/website#35658

Merged

Add integration test for Retroactive default StorageClass assignement #111746

Merged

liggitt reviewed Oct 14, 2022

View reviewed changes

jsafrane mentioned this pull request Nov 2, 2022

graduate RetroactiveDefaultStorageClass to beta #113329

Merged

cvvz mentioned this pull request Feb 27, 2023

PVs with annotations are not being dynamically provisioned on k8s 1.26.1 #116028

Closed

jingxu97 reviewed May 2, 2023

View reviewed changes

liggitt moved this from Assigned to API review completed, 1.25 in API Reviews Aug 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow retroactive storage class assigment to PVCs #111467

Allow retroactive storage class assigment to PVCs #111467

RomanBednar commented Jul 27, 2022 •

edited by jsafrane

jsafrane left a comment •

edited

k8s-triage-robot commented Jul 29, 2022

jsafrane commented Jul 29, 2022

jsafrane Jul 29, 2022

RomanBednar Jul 29, 2022

jsafrane Jul 29, 2022

RomanBednar Jul 29, 2022

jsafrane Jul 29, 2022

RomanBednar Jul 29, 2022

jsafrane Jul 29, 2022

RomanBednar Jul 29, 2022

jsafrane Jul 29, 2022

RomanBednar Jul 29, 2022

thockin commented Aug 2, 2022

k8s-ci-robot commented Aug 2, 2022

thockin commented Aug 2, 2022

liggitt Oct 14, 2022

RomanBednar Oct 18, 2022 •

edited

jingxu97 May 2, 2023

jsafrane May 3, 2023

RomanBednar May 4, 2023

		} else if class == nil {
		return claim, fmt.Errorf("can not assign storage class to PersistentVolumeClaim[%s]: default storage class not found", claimToClaimKey(claim))

		// Intentionally ignoring errors - unsuccessful storage class assignment is expected if there is no default class or PVC already has a storage class
		klog.V(2).Infof("can't update PersistentVolumeClaim[%s]: %v", claimToClaimKey(claim), err)

Allow retroactive storage class assigment to PVCs #111467

Allow retroactive storage class assigment to PVCs #111467

Conversation

RomanBednar commented Jul 27, 2022 • edited by jsafrane

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

jsafrane left a comment • edited

Choose a reason for hiding this comment

k8s-triage-robot commented Jul 29, 2022

jsafrane commented Jul 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Aug 2, 2022

k8s-ci-robot commented Aug 2, 2022

thockin commented Aug 2, 2022

Choose a reason for hiding this comment

RomanBednar Oct 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomanBednar commented Jul 27, 2022 •

edited by jsafrane

jsafrane left a comment •

edited

RomanBednar Oct 18, 2022 •

edited