Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

If a pvc has an empty storageclass name, don't try to assign a default StorageClass #122704

Merged
merged 1 commit into from
Jan 18, 2024
Merged

If a pvc has an empty storageclass name, don't try to assign a default StorageClass #122704

merged 1 commit into from
Jan 18, 2024
+61 −1

Conversation

carlory
Copy link
Member

@carlory carlory commented Jan 11, 2024 •
edited by liggitt

What type of PR is this?

/kind bug

What this PR does / why we need it:

kube-controller-manager log is https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/121895/pull-kubernetes-e2e-kind-alpha-features/1745322068624281600/artifacts/kind-control-plane/pods/kube-system_kube-controller-manager-kind-control-plane_adb1f34692d5d1ca5fb847c6cb9a07eb/kube-controller-manager/0.log.20240111-061905

 2024-01-11T06:16:46.650889123Z stderr F E0111 06:16:46.650788       1 pv_controller_base.go:270] "Could not sync volume" err=<
 2024-01-11T06:16:46.650919219Z stderr F     can't update PersistentVolumeClaim["volume-attributes-class-3934/test-claim-1"]: PersistentVolumeClaim "test-claim-1" is invalid: spec: Forbidden: spec is immutable after creation except resources.requests and volumeAttributesClassName for bound claims
 2024-01-11T06:16:46.650924249Z stderr F       core.PersistentVolumeClaimSpec{
 2024-01-11T06:16:46.650928921Z stderr F         ... // 2 identical fields
 2024-01-11T06:16:46.650933072Z stderr F         Resources:        {Requests: {s"storage": {i: {...}, s: "1Gi", Format: "BinarySI"}}},
 2024-01-11T06:16:46.650937575Z stderr F         VolumeName:       "",
 2024-01-11T06:16:46.650941831Z stderr F     -   StorageClassName: &"",
 2024-01-11T06:16:46.650946361Z stderr F     +   StorageClassName: &"standard",
 2024-01-11T06:16:46.650950419Z stderr F         VolumeMode:       &"Filesystem",
 2024-01-11T06:16:46.650954604Z stderr F         DataSource:       nil,
 2024-01-11T06:16:46.650958639Z stderr F         ... // 2 identical fields
 2024-01-11T06:16:46.650962711Z stderr F       }
 2024-01-11T06:16:46.650966976Z stderr F  > logger="persistentvolume-binder-controller" PVC="volume-attributes-class-3934/test-claim-1"

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fixes a regression in 1.26+ default configurations: If a pvc has an empty storageClassName, persistentvolume controller won't try to assign a default StorageClass

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

- [KEP]: https://github.com/kubernetes/enhancements/issues/3333

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has 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. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 11, 2024
@k8s-ci-robot
Copy link
Contributor

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 k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jan 11, 2024
@carlory carlory marked this pull request as ready for review January 11, 2024 08:54
@k8s-ci-robot 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 Jan 11, 2024
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed 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 Jan 11, 2024
@carlory
Copy link
Member Author

carlory commented Jan 11, 2024

/assign @saad-ali

@carlory
Copy link
Member Author

carlory commented Jan 11, 2024

/cc @RomanBednar

@RomanBednar
Copy link
Contributor

@carlory My memory is kinda blurry on this but I believe we agreed on allowing updates only from nil values: https://github.com/kubernetes/kubernetes/pull/111467/files#diff-c713e8919642d873fdf48fe8fb6d43e5cb2f53fd601066ff53580ea655948f0dR2242

That means the controller should not attempt to update from "" and instead consider this case as if the class is already set. But shouldn't we rather check here if claim.Spec.StorageClassName is ""? Check for nil value is already in the GetPersistentVolumeClaimClass helper.

@carlory
Copy link
Member Author

carlory commented Jan 13, 2024 •
edited

For historical reasons, there are two ways for a PVC to specify a storage class :

Option 1: set spec.StorageClassName to a non-empty string. This is the preferred way. (empty string is valid)
Option 2: set metadata.annotations["volume.beta.kubernetes.io/storage-class"] to a non-empty string. This is the legacy way. It is deprecated but still supported now. (empty string is valid)

Before kubernetes v1.25, it is impossible to set a storage class for a pending PVC after the PVC is created.

  • Annotate a existing PVC with volume.beta.kubernetes.io/storage-class is not allowed.
  • Change the spec.StorageClassName field of a existing PVC from nil/empty-string to a non-empty string is not allowed.

So, it's very inconvenient for users to make a Pending PVC to be Bound when no storage class is specified in the PVC manifest. In this case, users have to delete the PVC and recreate it with a storage class specified.

Thanks to KEP-3333, the spec.StorageClassName field can be changed from nil to a non-empty string. If a default storage class is configured in the cluster, the kube-controller-manager will set the spec.StorageClassName field of a pending PVC to the default storage class. So the PVC will be bound automatically.

The KEP also introduces a behavior change: It changes the meaning of pvc.spec.storageClassName=nil and pv.spec.storageClassName="". They are no longer equivalent.

so, the controller shouldn't assgin a default storage class to a pending PVC if the volume.beta.kubernetes.io/storage-class annotaion exists or the spec.StorageClassName field is set.

@carlory carlory force-pushed the fix-introactive-storageclass branch from e75c68b to 8af9a17 Compare January 13, 2024 13:25
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 13, 2024
@carlory
Copy link
Member Author

carlory commented Jan 13, 2024

cc @RomanBednar

@RomanBednar
Copy link
Contributor

so, the controller shouldn't assgin a default storage class to a pending PVC if the volume.beta.kubernetes.io/storage-class annotaion exists or the spec.StorageClassName field is set.

I believe this is correct, and it matches the validation - updating from nil with beta annotation would fail and so would update from "" which is the error you posted.

I'd like to get more eyes on this though, @jsafrane can you please take a look?

@jsafrane
Copy link
Member

/lgtm
/approve
Good catch!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 2d7cb7b72ec95b7988586f507903a48903b68402

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlory, jsafrane

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2024
@k8s-ci-robot k8s-ci-robot merged commit eabd6e4 into kubernetes:master Jan 18, 2024
17 of 18 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.30 milestone Jan 18, 2024
This was referenced Jan 19, 2024
Merged
Merged
Merged
Merged
@carlory
Copy link
Member Author

carlory commented Jan 19, 2024 •
edited

/cc @jsafrane @RomanBednar

I cherry-pick this PR into 1.29, 1.28, 1.27 and 1.26.

FYI:

k8s-ci-robot added a commit that referenced this pull request Feb 8, 2024
@k8s-ci-robot
Merge pull request #122858 from carlory/automated-cherry-pick-of-#122…
85ca651 
…704-upstream-release-1.26

Automated cherry pick of #122704: If a pvc has an empty storageclass name, don't try to assign
k8s-ci-robot added a commit that referenced this pull request Feb 8, 2024
@k8s-ci-robot
Merge pull request #122854 from carlory/automated-cherry-pick-of-#122…
ef20857 
…704-upstream-release-1.29

Automated cherry pick of #122704: If a pvc has an empty storageclass name, don't try to assign
k8s-ci-robot added a commit that referenced this pull request Feb 8, 2024
@k8s-ci-robot
Merge pull request #122855 from carlory/automated-cherry-pick-of-#122…
e463613 
…704-upstream-release-1.28

Automated cherry pick of #122704: If a pvc has an empty storageclass name, don't try to assign
k8s-ci-robot added a commit that referenced this pull request Feb 9, 2024
@k8s-ci-robot
Merge pull request #122856 from carlory/automated-cherry-pick-of-#122…
bb2c149 
…704-upstream-release-1.27

Automated cherry pick of #122704: If a pvc has an empty storageclass name, don't try to assign
@liggitt liggitt added the kind/regression Categorizes issue or PR as related to a regression from a prior release. label Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@humblec humblec Awaiting requested review from humblec

@jingxu97 jingxu97 Awaiting requested review from jingxu97

@chrishenzie chrishenzie Awaiting requested review from chrishenzie

@RomanBednar RomanBednar Awaiting requested review from RomanBednar

@jsafrane jsafrane Awaiting requested review from jsafrane

Assignees

@jsafrane jsafrane

@saad-ali saad-ali

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/regression Categorizes issue or PR as related to a regression from a prior release. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
SIG Apps
Archived in project
Milestone
v1.30
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@carlory @k8s-ci-robot @RomanBednar @jsafrane @liggitt @saad-ali