Amazon EBS Storage Driver #248
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
| WaitVolumeCreate = "create" | ||
| //@enum WaitAction | ||
| WaitVolumeAttach = "attach" | ||
| //@enum WaitAction |
There was a problem hiding this comment.
comment on exported const WaitVolumeDetach should be of the form "WaitVolumeDetach ..."
proudh
force-pushed
the
feature/ec2-driver
branch
from
d6bee7f to
900dfc2
Compare
Current coverage is 25.55% (diff: 16.39%)@@ master #248 diff @@
==========================================
Files 41 43 +2
Lines 2484 2606 +122
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 646 666 +20
- Misses 1770 1872 +102
Partials 68 68
|
proudh
force-pushed
the
feature/ec2-driver
branch
from
900dfc2 to
ccaa9ff
Compare
proudh
force-pushed
the
feature/ec2-driver
branch
3 times, most recently
from
9699c2f to
d3e8b0a
Compare
|
Hi @proudh, Can you please attach a successful test run code cov output to this PR? Thank you. |
|
Here is the test output: |
akutz
requested changes
Sep 16, 2016
There was a problem hiding this comment.
Hi @proudh,
This looks really good, but there are a few changes that need to be handled prior to merging this PR.
- The manner in which the config compatibility is handled is incomplete and perhaps not in the correct location.
- The config keys are driver specific so instead of being placed in
api/types/types_config_compat.goyou could create a similar file atapi/drivers/storage/ebs/ebs_config_compat.go. - Just having the keys defined doesn't make anything backwards compatible. You must also reproduce this function. Please do so in an
init()call in the same suggested file,api/drivers/storage/ebs/ebs_config_compat.go. - Because you're defining the back-compat keys as constants it also makes since to define the same EBS keys as constants. This will make things more error-proof when defining the back-compat links. Again, use the suggested file to do so.
- The config keys are driver specific so instead of being placed in
- Please rename any files with
ec2in them toebs. For example,drivers/storage/ebs/ec2.goshould bedrivers/storage/ebs/ebs.go. The files include:Old File Name New File Name drivers/storage/ebs/ec2.godrivers/storage/ebs/ebs.godrivers/storage/ebs/executor/ec2_executor.godrivers/storage/ebs/executor/ebs_executor.godrivers/storage/ebs/storage/ec2_driver.godrivers/storage/ebs/storage/ebs_driver.godrivers/storage/ebs/tests/ec2_test.godrivers/storage/ebs/tests/ebs_test.go - Please remove this section from
glide.yaml. I understand the point of adding it, even as a comment. However, tags or branch names should be used inglide.yamlwhere possible. Additionall, since the EFS driver already requires this dependency, please simply change the comment from# EFSto# EFS and EBS. Does that make sense? Thanks!
|
Hi @proudh, The table was not formatted correctly in my PR review. Here you go:
|
akutz
reviewed
Sep 16, 2016
|
|
||
| // Retrieve config arguments | ||
| func (d *driver) accessKey() string { | ||
| if d.config.GetString("ebs.accessKey") != "" { |
There was a problem hiding this comment.
Hi @proudh,
Wherever you're following this convention:
if d.config.GetVALUE("key") != DEFAULT {
return d.config.GetVALUE("key")
}It should be changed to the following to make it more efficient:
if val := d.config.GetVALUE("key"); val != DEFAULT {
return val
}
This commit ports volume management functionality for AWS EBS for EC2 instances from REX-Ray 0.3.3, as per issue #183.
proudh
force-pushed
the
feature/ec2-driver
branch
from
d3e8b0a to
1793ce2
Compare
|
Test output after code review changes and rebase: |
akutz
reviewed
Sep 16, 2016
| "github.com/akutz/gofig" | ||
| "github.com/akutz/goof" | ||
|
|
||
| ebsConfig "github.com/emccode/libstorage/api/drivers/storage/ebs" |
There was a problem hiding this comment.
Hi @proudh,
No need to import this twice as separate packages. I'll handle it later. Just calling attention to it for your future projects :)
|
|
||
| func (d *driver) Init(ctx types.Context, config gofig.Config) error { | ||
| // Ensure backwards compatibility with ebs and ec2 in config | ||
| ebsConfig.BackCompat(config) |
There was a problem hiding this comment.
akutz
approved these changes
Sep 16, 2016
|
Just a heads up that I am currently testing the EBS driver functionality. I'll test out basic rexray CLI usage, docker volumes, and pre-emption. @cduchesne @akutz As part of that, I can already say I will have a PR to update docs for both rexray and libstorage. |
|
Just to document what I tested... I installed rexray with the latest libstorage on Ubuntu 16.04 and CentOS 7.2. I did both single-node (all in one) and 2-node setups with one node as a libstorage server. I tested I found a crash for preemption that was fixed with #271 I added docs with #273. The docs were mostly taken from rexray 0.3.3 docs, with some relevant changes and inspiration from the EFS driver. It may be the case that there is more work to do around IAM if you want to use a libstorage server and IAM roles. I was just using my admin keys directly. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds basic functionality for the EBS driver that was originally in REX-Ray 0.3.3 under the EC2 driver as per issue #183. Snapshot functionality is written but is awaiting other storage drivers to implement snapshots as well.
ec2toebswith backwards compatebsthat points to same constructor asec2ec2package(s) toebsec2toebsec2config prefixes. See this file for an example of what that means.rexrayTagconfig option to match the EFS driver'stagconfig name