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

KEP-2644: [HonorPVReclaimPolicy] Support Statically provisioned volumes #3552

Merged
merged 1 commit into from
Oct 6, 2022
Merged

KEP-2644: [HonorPVReclaimPolicy] Support Statically provisioned volumes #3552

merged 1 commit into from
Oct 6, 2022

Conversation

deepakkinni
Copy link
Member

@deepakkinni deepakkinni commented Sep 27, 2022 •
edited
Loading

Signed-off-by: Deepak Kinni dkinni@vmware.com

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory labels Sep 27, 2022
@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 27, 2022
@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 Oct 5, 2022
@deepakkinni deepakkinni changed the title KEP-2644: Graduate HonorPVReclaimPolicy to Beta in v1.26 KEP-2644: [HonorPVReclaimPolicy] Support Statically provisioned volumes Oct 5, 2022
jsafrane
jsafrane reviewed Oct 5, 2022
View reviewed changes
keps/sig-storage/2644-honor-pv-reclaim-policy/README.md Outdated
@@ -197,8 +208,22 @@ Once the volume is deleted from the backend, the finalizer can be removed. This

Note: This feature should work with CSI Migration disabled or enabled.

#### Statically Provisioned volumes

The finalizer `external-provisioner.volume.kubernetes.io/finalizer` is added only to statically provisioned volumes that are associated with a PVC.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will PV controller add the finalizer? Why is finalizer kubernetes.io/pv-controller not used here?

IMO, the finalizers should be the same as with dynamically provisioned volumes. I would not event mention it as a separate chapter, I would mention that statically provisioned volumes will behave the same as dynamically provisioned ones, except for "PV is not (yet?) associated with a PVC".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is a CSI driver volume external-provisioner.volume.kubernetes.io/finalizer is added, this is added by the external-provisioner. kubernetes.io/pv-controller is added to in-tree volumes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've addressed the comment, removed the statically and dynamically provisioned volumes section, and merged them into one as you specified.

KEP-2644:[HonorPVReclaimPolicy] Support Statically provisioned volumes
d55c5cf 
Signed-off-by: Deepak Kinni <dkinni@vmware.com>
@jsafrane
Copy link
Member

jsafrane commented Oct 6, 2022

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deepakkinni, 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 Oct 6, 2022
@k8s-ci-robot k8s-ci-robot merged commit 80aa21f into kubernetes:master Oct 6, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Oct 6, 2022
logicalhan pushed a commit to logicalhan/enhancements that referenced this pull request Oct 6, 2022
@k8s-ci-robot @logicalhan
Merge pull request kubernetes#3552 from deepakkinni/honor_pv_v3
8f4e187 
KEP-2644: [HonorPVReclaimPolicy] Support Statically provisioned volumes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xing-yang xing-yang xing-yang left review comments

@jsafrane jsafrane jsafrane left review comments

@saad-ali saad-ali Awaiting requested review from saad-ali

Assignees

@jsafrane jsafrane

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/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. 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
None yet
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.
4 participants
@deepakkinni @jsafrane @k8s-ci-robot @xing-yang