AWS EBS volume support #5138

justinsb · 2015-03-06T19:07:58Z

Code definitely doesn't work yet, and an open discussion in #5129 on whether we should write server & client code once (with a provider approach) or N times (with a type-per-cloud approach).

But would greatly appreciate any comments particularly on the pkg/cloudprovider/aws/aws.go code. The extra code is non-trivial because AWS volume mounting is pretty painful: you have to specify an unused mount device.

justinsb · 2015-03-06T19:08:39Z

AWS code is mostly here: bd9021e

markturansky · 2015-03-06T19:34:38Z

pkg/api/types.go

 	// kubelet's host machine and then exposed to the pod.
-	GCEPersistentDisk *GCEPersistentDiskVolumeSource `json:"persistentDisk"`
+	CloudPersistentDisk *CloudPersistentDiskVolumeSource `json:"persistentDisk"`


I don't think this is going to work. You'll overload this struct for every type of cloud provider, which means you have to type it somehow. Some properties would be relevant to some but not all of the providers.

Better, I think, to have an explicit struct per type.

We agree there will be commonalities between them. Good code structure can promote reuse for the common pieces.

Yeah, I'd started this before I saw that there were different viewpoints on #5129. Really looking for feedback on the AWS stuff specifically; once we figure out #5129 then I'll rebase the AWS work on to whatever we choose.

@justinsb could you modify this so it implements AWS persistent volumes as sibling to GCE PD. Similar to other proposed persistent volume plugins: #4612, and #4601.
I think there is general consensus on doing that for now, then improving the abstraction to promote code sharing after a couple of them are merged. See the discussion here on https://github.com/GoogleCloudPlatform/kubernetes/pull/4601/files#r25993686

markturansky · 2015-03-06T20:02:02Z

cc @swagiaal

piosz · 2015-03-31T09:52:36Z

Is this PR active? If not please close it according to https://github.com/GoogleCloudPlatform/kubernetes/blob/master/docs/devel/pull-requests.md

justinsb · 2015-03-31T13:01:58Z

I should be ready with the updated code this week. I'm currently working on load balancer support, and maintaining two big feature branches simultaneously is too hard, so I'm till that merges before working on EBS.

ghost · 2015-03-31T19:09:33Z

@justinsb I can work on this if you are too busy with other PRs. It was on my radar before you posted this.

justinsb · 2015-03-31T19:23:33Z

I think it's really close, and not sure if it makes any sense to attempt a hand-off.

Let me try to get some of my smaller PRs merged, so I only have to deal with this one and the ELB one... That should be manageable.

ghost · 2015-03-31T19:28:42Z

@justinsb Sounds good :)

justinsb · 2015-04-03T20:47:48Z

This now "works for me", but I need to run the e2e tests (which I have extended to cover AWS).

Should I squash? I don't know if anyone really cares to see my thought process here :-) And there hasn't been that much review activity yet...

markturansky · 2015-04-04T12:41:54Z

pkg/api/types.go

@@ -189,6 +189,9 @@ type VolumeSource struct {
 	// GCEPersistentDisk represents a GCE Disk resource that is attached to a
 	// kubelet's host machine and then exposed to the pod.
 	GCEPersistentDisk *GCEPersistentDiskVolumeSource `json:"gcePersistentDisk"`
+	// AWSPersistentDisk represents a AWS EBS disk that is attached to a
+	// kubelet's host machine and then exposed to the pod.
+	AWSPersistentDisk *AWSPersistentDiskVolumeSource `json:"awsPersistentDisk"`


Would AWSElasticBlockStore be more accurately named?

Perhaps. EBS is just a marketing term though, right? I'd rather think about the API in terms of what the user is trying to achieve - they want a persistent volume. Maybe it should be AWSPersistentVolume.

I guess the question is when Amazon announces ElasticNvram, which is still a block device, would we create a second entry in the API? (Or, if they had chosen to launch SSD under a different name). I'd argue we shouldn't, and as such we shouldn't mirror Amazon's trademark-du-jour...

I believe @bgrant0607's intention is to remove all these types of disk from VolumeSource and from the end user (pod authors and the like). The user will make a request for a persistent volume (via PVClaim) and get something. They won't know what and they shouldn't care.

The cluster admin, OTOH, deals with specific infrastructure. He needs to create a specific kind of disk. AWSElasticBlockStore maps cleanly to the AWS API type EbsBlockDevice.

OK, I'll change it. Avoiding another rebase is more important to me than nomenclature :-)

markturansky · 2015-04-04T12:54:40Z

Also see fuzzer.go for testing the new volume: https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/api/testing/fuzzer.go#L176

justinsb · 2015-04-07T21:00:43Z

Added fuzzer, rebased to avoid conflicts with iSCSI

justinsb · 2015-04-09T00:01:46Z

Now with passing e2e tests also!

http://public-kubernetes-meteor.s3-website-us-east-1.amazonaws.com/builds/kubernetes-e2e-aws/6/_1.xml

In particular:

<testcase name="PD should schedule a pod w/ a RW PD, remove it, then schedule it on another host" classname="Kubernetes e2e Suite run 1 of 1" time="60.902563537"/>

markturansky · 2015-04-09T00:40:14Z

cmd/e2e/e2e.go

+	if context.Provider == "aws" {
+		awsConfig := "[Global]\n"
+		if cloudConfig.Zone == "" {
+			glog.Error("gce_zone must be specified for AWS")


is "gce_zone" correct here?

This is tricky. I'm not sure whether we want flags for each provider (i.e. potentially duplicate kube_master, gce_project, gce_zone -> aws_kube_master, aws_gce_project, aws_gce_zone) or whether we should treat these as "cloud_project" & "cloud_zone". If the latter, a secondary question is whether we should rename the flags.

Edit: I don't much care either way. I'm guessing we'll end up with duplicating (i.e. aws_zone)

You can only have one cloud provider at a time, right? Perhaps this is a good place for generic cloud patterns, so long as it's not planned to allow a cluster span infrastructure providers... If that's the case, that's probably an easy small PR instead of adding more to this one.

As long as the concepts fundamentally match up, I'm in favor of generification.

I'm fine with not renaming in this PR, but please file a bug on this. We just need to figure out the ripple of renaming them.

markturansky · 2015-04-09T00:49:10Z

LGTM for what I can review. You'll need @thockin for the rest.

thockin · 2015-04-09T04:14:12Z

pkg/api/types.go

@@ -189,6 +189,9 @@ type VolumeSource struct {
 	// GCEPersistentDisk represents a GCE Disk resource that is attached to a
 	// kubelet's host machine and then exposed to the pod.
 	GCEPersistentDisk *GCEPersistentDiskVolumeSource `json:"gcePersistentDisk"`
+	// AWSElasticBlockStore represents an AWS EBS disk that is attached to a
+	// kubelet's host machine and then exposed to the pod.
+	AWSElasticBlockStore *AWSElasticBlockStoreVolumeSource `json:"awsElasticBlockStore"`


@bgrant0607 Are you OK with AWS support here to parallel GCE? It seems appropriate. We can EOL them both when persistent volumes are up to snuff.

@markturansky

Per comments from @markturansky - thanks!

justinsb · 2015-04-10T20:29:59Z

Opened issue #6699 to track renaming the e2e cmd arguments.

AWS EBS volume support

lavalamp · 2015-04-10T22:47:50Z

@thockin, @justinsb -- this definitely could have used a squash

thockin · 2015-04-10T22:49:31Z

Yeah, I messed up - meant to ask for it, forgot.

I wonder if there are tools that could help here?

On Fri, Apr 10, 2015 at 3:48 PM, Daniel Smith notifications@github.com
wrote:

@thockin https://github.com/thockin, @justinsb
https://github.com/justinsb -- this definitely could have used a squash

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

justinsb · 2015-04-11T00:17:04Z

Sorry, I should definitely have squashed it once we were getting closer. But squashing also obliterates history. I have looked, and haven't found any great solution. Even with squashing, I still don't love the merge-commits that the button produces.

I hear code.google.com will soon be available. Let's build v2 (on gerrit this time!)

Or we could probably figure out a script that squashes the commits, changes the author and does a push.

bgrant0607 · 2015-04-11T04:13:20Z

Sorry for the late response. Github notifications are useless, since I get hundreds per day.

Fortunately, I'm ok with this. Next time, please ask someone to ping me not through github for an API change.

delianides · 2015-04-30T15:48:32Z

Is there going to be any documentation for this? How exactly does this work with availability zones? Should the volume exists in the same AZ as the master?

tslater · 2015-06-04T17:10:35Z

I would love to see some documentation as well.

googlebot added the cla: yes label Mar 6, 2015

markturansky reviewed Mar 6, 2015
View reviewed changes

justinsb force-pushed the cloud_pd branch 3 times, most recently from 30c81c4 to 18a1d11 Compare March 27, 2015 16:01

justinsb force-pushed the cloud_pd branch from 18a1d11 to 40e662d Compare April 2, 2015 14:26

justinsb mentioned this pull request Apr 2, 2015

Make AWS configuration file optional (fall-back to metadata service) #6370

Merged

justinsb changed the title ~~WIP: AWS EBS volume support~~ AWS EBS volume support Apr 3, 2015

markturansky reviewed Apr 4, 2015
View reviewed changes

justinsb force-pushed the cloud_pd branch from dadf2a1 to 4d7d3ad Compare April 7, 2015 20:34

justinsb force-pushed the cloud_pd branch 3 times, most recently from 0557a5b to 60dfbc6 Compare April 8, 2015 23:37

markturansky reviewed Apr 9, 2015
View reviewed changes

thockin reviewed Apr 9, 2015
View reviewed changes

justinsb added 19 commits April 10, 2015 13:25

Add comment about EBS status being a bit slow through API

aa60510

Rename pdName -> volumeId for AWS persistent volumes

95b68ae

Fixup merge mistakes

2e91fdd

Add AWSPersistentDisk to fuzzer

b3666ed

Rename AWSPersistentDisk -> AWSElasticBlockStore, aws-pd -> aws-ebs

9711e77

Per comments from @markturansky - thanks!

Apply latest changes from copy-and-pasted gce_pd

a20484b

Add missing conversion for v1beta2

c7c9695

Make fetching the aws instance id optional, so we can use it on e2e

4e17677

Remove now-unused instanceId parameter from newAwsCloud

5a887e8

Support multiple k8s clusters

034412a

Provide more output during a disk delete

9561366

Grammar fix: s/a AWS/an AWS/g

7e758fe

Style: Ebs -> EBS

46f9c2c

Style: awsId -> awsID

2afc184

Style: Aws -> AWS

98c9ebb

Style: volumeId -> volumeID

933cf60

Rename aws_pd -> aws_ebs

503e19e

Fix some mistaken volumeId -> volumeID changes

9462471

Rename aws_pd.go -> aws_ebs.go, aws_pd_test.go -> aws_ebs_test.go

7626914

justinsb force-pushed the cloud_pd branch from 5fd50c9 to 7626914 Compare April 10, 2015 20:25

justinsb mentioned this pull request Apr 10, 2015

Clean up parameters in e2e tests #6699

Closed

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

Merge pull request #5138 from justinsb/cloud_pd

4a7b0ee

AWS EBS volume support

thockin merged commit 4a7b0ee into kubernetes:master Apr 10, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS EBS volume support #5138

AWS EBS volume support #5138

justinsb commented Mar 6, 2015

justinsb commented Mar 6, 2015

markturansky Mar 6, 2015

justinsb Mar 6, 2015

ghost Mar 11, 2015

markturansky commented Mar 6, 2015

piosz commented Mar 31, 2015

justinsb commented Mar 31, 2015

ghost commented Mar 31, 2015

justinsb commented Mar 31, 2015

ghost commented Mar 31, 2015

justinsb commented Apr 3, 2015

markturansky Apr 4, 2015

justinsb Apr 7, 2015

markturansky Apr 7, 2015

justinsb Apr 7, 2015

markturansky commented Apr 4, 2015

justinsb commented Apr 7, 2015

justinsb commented Apr 9, 2015

markturansky Apr 9, 2015

justinsb Apr 9, 2015

markturansky Apr 9, 2015

thockin Apr 9, 2015

thockin Apr 10, 2015

markturansky commented Apr 9, 2015

thockin Apr 9, 2015

justinsb commented Apr 10, 2015

lavalamp commented Apr 10, 2015

thockin commented Apr 10, 2015

justinsb commented Apr 11, 2015

bgrant0607 commented Apr 11, 2015

delianides commented Apr 30, 2015

tslater commented Jun 4, 2015

AWS EBS volume support #5138

AWS EBS volume support #5138

Conversation

justinsb commented Mar 6, 2015

justinsb commented Mar 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markturansky commented Mar 6, 2015

piosz commented Mar 31, 2015

justinsb commented Mar 31, 2015

ghost commented Mar 31, 2015

justinsb commented Mar 31, 2015

ghost commented Mar 31, 2015

justinsb commented Apr 3, 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

markturansky commented Apr 4, 2015

justinsb commented Apr 7, 2015

justinsb commented Apr 9, 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

markturansky commented Apr 9, 2015

Choose a reason for hiding this comment

justinsb commented Apr 10, 2015

lavalamp commented Apr 10, 2015

thockin commented Apr 10, 2015

justinsb commented Apr 11, 2015

bgrant0607 commented Apr 11, 2015

delianides commented Apr 30, 2015

tslater commented Jun 4, 2015