Added volume.Creater interface and simple HostPath implementation #13650

markturansky · 2015-09-07T19:59:18Z

Part of #6773

volume.Creater interface allows a CreatableVolumePlugin to create volumes in the underlying infrastructure.

This PR is the framework for Creater and a deliberately gimped HostPath implementation for testing/example. Each plugin would implement Creater, as appropriate, in a separate PR.

@thockin @saad-ali @kubernetes/rh-storage

k8s-bot · 2015-09-07T20:01:42Z

GCE e2e build/test failed for commit 2fb5322deb2d2ebce6bbdfd47f0b23f334f47dd3.

k8s-github-robot · 2015-09-07T20:15:18Z

Labelling this PR as size/L

markturansky · 2015-09-09T18:55:51Z

@thockin I applied the same /tmp/.+ validation here in Creater that you suggested in Deleter.

This PR is half as large as Deleter. No calling code yet. That's the provisioning controller PR to come.

k8s-bot · 2015-09-09T20:00:45Z

GCE e2e build/test passed for commit 247fd865eeb75ce440f21c961f12000a84c429ed.

k8s-bot · 2015-09-09T20:01:22Z

GCE e2e build/test failed for commit 23c584cd4046ee073a6ab382cc09286d31fcfbdc.

k8s-bot · 2015-09-09T20:49:01Z

GCE e2e build/test failed for commit 753e42cf5019a6c6027aaa346156f8556136712a.

thockin · 2015-09-10T06:50:12Z

I am a little anxious about the proliferation of volume interfaces, but this flows from the rest, so I have no better answer (or I would have said it already :)

@saad-ali for final

k8s-bot · 2015-09-10T14:13:52Z

GCE e2e build/test passed for commit f5b3ce6b76c289698fb01981ce59d88dda789ff7.

k8s-bot · 2015-09-10T15:16:38Z

GCE e2e build/test passed for commit 930ab92a880351654535dac57c85c6eda7e73d2b.

markturansky · 2015-09-11T16:18:18Z

@saad-ali all green for this one, too.

pmorie · 2015-09-17T16:35:15Z

pkg/volume/plugins.go

+	VolumePlugin
+	// NewCreater creates a new volume.Creater which knows how to create this resource
+	// in accordance with the underlying storage provider
+	NewCreater(spec *Spec) (Creater, error)


I think this is going to need to take information about the pod spec, too. See #12944

Don't want that to block this going in, I just want to call it out. I don't know if we'll be getting to volume security for shared storage any time soon.

There won't be a pod when a PV is being created.

Security is important, though, and this constructor func was changed to work exactly like Builder, which takes VolumeOptions. We've got access to the same security info on the options that Builder has.

markturansky · 2015-09-17T19:42:38Z

@saad-ali This is ready for final review. Follows the same pattern of passing Options to a constructor and plumbing it through.

pmorie · 2015-09-17T19:45:39Z

pkg/volume/host_path/host_path.go

+// Create for hostPath simply creates a local /tmp/hostpath_pv/%s directory as a new PersistentVolume.
+// This Creater is meant for development and testing only and WILL NOT WORK in a multi-node cluster.
+func (r *hostPathCreater) Create() (*api.PersistentVolume, error) {
+	fullpath := fmt.Sprintf("/tmp/hostpath_pv/%s", util.NewUUID())


This isn't going to work with SELinux enforcing. The SELinux type will be tmp_t which is (rightfully) not usable from the SELinux context docker containers get.

That's great, then. We definitely don't want anyone using this by default in any cluster.

It's for development and local testing only. The provisioner will have integration tests that cover it fully. I don't need a writer pod to test this feature.

It's for development

Do you develop with setenforce 0?

That's right, I think you use a mac for development. Just make sure someone who wants to develop on this feature doesn't have to setenforce 0 please.

k8s-bot · 2015-09-18T14:01:18Z

GCE e2e build/test failed for commit 746a759e16360285cf4fd182a3012ce95d94c062.

k8s-bot · 2015-09-18T14:19:23Z

GCE e2e build/test failed for commit e56ba0568c9ca7c3d6ae6ba98d5ec5daf14f070e.

markturansky · 2015-09-18T16:08:50Z

@k8s-bot test this

k8s-bot · 2015-09-18T16:51:47Z

GCE e2e build/test passed for commit e56ba0568c9ca7c3d6ae6ba98d5ec5daf14f070e.

k8s-bot · 2015-09-18T18:08:21Z

GCE e2e build/test passed for commit 9c7d03b.

saad-ali · 2015-09-19T00:37:14Z

Thanks for the clarifications Mark!

I think the volume interface should have been named mounter or something, because it's responsible for taking a volume or PV and attaching/mounting it to the node. It doesn't make sense to stick Creater and Deleter in there because they are responsible for creating/deleting the volume independent of the node. Because we don't have the right separation between these responsibilities is why we end up with funkiness like VolumeOptions: it contains parameters that are specific to mounting (RootContext) and, now, specific to creation (CapacityMB, PersistentVolumeReclaimPolicy, etc.), but passed to both.

All that said, for the sake of expediency, it's fine as is--we can refactor this code later.

LGTM

@pmorie?

pmorie · 2015-09-20T07:16:34Z

pkg/volume/volume.go

@@ -65,6 +66,12 @@ type Recycler interface {
 	Recycle() error
 }

+// Create adds a new resource in the storage provider and creates a PersistentVolume for the new resource.
+// Calls to Create should block until complete.
+type Creater interface {


Shouldn't this be Creator? I don't want to pick a nit at the last minute if this is otherwise good. Follow-up if you agree?

pmorie · 2015-09-20T07:26:58Z

@saad-ali

I think the volume interface should have been named mounter or something, because it's responsible for taking a volume or PV and attaching/mounting it to the node. It doesn't make sense to stick Creater and Deleter in there because they are responsible for creating/deleting the volume independent of the node. Because we don't have the right separation between these responsibilities is why we end up with funkiness like VolumeOptions: it contains parameters that are specific to mounting (RootContext) and, now, specific to creation (CapacityMB, PersistentVolumeReclaimPolicy, etc.), but passed to both.

I'm in favor of a distinction between a volume's lifecycle with respect to the node and to the lifecycle of persistent volumes. Specifically regarding VolumeOptions, we would separate out a PersistentVolumeOptions and the PV controller would probably get both eventually (when we solve SELinux for persistent volumes).

I'll bounce shippable and see if we can get a green. LGTM other than Creator which is fine as a follow-up.

markt-gh · 2015-09-20T13:57:22Z

There is an API "creater", too, which I was happy to copy. Recycler,
Creater, Deleter...

I know Creator is proper spelling. I'd rather type the wrong one and it has
a precedent elsewhere.

I agree with the code factoring. I can move provisioning stuff elsewhere in
a follow-up PR.

On Sunday, September 20, 2015, Paul Morie notifications@github.com wrote:

Reopened #13650 #13650.

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

pmorie · 2015-09-20T18:56:38Z

There is an API "creater", too, which I was happy to copy. Recycler,
Creater, Deleter...

Ugh. Spelling matters to me in this area; I don't want to have to remember the particular wrong spelling we use. Maybe we should change the precedent Creater too.

pmorie · 2015-09-20T18:56:57Z

LGTM.

saad-ali · 2015-09-20T23:13:05Z

Thanks @pmorie
Adding LGTM label

k8s-github-robot · 2015-09-20T23:33:59Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2015-09-20T23:57:09Z

GCE e2e build/test passed for commit 9c7d03b.

k8s-github-robot · 2015-09-20T23:57:36Z

Automatic merge from submit-queue

Auto commit by PR queue bot

markturansky · 2015-10-02T19:09:38Z

pkg/volume/volume.go

+// Create adds a new resource in the storage provider and creates a PersistentVolume for the new resource.
+// Calls to Create should block until complete.
+type Creater interface {
+	Create() (*api.PersistentVolume, error)


I think now this is a little bit backward or should at least be 2 operations:

the provisioner needs to provide a template PV for it's plugin according to the volumeOptions.

the provisioner needs to invoke its internal logic for creation of the resource in the infrastructure.

A Claim's provisioning should be like this:

Create PVC with experimental annotation. will only bind to a PV with a matching pre-bind annotation.

Watch claim, Create PV - but unfulfilled in infrastructure. special annotation prevents binding until fulfillment.

Watch volumes, create resource -- use real PV name to link to provider -- remove unfulfilled annotation and save real volumeID.

claim will bind to newly provisioned volume

Yeah, I think that if we're retooling the controllers this makes more sense to be lifted out of the plugin.

Also, we've done sort of a poor job of defining exactly what logic belongs in the the GCE volume code vs what belongs in the GCE cloud provider code. More and more I am feeling that the GCE-API stuff should be in the cloud provider - the GCE specific provider, not as a generic abstraction. That module becomes our "one true place" to interface with GCE APIs. The GCEPD volume uses that to get PDs attached and mounted. (I mean eventually :)

@thockin @markturansky

That module becomes our "one true place" to interface with GCE APIs. The GCEPD volume uses that to get PDs attached and mounted. (I mean eventually :)

Thinking through that a bit -- so we will wind up with a provisioner for GCEPD that gets the cloud provider from the VolumeHost interface, the makes a downcast to GCECloudProvider, that uses some special function of that type to create a new PD.

That seems simple enough, but it's not going to be as cut and dry for other volume types. NFS, for example:

You want a provisioner that knows how to provision EFS volumes that are used as NFS volumes

You want a provisioner that knows how to provision using the netapp ONTAP API

What happens when you have a cluster with an aws cloud provider and you want to provision some NFS volumes via EFS and some via ONTAP against a machine running the ONTAP server in AWS?

Maybe we solve (3) by differentiating distinct volume types for differently provisioned volumes of the same type, but that doesn't sit quite right with me. I guess the question is whether there is really utility for an encompassing 'type' for each permutation of (client-side-concerns,provisioning-concerns) versus decoupling them.

Thinking through that a bit -- so we will wind up with a provisioner for GCEPD that gets the cloud provider from the VolumeHost interface, the makes a downcast to GCECloudProvider, that uses some special function of that type to create a new PD.

Correct - it's just a matter of where the code lives.

That seems simple enough, but it's not going to be as cut and dry for other volume types. NFS, for example:

You want a provisioner that knows how to provision EFS volumes that are used as NFS volumes
You want a provisioner that knows how to provision using the netapp ONTAP API
What happens when you have a cluster with an aws cloud provider and you want to provision some NFS volumes via EFS and some via ONTAP against a machine running the ONTAP server in AWS?

The NetApp NFS provisioner is a totally different beast than the GCE
provisioner. I do not think we should have a single provisioner
that knows how to provision every volume type.

Maybe we solve (3) by differentiating distinct volume types for differently provisioned volumes of the same type, but that doesn't sit quite right with me. I guess the question is whether there is really utility for an encompassing 'type' for each permutation of (client-side-concerns,provisioning-concerns) versus decoupling them.

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

@thockin

The NetApp NFS provisioner is a totally different beast than the GCE
provisioner. I do not think we should have a single provisioner
that knows how to provision every volume type.

I wasn't talking about a single provisioner that knows how to provision all types. I was saying, we'll have an EFS provisioner and netapp provisioner, do we need to differentiate entire plugins for those since they're both NFS on the client side (afaik), or do we need to have a mapping of the PV concerns to plugins that allows for us to DNR on volume plugins just to differentiate provisioners.

The provisioner will map what it gets from PVC onto what it knows to
create. That might start with "class = gold" mapping to netapp while
"class = bronze" maps to NFS on a RasPi.

On Fri, Oct 2, 2015 at 3:32 PM, Paul Morie notifications@github.com wrote:

In pkg/volume/volume.go
#13650 (comment)
:

@@ -65,6 +66,12 @@ type Recycler interface {
Recycle() error
}

+// Create adds a new resource in the storage provider and creates a PersistentVolume for the new resource.
+// Calls to Create should block until complete.
+type Creater interface {

Create() (*api.PersistentVolume, error)

@thockin https://github.com/thockin

The NetApp NFS provisioner is a totally different beast than the GCE
provisioner. I do not think we should have a single provisioner
that knows how to provision every volume type.

I wasn't talking about a single provisioner that knows how to provision
all types. I was saying, we'll have an EFS provisioner and netapp
provisioner, do we need to differentiate entire plugins for those since
they're both NFS on the client side (afaik), or do we need to have a
mapping of the PV concerns to plugins that allows for us to DNR on volume
plugins just to differentiate provisioners.

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/13650/files#r41074722.

googlebot added the cla: yes label Sep 7, 2015

k8s-github-robot assigned thockin Sep 7, 2015

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 7, 2015

markturansky mentioned this pull request Sep 7, 2015

PersistentVolume dynamic provisioning #6773

Closed

markturansky force-pushed the prov_creater branch from 2fb5322 to 23c584c Compare September 9, 2015 18:54

markturansky force-pushed the prov_creater branch from 23c584c to 247fd86 Compare September 9, 2015 19:04

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2015

markturansky force-pushed the prov_creater branch from 247fd86 to 753e42c Compare September 9, 2015 20:31

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2015

thockin assigned saad-ali and unassigned thockin Sep 10, 2015

markturansky force-pushed the prov_creater branch from 753e42c to f5b3ce6 Compare September 10, 2015 13:38

markturansky force-pushed the prov_creater branch from f5b3ce6 to 930ab92 Compare September 10, 2015 14:24

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2015

pmorie reviewed Sep 17, 2015
View reviewed changes

markturansky force-pushed the prov_creater branch 3 times, most recently from d55a0ad to 1e0cedd Compare September 17, 2015 19:36

pmorie reviewed Sep 17, 2015
View reviewed changes

markturansky added 3 commits September 18, 2015 13:14

Added volume.Creater interface and simple HostPath implementation

3bed0a2

added pv attrs to volumeOptions, improved tests

a1692e0

added separate clients per caller in integration tests

9c7d03b

markturansky force-pushed the prov_creater branch from e56ba05 to 9c7d03b Compare September 18, 2015 17:24

pmorie reviewed Sep 20, 2015
View reviewed changes

pmorie closed this Sep 20, 2015

pmorie reopened this Sep 20, 2015

pmorie mentioned this pull request Sep 20, 2015

Refactor to distinguish between volume lifecycle on node and within cluster for persistent volumes #14244

Closed

saad-ali added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge labels Sep 20, 2015

k8s-github-robot pushed a commit that referenced this pull request Sep 20, 2015

Merge pull request #13650 from markturansky/prov_creater

568c033

Auto commit by PR queue bot

k8s-github-robot merged commit 568c033 into kubernetes:master Sep 20, 2015

This was referenced Sep 25, 2015

Add flocker volume plugin #14328

Merged

Add support for flex volume. #13840

Merged

markturansky reviewed Oct 2, 2015
View reviewed changes

Added volume.Creater interface and simple HostPath implementation #13650

Added volume.Creater interface and simple HostPath implementation #13650

Conversation

markturansky commented Sep 7, 2015

k8s-bot commented Sep 7, 2015

k8s-github-robot commented Sep 7, 2015

markturansky commented Sep 9, 2015

k8s-bot commented Sep 9, 2015

k8s-bot commented Sep 9, 2015

k8s-bot commented Sep 9, 2015

thockin commented Sep 10, 2015

k8s-bot commented Sep 10, 2015

k8s-bot commented Sep 10, 2015

markturansky commented Sep 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markturansky commented Sep 17, 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

k8s-bot commented Sep 18, 2015

k8s-bot commented Sep 18, 2015

markturansky commented Sep 18, 2015

k8s-bot commented Sep 18, 2015

k8s-bot commented Sep 18, 2015

saad-ali commented Sep 19, 2015

Choose a reason for hiding this comment

pmorie commented Sep 20, 2015

markt-gh commented Sep 20, 2015

pmorie commented Sep 20, 2015

pmorie commented Sep 20, 2015

saad-ali commented Sep 20, 2015

k8s-github-robot commented Sep 20, 2015

k8s-bot commented Sep 20, 2015

k8s-github-robot commented Sep 20, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment