WIP: Persistent Storage API #5105

markturansky · 2015-03-05T21:54:41Z

Continuing from #4055. Follows #5096 (proposal).

@bgrant0607 @thockin @smarterclayton @pmorie

googlebot · 2015-03-05T21:54:43Z

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-06T15:12:21Z

pkg/api/types.go

+
+	// HostPath represents a directory on the host.  It is useful for testing PersistentVolumes.
+	HostPath *HostPathVolumeSource `json:"hostPath"`
+}


@bgrant0607 Regarding your comment on #5129 (not wanting cloud objects in the API) and feedback on #4055, I think this PersistentVolumeSource is what you and @thockin were suggesting as the struct that contains these volumes for the administrator provisioning PVs.

An API review would be greatly appreciated :)

FYI, mentioning me using @bgrant0607 is useless, until github provides a way for me to revert to watching issues rather than participating. Please ping me on IRC or email me directly.

This is an improvement, in that the user's pod need only specify the claim and not the source. Eventually, I'd like the definitions of these types to be in separate files and embedded via a plugin mechanism.

thockin · 2015-03-06T22:13:45Z

starting review, needs a rebase

thockin · 2015-03-06T22:39:45Z

pkg/api/types.go

+
+// Similar to VolumeSource but meant for the administrator who creates PVs.
+type PersistentVolumeSource struct {
+


errant blank line

comment that exactly one of the following must be set

thockin · 2015-03-06T23:43:58Z

done with this pass

bgrant0607 · 2015-03-08T04:31:19Z

pkg/api/types.go

@@ -180,6 +180,139 @@ type VolumeSource struct {
 	GitRepo *GitRepoVolumeSource `json:"gitRepo"`
 	// Secret represents a secret that should populate this volume.
 	Secret *SecretVolumeSource `json:"secret"`
+	// PersistentVolumeClaimVolumeSource represents a reference to a PersistentVolumeClaim in the same namespace
+	PersistentVolumeClaimVolumeSource *PersistentVolumeClaimVolumeSource `json:"persistentVolumeClaim,omitempty"`


The field would normally have the same name as the field. Is it not possible to name this field PersistentVolumeClaim? Or at least eliminate the extra Volume? Or, you could just inline the struct.

You're trying to support both direct reference and indirect reference via PersistentVolumeClaim?

Sorry, nevermind. Was confused since PersistentVolumeClaimVolumeSource didn't immediately follow this struct.

…d/required

thockin · 2015-03-23T17:51:50Z

For the sake of progress, I'm open to a finer-grained PR breakdown that I might otherwise be.

I like your proposed list, but maybe even more granular. Break the API step into a few steps.

All API types except PVClaimVolumeSource (all "master side" type) and related tweaks to keep the apiserver building and passing tests. This includes internal, versioned, conversions, and validation, and tests thereof. No REST endpoints, no implementation.
REST endpoints
PVClaimVolumeSource and kubelet

then the rest that you identified

I think overall the API piece of this PR is pretty solid. It's your choice to either clone this branch & PR to a new one for just API or to clone this branch and strip out non-API stuff (i.e. re-use this PR number or not :)

markturansky · 2015-03-23T17:55:42Z

I'll start now on a new clean PR for the API types as you suggest.

ghost · 2015-03-23T17:57:49Z

pkg/api/latest/latest.go

+		"Node":             true,
+		"Minion":           true,
+		"Namespace":        true,
+		"PersistentVolume": true,


I think we're going to want PersistentVolumes to be inside namespaces. In the future general case, individual users will be able to create their own PV's, and want to put them in namespaces to avoid clashes. In the first iteration, where the "storage administrator" creates all PV's on behalf of users, would there be any downside to having those inside namespaces too (even if it's just a global "admin-created-volumes" namespace or similar)?

@quinton-hoole I think the current implementations of VolumeSource supports what you are asking.

Example, Tim was thinking the new NFSMount plugin can stay in VolumeSource as well as be represented in the new PersistentVolumeSource. He saw re-use for that in a pod beyond an admin-provisioned volume. In this case, the NFS share is a pet and continues to live outside the lifecycle of a pod. It can reused by subsequent pods.

Does the correctly address what you are saying?

Also, I don't see a problem with pinning some PVs to a specific namespace (the admin provisions it purposefully that way), except for how the code handles namespaced PVs versus non-namespaced ones.

ghost · 2015-03-23T18:21:14Z

pkg/api/types.go

+	// used for PersistentVolumes that are bound
+	VolumeBound PersistentVolumePhase = "Bound"
+	// used for PersistentVolumes where the bound PersistentVolumeClaim was deleted
+	VolumeReleased PersistentVolumePhase = "Released"


How is Released different from Available?

Released means it was used and consumed as a resource. It is not yet "ready" for a new claim because it needs to be recycled.

A follow-on task for provisioning (dynamic Create and Recycle methods) follows the initial implementation of persistent storage.

bgrant0607 · 2015-03-26T21:48:41Z

FYI: I do intend to get to this, but I'm prioritizing issues blocking v1beta3 at the moment. I believe this should be additive, not blocking.

It might be helpful to schedule a review meeting for next week to ensure this makes progress, however.

bgrant0607 · 2015-03-26T21:49:07Z

Also see suggestions here: #5492 (comment)

bgrant0607 · 2015-03-27T21:50:46Z

Trying to catch up: What's the status of this PR? Are smaller pieces being submitted as smaller PRs? Should I review the API at this point? What other parts of the PR need more review? Would it help to schedule a meeting next week?

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

The API is merged with #5800.

The client green and pending review with #6002.

I've got a WIP for the PersistentVolumeManager in #6105.

I need to create a Claim volume branch from the code here. I think the volume is the simpler of the two pieces remaining.

markturansky · 2015-04-10T20:26:51Z

PVC volume plugin: #6695

Closing this PR. All pieces accounted for in linked PRs.

googlebot added the cla: no label Mar 5, 2015

markturansky reviewed Mar 6, 2015
View reviewed changes

markturansky mentioned this pull request Mar 6, 2015

PersistentDisk: one per cloud, or one for all clouds? #5129

Closed

markturansky force-pushed the pv_api branch from 2ceeb29 to fd2dc06 Compare March 6, 2015 22:36

thockin reviewed Mar 6, 2015
View reviewed changes

bgrant0607 reviewed Mar 8, 2015
View reviewed changes

markturansky added 6 commits March 23, 2015 13:03

added descriptions to each versioned types.go

e15ff33

rebased, added fields.Selector

f16f5dd

added missing descriptions, removed future PV structs, and rebased

d4e6bab

rebased and updated to changes

bebacb3

Added test-cmd.sh tests to create/delete PV & PVC

aa694df

removed name check, per feedback

b7cff5b

markturansky force-pushed the pv_api branch from 5105f22 to b7cff5b Compare March 23, 2015 17:05

improved comment for PersistentVolumePhase, tweaked wording of desire…

7ec38af

…d/required

ghost reviewed Mar 23, 2015
View reviewed changes

markturansky mentioned this pull request Mar 23, 2015

PersistentVolume & PersistentVolumeClaim API types #5800

Merged

ghost reviewed Mar 23, 2015
View reviewed changes

markturansky mentioned this pull request Mar 27, 2015

Node upgrades: e2e test: ensure persistent volumes survive when pods die #6084

Closed

markturansky mentioned this pull request Mar 27, 2015

PersistentVolumeClaimBinder #6105

Merged

markturansky mentioned this pull request Mar 31, 2015

[DO NOT MERGE] Persistent Storage #4055

Closed

markturansky closed this Apr 10, 2015

markturansky mentioned this pull request Apr 27, 2015

ResourceRequest Limits does not work as described #7355

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Persistent Storage API #5105

WIP: Persistent Storage API #5105

markturansky commented Mar 5, 2015

googlebot commented Mar 5, 2015

markturansky Mar 6, 2015

bgrant0607 Mar 8, 2015

thockin commented Mar 6, 2015

thockin Mar 6, 2015

thockin Mar 6, 2015

thockin commented Mar 6, 2015

bgrant0607 Mar 8, 2015

bgrant0607 Mar 8, 2015

thockin commented Mar 23, 2015

markturansky commented Mar 23, 2015

ghost Mar 23, 2015

markturansky Mar 23, 2015

ghost Mar 23, 2015

markturansky Mar 23, 2015

bgrant0607 commented Mar 26, 2015

bgrant0607 commented Mar 26, 2015

bgrant0607 commented Mar 27, 2015

markturansky commented Mar 27, 2015

markturansky commented Apr 10, 2015


		// Similar to VolumeSource but meant for the administrator who creates PVs.
		type PersistentVolumeSource struct {

WIP: Persistent Storage API #5105

WIP: Persistent Storage API #5105

Conversation

markturansky commented Mar 5, 2015

googlebot commented Mar 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Mar 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Mar 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Mar 23, 2015

markturansky commented Mar 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Mar 26, 2015

bgrant0607 commented Mar 26, 2015

bgrant0607 commented Mar 27, 2015

markturansky commented Mar 27, 2015

markturansky commented Apr 10, 2015