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.
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
Allow retroactive storage class assigment to PVCs #111467
Allow retroactive storage class assigment to PVCs #111467
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Why is the PR marked as WIP?
- Please fix gofmt.
c1e667e
to
dc5ed8d
Compare
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. |
/triage accepted |
dc5ed8d
to
e5602bb
Compare
class, err := util.GetDefaultClass(ctrl.classLister) | ||
if err != nil { | ||
return claim, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think errors from GetDefaultClass can be ignored (just log it here and return nil).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} else if class == nil { | ||
return claim, fmt.Errorf("can not assign storage class to PersistentVolumeClaim[%s]: default storage class not found", claimToClaimKey(claim)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, just log this at very low log level (4) and return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
if err != nil { | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error cannot be ignored. Caller should return it and not just log it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have spotted it earlier, claim update error should not be ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, removing the comment and returning error.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't return error here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
e5602bb
to
37ef371
Compare
12758cb
to
056b4f9
Compare
056b4f9
to
15b5b7c
Compare
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 |
[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:
Approvers can indicate their approval by writing |
15b5b7c
to
2f533cd
Compare
/lgtm |
|
||
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this updated the storage class, don't we need to restart syncUnboundClaim so we recheck things like IsDelayBindingMode and findBestMatchForClaim?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
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{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PVC storageclassName field is not allowed to be updated after creation, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC we relaxed PVC validation as part of this feature, it can be updated from nil
to something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, the exact conditions for update from nil are set here: https://github.com/kubernetes/kubernetes/pull/111467/files#diff-c713e8919642d873fdf48fe8fb6d43e5cb2f53fd601066ff53580ea655948f0dR2238-R2251
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
kubernetes/enhancements#3333