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

Azure storage driver #372

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@Andrey-mp
Contributor

Andrey-mp commented Dec 26, 2016

This patchset adds support of Azure Cloud.
There are several operations implemented in patchset:
create, attach, detach, remove (all base operations
for volumes).

@CLAassistant

This comment has been minimized.

CLAassistant commented Dec 26, 2016

CLA assistant check
All committers have signed the CLA.

)
const (
ConfigAZURE = Name

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

ClientSecretKey = "clientSecret"
CertPathKey = "certPath"
// Storage auth keys

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

DefaultMaxRetries = 10
DefaultUseHTTPS = true
// Config keys:

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

// DefaultMaxRetries is the max number of times to retry failed operations
DefaultMaxRetries = 10
DefaultUseHTTPS = true

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

ContainerKey = "container"
// Use https or not for making Azure URI's
UseHTTPS = "useHTTPS"
// Tag

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

ResourceGroupKey = "resourceGroup"
// Name of container in the storage account ('vhds' by default)
ContainerKey = "container"
// Use https or not for making Azure URI's

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

SubscriptionIDKey = "subscriptionID"
// Name of resource group
ResourceGroupKey = "resourceGroup"
// Name of container in the storage account ('vhds' by default)

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

// ID of subscription
SubscriptionIDKey = "subscriptionID"
// Name of resource group

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

StorageAccessKey = "storageAccessKey"
// TODO: add option to pass StorageURI
// ID of subscription

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

// Name of storage account
StorageAccount = "storageAccount"
// Access key of storage account

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

// Path to application certificate in case of authorization via certificate
CertPathKey = "certPath"
// Name of storage account

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

ClientIDKey = "clientID"
// Secret of this application
ClientSecretKey = "clientSecret"
// Path to application certificate in case of authorization via certificate

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

TenantIDKey = "tenantID"
// Application ID
ClientIDKey = "clientID"
// Secret of this application

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

// Config keys:
// Directory ID
TenantIDKey = "tenantID"
// Application ID

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

// Use https prefix by default or not for Azure URI's
DefaultUseHTTPS = true
// Config keys:

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

// TagDelimiter separates tags from volume or snapshot names
TagDelimiter = "/"
// Use https prefix by default or not for Azure URI's

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

ContainerKey = "container"
// UseHTTPS is a flag about use https or not for making Azure URI's
UseHTTPS = "useHTTPS"
// TagKey

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

ContainerKey = "container"
// UseHTTPS is a flag about use https or not for making Azure URI's
UseHTTPS = "useHTTPS"
// TagKey

This comment has been minimized.

@houndci-bot

houndci-bot Dec 26, 2016

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

@codecov-io

This comment has been minimized.

codecov-io commented Dec 26, 2016

Codecov Report

Merging #372 into master will increase coverage by -0.36%.

@@            Coverage Diff             @@
##           master     #372      +/-   ##
==========================================
- Coverage   30.83%   30.47%   -0.36%     
==========================================
  Files          29       29              
  Lines        1719     1739      +20     
==========================================
  Hits          530      530              
- Misses       1131     1151      +20     
  Partials       58       58
Impacted Files Coverage Δ
drivers/storage/vfs/executor/vfs_executor.go 2.1% <ø> (-0.15%)
api/types/types_drivers_executor.go 0% <ø> (ø)

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 1abc39c...c1e6e4a. Read the comment docs.

@akutz akutz self-assigned this Jan 10, 2017

@akutz

This comment has been minimized.

Collaborator

akutz commented Jan 18, 2017

Hi @Andrey-mp,

Do you mind rebasing this brand onto master? Thank you!

@akutz

Hi @Andrey-mp,

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!

@@ -847,6 +847,7 @@ $1:
BUILD_TAGS="$$(BUILD_TAGS)" GOOS=$2 GOARCH=amd64 $$(MAKE) $$@
$1-clean:
rm -f $1
rm -f $(EXECUTORS_GENERATED)

This comment has been minimized.

@akutz

akutz Jan 18, 2017

Collaborator

Good catch!

const (
// ConfigAZURE is a config key
ConfigAZURE = Name

This comment has been minimized.

@akutz

akutz Jan 18, 2017

Collaborator

Hi @Andrey-mp,

This is the only place I will mention this in my review, but why did you make AZURE capitalized throughout your use of it in constant/variable names? I checked, and it seems Microsoft doesn't use all-caps to write Azure, so I'm just wondering why you did?

image

This comment has been minimized.

@Andrey-mp

Andrey-mp Jan 23, 2017

Contributor

Will be renamed.

ctx types.Context,
opts types.Store) (string, error) {
// All possible device paths on Linux instances are /dev/sd[c-p]
letters := []string{

This comment has been minimized.

@akutz

akutz Jan 18, 2017

Collaborator

Since Go will keep malloc'd memory on the stack if it does not leak it probably makes more sense to keep this variable local to this function, but since mallocs are expensive in Go, I'm wondering if it makes sense to turn this into a package-scoped field that is allocated once on the heap.

localDeviceMapping := localDevices.DeviceMap
for localDevice := range localDeviceMapping {
re, _ := regexp.Compile(`^/dev/` +

This comment has been minimized.

@akutz

akutz Jan 18, 2017

Collaborator

RegEx compilations are pretty expensive. The pieces of this regex are static, so why not use regexp.MustCompile once and assign the result to a package-level field. Compiling the regex in a loop seems pretty expensive.

disks := *vm.StorageProfile.DataDisks
for i, disk := range disks {
if disk.Name != nil && *disk.Name == volumeID {

This comment has been minimized.

@akutz

akutz Jan 18, 2017

Collaborator

Hi @Andrey-mp,

I wasn't sure if you intended to do a case-sensitive comparison or not. If not then perhaps see strings.EqualFold(string, string) bool.

return name
}
// Retrieve config arguments

This comment has been minimized.

@akutz

akutz Jan 18, 2017

Collaborator

Hi @Andrey-mp,

I'm not going to make this comment on each of the below functions, 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 #87 in your azure.go:

r.Key(gofig.String, "", "", "", ConfigAZURESubscriptionIDKey)

In addition to notifying Gofig about the property azure.subscriptionID, the above function call instructs Viper to bind the property azure.subscriptionID to the CLI flag --azureSubscriptionID and the environment variable AZURE_SUBSCRIPTIONID. 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 azure.subscriptionID but change the CLI flag to azureSubID and the environment variable to AZURE_SUB_ID we could rewrite the above line like so:

r.Key(gofig.String, "", "", "", 
	ConfigAZURESubscriptionIDKey,
	"azureSubID",
	"AZURE_SUB_ID")

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

This comment has been minimized.

@Andrey-mp

Andrey-mp Jan 24, 2017

Contributor

Done.

for _, blob := range *blobs {
volumeSD, error := d.toTypeVolume(ctx, &blob, attachments)
if error != nil {
ctx.WithError(error).Error("Failed to convert volume")

This comment has been minimized.

@akutz

akutz Jan 18, 2017

Collaborator

Hi @Andrey-mp,

Do you intend to have this error logged but not returned? If so perhaps log it as a warning instead?

@akutz akutz requested a review from codenrhoden Jan 18, 2017

@codenrhoden

Echo'ing what @akutz said, thanks for the very thorough and complete PR! Hurray for tests.

Most of my comments are minor. My main feedback is that I don't think the filtering in toTypeVolume is necessary, and i also think that handling of the attachmentState can be handled by the framework instead of the driver.

// ContainerKey is a name of container in the storage account ('vhds' by default)
ContainerKey = "container"
// UseHTTPS is a flag about use https or not for making Azure URI's
UseHTTPS = "useHTTPS"

This comment has been minimized.

@codenrhoden

codenrhoden Jan 18, 2017

Contributor

Looks like both UseHTTPS and StorageAccount are used only when setting up config keys. To be consistent with all the other keys used in this manner, I feel like these should be renamed to UseHTTPSKey and StorageAccountKey.

const procPartitions = "/proc/partitions"
var devRX = regexp.MustCompile(`^sd[a-z]$`)

This comment has been minimized.

@codenrhoden

codenrhoden Jan 18, 2017

Contributor

This is looking for /dev/sd[a-z], but the comment in NextDevice says it can only be /dev/sd[c-p]. Are these both true?

If newly attached devices can only go to p, do we need to include all the way up to z?

Functionally I'm sure this works, it just jumps out to me as an inconsistency.

This comment has been minimized.

@Andrey-mp

Andrey-mp Jan 24, 2017

Contributor

Done.

This comment has been minimized.

@codenrhoden

codenrhoden Jan 31, 2017

Contributor

This looks the same to me. availLetters is still defining [c-z], the regex is looking for [a-z], and the first comment inside of NextDevice says that it can be [c-p].

}
writeHkey(hkey, &d.subscriptionID)
writeHkey(hkey, &d.resourceGroup)

This comment has been minimized.

@codenrhoden

codenrhoden Jan 18, 2017

Contributor

resourceGroup doesn't seem to be a part of uniquely identifying a session. I don't think it needs to be a part of the hash key. It's never used in this function again.

iid := context.MustInstanceID(ctx)
return &types.Instance{
Name: iid.ID,
//Region: iid.Fields[azure.InstanceIDFieldRegion],

This comment has been minimized.

@codenrhoden

codenrhoden Jan 18, 2017

Contributor

Seems like this can be removed for now?

"size_in_bytes": size,
}
volume, _ := d.getVolume(ctx, volumeName, types.VolAttNone)

This comment has been minimized.

@codenrhoden

codenrhoden Jan 18, 2017

Contributor

should an error here be ignored? What if the API failed and the check wasn't able to be performed?

ID: blob.Metadata["microsoftazurecompute_vmname"],
Driver: d.name,
}
if attachments.Mine() {

This comment has been minimized.

@codenrhoden

codenrhoden Jan 18, 2017

Contributor

Is it necessary for the Azure driver to do this filtering itself? I don't think that it is, and I don't think that it needs to handle setting attachState either. The framework handles this for you, here: https://github.com/codedellemc/libstorage/blob/release/0.4.0/api/server/router/volume/volume_routes.go#L131-L209

If a driver really does need to calculate attachState itself, it can do so. That will override the framework's default behavior of saying the volume is available if there are no attachments returned, it is attached if it is attached to the IID of the caller, or unavailable if attached to a different IID.

It is the intended behavior that attachState is not set if attachments.Requested() is false.

}
// IsAzureInstance returns a flag indicating whether the executing host is an Azure
// instance based on whether or not the metadata URL can be accessed.

This comment has been minimized.

@codenrhoden

codenrhoden Jan 18, 2017

Contributor

The comment here does not match how the function is actually implemented. It does not use metadata that I can see.

// UUID can be obtained as descried in https://azure.microsoft.com/en-us/blog/accessing-and-using-azure-vm-unique-id/
// but this code will use hostname as ID
hostname, err = os.Hostname()

This comment has been minimized.

@codenrhoden

codenrhoden Jan 18, 2017

Contributor

How likely is this to happen? I'm guess that AZURE_INSTANCE_ID should normally be set. But falling back to a hostname seems iffy, unless Asure has requirements on whether hostnames have to be unique?

May be a non-issue, just curious.

This comment has been minimized.

@Andrey-mp

Andrey-mp Jan 23, 2017

Contributor

Azure doesn't set AZURE_INSTANCE_ID inside VM-s. This variable was implemented for unit tests only.
Azure set hostname inside of VM equals to name of VM in Azure engine. And this name is unique in the engine.
So because there is no other way to obtain instance identifier (that is suitable for REST API) then we get it from the hostname.

// ContainerKey is a name of container in the storage account
// ('vhds' by default)
ContainerKey = "container"
// UseHTTPS is a flag about use https or not for making Azure URI's

This comment has been minimized.

@houndci-bot

houndci-bot Jan 23, 2017

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

Support Azure for libstorage
This patchset adds support of Azure Cloud.
There are several operations implemented in patchset:
create, attach, detach, remove (all base operations
for volumes).
@Andrey-mp

This comment has been minimized.

Contributor

Andrey-mp commented Jan 24, 2017

Hi all.

We've thought that we fixed all your comments :)
Also I run gometalinter and it doesn't show me valuable errors.
We are waiting for next review.

@akutz akutz changed the title from Support Azure for libstorage to Azure storage driver Jan 26, 2017

@akutz akutz referenced this pull request Jan 26, 2017

Closed

Azure storage driver #528

@codenrhoden

Just have the one remaining comment on device names. I'll start playing with this on Azure!

I'd like to see a first attempt made at adding the driver into the docs, at .docs/user-guide/storage-providers.md.

Should be interesting, as I haven't used Azure before.

const procPartitions = "/proc/partitions"
var devRX = regexp.MustCompile(`^sd[a-z]$`)

This comment has been minimized.

@codenrhoden

codenrhoden Jan 31, 2017

Contributor

This looks the same to me. availLetters is still defining [c-z], the regex is looking for [a-z], and the first comment inside of NextDevice says that it can be [c-p].

@Andrey-mp

This comment has been minimized.

Contributor

Andrey-mp commented Jan 31, 2017

Hi Travis,
We've got this code from the EBS driver that is very similar to Azure.
And in it code I see the same - NextDevice and LocalDevices has different letters...
And other executors have different behaviour for these functions also.

@codenrhoden

This comment has been minimized.

Contributor

codenrhoden commented Feb 17, 2017

Closing in deference to #421

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment