FittedCloud storage driver #408

jack-fittedcloud · 2017-02-07T16:36:59Z

FittedCloud EBS Optimizer driver

This driver implements thin-provisioned EBS volumes.

houndci-bot · 2017-02-07T16:37:11Z

drivers/storage/fittedcloud/tests/fittedcloud_test.go

+
+	// Check if name and size are same, and volume is encrypted
+	assert.Equal(t, volumeName, reply.Name)
+	fc_size := (size + fcagent.FcVolMaxCloudDisk - 1) / fcagent.FcVolMaxCloudDisk


don't use underscores in Go names; var fc_size should be fcSize

houndci-bot · 2017-02-07T16:37:11Z

drivers/storage/fittedcloud/tests/fittedcloud_test.go

+
+	// Check if name and size are same
+	assert.Equal(t, volumeName, reply.Name)
+	fc_size := (size + fcagent.FcVolMaxCloudDisk - 1) / fcagent.FcVolMaxCloudDisk


don't use underscores in Go names; var fc_size should be fcSize

houndci-bot · 2017-02-07T16:37:12Z

drivers/storage/fittedcloud/storage/fittedcloud_storage.go

+
+	var (
+		encryption bool
+		kmsKeyId   string


var kmsKeyId should be kmsKeyID

houndci-bot · 2017-02-07T16:37:12Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+	if len(strs) == 2 {
+		if strs[1] == "NOTFOUND" {
+			return cmdOut, errors.New("fcvol not found")
+		} else {


if block ends with a return statement, so drop this else and outdent its block

houndci-bot · 2017-02-07T16:37:12Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+	return string(cmdOut), err
+}
+
+func GetFcVolName(devName string) (string, error) {


exported function GetFcVolName should have comment or be unexported

houndci-bot · 2017-02-07T16:37:13Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+	}
+}
+
+func DelVol(fcVolName string) (string, error) {


exported function DelVol should have comment or be unexported

houndci-bot · 2017-02-07T16:37:13Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+	if cmdOut, err = exec.Command(cmd, args...).Output(); err != nil {
+		log.Debug(err)
+		return string(cmdOut), err
+	} else {


if block ends with a return statement, so drop this else and outdent its block

houndci-bot · 2017-02-07T16:37:14Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+	)
+	size_gb := strconv.Itoa(sizeGb)
+	initGb := (sizeGb + FcVolMaxCloudDisk - 1) / FcVolMaxCloudDisk
+	init_gb := strconv.Itoa(initGb)


don't use underscores in Go names; var init_gb should be initGb

houndci-bot · 2017-02-07T16:37:14Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+		err       error
+		fcVolName string
+	)
+	size_gb := strconv.Itoa(sizeGb)


don't use underscores in Go names; var size_gb should be sizeGb

houndci-bot · 2017-02-07T16:37:15Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+	return string(cmdOut), err
+}
+
+func CreateVol(tagName string, sizeGb int, encryption bool, kmsKeyId string) (string, error) {


exported function CreateVol should have comment or be unexported
func parameter kmsKeyId should be kmsKeyID

houndci-bot · 2017-02-07T16:37:15Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+	FcVolMaxCloudDisk = 6
+)
+
+func IsRunning() (string, error) {


exported function IsRunning should have comment or be unexported

houndci-bot · 2017-02-07T16:37:15Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+)
+
+const (
+	FcVolMaxCloudDisk = 6


exported const FcVolMaxCloudDisk should have comment (or a comment on this block) or be unexported

CLAassistant · 2017-02-07T16:38:00Z

All committers have signed the CLA.

codecov-io · 2017-02-07T16:46:06Z

Codecov Report

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

@@              Coverage Diff              @@
##           release/0.5.0    #408   +/-   ##
=============================================
  Coverage           31.1%   31.1%           
=============================================
  Files                 29      29           
  Lines               1752    1752           
=============================================
  Hits                 545     545           
  Misses              1149    1149           
  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 cb80e3e...1bb7127. Read the comment docs.

akutz · 2017-02-07T17:13:01Z

Hi @jack-fittedcloud,

Thank you for this PR. I've enjoyed our chats via e-mail up this point, so I look forward to working with you and your team to get this PR merged and into an upcoming release of libStorage and REX-Ray.

houndci-bot · 2017-02-07T18:36:36Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+	return string(cmdOut), err
+}
+
+// Given a disk name, returns the FittedCloud volume name.


comment on exported function GetFcVolName should be of the form "GetFcVolName ..."

houndci-bot · 2017-02-07T18:36:36Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+	return fcVolName, nil
+}
+
+// Delete a FittedCloud volume by name


comment on exported function DelVol should be of the form "DelVol ..."

houndci-bot · 2017-02-07T18:36:36Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+}
+
+// Create a FittedCluod volume
+func CreateVol(tagName string, sizeGb int, encryption bool, kmsKeyId string) (string, error) {


func parameter kmsKeyId should be kmsKeyID

houndci-bot · 2017-02-07T18:36:37Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+	return string(cmdOut), err
+}
+
+// Create a FittedCluod volume


comment on exported function CreateVol should be of the form "CreateVol ..."

houndci-bot · 2017-02-07T18:36:37Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+	FcVolMaxCloudDisk = 6
+)
+
+// Is FittedCloud Agent running?


comment on exported function IsRunning should be of the form "IsRunning ..."

houndci-bot · 2017-02-07T18:36:38Z

drivers/storage/fittedcloud/fcagent/fcagent.go

+)
+
+const (
+	// Max number of FittedCloud cloud disks


comment on exported const FcVolMaxCloudDisk should be of the form "FcVolMaxCloudDisk ..."

akutz · 2017-02-16T05:45:01Z

Hi @jack-fittedcloud,

Have you validated this driver with your own internal testing?

jack-fittedcloud · 2017-02-16T13:54:19Z

Hi @akutz,

Yes, we have. We also ran the fittedcloud.test.

jack-fittedcloud · 2017-02-16T18:54:08Z

This doc should give a better idea of how to use it: FittedCloud EBS Driver Technical Preview

akutz · 2017-02-16T21:43:05Z

Hi @jack-fittedcloud,

Honestly...this looks good. Want to be part of the upcoming libStorage 0.5.0 and REX-Ray 0.80 releases? We need you to rebase this onto the libStorage branch release/0.5.0 if so, and once you do that, please re-run your tests and paste the resulting output and emitted coverage file from -test.cover or -cover into a Gist as two files and link them here.

If everything is fine after that I want to merge you into release/0.5.0 so you can be part of LS 0.5.0-rc2.

akutz · 2017-02-16T21:54:06Z

Hi @jack-fittedcloud,

FYI, please be aware there was one breaking change to a StorageDriver function's signature -- VolumeRemove. You can read more about it in #409.

jack-fittedcloud · 2017-02-16T21:58:51Z

Thanks @akutz. I'll be right on it.

akutz · 2017-02-17T00:40:30Z

Hi @jack-fittedcloud,

Please squash this branch.

akutz · 2017-02-17T01:12:36Z

Hi @jack-fittedcloud,

To help you squash your branch and update it to reflect the changes I already mentioned above (since your branch still fails to build due to a lack of the aforesuggested signature refactor) I created a branch with your PR. Please just reset your PR's branch to match mine by doing the following:

# add my repo as a remote
git remote add kutz https://github.com/akutz/libstorage.git

# fetch my repo
git fetch kutz

# make sure you're on the correct branch
git checkout fittedcloud

# reset the your branch to the tip of mine
git reset --hard kutz/feature/fittedcloud

A few notes on which I was being a little lenient, but having rebased your PR and taken a second look I'd like to ask you to handle these items:

The branch name should follow GitFlow or GitHubFlow per our developer docs, and thus be feature/fittedcloud.
The column width of source files should be limited to 80 characters.
The storage driver should not ever be invoking the executor. The executor is local only and the storage driver is executed remotely, not on the clients.
A commit's message (and a PR's description) should be useful. See how I updated your original commit message:

FittedCloud storage driver

This patch adds support for the FittedCloud storage driver. FittedCloud
provides dynamic sizing and optimization of EBS volumes. More
information about FittedCloud is available at
http://www.fittedcloud.com/.

For more information on writing commit messages please see this page in the libStorage developer documentation.

jack-fittedcloud · 2017-02-17T12:53:04Z

Hi @akutz,

Sorry I shouldn't have pushed the code yesterday since I was not done. Thanks for your comments. I will work on them today.

The reason I called the executor was to get the next available device name because in our case we might have multiple volumes constituting a fittedcloud optimized volume. To attach those volumes I need to find the next available device names. I can change it so that it's local to the storage driver.

akutz · 2017-02-17T13:34:00Z

Hi Jack, The next device name is part of the attach ops args though. It's sent via the API call.

…

-- -a

On Feb 17, 2017, at 6:53 AM, jack-fittedcloud ***@***.***> wrote: Hi @akutz, Sorry I shouldn't have pushed the code yesterday since I was not done. Thanks for your comments. I will work on them today. The reason I called the executor was to get the next available device name because in our case we might have multiple volumes constituting a fittedcloud optimized volume. To attach those volumes I need to find the next available device names. I can change it so that it's local to the storage driver. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jack-fittedcloud · 2017-02-17T15:24:15Z

In our case, a Docker volume maps to one or more EBS volumes, depending on the used size of the Docker volume. The next device passed in to the VolumeAttach call is merely one of the constituent EBS volumes carrying the volume name in the "Name" tag. Fittedcloud storage driver then have to attach all EBS volumes carrying the same "Name" tag, as well as the "FittedCloudCreated" tag. This is currently done locally to the storage driver, which I realized means we can't support remote storage server in this version. Our plan is to support that in the next version so we can have something useful for people to start using. Does that make sense?

akutz · 2017-02-17T17:46:59Z

Hi @jack-fittedcloud,

RE: support. Hmm...What do you need today to support remote? The ability to pass more than one next device value or the ability to do a one-to-many mapping for Docker vols to EBS vols?

jack-fittedcloud · 2017-02-17T18:54:09Z

@akutz, it's not that. The existing Docker/REX-Ray abstraction makes sense.
When we started working on this project we saw pretty close matches between the storage driver's volume operations and our Agent's, therefore we chose to integrate our logic into the driver. This worked out quite well so far until we looked at the remote storage server support. In our case, volume operations (create/attach/detach/remove) are initiated by our Agents which sit right on each host, not a remote server. To support remote I imagine I will need to move logic to the "client" (the executor?) Honestly I don't yet have a clear picture of how it should be done. Thoughts?

akutz · 2017-02-17T19:09:17Z

Hi @jack-fittedcloud,

I'm going to respond to you via e-mail.

houndci-bot · 2017-02-17T23:39:52Z

drivers/storage/fittedcloud/utils/devices.go

+
+var xvdRX = regexp.MustCompile(`^xvd[a-z]$`)
+
+// Retrieve device paths currently attached and/or mounted


comment on exported function LocalDevices should be of the form "LocalDevices ..."

houndci-bot · 2017-02-17T23:39:52Z

drivers/storage/fittedcloud/executor/fittedcloud_executor.go

+	registry.RegisterStorageExecutor(fittedcloud.Name, NewDriver)
+}
+
+func NewDriver() types.StorageExecutor {


exported function NewDriver should have comment or be unexported

jack-fittedcloud · 2017-02-21T04:48:40Z

fittedcloud.test
output
-test.coverprofile emitted coverage file

jack-fittedcloud · 2017-02-21T15:56:46Z

Hi @akutz,

Is there a tool I can use to format the source file width to 80 chars? Also, how do I rename my PR's branch to feature/fittedcloud? Should I close this one and create a new PR?

This patch adds support for the FittedCloud storage driver. FittedCloud provides dynamic sizing and optimization of EBS volumes. More information about FittedCloud is available at http://www.fittedcloud.com/.

akutz · 2017-02-21T17:22:44Z

Hi @jack-fittedcloud,

There's no tool to format the width of the source files. I just do that manually by setting a gutter in Atom. As for the branch name, don't worry about it for this PR. Please keep it in mind for the future. Thank you.

houndci-bot reviewed Feb 7, 2017

View reviewed changes

drivers/storage/fittedcloud/storage/fittedcloud_storage.go Outdated

var (

encryption bool

kmsKeyId string

Copy link

houndci-bot Feb 7, 2017

Choose a reason for hiding this comment

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

var kmsKeyId should be kmsKeyID

houndci-bot reviewed Feb 7, 2017

View reviewed changes

akutz self-assigned this Feb 7, 2017

akutz added fitted-cloud storagedrivers work-in-progress labels Feb 7, 2017

houndci-bot reviewed Feb 7, 2017

View reviewed changes

akutz changed the base branch from master to release/0.5.0 February 17, 2017 00:39

houndci-bot reviewed Feb 17, 2017

View reviewed changes

FittedCloud storage driver

1bb7127

This patch adds support for the FittedCloud storage driver. FittedCloud provides dynamic sizing and optimization of EBS volumes. More information about FittedCloud is available at http://www.fittedcloud.com/.

akutz merged commit 99f21ee into thecodeteam:release/0.5.0 Feb 23, 2017


		var xvdRX = regexp.MustCompile(`^xvd[a-z]$`)

		// Retrieve device paths currently attached and/or mounted

FittedCloud storage driver #408

FittedCloud storage driver #408

Conversation

jack-fittedcloud commented Feb 7, 2017

FittedCloud EBS Optimizer driver

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

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

CLAassistant commented Feb 7, 2017 • edited

codecov-io commented Feb 7, 2017 • edited

Codecov Report

akutz commented Feb 7, 2017

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

akutz commented Feb 16, 2017

jack-fittedcloud commented Feb 16, 2017

jack-fittedcloud commented Feb 16, 2017 • edited

akutz commented Feb 16, 2017

akutz commented Feb 16, 2017

jack-fittedcloud commented Feb 16, 2017

akutz commented Feb 17, 2017

akutz commented Feb 17, 2017 • edited

jack-fittedcloud commented Feb 17, 2017

akutz commented Feb 17, 2017 via email

jack-fittedcloud commented Feb 17, 2017

akutz commented Feb 17, 2017

jack-fittedcloud commented Feb 17, 2017

akutz commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-fittedcloud commented Feb 21, 2017

jack-fittedcloud commented Feb 21, 2017

akutz commented Feb 21, 2017 • edited

CLAassistant commented Feb 7, 2017 •

edited

codecov-io commented Feb 7, 2017 •

edited

jack-fittedcloud commented Feb 16, 2017 •

edited

akutz commented Feb 17, 2017 •

edited

akutz commented Feb 21, 2017 •

edited