PersistentVolumeClaimBinder #6105

markturansky · 2015-03-27T22:06:07Z

Following #6002 (client).
From #5105.

googlebot · 2015-03-27T22:06:08Z

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

markturansky · 2015-03-30T17:26:05Z

@derekwaynecarr you can ignore all these client commits on this PR. They come by "soft resetting" the client branch to my manager branch. They will drop off once the client is merged.

This manager implementation can use some cleanup after the client is merged and before I ask for a review.

markturansky · 2015-04-07T16:29:37Z

@thockin This is ready for review.

The two big additions are:

PersistentVolumeManager is a simple "sync the world" loop like every other controller. I would like to refactor this into @lavalamp's DeltaFIFO queue as a follow-on task.
custom cache.Store/Index in pkg/volumemanager/types.go that indexes PVs by access modes and keeps them sorted for easy matching.

markturansky · 2015-04-08T16:50:45Z

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

@@ -190,6 +194,9 @@ func (s *CMServer) Run(_ []string) error {
 	namespaceManager := namespace.NewNamespaceManager(kubeClient)
 	namespaceManager.Run(s.NamespaceSyncPeriod)

+	persistentVolumeManager := volumemanager.NewPersistentVolumeManager(kubeClient)
+	persistentVolumeManager.Run(s.NamespaceSyncPeriod)


wrong period variable.

…utine for sync funcs

… test case.

…d to better phase transitioning in control loops

markturansky · 2015-04-27T16:27:15Z

Rebased again.

@thockin PTAL?

thockin · 2015-04-27T17:03:30Z

On it

thockin · 2015-04-27T17:37:06Z

I'm going to do a review of this as-is, but there's sort of a monster problem: This should use a "binding" sub-resource, as with pods. We've drawn the analogy of claims and PVs being like pods and nodes - we should follow the same pattern. That seems like a significant change, so I am willing to proceed on this path as long as we have a strong promise to fix it ASAP. This is, strictly speaking, an API change (albeit constrained to ~1 consumer - you).

As per our IRC chat, there are a lot of things this might change:

Do we bind the Claim to the PV or vice-versa?
Do we move the Claim.VolumeRef to Spec?
Do we allow users to ask for a specific PV?

thockin · 2015-04-27T17:40:40Z

pkg/api/testing/fuzzer.go

@@ -201,6 +201,14 @@ func FuzzerFor(t *testing.T, version string, src rand.Source) *fuzz.Fuzzer {
 			c.FuzzNoCustom(s) // fuzz self without calling this function again
 			s.Type = api.SecretTypeOpaque
 		},
+		func(pv *api.PersistentVolume, c fuzz.Continue) {
+			c.FuzzNoCustom(pv) // fuzz self without calling this function again
+			pv.Status.Phase = api.VolumePending


These are not fuzzing. You should pick a random number [0-num_phases) and index into a table of phase strings.

thockin · 2015-04-27T18:24:18Z

Looks good overall. Let's have the discussion about sub-resource. I think that doing it right means flipping the current status/spec arrangement and that is a tedious and breaking API change.

As per IRC - maybe we can flag gate this entirely? get this committed and off-the-books, let your folks proceed with testing, but force anyone who wants to use it to say --enable_alpha_persistent_volumes_support or something

markturansky · 2015-04-27T19:01:17Z

Immediate feedback addressed and added flag to optionally start the binder in controller-manager.

Regarding the proposed change for binding, I will create a new PR as a follow-on task.

thockin · 2015-04-27T20:01:47Z

pkg/volumeclaimbinder/persistent_volume_claim_binder.go

+		if volume.Spec.ClaimRef == nil {
+			return fmt.Errorf("PersistentVolume[%s] expected to be bound but found nil claimRef: %+v", volume)
+		} else {
+			// TODO: implement Recycle method on plugins


Just to be clear, at this point the admin must remove and recreate the PV object or set Phase manually, yes? There's no other way to revive it?

Yes, the current reclamation process is manual.

thockin · 2015-04-27T20:05:39Z

LGTM - do you want to fix the last nit or not worthwhile?

markturansky · 2015-04-27T20:14:37Z

@thockin i'm good as-is and will start looking to the Binding subresource.

PersistentVolumeClaimBinder

googlebot added the cla: no label Mar 27, 2015

markturansky mentioned this pull request Mar 27, 2015

WIP: Persistent Storage API #5105

Closed

markturansky force-pushed the yoko_pv_mgr branch from bffbad9 to f5700d4 Compare March 30, 2015 14:49

markturansky mentioned this pull request Mar 30, 2015

PersistentVolume & PersistentVolumeClaim Client #6002

Merged

brendandburns added cla: yes and removed cla: no labels Mar 30, 2015

markturansky mentioned this pull request Mar 31, 2015

WIP: Persistent Storage Volumes #5344

Closed

markturansky force-pushed the yoko_pv_mgr branch from f5700d4 to d3ad543 Compare April 4, 2015 17:09

googlebot added cla: no and removed cla: yes labels Apr 4, 2015

markturansky force-pushed the yoko_pv_mgr branch 2 times, most recently from c08eb87 to 6887558 Compare April 7, 2015 16:25

markturansky reviewed Apr 8, 2015
View reviewed changes

markturansky added 9 commits April 27, 2015 12:05

goformatted

6c6aab6

renamed exported symbol for pvOrderedIndex, added locks, remove go ro…

f26df6a

…utine for sync funcs

narrowed client interface to allow easier testing. added PVC deletion…

e1b885c

… test case.

Edited README

49883e7

Refactored to DeltaFifo

b634f17

Added more to README, tweaked pod, added service, fixed validation

2cf4e65

Added integration test, fixed a validation issue

37d7f3f

Added pending phase for volumes. added defaults for PV/PVC. refactore…

a04420e

…d to better phase transitioning in control loops

fixed prefix name in validation

2fe8ed5

markturansky force-pushed the yoko_pv_mgr branch from b0124f1 to 2fe8ed5 Compare April 27, 2015 16:06

thockin reviewed Apr 27, 2015
View reviewed changes

markturansky force-pushed the yoko_pv_mgr branch from 5db784a to 91c6e48 Compare April 27, 2015 18:58

addressed feedback. added opt-in cmd line flag

beacd87

markturansky force-pushed the yoko_pv_mgr branch from 91c6e48 to beacd87 Compare April 27, 2015 19:00

thockin reviewed Apr 27, 2015
View reviewed changes

thockin added a commit that referenced this pull request Apr 27, 2015

Merge pull request #6105 from markturansky/yoko_pv_mgr

635c393

PersistentVolumeClaimBinder

thockin merged commit 635c393 into kubernetes:master Apr 27, 2015

markturansky mentioned this pull request Apr 28, 2015

Migrate PV binding from PV.Spec.ClaimRef to Binding subresource #7438

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PersistentVolumeClaimBinder #6105

PersistentVolumeClaimBinder #6105

markturansky commented Mar 27, 2015

googlebot commented Mar 27, 2015

markturansky commented Mar 30, 2015

markturansky commented Apr 7, 2015

markturansky Apr 8, 2015

markturansky commented Apr 27, 2015

thockin commented Apr 27, 2015

thockin commented Apr 27, 2015

thockin Apr 27, 2015

thockin commented Apr 27, 2015

markturansky commented Apr 27, 2015

thockin Apr 27, 2015

markturansky Apr 27, 2015

thockin commented Apr 27, 2015

markturansky commented Apr 27, 2015

PersistentVolumeClaimBinder #6105

PersistentVolumeClaimBinder #6105

Conversation

markturansky commented Mar 27, 2015

googlebot commented Mar 27, 2015

markturansky commented Mar 30, 2015

markturansky commented Apr 7, 2015

markturansky Apr 8, 2015

Choose a reason for hiding this comment

markturansky commented Apr 27, 2015

thockin commented Apr 27, 2015

thockin commented Apr 27, 2015

thockin Apr 27, 2015

Choose a reason for hiding this comment

thockin commented Apr 27, 2015

markturansky commented Apr 27, 2015

thockin Apr 27, 2015

Choose a reason for hiding this comment

markturansky Apr 27, 2015

Choose a reason for hiding this comment

thockin commented Apr 27, 2015

markturansky commented Apr 27, 2015