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

fix the error prone account creation method of blob disk #59739

Merged
merged 1 commit into from Feb 13, 2018

Conversation

@andyzhangx
Member

andyzhangx commented Feb 12, 2018

What this PR does / why we need it:
use new account generation method for blob disk to fix the error prone account creation method of blob disk

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #59738

Special notes for your reviewer:

Release note:

fix the error prone account creation method of azure blob disk

/assign @karataliu
/sig azure

@feiskyer

LGTM

@@ -555,7 +543,7 @@ func (c *BlobDiskController) findSANameForDisk(storageAccountType storage.SkuNam
countAccounts := 0 // account of this type.
for _, v := range c.accounts {
// filter out any stand-alone disks/accounts
if strings.Index(v.name, storageAccountNameMatch) != 0 {
if strings.Index(v.name, azureDiskAccountNamePrefix) != 0 {

This comment has been minimized.

@karataliu

karataliu Feb 12, 2018

Contributor

!strings.HasPrefix better

This comment has been minimized.

@andyzhangx
storageAccountNameMatch = storageAccountNamePrefix
// Used as a template to create new names for relevant accounts
storageAccountNamePrefix = storageAccountNamePrefix + "%s"
defaultContainerName = MakeCRC32(c.common.resourceGroup + c.common.location + c.common.subscriptionID)

This comment has been minimized.

@karataliu

karataliu Feb 12, 2018

Contributor

Any reason we need a unique container name? What about using a fixed one

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

just keep it, it's not related to this PR, I just used the original code.
unique or fixed name, all looks good.

@@ -481,7 +469,7 @@ func (c *BlobDiskController) getAllStorageAccounts() (map[string]*storageAccount
accounts := make(map[string]*storageAccountState)
for _, v := range *accountListResult.Value {
if strings.Index(*v.Name, storageAccountNameMatch) != 0 {
if strings.Index(*v.Name, azureDiskAccountNamePrefix) != 0 {

This comment has been minimized.

@karataliu

karataliu Feb 12, 2018

Contributor

The code accesses *v.Name first, and check v.Name == nil in next block? Consider fixing it?

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

Good catch. fixed.

@@ -27,6 +27,7 @@ import (
const (
defaultStorageAccountType = string(storage.StandardLRS)
fileShareAccountNamePrefix = "f"
azureDiskAccountNamePrefix = "dk"

This comment has been minimized.

@karataliu

karataliu Feb 12, 2018

Contributor

why not 'd', if 'k' has special meaning consider add some comments here

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

This is for support kind = Shared accounts. one letter d is not enough, and I also don't want to use disk it has 4 letters, so I use dk it's not a word, to differentiate customer self created accounts. A little balancing tricky ...

Update, I used ds finally, stands for disk shared

@@ -481,7 +469,7 @@ func (c *BlobDiskController) getAllStorageAccounts() (map[string]*storageAccount
accounts := make(map[string]*storageAccountState)
for _, v := range *accountListResult.Value {
if strings.Index(*v.Name, storageAccountNameMatch) != 0 {
if strings.Index(*v.Name, azureDiskAccountNamePrefix) != 0 {

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

Good catch. fixed.

@@ -555,7 +543,7 @@ func (c *BlobDiskController) findSANameForDisk(storageAccountType storage.SkuNam
countAccounts := 0 // account of this type.
for _, v := range c.accounts {
// filter out any stand-alone disks/accounts
if strings.Index(v.name, storageAccountNameMatch) != 0 {
if strings.Index(v.name, azureDiskAccountNamePrefix) != 0 {

This comment has been minimized.

@andyzhangx
@@ -27,6 +27,7 @@ import (
const (
defaultStorageAccountType = string(storage.StandardLRS)
fileShareAccountNamePrefix = "f"
azureDiskAccountNamePrefix = "dk"

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

This is for support kind = Shared accounts. one letter d is not enough, and I also don't want to use disk it has 4 letters, so I use dk it's not a word, to differentiate customer self created accounts. A little balancing tricky ...

Update, I used ds finally, stands for disk shared

storageAccountNameMatch = storageAccountNamePrefix
// Used as a template to create new names for relevant accounts
storageAccountNamePrefix = storageAccountNamePrefix + "%s"
defaultContainerName = MakeCRC32(c.common.resourceGroup + c.common.location + c.common.subscriptionID)

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

just keep it, it's not related to this PR, I just used the original code.
unique or fixed name, all looks good.

@andyzhangx

This comment has been minimized.

Member

andyzhangx commented Feb 12, 2018

@karataliu now below are the prefix nameing:

+	fileShareAccountNamePrefix     = "f"
 +	sharedDiskAccountNamePrefix    = "ds"
 +	dedicatedDiskAccountNamePrefix = "dd"

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Feb 12, 2018

@andyzhangx

This comment has been minimized.

Member

andyzhangx commented Feb 12, 2018

@karataliu I added a new common func SearchStorageAccount for CreateFileShare and CreateVolume funcs, this PR should be the last of series of PRs which is targeting to refactor k8s volume on azure code stating from 2017.9, I will cherry pick this PR to v1.8, v1.9. Thanks for your code review.

@@ -75,3 +79,52 @@ func (az *Cloud) getStorageAccesskey(account string) (string, error) {
}
return "", fmt.Errorf("no valid keys")
}
// SearchStorageAccount search storage account, create one storage account(with genAccountNamePrefix) if not found, return accountName, accountKey
func (az *Cloud) SearchStorageAccount(accountName, accountType, location, genAccountNamePrefix string) (string, string, error) {

This comment has been minimized.

@karataliu

karataliu Feb 12, 2018

Contributor

This func is to be used in package only, consider making it lower case

Also 'search' looks like a read only operation, may be use 'ensure' better, like other places

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

fixed, I am also considering a good name, ensureStorageAccount is better

fileShareAccountNamePrefix = "f"
defaultStorageAccountType = string(storage.StandardLRS)
fileShareAccountNamePrefix = "f"
sharedDiskAccountNamePrefix = "ds"

This comment has been minimized.

@karataliu

karataliu Feb 12, 2018

Contributor

Those should be placed in the file where it is in use?
azure_storage.go now contains file-service only code, maybe we need to merge it to azure_file.go someday.

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

yes, just keep it in this PR.

// find the access key with this account
accountKey, err := az.getStorageAccesskey(accountName)
accountName, accountKey, err := az.SearchStorageAccount(accountName, accountType, location, fileShareAccountNamePrefix)

This comment has been minimized.

@karataliu

karataliu Feb 12, 2018

Contributor

We'd better not assign value to 'accountName' here.

Consider the code in SearchStorageAccount:

+	accountKey, err := az.getStorageAccesskey(accountName)
 +	if err != nil {
 +		return "", "", fmt.Errorf("could not get storage key for storage account %s: %v", accountName, err)
 +	}

If user specifies 'AccountName', but later getStorageAccesskey fails, then SearchStorageAccount returns empty for first parameter.

In this case the follow-up error reported would use empty value for accountName, which does not sound good to user.

Also for CreateVolume.

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

good catch, fixed both funcs.

@@ -555,7 +516,7 @@ func (c *BlobDiskController) findSANameForDisk(storageAccountType storage.SkuNam
countAccounts := 0 // account of this type.
for _, v := range c.accounts {
// filter out any stand-alone disks/accounts
if strings.Index(v.name, storageAccountNameMatch) != 0 {
if strings.Index(v.name, sharedDiskAccountNamePrefix) != 0 {

This comment has been minimized.

@karataliu

karataliu Feb 12, 2018

Contributor

Also strings.HasPrefix ?

This comment has been minimized.

@andyzhangx
use new account generation method for blob disk
fix comments

change azureDiskSharedAccountNamePrefix var

rename sharedDiskAccountNamePrefix

use default vhd container name as "vhds"

use one commaon func: SearchStorageAccount

fix comments
@andyzhangx

@karataliu really good comments, PTAL.

@@ -555,7 +516,7 @@ func (c *BlobDiskController) findSANameForDisk(storageAccountType storage.SkuNam
countAccounts := 0 // account of this type.
for _, v := range c.accounts {
// filter out any stand-alone disks/accounts
if strings.Index(v.name, storageAccountNameMatch) != 0 {
if strings.Index(v.name, sharedDiskAccountNamePrefix) != 0 {

This comment has been minimized.

@andyzhangx
fileShareAccountNamePrefix = "f"
defaultStorageAccountType = string(storage.StandardLRS)
fileShareAccountNamePrefix = "f"
sharedDiskAccountNamePrefix = "ds"

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

yes, just keep it in this PR.

// find the access key with this account
accountKey, err := az.getStorageAccesskey(accountName)
accountName, accountKey, err := az.SearchStorageAccount(accountName, accountType, location, fileShareAccountNamePrefix)

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

good catch, fixed both funcs.

@@ -75,3 +79,52 @@ func (az *Cloud) getStorageAccesskey(account string) (string, error) {
}
return "", fmt.Errorf("no valid keys")
}
// SearchStorageAccount search storage account, create one storage account(with genAccountNamePrefix) if not found, return accountName, accountKey
func (az *Cloud) SearchStorageAccount(accountName, accountType, location, genAccountNamePrefix string) (string, string, error) {

This comment has been minimized.

@andyzhangx

andyzhangx Feb 12, 2018

Member

fixed, I am also considering a good name, ensureStorageAccount is better

@karataliu

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 13, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 13, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, karataliu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these OWNERS Files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 13, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 13, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 91c783e into kubernetes:master Feb 13, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation andyzhangx authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

k8s-merge-robot added a commit that referenced this pull request Mar 16, 2018

Merge pull request #60758 from andyzhangx/automated-cherry-pick-of-#5…
…9739-upstream-release-1.7

Automatic merge from submit-queue.

Automated cherry pick of #59739

Cherry pick of #59739 on release-1.7.

#59739: use new account generation method for blob disk

k8s-merge-robot added a commit that referenced this pull request Mar 22, 2018

Merge pull request #60756 from andyzhangx/automated-cherry-pick-of-#5…
…9739-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #59739: use new account generation method for blob disk

Cherry pick of #59739 on release-1.9.

#59739: use new account generation method for blob disk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment