[WIP] Set default storage class on PVC sync if default StorageClass is present #100031

LittleChimera · 2021-03-09T20:23:37Z

In case where there's no default StorageClass at the time of creation of a PVC, the PVC will stay without storage class.
This logic was thus far only executed in AdmissionWebhook which would add a storageClassName to PVC with no defined storageClass.
Therefore, PVC will stay without storageClassName, if the PVC manifest is not reapplied to the cluster.

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #99629

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

In case where there's no default StorageClass at the time of creation of a PVC, the PVC will stay without storage class. This logic was thus far only executed in AdmissionWebhook which would add a storageClassName to PVC with no defined storageClass. Therefore, PVC will stay without storageClassName, if the PVC manifest is not reapplied to the cluster.

k8s-ci-robot · 2021-03-09T20:23:41Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2021-03-09T20:23:45Z

@LittleChimera: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-03-09T20:23:45Z

Welcome @LittleChimera!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-03-09T20:23:46Z

Hi @LittleChimera. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-03-09T20:24:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: LittleChimera
To complete the pull request process, please assign thockin after the PR has been reviewed.
You can assign the PR to them by writing /assign @thockin in a comment when ready.

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

Needs approval from an approver in each of these files:

pkg/controller/volume/persistentvolume/OWNERS

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

jsafrane

Please add some unit tests too!

jsafrane · 2021-05-21T15:19:04Z

pkg/controller/volume/persistentvolume/pv_controller.go

+				if storageClasses, err := ctrl.kubeClient.StorageV1().StorageClasses().List(context.TODO(), metav1.ListOptions{}); err != nil {
+					for _, sc := range storageClasses.Items {
+						if sutil.IsDefaultAnnotation(sc.ObjectMeta) {
+							claim.Spec.StorageClassName = &sc.Name
+							ctrl.updateClaim(claim)
+							return nil
+						}
+					}
+				}


Please extract all this getting default SC and modifying PV into a separate function and just call it. I like the giant if-then-else in syncUnboundClaim as clean as possible.

jsafrane · 2021-05-21T15:20:57Z

pkg/controller/volume/persistentvolume/pv_controller.go

@@ -359,8 +360,19 @@ func (ctrl *PersistentVolumeController) syncUnboundClaim(claim *v1.PersistentVol
 					return err
 				}
 				return nil
+			case storagehelpers.GetPersistentVolumeClaimClass(claim) == "":
+				if storageClasses, err := ctrl.kubeClient.StorageV1().StorageClasses().List(context.TODO(), metav1.ListOptions{}); err != nil {


Setting the default storage class should be done at line 337, "// User did not care which PV they get" - if the PVC has no storage class, file it there and return. Updated PVC will be enqueued shortly and it will have a SC.

Hold on, maybe leave the code here - if there is a PV without a storage class, we try to bind to it once. If there is no such PV, we file the default SC into the PVC and try again, hoping for dynamic provisioning. This will keep at least a bit of the existing behavior there.

@msau42 what do you think? It's a bit confusing.

thockin · 2021-05-21T15:42:04Z

Is this really something we want to do? If I recall correctly, having no class is semantically meaningful, isn't it?

…

On Fri, May 21, 2021 at 8:27 AM Jan Šafránek ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/controller/volume/persistentvolume/pv_controller.go <#100031 (comment)> : > @@ -359,8 +360,19 @@ func (ctrl *PersistentVolumeController) syncUnboundClaim(claim *v1.PersistentVol return err } return nil + case storagehelpers.GetPersistentVolumeClaimClass(claim) == "": + if storageClasses, err := ctrl.kubeClient.StorageV1().StorageClasses().List(context.TODO(), metav1.ListOptions{}); err != nil { Hold on, maybe leave the code here - if there is a PV without a storage class, we try to bind to it once. If there is no such PV, we file the default SC into the PVC and try again, hoping for dynamic provisioning. This will keep at least a bit of the existing behavior there. @msau42 <https://github.com/msau42> what do you think? It's a bit confusing. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#100031 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVFYPLGCLB3TGPRQKXDTOZ3VPANCNFSM4Y4M6WHQ> .

LittleChimera · 2021-05-21T20:56:13Z

Thank you for the feedback! ❤️

Is this really something we want to do? If I recall correctly, having no class is semantically meaningful, isn't it?
…

Please add some unit tests too!

I was actually trying to modify these test cases which specify that nothing should be done when storageClass is set to an empty value (with this PR, I would expect that in these two cases, the default storageClass is overridden on this PVC instead, and then a PV provisioned and bound):
https://github.com/jsafrane/kubernetes/blob/bb5d562f37ab029d2b562e23bd23e906ee099a45/pkg/controller/volume/persistentvolume/provision_test.go#L299-L316

Originally, there were written as part of dynamic provisioning. I cannot make out a reason as to why this behaviour was chosen.
#29006

Default storage classes were added later, and I cannot find again the reason for this behavior:
#30900

I would expect that storageClass is not overwritten by dynamic provisioner only if the associated PV was created manually.
I don't see a use-case for using an empty value, if the claim is not bound and doesn't specify a persistentVolume.
Even for manual use-cases, the usual value that's typically used for the storageClass would be "none" or "local".
Does this make sense to you or am I missing something?

msau42 · 2021-05-21T21:03:54Z

Is this really something we want to do? If I recall correctly, having no class is semantically meaningful, isn't it?

I think we definitely don't want to set the default storageclass for PVCs that have already been bound.

For unbound PVCs, I see two scenarios:

Someone deployed all their components in parallel, including default StorageClass and workloads that use it. The ordering constraint currently imposed means that sometimes the default storageclass doesn't get set on those PVCs and they're stuck forever in an unbound state. This forces folks to order their deployments or hardcode a storageclass name in their PVC. One could argue that we have this ordering issue with other objects too, like CRDs and CRs.
Someone does not have a default storageclass and they manually provide PVs for the PVCs. In this case, a nil PVC.storageclassname will bind to the PV. This is the scenario that may be impacted if we make this change. Note that it only breaks if a default storageclass gets installed in the cluster at some later point. The sequence that may break is:
1. User creates PVC with empty StorageClass
2. User creates a default StorageClass
3. User creates a PV with empty StorageClass

IMO the scenario of depending on this behavior before installing a default StorageClass in the system seems a bit contrived and requires careful deployment ordering by the user. It could be mitigated by creating the PVC with "" storageclass.

That being said, I do agree that this makes explaining the API semantics more complicated. There are other issues too, such as validation not allowing updates to storageclassname, and maybe some concerns (that I haven't thought about yet) with pvc templates (statefulset, generic ephemeral volumes).

In summary, this is not as simple to change, we need to think more deeply on the api compatibility and impact to other features.

LittleChimera · 2021-05-22T09:35:00Z

One could argue that we have this ordering issue with other objects too, like CRDs and CR

I think that ordering probably isn't the best way to solve this. In case of CRDs and Namespaces, which fall into this category, API server doesn't accept applying the dependencies. In other words, either you get expected results, or you get an error. Doing this with StorageClasses would make two different results possible based on which order you apply your resources. I.e., this solution would not be eventually consistent, because you would be able to get different results based on order of operations.

Someone does not have a default storageclass and they manually provide PVs for the PVCs. In this case, a nil PVC.storageclassname will bind to the PV. This is the scenario that may be impacted if we make this change. Note that it only breaks if a default storageclass gets installed in the cluster at some later point. The sequence that may break is:

User creates PVC with empty StorageClass

User creates a default StorageClass

User creates a PV with empty StorageClass

It's a good point. But, I think we can break this down further in two scenarios:

User expects a default storage class present on their system. In this case, if you want to provide PV manually, you're expected to define some storage class - e.g. "", "none", "manual". Otherwise, the storageClass will be set to default one by the mutating admission webhook.
User doesn't expect a default storage class present on their system. In this case, the change would be irrelevant. Adding a default StorageClass at a much later stage would not make a difference because a PV will be bound to the PVC.

I think it should be enough to change the logic so that storageClass can change only for PVCs with nil value. I.e. users that don't specify storage class on their PVC, expect that a default one will be set eventually. Users who want to provision PVs manually fall into one of the two scenarios above which both seem safe to me. Additionally, this might require changes to defaulter I think, so that might cause additional issues.

Do you think that makes sense and is worth a try?

That being said, I do agree that this makes explaining the API semantics more complicated. There are other issues too, such as validation not allowing updates to storageclassname, and maybe some concerns (that I haven't thought about yet) with pvc templates (statefulset, generic ephemeral volumes).

I didn't know about generic ephemeral volumes before, but from what I've read I don't see any clear issues for them or STS provisoned PVCs.
I still agree there might be some other concerns that we're missing.

jsafrane · 2021-05-24T08:47:53Z

Is this really something we want to do?

I think that issues described my @msau42 are real, we have the same ordering problem too. Similarly, it's not possible to reliably change the default storage class - most fields are read-only and currently we must delete the storage class and re-create it later, hoping that no PVC is created in the meantime.

If I recall correctly, having no class is semantically meaningful, isn't it?

There is a difference between storageClassName: "" (PVC does not want any storage class) and storageClassName: nil (PVC wants the default one and it falls back to "" if there is no default storage class at the point when PVC was created). IMO, it makes sense to extend nil to "PVC wants the default storage class, even if it's created later".

If I remember correctly, with fall back from nil to "" we wanted to keep the existing PVC behavior from time when there were no storage classes at all.

I also remember that we used annotation + admission plugin for the default storage class to have possibility to have different defaulting rules / admission plugins in the future, e.g. a different default class for different namespaces. If this was real, then we cannot implement the defaulting in PV controller, because it does not know what the defaulting rules are. But I haven't seen these different defaulting rules used in the wild, IMO everyone uses our admission plugin.

k8s-triage-robot · 2021-08-22T09:30:40Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

LittleChimera · 2021-08-22T10:32:59Z

/remove-lifecycle stale

k8s-triage-robot · 2021-11-20T11:10:41Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

LittleChimera · 2021-12-05T11:40:59Z

/remove-lifecycle stale

dims · 2022-01-14T13:35:34Z

/close

k8s-ci-robot · 2022-01-14T13:36:52Z

@dims: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dims · 2022-01-14T13:37:02Z

please reopen if you still need this WIP PR. Trying to declutter the open PR(s)

mkimuram · 2022-05-25T15:08:53Z

pkg/controller/volume/persistentvolume/pv_controller.go

@@ -359,8 +360,19 @@ func (ctrl *PersistentVolumeController) syncUnboundClaim(claim *v1.PersistentVol
 					return err
 				}
 				return nil
+			case storagehelpers.GetPersistentVolumeClaimClass(claim) == "":


@jsafrane I think that we can change it to case claim.Spec.StorageClassName == nil: to keep the behavior compatible for the case that users intentionally specify "" to Spec.StorageClassName (Default StorageClass should be set to StorageClasssName only when StorageClassName == nil).

I've added some codes on top of this PR in order to:

Work (fix wrong error check, loosen validation)

Avoid trying to add default StorageClass when "" is intentionally set by users (although, it is actually blocked by validation)

Only allow one default StorageClass

master...mkimuram:storage-class-default-pv-controller-v3

It would be better tighten validation a bit more, but there won't be a big user facing change.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 9, 2021

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 9, 2021

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 9, 2021

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Mar 9, 2021

k8s-ci-robot requested review from msau42 and thockin March 9, 2021 20:24

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 9, 2021

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 11, 2021

LittleChimera changed the title ~~Set default storage class on PVC sync if default StorageClass is present~~ [WIP] Set default storage class on PVC sync if default StorageClass is present Mar 11, 2021

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2021

jsafrane reviewed May 21, 2021

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 22, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 22, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 20, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 5, 2021

k8s-ci-robot closed this Jan 14, 2022

jsafrane mentioned this pull request Mar 7, 2022

Webhook to ensure single default VolumeSnapshotClass kubernetes-csi/external-snapshotter#667

Closed

mkimuram reviewed May 25, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Set default storage class on PVC sync if default StorageClass is present #100031

[WIP] Set default storage class on PVC sync if default StorageClass is present #100031

LittleChimera commented Mar 9, 2021

k8s-ci-robot commented Mar 9, 2021

k8s-ci-robot commented Mar 9, 2021

k8s-ci-robot commented Mar 9, 2021

k8s-ci-robot commented Mar 9, 2021

k8s-ci-robot commented Mar 9, 2021

jsafrane left a comment

jsafrane May 21, 2021

jsafrane May 21, 2021

jsafrane May 21, 2021

thockin commented May 21, 2021 via email

LittleChimera commented May 21, 2021

msau42 commented May 21, 2021

LittleChimera commented May 22, 2021 •

edited

Loading

jsafrane commented May 24, 2021

k8s-triage-robot commented Aug 22, 2021

LittleChimera commented Aug 22, 2021

k8s-triage-robot commented Nov 20, 2021

LittleChimera commented Dec 5, 2021

dims commented Jan 14, 2022

k8s-ci-robot commented Jan 14, 2022

dims commented Jan 14, 2022

mkimuram May 25, 2022

mkimuram May 25, 2022

[WIP] Set default storage class on PVC sync if default StorageClass is present #100031

[WIP] Set default storage class on PVC sync if default StorageClass is present #100031

Conversation

LittleChimera commented Mar 9, 2021

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.:

k8s-ci-robot commented Mar 9, 2021

k8s-ci-robot commented Mar 9, 2021

k8s-ci-robot commented Mar 9, 2021

k8s-ci-robot commented Mar 9, 2021

k8s-ci-robot commented Mar 9, 2021

jsafrane left a comment

Choose a reason for hiding this comment

jsafrane May 21, 2021

Choose a reason for hiding this comment

jsafrane May 21, 2021

Choose a reason for hiding this comment

jsafrane May 21, 2021

Choose a reason for hiding this comment

thockin commented May 21, 2021 via email

LittleChimera commented May 21, 2021

msau42 commented May 21, 2021

LittleChimera commented May 22, 2021 • edited Loading

jsafrane commented May 24, 2021

k8s-triage-robot commented Aug 22, 2021

LittleChimera commented Aug 22, 2021

k8s-triage-robot commented Nov 20, 2021

LittleChimera commented Dec 5, 2021

dims commented Jan 14, 2022

k8s-ci-robot commented Jan 14, 2022

dims commented Jan 14, 2022

mkimuram May 25, 2022

Choose a reason for hiding this comment

mkimuram May 25, 2022

Choose a reason for hiding this comment

LittleChimera commented May 22, 2021 •

edited

Loading