postpone pv deletion if it is bound to a pvc #1608

NickrenREN · 2018-01-16T06:44:35Z

add finalizer to postpone pv deletion if it is bound to a pvc
xref: kubernetes/enhancements#499

/sig storage

pospispa · 2018-01-16T10:04:12Z

contributors/design-proposals/storage/postpone-pv-deletion.md

+
+## Motivation
+
+User can delete a Persistent Volume (PV) that is being used by a PVC.  It may result in data loss.


Nit: actually, only an admin can delete a PV. So this feature is for admins and doesn't affect ordinary users.

yeah, thanks

pospispa · 2018-01-16T10:07:34Z

contributors/design-proposals/storage/postpone-pv-deletion.md

+
+### API Server, PV Admission Controller, PV Create: 
+
+A new plugin for PV admission controller will be created. The plugin will automatically add finalizer information into newly created PV's metadata, just like `PVC protection` does.


Nit: IMO, a link to PVC Protection design or documentation may be useful.

yes, thanks, updated

pospispa · 2018-01-16T10:21:09Z

contributors/design-proposals/storage/postpone-pv-deletion.md

+
+### Add this logic to the existing PV controller instead of creating a new admission and protection controller
+When we bind PV to PVC, we add finalizer for PV and remove finalizer when PV is no longer bound to a PVC.
+


Could we add a part or the whole logic to the existing PVC Protection admission plugin and PVC Protection controller?

sorry for the unclear expression, this is not propose to add the logic to the existing PVC protection admission plugin and PVC Protection controller.
I am considering if we can add PV protection logic to the existing PV controller under the package pkg/controller/volume/persistentvolume.

Your expression is clear. I understand that you're considering to extend PV Controller.
My question may have been better.
Anyway, in my opinion we should consider these options:

extend PV controller.

extend PVC Protection admission plugin and PVC Protection controller.

Good point. I talked with @jsafrane offline. He thinks if there is a big chunk of code that two controllers would share , we need to merge them.

I do not think the existing PV controller shares much code with PV protection controller, so we do not need to bind them together;

Since you want to move PVC protection to beta and i plan to add PV protection as alpha in 1.10, and also, they two will not share much code, so maybe we can implement them as two separate controller at first, if necessary, merge them later .
WDYT.

I was thinking about the joining PV and PVC protection admission plugins together and the PV and PVC protection controllers together from a perspective of monolithic approach versus small programs communicating with each other approach.

AFAIK, Kubernetes is moving towards small core plus many controllers and plugins around approach. That's why joining PV and PVC protection admission plugins together and joining PV and PVC protection controllers together will go against this approach. So I'm against joining PV and PVC protection plugins/controllers together.

In case you find functions in PVC protection controller that are useful also for PV protection controller you can reuse them in PV protection controller anyway.

In the end, I'd like to have one "library" or common code that anyone could use to protect X from deleting when Y with some attributes exists. One would use (not copy!) the library to protect PVCs when pod with pod.volumes[x].persistentVolumeClaim.name == PVC.name filling only the right informers and callbacks with checks. It can be after alpha, when we can see how this library code would look like.

pospispa · 2018-01-18T16:30:11Z

I have very limited knowledge of PV controller so I'm not able to review the proposed changes for PV controller. The rest looks good to me.

NickrenREN · 2018-01-18T16:38:04Z

Thanks @pospispa , and @jsafrane any comments ?

msau42 · 2018-01-18T18:32:36Z

@kubernetes/sig-storage-proposals

jsafrane · 2018-01-19T09:39:08Z

One thing to consider is the admission plugin. We don't want extra admission plugin for each class of objects we want protect - they need to be explicitly mentioned on API server command line and it's hard to maintain.

I propose there is only one admission plugin, say StorageProtection (or ObjectProtection?) that handles both PV and PVC (depending on alpha feature gates).

In addition, I'd like to have only one feature gate. I hope the PV protection won't be much different from PVC protection and it could go directly to beta. What do you think?

jsafrane

It looks good for alpha, for GA we need to consider merging the common parts into one package.

jsafrane · 2018-01-19T09:40:40Z

contributors/design-proposals/storage/postpone-pv-deletion.md

+
+### API Server, PV Admission Controller, PV Create: 
+
+A new plugin for PV admission controller will be created. The plugin will automatically add finalizer information into newly created PV's metadata, just like [PVC protection](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/storage/postpone-pvc-deletion-if-used-in-a-pod.md) does.


Please share the same admission plugin as PVC protection and rename it to something more general. It's tedious to maintain API server command line with list of all protection plugins that should run.

jsafrane · 2018-01-19T09:47:50Z

contributors/design-proposals/storage/postpone-pv-deletion.md

+
+### Add this logic to the existing PV controller instead of creating a new admission and protection controller
+When we bind PV to PVC, we add finalizer for PV and remove finalizer when PV is no longer bound to a PVC.
+


In the end, I'd like to have one "library" or common code that anyone could use to protect X from deleting when Y with some attributes exists. One would use (not copy!) the library to protect PVCs when pod with pod.volumes[x].persistentVolumeClaim.name == PVC.name filling only the right informers and callbacks with checks. It can be after alpha, when we can see how this library code would look like.

NickrenREN · 2018-01-19T15:50:47Z

It looks good for alpha, for GA we need to consider merging the common parts into one package.

yeah, we can implement PV protection at first, and then let's see if we need to merge them later

I propose there is only one admission plugin, say StorageProtection (or ObjectProtection?) that handles
both PV and PVC (depending on alpha feature gates).

Sounds good. What about StorageClass protection? should we add it too ?

In addition, I'd like to have only one feature gate. I hope the PV protection won't be much different from PVC protection and it could go directly to beta. What do you think?

I do not have a strong opinion on what version PV protection should be. But if PV protection goes directly to beta, feature gate is not needed since it is implemented for alpha features.
BTW: I am coding for the feature implementation, but not very sure if i can have enough time to write e2e tests for this feature this quarter, is it ok to let it go to beta ? or maybe alpha first ?

jsafrane · 2018-01-19T16:10:02Z

What about StorageClass protection? should we add it too ?

I'd skip it for now. We might bring a table what needs to be protected to sig-storage.

But if PV protection goes directly to beta, feature gate is not needed since it is implemented for alpha features

Not really, admin can disable beta features if they're buggy or annoying.

NickrenREN · 2018-01-19T16:14:24Z

I'd skip it for now. We might bring a table what needs to be protected to sig-storage.

Ok, agree, cool

Not really, admin can disable beta features if they're buggy or annoying.

oh, great to know it, thanks. so we can let it go to beta directly without e2e tests ?

jsafrane · 2018-01-19T16:17:32Z

Regarding tests... As feature owner you're responsible that there are enough e2e tests to cover the feature. You don't need to write them, find someone else who writes them. There is a large community waiting for their first Kubernetes commit :-).

NickrenREN · 2018-01-19T16:20:44Z

Ok, i will try to add e2e tests myself. If i really do not have enough time, will find someone to help me.
Thanks @jsafrane.

liggitt · 2018-01-19T16:26:35Z

We don't want extra admission plugin for each class of objects we want protect - they need to be explicitly mentioned on API server command line and it's hard to maintain.

soon you will not need to maintain an explicit list of admission plugins on the command line. we're moving to a model where there is a default set of admission plugins, and you can enable things not in that default set if you want, or disable things in that set if you don't want them (kubernetes/kubernetes#58123)

@NickrenREN

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Postpone PV deletion with finalizer when it is being used Postpone PV deletion if it is bound to a PVC xref: kubernetes/community#1608 **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes #33355 **Special notes for your reviewer**: **Release note**: ```release-note Postpone PV deletion when it is being bound to a PVC ``` WIP, assign to myself first /assign @NickrenREN

jsafrane · 2018-02-02T15:19:39Z

/lgtm

jsafrane · 2018-02-02T15:21:03Z

/assign @saad-ali @childsb
for approval

childsb · 2018-02-02T16:36:04Z

This LGTM, but i'll leave it for @saad-ali to give any final comment/feedback.

@jsafrane

Automatic merge from submit-queue (batch tested with PRs 55439, 58564, 59028, 59169, 59259). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. kubectl: Add Terminating state to PVs kubectl shows PV `Terminating` status, just like Pod and [PVC](#55873) **What this PR does / why we need it**: We will postpone PV deletion if it is bound to a PVC, see #58743, so we may keep PV waiting for deletion for a longer time than before so users should know what is going on. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: xref: kubernetes/community#1608 **Special notes for your reviewer**: **Release note**: ```release-note NONE ``` /sig cli /sig storage /assign @jsafrane I tested this PR on my local host. ``` nickren@nickren-14:~/test/test$ kubectl delete -f pv.yaml persistentvolume "task-pv-volume" deleted nickren@nickren-14:~/test/test$ kubectl get pv NAME CAPACITY ACCESS MODES RECLAIM POLICY STATUS CLAIM STORAGECLASS REASON AGE task-pv-volume 1Gi RWO Delete Terminating default/task-pv-claim standard 27s nickren@nickren-14:~/test/test$ kubectl describe pv task-pv-volume Name: task-pv-volume Labels: type=local Annotations: pv.kubernetes.io/bound-by-controller=yes Finalizers: [kubernetes.io/pv-protection] StorageClass: standard Status: Terminating (since Thu, 01 Feb 2018 13:18:51 +0800) Claim: default/task-pv-claim Reclaim Policy: Delete Access Modes: RWO Capacity: 1Gi Message: Source: Type: HostPath (bare host directory volume) Path: /tmp/data HostPathType: Events: <none> ```

childsb · 2018-02-08T21:53:20Z

/approve

k8s-ci-robot · 2018-02-08T21:53:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: childsb, jsafrane, NickrenREN

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 OWNERS Files:

~~contributors/design-proposals/storage/OWNERS~~ [childsb]

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

@jsafrane

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add e2e test for PV protection Add e2e test for PV protection **What this PR does / why we need it**: **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: xref: kubernetes/community#1608 **Special notes for your reviewer**: hold until #58743 gets merged **Release note**: ```release-note NONE ``` /sig storage /hold /assign @jsafrane

postpone pv deletion if it is bound to a pvc

k8s-ci-robot assigned pospispa Jan 16, 2018

k8s-ci-robot added 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 16, 2018

k8s-ci-robot requested review from childsb and saad-ali January 16, 2018 06:44

NickrenREN changed the title ~~postpone pv deletion if it is bound by a pvc~~ postpone pv deletion if it is bound to a pvc Jan 16, 2018

NickrenREN force-pushed the pv-protection branch from 13cfe72 to dbb00ae Compare January 16, 2018 07:16

pospispa reviewed Jan 16, 2018

View reviewed changes

NickrenREN force-pushed the pv-protection branch 2 times, most recently from 47e9013 to 8864599 Compare January 16, 2018 12:56

k8s-ci-robot added the kind/design Categorizes issue or PR as related to design. label Jan 18, 2018

jsafrane reviewed Jan 19, 2018

View reviewed changes

NickrenREN mentioned this pull request Jan 24, 2018

Postpone PV deletion with finalizer when it is being used kubernetes/kubernetes#58743

Merged

NickrenREN force-pushed the pv-protection branch 2 times, most recently from 6b2f3c1 to adaec33 Compare February 1, 2018 05:53

postpone pv deletion if it is refereced by a pvc

0c1daa3

NickrenREN force-pushed the pv-protection branch from adaec33 to 0c1daa3 Compare February 1, 2018 05:58

This was referenced Feb 1, 2018

Add e2e test for PV protection kubernetes/kubernetes#59090

Merged

kubectl: Add Terminating state to PVs kubernetes/kubernetes#59169

Merged

k8s-ci-robot assigned jsafrane Feb 2, 2018

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

k8s-ci-robot assigned childsb and saad-ali Feb 2, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2018

k8s-ci-robot merged commit 987460f into kubernetes:master Feb 8, 2018

NickrenREN deleted the pv-protection branch February 15, 2018 11:49

pospispa mentioned this pull request Mar 14, 2018

StorageObjectInUseProtection (was Persistent Volume Protection) kubernetes/enhancements#499

Closed

MadhavJivrajani pushed a commit to MadhavJivrajani/community that referenced this pull request Nov 30, 2021

Merge pull request kubernetes#1608 from NickrenREN/pv-protection

7ee6686

postpone pv deletion if it is bound to a pvc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

postpone pv deletion if it is bound to a pvc #1608

postpone pv deletion if it is bound to a pvc #1608

NickrenREN commented Jan 16, 2018 •

edited

Loading

pospispa Jan 16, 2018

NickrenREN Jan 16, 2018 •

edited

Loading

pospispa Jan 16, 2018

NickrenREN Jan 16, 2018 •

edited

Loading

pospispa Jan 16, 2018

NickrenREN Jan 16, 2018 •

edited

Loading

pospispa Jan 16, 2018

NickrenREN Jan 16, 2018 •

edited

Loading

pospispa Jan 18, 2018

jsafrane Jan 19, 2018

pospispa commented Jan 18, 2018

NickrenREN commented Jan 18, 2018

msau42 commented Jan 18, 2018

jsafrane commented Jan 19, 2018

jsafrane left a comment

jsafrane Jan 19, 2018

jsafrane Jan 19, 2018

NickrenREN commented Jan 19, 2018

jsafrane commented Jan 19, 2018

NickrenREN commented Jan 19, 2018

jsafrane commented Jan 19, 2018

NickrenREN commented Jan 19, 2018

liggitt commented Jan 19, 2018

jsafrane commented Feb 2, 2018

jsafrane commented Feb 2, 2018

childsb commented Feb 2, 2018

childsb commented Feb 8, 2018

k8s-ci-robot commented Feb 8, 2018


		## Motivation

		User can delete a Persistent Volume (PV) that is being used by a PVC. It may result in data loss.


		### API Server, PV Admission Controller, PV Create:

		A new plugin for PV admission controller will be created. The plugin will automatically add finalizer information into newly created PV's metadata, just like `PVC protection` does.


		### Add this logic to the existing PV controller instead of creating a new admission and protection controller
		When we bind PV to PVC, we add finalizer for PV and remove finalizer when PV is no longer bound to a PVC.

postpone pv deletion if it is bound to a pvc #1608

postpone pv deletion if it is bound to a pvc #1608

Conversation

NickrenREN commented Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

NickrenREN Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickrenREN Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickrenREN Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickrenREN Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pospispa commented Jan 18, 2018

NickrenREN commented Jan 18, 2018

msau42 commented Jan 18, 2018

jsafrane commented Jan 19, 2018

jsafrane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickrenREN commented Jan 19, 2018

jsafrane commented Jan 19, 2018

NickrenREN commented Jan 19, 2018

jsafrane commented Jan 19, 2018

NickrenREN commented Jan 19, 2018

liggitt commented Jan 19, 2018

jsafrane commented Feb 2, 2018

jsafrane commented Feb 2, 2018

childsb commented Feb 2, 2018

childsb commented Feb 8, 2018

k8s-ci-robot commented Feb 8, 2018

NickrenREN commented Jan 16, 2018 •

edited

Loading

NickrenREN Jan 16, 2018 •

edited

Loading

NickrenREN Jan 16, 2018 •

edited

Loading

NickrenREN Jan 16, 2018 •

edited

Loading

NickrenREN Jan 16, 2018 •

edited

Loading