Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Google Compute Engine (GCE) storage driver #394

Merged
merged 1 commit into from
Feb 13, 2017

Conversation

codenrhoden
Copy link
Contributor

@codenrhoden codenrhoden commented Jan 19, 2017

This patch re-introduces the GCE persistent disk driver.

Still TODO here:

  • docs
  • testplan/vagrant/multi-node testing
  • tag-based filtering
  • supporting SSD-based disks by default

@codecov-io
Copy link

codecov-io commented Jan 19, 2017

Codecov Report

Merging #394 into release/0.5.0 will not change coverage.
The diff coverage is n/a.

@@              Coverage Diff               @@
##           release/0.5.0     #394   +/-   ##
==============================================
  Coverage          30.49%   30.49%           
==============================================
  Files                 29       29           
  Lines               1741     1741           
==============================================
  Hits                 531      531           
  Misses              1152     1152           
  Partials              58       58

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f8b9c6...f5ef3be. Read the comment docs.

@akutz akutz changed the title WIP: Add GCE driver Google Compute Engine (GCE) storage driver Jan 26, 2017
@codenrhoden codenrhoden force-pushed the feature/gce branch 3 times, most recently from 6019bfd to 9329942 Compare January 30, 2017 16:55
@codenrhoden
Copy link
Contributor Author

Okay, I just pushed a commit to add docs. This is ready for "real" review now.

@cduchesne cduchesne mentioned this pull request Feb 6, 2017
19 tasks
@akutz akutz changed the base branch from master to release/0.5.0 February 9, 2017 23:12
@akutz
Copy link
Collaborator

akutz commented Feb 9, 2017

HI @codenrhoden,

Please squash and rebase. I believe this has been tested and ready for merge otherwise; is that correct?

@cduchesne
Copy link
Contributor

cduchesne commented Feb 10, 2017

I have tested this driver and it worked quite well for me. Below are a few things I think we should modify before merging

rexray tag functionality
I am curious if we're stuck to using the volume's Description field for volume tagging. This field is not editable from the Web UI after a volume has been created. Is this something that you can edit post-creation via api? I'm curious how hard it would be to implement this feature with the GCE Labels feature, which is easily editable from within the Web UI.

name of driver
we should name it gcepd (GCE Persistent Disks) instead of gce to avoid potential problems we faced with naming our initial aws ebs driver?

documentation of api permissions
we should document which specific api permissions are required to support using this driver

note limit of persistent disks for smaller instances in documentation
we should point out that for nodes with 3.75GB or less memory, max # of gce persistent disks is 4 which includes the boot disk. We can link to https://cloud.google.com/compute/docs/disks/ for reference as well.

10GB size minimum
we should try to find a way to have REX-Ray spit out a better notification about not making a large enough volume if a user tries to create something under 10GB

@codenrhoden
Copy link
Contributor Author

@cduchesne

Agree with all your comments. A couple of notes:

re: rexray tag. Thanks for showing me that the GCE GUI allows you to use labels, though it is only after disk creation. This implies that they themselves are using the Beta API, as that feature is only in the beta API (and why I didn't use it). Using the standard API, there is no way to modify the description post disk creation. I can use the label feature if we switch to the beta API, which you said you were okay with.

re: size minimum this is mostly work we'll have to do in rexray, not libS. Right now libS will return an error of "invalid size" or something similar, but i can make it more specific. I hadn't wanted to hardcode a message about the exact size minimum, but now that I think about it I have to hardcode the size to check against, so it's not a big deal.

@codenrhoden
Copy link
Contributor Author

@cduchesne I've addressed all comments except the rexray tag. Broken out into separate commits so they can be viewed easily. Will work on tags next. once all that is done, will squash down to one commit in preparation for merge.

@@ -589,6 +589,7 @@ remote storage systems. Currently the following storage drivers are supported:
[EBS](./storage-providers.md#aws-ebs) | ebs, ec2
[EFS](./storage-providers.md#aws-efs) | efs
[RBD](./storage-providers.md#ceph-rbd) | rbd
[GCE](./storage-providers.md#gce) | gce
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @codenrhoden,

This driver abbreviation still reflects the old gce and not the updated gcepd.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Darn, I knew I would miss a place! thanks


## GCEPD

The Google Compute Engine (GCE) Persistent Disk driver registers a driver named
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @codenrhoden,

This driver abbreviation still reflects the old gce and not the updated gcepd. I'd add a single acronym after the Persistent Disk as (GCEPD). Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left this one on purpose. But not for a strong reason. I can change it as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @codenrhoden,

It's up to you really. I only pointed it out because it may be the only place we reference GCE? I just want to avoid confusion if possible. If you don't think it's confusing then leave it. It just stood out, but I do not feel strongly about it.

Makefile Outdated
@@ -1078,8 +1078,18 @@ test-debug:
env LIBSTORAGE_DEBUG=true $(MAKE) test

test-rbd:
DRIVERS=rbd $(MAKE) deps
DRIVERS=rbd $(MAKE) ./drivers/storage/rbd/tests/rbd.test
env DRIVERS=rbd $(MAKE) deps
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @codenrhoden,

The reason I don't use the env command in some places is because of what it does. The env command removes defined aliases and shell functions on which the build may rely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my inspiration here was the test-debug target directly above. What is it about its scenario that makes env desirable and the driver-specific test builds not?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @codenrhoden,

The difference is that I learned more about the env command as I worked on this project and only used it at the beginning out of not fully understanding the difference. I rarely use the test-debug target and so have never changed it.

This patch re-introduces the GCE persistent disk driver.
@akutz akutz merged commit aad8d62 into thecodeteam:release/0.5.0 Feb 13, 2017
@akutz akutz removed the in progress label Feb 13, 2017
@cduchesne
Copy link
Contributor

looks good to me, thanks @codenrhoden !

@codenrhoden codenrhoden deleted the feature/gce branch February 16, 2017 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants