DigitalOcean driver #392

protochron · 2017-01-18T21:31:59Z

This implements a first-pass at a DigitalOcean storage provider. Right now it supports everything except for snapshots which I'm planning on adding as a followup PR.

TODOs:

fix the tests
update the README

CLAassistant · 2017-01-18T21:32:20Z

All committers have signed the CLA.

protochron · 2017-01-18T21:37:34Z

drivers/storage/digitalocean/storage/digitalocean_storage.go

+	// Collect attachment info for the volume
+	var atts []*types.VolumeAttachment
+	if attachments.Requested() {
+		for _, id := range volume.DropletIDs {


This is the only good way you can find out whether or not a volume is attached. If you're using VolumeInspect with VolAttReqTrue like we do in the tests, this causes it to skip adding an attachments block entirely which doesn't seem to be expected behavior. Should it default to adding a block anyway even if the volume isn't attached to a host?

Hi @protochron,

Ah, I see the issue. Hang on.

Hi @protochron,

So the issue is that your tests are perhaps using an over-eager attachments mask, thus causing volumes to be filtered out by the framework and not returned to you. This is by design. See the attachments related querying tests for the VFS storage driver. I use the VFS driver as the default means of validating the framework since VFS is pure-software with no external dependencies.

What you need to do is review the bit mask constants for querying volumes with attachments.

VolumeAttachmentTypes code definitions

libStorage API reference for querying volumes with attachment information

Finally, here is a series of predefined masks for the attachment bits. Notice that VolAttReqTrue is really an alias for VolAttReqWithDevMapOnlyVolsAttachedToInstance which is 1 | 2 | 4 | 8 and requests attachment information before returning only volumes attached to the instance provided via the InstanceID header with an attempt to match the attachments to the provided local device information via the local devices header.

protochron · 2017-01-18T21:40:04Z

drivers/storage/digitalocean/tests/digitalocean_test.go

+		t.FailNow()
+	}
+	apitests.LogAsJSON(reply, t)
+	assert.Len(t, reply.Attachments, 1)


This currently fails since VolumeInspect is not returning attachments if the volume isn't attached anywhere. See the comment I left in the VolumeInspect implementation for details.

akutz

Hi @protochron,

This is a great PR -- thank you so very much the obvious time and effort you spent on this. I have some suggestions/questions to which I hope you're able to respond below.

One question I had regarding the entire PR is whether or not you used the GoMetalinter or at least go-fmt to ensure the files are consistent in their syntax/style?

Also, if possible can you try and limit the width of lines to 80 chars. I may be pedantic, but I find it makes things easier to read.

Thank you again!

akutz · 2017-01-18T21:37:21Z

drivers/storage/digitalocean/executor/digitalocean_executor.go

+
+func (d *driver) token() string {
+	var token string
+	envToken := os.Getenv("DIGITALOCEAN_ACCESS_TOKEN")


Hi @protochron,

I'm not going to make this comment on each of the below calls, but this remark does apply to all of the code below related to retrieving configuration properties.

There is no need to check the environment variables directly. Please see this example for the EBS driver (ignore the backwards-compatability checks). The gofig package uses @spf13's Viper, which in turn enables the assignment of environment variables to defined configuration properties.

Let's look at line #32 in your digitalocean.go:

r.Key(gofig.String, "", "", "", Name+"."+"token")

In addition to notifying Gofig about the property digitalocean.token, the above function call instructs Viper to bind the property digitalocean.token to the CLI flag --digitaloceanToken and the environment variable DIGITALOCEAN_TOKEN. You can actually use Gofig to overwrite which CLI flag or environment variable the property is bound via the Key function's trailing variadic parameter.

For example, if we wanted to still use the config property digitalocean.token but change the CLI flag to digiToken and the environment variable to DIGI_TOKEN we could rewrite the above line like so:

r.Key(gofig.String, "", "", "", Name+"."+"token", "digiToken", "DIGI_TOKEN")

Once the configuration properties are bound to a CLI flag or environment variable, invoking config.GetString("digitalocean.token") checks first the environment variable, CLI flag argument, and finally a configuration property in that order.

akutz · 2017-01-18T21:38:32Z

drivers/storage/digitalocean/storage/digitalocean_storage.go

+	"github.com/codedellemc/libstorage/api/context"
+	"github.com/codedellemc/libstorage/api/registry"
+	"github.com/codedellemc/libstorage/api/types"
+	"github.com/codedellemc/libstorage/drivers/storage/digitalocean"


Hi @protochron,

Maybe import this as a shorter alias?

digocn "github.com/codedellemc/libstorage/drivers/storage/digitalocean"

That way it reduces line width where the package is referenced?

akutz · 2017-01-18T21:39:09Z

drivers/storage/digitalocean/storage/digitalocean_storage.go

+	"github.com/codedellemc/libstorage/api/registry"
+	"github.com/codedellemc/libstorage/api/types"
+	"github.com/codedellemc/libstorage/drivers/storage/digitalocean"
+	"github.com/codedellemc/libstorage/drivers/storage/digitalocean/utils"


Hi @protochron,

Maybe import this as a non-generic utils alias?

doUtils "github.com/codedellemc/libstorage/drivers/storage/digitalocean/utils"

That way it is clear what utils package is used when referencing it in the file.

codecov-io · 2017-01-21T22:50:10Z

Codecov Report

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

@@              Coverage Diff               @@
##           release/0.5.0     #392   +/-   ##
==============================================
  Coverage          30.44%   30.44%           
==============================================
  Files                 29       29           
  Lines               1741     1741           
==============================================
  Hits                 530      530           
  Misses              1153     1153           
  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 d80acc5...460e4e9. Read the comment docs.

protochron · 2017-01-21T22:51:52Z

@akutz I went through and wrapped everything to 80 characters. I write everything using vim-go which runs go-fmt on every save. Did something stand out to you from a stylistic standpoint?

akutz

Hi @protochron,

This looks fantastic. Aside from my inline comments, here are a few additional notes:

Please try and limit file width to 80 columns. I find it makes for more legible and readable code. That helps promote sustainable sources in my experience.
You need to add a coverage.mk file to your tests package. Please see the EBS coverage.mk file at https://github.com/codedellemc/libstorage/blob/master/drivers/storage/ebs/tests/coverage.mk for an example.

Thanks again!

akutz · 2017-01-27T23:20:26Z

drivers/storage/digitalocean/digitalocean.go

+
+const (
+	// Name is the name of the driver
+	Name = "digitalocean"


Hi @protochron,

Does DigitalOcean have any type of abbreviation you'd like to use here? To make configuration properties a little easier/shorter to read/write?

DO is what we typically use, but that also might be confusing

Hi @protochron,

I honestly don't mind do as long as it doesn't cause a conflict with parsing the YAML (reserved words).

I think we decided on dobs on Slack

akutz · 2017-01-27T23:21:20Z

drivers/storage/digitalocean/digitalocean.go

+	r.Key(gofig.String, "", "", "",
+		ConfigDOToken,
+		"digitaloceanAccessToken",
+		"DIGITALOCEAN_ACCESS_TOKEN")


Hi @protochron,

Is there a reason you explicitly specified the CLI flag and environment variable names here?

akutz · 2017-01-27T23:28:16Z

drivers/storage/digitalocean/executor/digitalocean_executor.go

+
+var (
+	diskPrefix = regexp.MustCompile(
+		fmt.Sprintf("^%s(.*)", do.VolumePrefix))


Don't use fmt.Sprintf here as do.VolumePrefix is a constant. The code as it is malloc's two new strings just to create a regex. Since do.VolumePrefix is a constant do this:

diskPrefix = regexp.MustCompile(`^` + do.VolumePrefix + `(.*)`)

Golang lacks native string interning, so I avoid creating new ones where possible when you're just going to throw them away as an initialization piece for some other object.

akutz · 2017-01-27T23:29:39Z

drivers/storage/digitalocean/executor/digitalocean_executor.go

+	return "", types.ErrNotImplemented
+}
+
+func (d *driver) LocalDevices(


Hi @protochron,

While none of the drivers make it a point to note this, and some don't do it, the LocalDevices function is meant only to return devices relevant to the relevant storage platform.

By that I'm taking it that you mean that LocalDevices needs to filter out any mounted drives that are not actually DigitalOcean volumes? That will require digitalocean/godo#122 being merged so that there's an easy API call to check each mounted volume

Hi @protochron,

Well, the idea is that the executor implementation should not import any non-stdlib dependencies beyond those used broadly, such as some of my support libraries - gofig, gotil, etc. It's to keep the size of the executor to a minimum. LocalDevices doesn't have to filter anything -- it's just ideal if it does. For example, for EBS it is able to filter based on the name of the devices, xvdX, as opposed to sdX, you know?

Hi @protochron,

Do you have time for a quick phone call? I just followed you on Twitter. Follow me and I'll send you my number. I want to explain the purpose of the LocalDevices function. It's easier via voice.

Hi @protochron,

I sent you my number via a Twitter DM. Please call at your convenience.

akutz · 2017-01-27T23:33:09Z

drivers/storage/digitalocean/storage/digitalocean_storage.go

+	log "github.com/Sirupsen/logrus"
+	"github.com/digitalocean/godo"
+
+	gofig "github.com/akutz/gofig/types"


Hi @protochron,

I won't repeat this comment in other files, but it's relevant to them. I prefer import blocks to be like so:

import ( stdlib external_pkgs external_pkgs_relevant_to_storage_driver project_pkgs project_pkgs_relevant_to_storage_driver

For this file's imports I'd organize them like so:

import ( "fmt" "path/filepath" "strconv" "time" gofig "github.com/akutz/gofig/types" "github.com/akutz/goof" log "github.com/Sirupsen/logrus" "github.com/digitalocean/godo" "github.com/codedellemc/libstorage/api/context" "github.com/codedellemc/libstorage/api/registry" "github.com/codedellemc/libstorage/api/types" do "github.com/codedellemc/libstorage/drivers/storage/digitalocean" doUtils "github.com/codedellemc/libstorage/drivers/storage/digitalocean/utils" )

Not all my own sources follow this as I've developed this preference over time, but I do try to update files as I work on them. It just makes it easier to scan the imports when looking for specific types of packages.

akutz · 2017-01-27T23:39:58Z

drivers/storage/digitalocean/storage/digitalocean_storage.go

+		fields["token"] = "******"
+	}
+
+	fields["region"] = d.config.GetString("digitalocean.region")


Hi @protochron,

You should use the constants declared in github.com/codedellemc/libstorage/drivers/storage/digitalocean instead of free-form strings in order to prevent possible error. You could even consider writing helper functions such as those found in the EBS driver

Good catch! I thought I fixed all of those

akutz · 2017-01-27T23:43:11Z

drivers/storage/digitalocean/storage/digitalocean_storage.go

+// https://www.digitalocean.com/community/tutorials/how-to-use-block-storage-on-digitalocean#preparing-volumes-for-use-in-linux
+func (d *driver) NextDeviceInfo(
+	ctx types.Context) (*types.NextDeviceInfo, error) {
+	return nil, nil


Hi @protochron,

If this is not implemented then please return the appropriate error, ex. return nil, types.ErrNotImplemented.

akutz · 2017-01-27T23:47:03Z

drivers/storage/digitalocean/tests/terraform/.gitignore

@@ -0,0 +1 @@
+terraform.tfstate*


Hi @protochron,

I prefer to add entries to the root .gitignore, but as long as the entry is relevant to the current directory, that's fine. I've just managed projects in the past where an entry is relevant to some other directory, and the .gitignore is removed and the other files get added accidentally.

akutz · 2017-01-27T23:50:56Z

drivers/storage/digitalocean/utils/client.go

+}
+
+func userAgent() string {
+	return "libstorage/" + api.Version.SemVer


Hi @protochron,

Well damn, I love your idea of the user-agent for libStorage clients. I'm going to steal this. Thanks!

akutz · 2017-01-28T00:09:59Z

Hi @protochron,

It would be great to see the addition of a Vagrantfile that supports turn-key testing using the DigitalOcean Vagrant plug-in.

The EBS Vagrantfile is a good example of what I mean. It stands up up to three nodes, a libStorage server and two client nodes, and then validates certain operations.

dcoraboeuf · 2017-02-22T14:53:54Z

Hi,

I'm eager to test the RexRay in DigitalOcean, in order to supply a resilient storage for a Docker Swarm. I'm looking for the documentation on how to configure RexRay for DigitalOcean - do you know where I can find it?

Thanks,
Damien.

kacole2 · 2017-02-22T15:27:20Z

@dcoraboeuf you're quick to the draw. the 0.8-RC1 was pushed out yesterday. This part of the libStorage framework which is at 0.5-RC1. I just checked the 0.5-RC1 documentation and it's missing. I'm also checking the 0.5 milestone and it's not listed. @cduchesne I think this needs to be addressed.

akutz · 2017-02-22T15:28:14Z

Hi @kacole2,

I'm on the hook for the DigitalOcean documentation, so it's my bad. I will address it before GA.

protochron force-pushed the digitalocean_volumes branch from 5a3c43a to f7ce053 Compare January 18, 2017 21:34

protochron commented Jan 18, 2017

View reviewed changes

akutz reviewed Jan 18, 2017

View reviewed changes

protochron force-pushed the digitalocean_volumes branch 3 times, most recently from 0a15f25 to 530b225 Compare January 21, 2017 22:45

protochron force-pushed the digitalocean_volumes branch from 530b225 to 709b80e Compare January 21, 2017 22:57

akutz added enhancement storagedrivers labels Jan 27, 2017

akutz self-assigned this Jan 27, 2017

akutz added this to the 0.6.0 milestone Jan 27, 2017

akutz added digitalocean in progress labels Jan 27, 2017

akutz requested changes Jan 27, 2017

View reviewed changes

cduchesne changed the title ~~Add DigitalOcean driver~~ DigitalOcean driver Feb 6, 2017

cduchesne mentioned this pull request Feb 13, 2017

DigitalOcean storage driver rexray/rexray#526

Closed

protochron force-pushed the digitalocean_volumes branch 7 times, most recently from 627dcaa to a76b756 Compare February 15, 2017 17:04

protochron changed the base branch from master to release/0.5.0 February 15, 2017 21:14

Add DigitalOcean driver

460e4e9

protochron force-pushed the digitalocean_volumes branch from fa0399f to 460e4e9 Compare February 15, 2017 21:14

akutz approved these changes Feb 15, 2017

View reviewed changes

akutz merged commit c3a8008 into thecodeteam:release/0.5.0 Feb 15, 2017

akutz removed the in progress label Feb 15, 2017

cduchesne modified the milestones: 0.5.0, 0.6.0 Feb 21, 2017

akutz mentioned this pull request Feb 22, 2017

Fix links between config and storage providers #428

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DigitalOcean driver #392

DigitalOcean driver #392

protochron commented Jan 18, 2017 •

edited

Loading

CLAassistant commented Jan 18, 2017 •

edited

Loading

protochron Jan 18, 2017 •

edited

Loading

akutz Jan 18, 2017

akutz Jan 18, 2017

protochron Jan 18, 2017

akutz left a comment

akutz Jan 18, 2017

akutz Jan 18, 2017

akutz Jan 18, 2017

codecov-io commented Jan 21, 2017 •

edited

Loading

protochron commented Jan 21, 2017

akutz left a comment

akutz Jan 27, 2017

protochron Feb 12, 2017

akutz Feb 13, 2017

protochron Feb 15, 2017

akutz Jan 27, 2017

akutz Jan 27, 2017

akutz Jan 27, 2017

protochron Feb 12, 2017

akutz Feb 13, 2017

akutz Feb 13, 2017

akutz Feb 13, 2017

akutz Jan 27, 2017

akutz Jan 27, 2017

protochron Feb 12, 2017

akutz Jan 27, 2017

akutz Jan 27, 2017

akutz Jan 27, 2017

akutz commented Jan 28, 2017

dcoraboeuf commented Feb 22, 2017

kacole2 commented Feb 22, 2017

akutz commented Feb 22, 2017

		@@ -0,0 +1 @@
		terraform.tfstate*

DigitalOcean driver #392

DigitalOcean driver #392

Conversation

protochron commented Jan 18, 2017 • edited Loading

CLAassistant commented Jan 18, 2017 • edited Loading

protochron Jan 18, 2017 • edited Loading

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 left a 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

codecov-io commented Jan 21, 2017 • edited Loading

Codecov Report

protochron commented Jan 21, 2017

akutz left a 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

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

akutz commented Jan 28, 2017

dcoraboeuf commented Feb 22, 2017

kacole2 commented Feb 22, 2017

akutz commented Feb 22, 2017

protochron commented Jan 18, 2017 •

edited

Loading

CLAassistant commented Jan 18, 2017 •

edited

Loading

protochron Jan 18, 2017 •

edited

Loading

codecov-io commented Jan 21, 2017 •

edited

Loading