[do not merge]: Allow reclamation of PersistentVolumes #8334

markturansky · 2015-05-15T18:26:50Z

New pv.Spec.ReclamationPolicy
- Allows Recycle/Delete on release. Retain by default.
New volume plugin interfaces
- RecyclableVolumePlugin -- allows recycling of PVs
- DeletableVolumePlugin -- allows deletion of PVs from the infrastructure
New PersistentVolumeRecycler controller loop
- single threaded initially for correctness
- separate from PersistentVolumeClaimBinder because we don't want the binder to block on reclamation tasks
- works with the binder to remove or re-add reclaimed volumes to/from the pool of available volumes

Needs implementations:

Basic scrub impl: run a pod w/ busybox "rm -rf" on the volume
- develop/test with HostPath, use for NFS, potentially other volumes
Cloud volumes: delete via API

PVs deleted from cloud providers as policy will be reprovisioned by #6773

Edit: The purpose of this PR is to dynamically reclaim used and released resources. As PersistentVolumeClaims are deleted, their backing PersistentVolumes are released, but they are not yet reusable. Either they need to be scrubbed of their data before become Available again or the volumes should be deleted from the infrastructure.

The scrubbing use case is a build server using NFS as scratch space. No need for an admin to reprovision the cluster after the volumes are used up. Basic scrub is sufficient to put them back into the pool. Don't use basic scrub at The Fed or the NSA.

markturansky · 2015-05-15T18:28:12Z

@pmorie @thockin would love feedback on the direction, particularly around the new types of volume plugins (to allow some but not all PVs to be recycled or deleted).

markturansky · 2015-05-18T20:32:54Z

I will remove Deleter as an interface to narrow the scope to just Recycler. I would like confirmation that using new types of plugins is the correct way of handling this, though, so the current Recycler can be implemented and we'll know the future Deleter would be the correct solution.

pmorie · 2015-05-18T20:35:37Z

Gonna take a pass at this today at some point.

thockin · 2015-05-22T21:21:43Z

cmd/kube-controller-manager/app/controllermanager.go

@@ -256,6 +256,11 @@ func (s *CMServer) Run(_ []string) error {

 	pvclaimBinder := volumeclaimbinder.NewPersistentVolumeClaimBinder(kubeClient, s.PVClaimBinderSyncPeriod)
 	pvclaimBinder.Run()
+	pvRecycler, err := volumeclaimbinder.NewPersistentVolumeRecycler(kubeClient, s.PVClaimBinderSyncPeriod, ProbeRecyclableVolumePlugins())
+	if err != nil {
+		glog.Errorf("Failed to start persistent volume recycler: %+v", err)


is this an error or a Fatal?

thockin · 2015-05-22T22:04:43Z

Overall LGTM, I'll let it percolate. It'll need a finer-grained review when I have a bit more time for it.

markturansky · 2015-05-23T01:03:51Z

Thanks @thockin. I'll ping again when ready for a full review.

saad-ali · 2015-05-26T18:51:59Z

Thank you very much for the pull request - we sincerely appreciate the work that goes into sending us a change.

We are currently in a feature-freeze and general code slush as we work toward our 1.0 release. Our initial triage indicates that this is a non-critical change at this time, so we'd like to leave it open and revisit it after 1.0. We expect the freeze to wrap up in early to mid July.

If you feel that this triage is incorrect, please respond and let us know why. We're currently accepting changes that meet any of the following criteria:

Changes directly related to v1.0 issues
Changes addressing P0 bugs
Changes to supported platforms to bring them up to par with the primary development platforms
Docs, user experience, and test related fixes

smarterclayton · 2015-05-26T19:10:53Z

This in on the RH priority list for merge.

smarterclayton · 2015-05-26T19:11:16Z

We can potentially push it if the risk is seen as high, but I haven't seen that triage performed.

thockin · 2015-05-26T19:47:04Z

Iff we can turn this around reasonably quickly we can get it in.

On Tue, May 26, 2015 at 12:11 PM, Clayton Coleman notifications@github.com
wrote:

We can potentially push it if the risk is seen as high, but I haven't seen
that triage performed.

—
Reply to this email directly or view it on GitHub
#8334 (comment)
.

smarterclayton · 2015-05-26T19:50:14Z

Mark is today or tomorrow reasonable or is there more still there?

On May 26, 2015, at 3:47 PM, Tim Hockin notifications@github.com wrote:

Iff we can turn this around reasonably quickly we can get it in.

On Tue, May 26, 2015 at 12:11 PM, Clayton Coleman notifications@github.com
wrote:

We can potentially push it if the risk is seen as high, but I haven't seen
that triage performed.

—
Reply to this email directly or view it on GitHub
#8334 (comment)
.

—
Reply to this email directly or view it on GitHub.

markturansky · 2015-05-26T19:50:50Z

I am only working through the busybox implementation. debugging it to scrub the volume. that is all.

markturansky · 2015-05-26T21:37:06Z

The basic implementation is in for HostPath but i'm working through bugs. Once it's good, I'll apply it to NFS.

eparis · 2015-05-26T23:12:02Z

Realistically this is a pretty key thing needed to make persistentVolumes useful, which are a 1.0 feature...

eparis · 2015-05-26T23:12:50Z

So @markturansky is basically full time on figuring this out right!

markturansky · 2015-05-26T23:27:54Z

@eparis hopefully it won't be a long night. the pod and watch currently works, just need to get the pod scrub command correct. I'll need a good test, too.

… recycler additions

…ix some small NFS issues

…insure volume was scrubbed

markturansky · 2015-05-29T16:26:13Z

@thockin e2e passes, but like the volumes e2e test (on which this was created), it requires privileged, so it won't run in the e2e suite by default.

Here is a full run of the recycler: https://gist.github.com/jsafrane/6dc167d752a70a570b7e

markturansky · 2015-05-29T17:00:44Z

all green tests, too, FWIW.

markturansky · 2015-05-29T21:29:27Z

This large PR is broken down into smaller, linked PRs. They should be reviewed and merged roughly in order, as each cherry-picks the the former.

markturansky · 2015-06-05T16:36:47Z

Closing this PR. All commits accounted for in #9024.

googlebot added the cla: yes label May 15, 2015

markturansky mentioned this pull request May 15, 2015

Allow Secrets on Volumes and PersistentVolumes #7150

Closed

markturansky force-pushed the pv_recycling branch from 140c16e to 7f8ef51 Compare May 19, 2015 19:56

markturansky mentioned this pull request May 22, 2015

PersistentVolume remains Bound after PVC is deleted openshift/origin#2434

Closed

markturansky force-pushed the pv_recycling branch 2 times, most recently from d1f16db to 105f92d Compare May 22, 2015 20:57

thockin reviewed May 22, 2015
View reviewed changes

markturansky force-pushed the pv_recycling branch 3 times, most recently from 41ad32b to e024dc9 Compare May 26, 2015 17:50

saad-ali added this to the v1.0-post milestone May 26, 2015

markturansky mentioned this pull request May 26, 2015

Finalize Kube items openshift/origin#2233

Closed

9 tasks

markturansky force-pushed the pv_recycling branch from 9aed8e8 to 8703f6f Compare May 26, 2015 21:22

markturansky added 15 commits May 29, 2015 11:45

Recycler framework

edcdc6d

fixed recyled phase changes

ea4c0b8

func pointer for recycler creation to allow easier testing/mocks.

21d946a

added func to create recyclers for easier testing. fixed unit test w/…

543b431

… recycler additions

rebased and regenerated conversions

0be30fc

renamed ReclamationPolicy to PersistentVolumeReclaimPolicy

85cb246

added basic scrub pod and watch, needs debugging

661dfdd

introduced func to get recyclable plugins, fixed test

f7c007a

fixed busybox script cmd

53d9cb2

tweaks scrubber

1918deb

fixed scrubber bug, go formatted

d8b6b64

added generated deep copy funcs

c282f13

Fixed json field naming for PVReclaimPolicy

0869209

Added failed state and better login in recycler. added PV e2e test. f…

9d36142

…ix some small NFS issues

added check to recycler to insure PV is Released, added test for same

6c3c632

markturansky force-pushed the pv_recycling branch from 4214c1b to 6c3c632 Compare May 29, 2015 15:49

markturansky added 3 commits May 29, 2015 11:52

changed permissions in test image to allow scrubbing. added check to …

3472f43

…insure volume was scrubbed

go formatted

65121b8

changed to fatal, per feedback

2119c61

This was referenced May 29, 2015

PersistentVolume Recycling API #9018

Closed

Fixes nfs-server security issue in image #9019

Merged

Add RecycableVolumePlugin interface #9020

Closed

NFS and HostPath Recycle implementations #9021

Closed

markturansky changed the title ~~WIP: Allow reclamation of PersistentVolumes~~ [do not merge]: Allow reclamation of PersistentVolumes May 29, 2015

markturansky mentioned this pull request May 29, 2015

PersistentVolumeRecycler controller #9024

Merged

markturansky closed this Jun 5, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[do not merge]: Allow reclamation of PersistentVolumes #8334

[do not merge]: Allow reclamation of PersistentVolumes #8334

markturansky commented May 15, 2015

markturansky commented May 15, 2015

markturansky commented May 18, 2015

pmorie commented May 18, 2015

thockin May 22, 2015

thockin commented May 22, 2015

markturansky commented May 23, 2015

saad-ali commented May 26, 2015

smarterclayton commented May 26, 2015

smarterclayton commented May 26, 2015

thockin commented May 26, 2015

smarterclayton commented May 26, 2015

markturansky commented May 26, 2015

markturansky commented May 26, 2015

eparis commented May 26, 2015

eparis commented May 26, 2015

markturansky commented May 26, 2015

markturansky commented May 29, 2015

markturansky commented May 29, 2015

markturansky commented May 29, 2015

markturansky commented Jun 5, 2015

[do not merge]: Allow reclamation of PersistentVolumes #8334

[do not merge]: Allow reclamation of PersistentVolumes #8334

Conversation

markturansky commented May 15, 2015

markturansky commented May 15, 2015

markturansky commented May 18, 2015

pmorie commented May 18, 2015

thockin May 22, 2015

Choose a reason for hiding this comment

thockin commented May 22, 2015

markturansky commented May 23, 2015

saad-ali commented May 26, 2015

smarterclayton commented May 26, 2015

smarterclayton commented May 26, 2015

thockin commented May 26, 2015

smarterclayton commented May 26, 2015

markturansky commented May 26, 2015

markturansky commented May 26, 2015

eparis commented May 26, 2015

eparis commented May 26, 2015

markturansky commented May 26, 2015

markturansky commented May 29, 2015

markturansky commented May 29, 2015

markturansky commented May 29, 2015

markturansky commented Jun 5, 2015