fix CreateVolume func: use search mode instead #54687

andyzhangx · 2017-10-27T09:01:50Z

What this PR does / why we need it:
This is a little fall back for CreateVolume func: use search mode for Dedicated kind as @rootfs suggested.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #52396

Special notes for your reviewer:
I reference the implmentation of v1.6 in the same CreateVolume func
https://github.com/kubernetes/kubernetes/blob/release-1.6/pkg/cloudprovider/providers/azure/azure_storage.go#L213-L247

Release note:

fix azure storage account exhausting issue by using azure disk mount

/sig azure

@rootfs @feiskyer @karataliu

rootfs · 2017-10-27T12:36:54Z

pkg/cloudprovider/providers/azure/azure_blobDiskController.go

 		if err != nil {
-			return "", "", 0, fmt.Errorf("no key found for storage account %s even after creating a new storage account", storageAccount)
+			// TODO: create a storage account and container


this can be fixed now?

yes, I would like to fix it in a new PR.

rootfs · 2017-10-27T12:37:12Z

/approve

rootfs · 2017-10-27T13:09:45Z

pkg/cloudprovider/providers/azure/azure_blobDiskController.go

@@ -79,52 +79,55 @@ func newBlobDiskController(common *controllerCommon) (*BlobDiskController, error
 	return &c, nil
 }

-// CreateVolume creates a VHD blob in a given storage account, will create the given storage account if it does not exist in current resource group


cannot comment on line 64, but it is a mess there. use a var block.

rootfs · 2017-10-27T13:13:03Z

pkg/cloudprovider/providers/azure/azure_blobDiskController.go

 	}
+	for _, account := range accounts {
+		glog.V(4).Infof("account %s type %s location %s", account.Name, account.StorageType, account.Location)
+		if (storageAccountType == "" || account.StorageType == storageAccountType) && (location == "" || account.Location == location) {


missed len(storageAccount) > 0. This happens when a storage account is specified and we want to use it.
see https://github.com/kubernetes/kubernetes/blob/release-1.6.3/pkg/cloudprovider/providers/azure/azure_storage.go#L208

@rootfs I know, but it's quite tricky, what if user specified storageAccount does not equal to storageAccountType and location? Add len(storageAccount) > 0 means we don't care about the mismatch of storageAccountType, location; while if there is no len(storageAccount) > 0 means we would also do the match.

rootfs · 2017-10-27T13:16:40Z

pkg/cloudprovider/providers/azure/azure_blobDiskController.go

-		return "", "", 0, err
-	}
+			container := blobClient.GetContainerReference(vhdContainerName)
+			_, err = container.CreateIfNotExists(&azstorage.CreateContainerOptions{Access: azstorage.ContainerAccessTypePrivate})


This wastes an api call. In most cases containers do exist. Create it only if you get an error.

See https://github.com/kubernetes/kubernetes/blob/release-1.6.3/pkg/cloudprovider/providers/azure/azure_blob.go#L48

andyzhangx · 2017-11-06T09:12:07Z

/test pull-kubernetes-unit

rootfs · 2017-11-13T17:25:45Z

/approve
/lgtm

andyzhangx · 2017-11-14T00:10:25Z

@brendandburns

andyzhangx · 2017-11-15T15:05:48Z

ping @brendandburns

jdumars · 2017-11-15T16:37:41Z

/approve no-issue

jdumars · 2017-11-15T16:41:51Z

@gmarek PTAL

fix review comments

andyzhangx · 2017-11-21T01:55:03Z

@rootfs there is a rebase, could you take a look, thx

andyzhangx · 2017-11-21T02:56:01Z

@feiskyer

jdumars · 2017-11-29T16:27:43Z

@enisoc thoughts on this making it into 1.9?

enisoc · 2017-11-29T22:14:26Z

@jdumars Preventing runaway account usage sounds like a qualifying bugfix to me. I'll leave it to you to upgrade to priority/critical-urgent on behalf of sig-azure if you think this is important for 1.9.0 and there are people with bandwidth to push this to completion.

feiskyer · 2017-11-30T02:24:57Z

Added back to 1.9 milestone. @jdumars Could you help to approve the milestone?

dims · 2017-11-30T02:29:32Z

/assign @sttts
/assign @saad-ali

One of you please approve?

k8s-github-robot · 2017-11-30T16:07:23Z

[MILESTONENOTIFIER] Milestone Pull Request Current

@andyzhangx @feiskyer @gmarek @krousey @rootfs @saad-ali @sttts

Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required

Pull Request Labels

sig/azure: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/bug: Fixes a bug discovered during the current release.

Help

jdumars · 2017-11-30T16:07:26Z

/approve

jdumars · 2017-11-30T16:10:36Z

@deads2k or @liggitt could you PTAL and approve if appropriate?

liggitt · 2017-11-30T16:31:47Z

@deads2k or @liggitt could you PTAL and approve if appropriate?

for the test changes, I'd prefer someone familiar with storage take a look. @saad-ali ?

saad-ali · 2017-11-30T20:17:04Z

/lgtm
/approve

k8s-github-robot · 2017-11-30T20:17:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, feiskyer, jdumars, rootfs, saad-ali

Associated issue: 52396

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

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

~~pkg/cloudprovider/providers/azure/OWNERS~~ [jdumars]
~~pkg/volume/azure_dd/OWNERS~~ [rootfs,saad-ali]
~~test/OWNERS~~ [saad-ali]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-30T21:12:11Z

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

k8s-github-robot · 2017-11-30T21:55:28Z

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

…4687-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #54687 Cherry pick of #54687 on release-1.8. #54687: fix CreateVolume: search mode for Dedicated kind

…4687-upstream-release-1.7 Automatic merge from submit-queue. Automated cherry pick of #54687: fix CreateVolume: search mode for Dedicated kind Cherry pick of #54687 on release-1.7. #54687: fix CreateVolume: search mode for Dedicated kind

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

k8s-github-robot assigned gmarek and krousey Oct 27, 2017

rootfs reviewed Oct 27, 2017

View reviewed changes

andyzhangx force-pushed the createvolume-fix branch from f78ebbd to 2008ef3 Compare November 6, 2017 08:29

k8s-ci-robot assigned rootfs Nov 13, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2017

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

andyzhangx added 2 commits November 19, 2017 11:16

fix CreateVolume: search mode for Dedicated kind

310168c

search by accounttype in CreateVolume func

760cc6f

fix review comments

andyzhangx force-pushed the createvolume-fix branch from 2008ef3 to 760cc6f Compare November 19, 2017 11:22

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 19, 2017

feiskyer added this to the v1.9 milestone Nov 21, 2017

k8s-github-robot added the milestone/incomplete-labels label Nov 21, 2017

k8s-github-robot removed the milestone/needs-approval label Nov 28, 2017

k8s-github-robot removed this from the v1.9 milestone Nov 28, 2017

feiskyer added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed milestone/removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 30, 2017

feiskyer added this to the v1.9 milestone Nov 30, 2017

k8s-github-robot added the milestone/needs-approval label Nov 30, 2017

k8s-ci-robot assigned saad-ali and sttts Nov 30, 2017

jdumars added status/approved-for-milestone labels Nov 30, 2017

k8s-github-robot removed the milestone/needs-approval label Nov 30, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2017

k8s-github-robot merged commit 3904cc7 into kubernetes:master Nov 30, 2017

andyzhangx mentioned this pull request Dec 18, 2017

Automated cherry pick of #54687 #57313

Merged

andyzhangx mentioned this pull request Feb 23, 2018

Automated cherry pick of #54687: fix CreateVolume: search mode for Dedicated kind #60282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix CreateVolume func: use search mode instead #54687

fix CreateVolume func: use search mode instead #54687

andyzhangx commented Oct 27, 2017

rootfs Oct 27, 2017

andyzhangx Oct 27, 2017

rootfs commented Oct 27, 2017

rootfs Oct 27, 2017

andyzhangx Nov 6, 2017

rootfs Oct 27, 2017

andyzhangx Oct 27, 2017 •

edited

Loading

andyzhangx Nov 6, 2017

rootfs Oct 27, 2017

andyzhangx Nov 6, 2017

andyzhangx commented Nov 6, 2017

rootfs commented Nov 13, 2017

andyzhangx commented Nov 14, 2017

andyzhangx commented Nov 15, 2017

jdumars commented Nov 15, 2017

jdumars commented Nov 15, 2017

andyzhangx commented Nov 21, 2017

andyzhangx commented Nov 21, 2017

jdumars commented Nov 29, 2017

enisoc commented Nov 29, 2017

feiskyer commented Nov 30, 2017

dims commented Nov 30, 2017

k8s-github-robot commented Nov 30, 2017

jdumars commented Nov 30, 2017

jdumars commented Nov 30, 2017

liggitt commented Nov 30, 2017

saad-ali commented Nov 30, 2017

k8s-github-robot commented Nov 30, 2017

k8s-github-robot commented Nov 30, 2017

k8s-github-robot commented Nov 30, 2017

fix CreateVolume func: use search mode instead #54687

fix CreateVolume func: use search mode instead #54687

Conversation

andyzhangx commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootfs commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx Oct 27, 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

andyzhangx commented Nov 6, 2017

rootfs commented Nov 13, 2017

andyzhangx commented Nov 14, 2017

andyzhangx commented Nov 15, 2017

jdumars commented Nov 15, 2017

jdumars commented Nov 15, 2017

andyzhangx commented Nov 21, 2017

andyzhangx commented Nov 21, 2017

jdumars commented Nov 29, 2017

enisoc commented Nov 29, 2017

feiskyer commented Nov 30, 2017

dims commented Nov 30, 2017

k8s-github-robot commented Nov 30, 2017

jdumars commented Nov 30, 2017

jdumars commented Nov 30, 2017

liggitt commented Nov 30, 2017

saad-ali commented Nov 30, 2017

k8s-github-robot commented Nov 30, 2017

k8s-github-robot commented Nov 30, 2017

k8s-github-robot commented Nov 30, 2017

andyzhangx Oct 27, 2017 •

edited

Loading