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

Add Ceph RBD driver #347

Merged
merged 1 commit into from
Dec 16, 2016
Merged

Conversation

codenrhoden
Copy link
Contributor

@codenrhoden codenrhoden commented Nov 29, 2016

This PR introduces the "rbd" storage driver, for interace with Ceph RBD devices.

This implementation does not use go-ceph, and therefore does not introduce new external dependencies. Instead it execs calls to the rbd executable, and parses the json output.

@codenrhoden codenrhoden changed the base branch from release/0.3.3 to master December 1, 2016 21:00
@codecov-io
Copy link

codecov-io commented Dec 12, 2016

Current coverage is 30.89% (diff: 100%)

Merging #347 into master will not change coverage

@@             master       #347   diff @@
==========================================
  Files            29         29          
  Lines          1719       1719          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            531        531          
  Misses         1130       1130          
  Partials         58         58          

Powered by Codecov. Last update b30a8fd...8adda99

@codenrhoden codenrhoden force-pushed the feature/rbd branch 2 times, most recently from df722c2 to f4bdfc5 Compare December 12, 2016 22:48
@codenrhoden
Copy link
Contributor Author

So far I've been rebasing this PR and and keeping the whole thing in one commit. That makes it hard to see that I'm adding new things, though. So far now, I'm going to push new commits with major new additions so you can see where I'm at.

I think that makes piecemeal reviews easier as well.

We can squash everything together at the end.

My latest commit adds tests in the tests folder. The tests are in line with all the other drivers (with some simplification and enhancements for the "inspect" tests). I think only two missing pieces now are docs, and a greater, scripted functional test that involves more than one server.

Code review is welcome!

@akutz
Copy link
Collaborator

akutz commented Dec 12, 2016

Hi @codenrhoden,

I don't rebase during the review process because the GitHub code review doesn't work if you make in-line comments and then update the branch with a rebased copy. I always perform additional commits during review -- at least until the very end.

Sounds good Travis!

@@ -586,8 +586,9 @@ remote storage systems. Currently the following storage drivers are supported:
[Isilon](./storage-providers.md#isilon) | isilon
[ScaleIO](./storage-providers.md#scaleio) | scaleio
[VirtualBox](./storage-providers.md#virtualbox) | virtualbox
[EBS](./storage-providers.md#ebs) | ebs, ec2
[EFS](./storage-providers.md#efs) | efs
[EBS](./storage-providers.md#aws-ebs) | ebs, ec2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good change, but please don't make it here as part of this PR. Please do it in a separate doc change.

[EBS](./storage-providers.md#ebs) | ebs, ec2
[EFS](./storage-providers.md#efs) | efs
[EBS](./storage-providers.md#aws-ebs) | ebs, ec2
[EFS](./storage-providers.md#aws-efs) | efs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good change, but please don't make it here as part of this PR. Please do it in a separate doc change.

return fmt.Sprintf("%s/24", a.ip.String())
}

var testIPs = []testIPInput{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Possibly optimize tests by parsing the unique IPs once and reusing the result.

@@ -0,0 +1,287 @@
// +build !libstorage_storage_driver libstorage_storage_driver_rbd
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,

I might add a sub-package called rbd-utils and make the file rbd-utils.go use the package main, thereby creating a new CLI. Then basically create a wrapper for this package so with the first arg of the CLI you can invoke a method from this file and the remaining args are the args for that function.

I was just thinking it would be a quick and easy way to easily validate the functionality of the utils package.

@codenrhoden codenrhoden changed the title WIP: Add Ceph RBD driver Add Ceph RBD driver Dec 15, 2016
@codenrhoden
Copy link
Contributor Author

I've removed the WIP designation on this PR, as I consider the work work to be feature-complete. I'm sure more things will be added, but it's ready for real review now. My latest commits added a test-rbd target to the Makefile for building the test binary, and also added a Vagrantfile and test plan in the tests directory.

@codenrhoden codenrhoden force-pushed the feature/rbd branch 2 times, most recently from 50f585b to 0ac1099 Compare December 15, 2016 22:00
@codenrhoden
Copy link
Contributor Author

@akutz. I think we are all green here. Take another look through when you can.

I did update the tests to use statically parsed IPs.

I can run my compiled tests package on the vagrant VM included in this PR as follows:

[vagrant@libstorage-rbd-test-admin ~]$ sudo ./rbd.test
PASS
coverage: 55.8% of statements in github.com/codedellemc/libstorage/drivers/storage/rbd, github.com/codedellemc/libstorage/drivers/storage/rbd/executor

This patch introduces the Ceph RBD driver
@akutz akutz added this to the 0.4.0 milestone Dec 16, 2016
@akutz akutz changed the base branch from master to release/0.4.0 December 16, 2016 00:14
@akutz akutz merged commit 8015f52 into thecodeteam:release/0.4.0 Dec 16, 2016
@akutz akutz removed the in progress label Dec 16, 2016
@codenrhoden codenrhoden deleted the feature/rbd branch February 16, 2017 15:38
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.

3 participants