fix the create azure file pvc failure if there is no storage account in current resource group #56557

andyzhangx · 2017-11-29T08:15:48Z

What this PR does / why we need it:
When create an azure file PVC, there will be error if there is no storage account in current resource group.
With this PR, a storage account will be created if there is no storage account in current resource group.

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 #56556

Special notes for your reviewer:

rephrase the code logic of CreateFileShare func.

if accountName is empty, then 
    find a storage account that matches accountType
    if no storage account found, then
        create a new account
else
    we only use user specified storage account

create a file share according to found storage account

Use func getStorageAccountName to get a unique storage account name by UUID, a storage account for azure file would be like f0b2b0bd40c010112e897fa. And in next PR, I will use this function to create storage account for azure disk, the storage account for azure disk would be like d8f3ad8ad92000f1e1e88bd.

Release note:

fix the create azure file pvc failure if there is no storage account in current resource group

/sig azure
/assign @rootfs

andyzhangx · 2017-11-29T09:00:34Z

/test pull-kubernetes-unit

rootfs · 2017-11-29T13:51:04Z

pkg/cloudprovider/providers/azure/azure_storage.go

+		// find a storage account that matches storageType
+		accounts, err = az.getStorageAccounts(storageType)
+		if err != nil || len(accounts) == 0 {
+			storageAccount = getAccountNameForNum(az.getNextAccountNum())


where is getNextAccountNum() defined? The only getNextAccountNum function I can find is defined for blob controller and it is quite buggy...

@rootfs yes, it's buggy, I used another way to generate random storage account name for file share, the generated account name could be like pvcfile4100861029

I will fix this buggy func getNextAccountNum() in another PR, will change the account name generation method, use new one getStorageAccountName by uuid in this PR

andyzhangx · 2017-12-01T11:54:56Z

/retest

rootfs · 2017-12-01T14:13:43Z

W1201 12:17:02.161] pkg/cloudprovider/providers/azure/azure_blobDiskController.go:94:52: not enough arguments in call to c.common.cloud.getStorageAccounts
W1201 12:17:02.161] 	have ()
W1201 12:17:02.162] 	want (string)
W1201 12:18:14.130] make[1]: *** [vet] Error 1

rootfs · 2017-12-01T16:28:56Z

pkg/cloudprovider/providers/azure/azure_storage.go

+		// find a storage account that matches storageType
+		accounts, err = az.getStorageAccounts(storageType)
+		if err != nil || len(accounts) == 0 {
+			uniqueID := az.controllerCommon.resourceGroup + az.controllerCommon.location + az.controllerCommon.subscriptionID


this reveals too much (perhaps sensitive) information

@rootfs there is hash like function MakeCRC32 which would transform uniqueID to a hash string, e,g. if uniqueID is resourcegroupwestuscc1624c7-3f1d-4ed3-a855-668a86e96ad8, it will convert to 2380111856, you could see no one could parse string 2380111856 to get info like resourcegroup, subscriptionID.
The reason I use both uniqueID and UUID is that it's unique for every subscriptionID, resourcegroup, thus the account name conflict possibility be near zero as 1/10 000 000 000 in one resourcegroup. If that possibility happens, user could delete the PVC and recreate PVC again.

where is that unique string transformed into a CRC string?

Suppose using uuid is enough, though we need substr to fit for the limit constraint.
Ref:
https://github.com/Azure/azure-quickstart-templates/blob/master/101-storage-account-create/azuredeploy.json#L20
https://docs.microsoft.com/en-us/azure/azure-resource-manager/resource-group-template-functions-string#uniquestring

andyzhangx · 2017-12-06T01:13:36Z

@rootfs if getStorageAccountName func is ok, I will use this func to fix similar issue where storage account for azure disk is not found:
https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/azure/azure_blobDiskController.go#L96

andyzhangx · 2017-12-11T23:59:40Z

ping @rootfs

jdumars · 2017-12-26T21:11:32Z

@rootfs PTAL? Thanks!!

andyzhangx · 2018-01-04T02:00:13Z

@rootfs I have answered your question in the PR, could you take a look? Thanks.

andyzhangx · 2018-01-08T06:27:22Z

rootfs on Dec 2, 2017  Member
this reveals too much (perhaps sensitive) information

@rootfs there is hash like function MakeCRC32 which would transform uniqueID to a hash string, e,g. if uniqueID is resourcegroupwestuscc1624c7-3f1d-4ed3-a855-668a86e96ad8, it will convert to 2380111856, you could see no one could parse string 2380111856 to get info like resourcegroup, subscriptionID.
The reason I use both uniqueID and UUID is that it's unique for every subscriptionID, resourcegroup, thus the account name conflict possibility be near zero as 1/10 000 000 000 in one resourcegroup. If that possibility happens, user could delete the PVC and recreate PVC again.

andyzhangx · 2018-01-10T02:49:02Z

@karataliu @feiskyer PTAL, thx.

feiskyer

/lgtm

karataliu · 2018-01-10T05:18:18Z

pkg/cloudprovider/providers/azure/azure_storageaccount.go

@@ -51,6 +51,9 @@ func (az *Cloud) getStorageAccounts() ([]accountWithLocation, error) {
 			storageType := ""
 			if acct.Sku != nil {
 				storageType = string((*acct.Sku).Name)
+				if matchingAccountType != "" && strings.ToLower(matchingAccountType) != strings.ToLower(storageType) {


!strings.EqualFold

Removed approved label.

@andyzhangx Could you fix this?

karataliu · 2018-02-05T09:08:21Z

pkg/cloudprovider/providers/azure/azure_storage.go

+				accountType = defaultStorageAccountType
+			}
+
+			glog.V(2).Infof("azureFile - no storage account found, begin to create a new storage account %s in resource group %s, location: %s, accountType: %s",


Better be 'no matching storage account found'

karataliu · 2018-02-05T09:08:23Z

pkg/cloudprovider/providers/azure/azure_storage.go

-		errResult = fmt.Errorf("failed to find a matching storage account")
+	// find the access key with this account
+	accountKey, err := az.getStorageAccesskey(accountName)
+	if err != nil {


Combine err check in same line

if I code like following, it looks ugly, since accountKey will be used in the next statement.

if accountKey, err := az.getStorageAccesskey(accountName); err != nil { ... } else { if err := az.createFileShare(accountName, accountKey, shareName, requestGiB); err != nil { }

karataliu · 2018-02-05T09:08:26Z

pkg/cloudprovider/providers/azure/azure_storageaccount.go

@@ -46,6 +46,9 @@ func (az *Cloud) getStorageAccounts() ([]accountWithLocation, error) {
 			storageType := ""
 			if acct.Sku != nil {
 				storageType = string((*acct.Sku).Name)
+				if matchingAccountType != "" && !strings.EqualFold(matchingAccountType, storageType) {


if acct.Sku is really nil we'd better also skip it.

Consider just making Line40 to verify 'acct.Name != nil && acct.Location !=nil && acct.Sku != nil' , then remaining part could be simplified.

matchingAccountType check could come first to avoid unnecessary 'name & loc' assignment check

good point, fixed

karataliu · 2018-02-05T09:10:11Z

Some comments on naming/misc, others look good.

karataliu

Some small comments. Could you please take a look and do a commit squash

karataliu · 2018-02-11T02:23:18Z

pkg/cloudprovider/providers/azure/azure_storageaccount.go

@@ -25,29 +25,29 @@ type accountWithLocation struct {
 	Name, StorageType, Location string
 }

-// getStorageAccounts gets the storage accounts' name, type, location in a resource group
-func (az *Cloud) getStorageAccounts() ([]accountWithLocation, error) {
+// getStorageAccounts gets name, type, location of all storage accounts in a resource group which matches matchingAccountType


and matchingLocation

karataliu · 2018-02-11T02:23:30Z

pkg/cloudprovider/providers/azure/azure_storageaccount.go

-				storageType = string((*acct.Sku).Name)
+
+			location := *acct.Location
+			if location != "" && !strings.EqualFold(matchingLocation, location) {


matchingLocation != "" ?

karataliu · 2018-02-11T02:25:57Z

pkg/cloudprovider/providers/azure/azure_storageaccount.go

 			}
-			accounts = append(accounts, accountWithLocation{Name: name, StorageType: storageType, Location: loc})
+			accounts = append(accounts, accountWithLocation{Name: *acct.Name, StorageType: storageType, Location: location})


We'd better use the actual type & location from acct object here, since storageType and location could be empty

storageType, Location is from acc object.

storageType := string((*acct.Sku).Name) if matchingAccountType != "" && !strings.EqualFold(matchingAccountType, storageType) { continue } location := *acct.Location if matchingLocation != "" && !strings.EqualFold(matchingLocation, location) { continue }

karataliu · 2018-02-11T02:27:17Z

pkg/cloudprovider/providers/azure/azure_blobDiskController.go

@@ -91,7 +91,7 @@ func (c *BlobDiskController) CreateVolume(name, storageAccount, storageAccountTy
 		accounts = append(accounts, accountWithLocation{Name: storageAccount})
 	} else {
 		// find a storage account
-		accounts, err = c.common.cloud.getStorageAccounts(storageAccountType)
+		accounts, err = c.common.cloud.getStorageAccounts(storageAccountType, location)


Note: We can also update upcoming account matching logic when fixing: '// TODO: create a storage account and container'

yes, I am going to fix it in next PR, it's in my plan.

use new storage account name generation method use uuid to generate account name change azure file account prefix use uniqueID to generate a storage account name fix comments fix comments fix comments fix a storage account matching bug only use UUID in getStorageAccountName func use shorter storage account prefix for azure file fix comments fix comments fix comments fix rebase build error rewrite CreateFileShare code logic fix gofmt issue fix test error fix comments fix a location matching bug

andyzhangx

@karataliu PTAL

andyzhangx · 2018-02-11T06:31:52Z

And I have also done a few manual tests on Azure env, it works well.

andyzhangx · 2018-02-11T07:51:29Z

/test pull-kubernetes-e2e-kops-aws

karataliu

/lgtm

note: update func comments for getStorageAccounts

k8s-ci-robot · 2018-02-11T07:52:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, feiskyer, 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:

~~pkg/cloudprovider/providers/azure/OWNERS~~ [andyzhangx,feiskyer,karataliu]
~~pkg/volume/azure_file/OWNERS~~ [andyzhangx,feiskyer,karataliu]

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

k8s-github-robot · 2018-02-11T08:48:33Z

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

fejta-bot · 2018-02-11T11:51:08Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-02-11T12:24:04Z

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

k8s-github-robot · 2018-02-11T13:03:31Z

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

…6557-upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #56557 Cherry pick of #56557 on release-1.9. #56557: create storage account if necessary when create azure file

…6557-upstream-release-1.7 Automatic merge from submit-queue. Automated cherry pick of #56557 Cherry pick of #56557 on release-1.7. #56557: create storage account if necessary when create azure file

…6557-upstream-release-1.8 Automated cherry pick of #56557

k8s-ci-robot assigned rootfs Nov 29, 2017

k8s-ci-robot added sig/azure release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 29, 2017

rootfs reviewed Nov 29, 2017

View reviewed changes

andyzhangx force-pushed the azurefile-createaccount branch 2 times, most recently from b494f06 to fdd53c5 Compare December 1, 2017 09:43

andyzhangx force-pushed the azurefile-createaccount branch 2 times, most recently from c7bce8a to 0a02b26 Compare December 1, 2017 15:16

rootfs reviewed Dec 1, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2017

andyzhangx force-pushed the azurefile-createaccount branch from 0a02b26 to 307b49a Compare December 17, 2017 11:08

kubernetes deleted a comment from k8s-github-robot Dec 17, 2017

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2017

feiskyer approved these changes Jan 10, 2018

View reviewed changes

k8s-ci-robot assigned feiskyer Jan 10, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 10, 2018

karataliu reviewed Jan 10, 2018

View reviewed changes

feiskyer removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2018

karataliu reviewed Feb 5, 2018

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2018

andyzhangx force-pushed the azurefile-createaccount branch from b17df2c to fd08023 Compare February 9, 2018 08:38

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2018

karataliu reviewed Feb 11, 2018

View reviewed changes

feiskyer added the kind/bug Categorizes issue or PR as related to a bug. label Feb 11, 2018

feiskyer added this to the v1.10 milestone Feb 11, 2018

andyzhangx commented Feb 11, 2018

View reviewed changes

andyzhangx force-pushed the azurefile-createaccount branch from 663a218 to aa21bef Compare February 11, 2018 06:30

karataliu approved these changes Feb 11, 2018

View reviewed changes

k8s-ci-robot assigned karataliu Feb 11, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2018

k8s-github-robot merged commit cfa6774 into kubernetes:master Feb 11, 2018

This was referenced Feb 26, 2018

Automated cherry pick of #56557 #60404

Merged

Automated cherry pick of #56557 #60409

Merged

Automated cherry pick of #56557 #60411

Merged

jpbetz added a commit that referenced this pull request Apr 4, 2018

Merge pull request #60409 from andyzhangx/automated-cherry-pick-of-#5…

5369b6b

…6557-upstream-release-1.8 Automated cherry pick of #56557

fix the create azure file pvc failure if there is no storage account in current resource group #56557

fix the create azure file pvc failure if there is no storage account in current resource group #56557

Conversation

andyzhangx commented Nov 29, 2017 • edited Loading

andyzhangx commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx commented Dec 1, 2017

rootfs commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karataliu Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

andyzhangx commented Dec 6, 2017 • edited Loading

andyzhangx commented Dec 11, 2017

jdumars commented Dec 26, 2017

andyzhangx commented Jan 4, 2018

andyzhangx commented Jan 8, 2018

andyzhangx commented Jan 10, 2018

feiskyer 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

karataliu commented Feb 5, 2018

karataliu 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

karataliu Feb 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx left a comment

Choose a reason for hiding this comment

andyzhangx commented Feb 11, 2018

andyzhangx commented Feb 11, 2018

karataliu left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 11, 2018

k8s-github-robot commented Feb 11, 2018

fejta-bot commented Feb 11, 2018

k8s-github-robot commented Feb 11, 2018

k8s-github-robot commented Feb 11, 2018

andyzhangx commented Nov 29, 2017 •

edited

Loading

karataliu Jan 16, 2018 •

edited

Loading

andyzhangx commented Dec 6, 2017 •

edited

Loading

karataliu Feb 11, 2018 •

edited

Loading