Skip to content
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 azure storage account creation failure #65846

Merged
merged 1 commit into from Jul 6, 2018

Conversation

@andyzhangx
Copy link
Member

andyzhangx commented Jul 5, 2018

What this PR does / why we need it:
fix azure storage account creation failure

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

Special notes for your reviewer:
This bug is due to azure-sdk-for-go API change introduced in v1.11:
https://github.com/Azure/azure-sdk-for-go/blob/fbe7db0e3f9793ba3e5704efbab84f51436c136e/services/storage/mgmt/2017-10-01/storage/models.go#L381-L382

there is a new field Kind which is required, so any sdk upgrade from and old version would break the storage account creation since old code won't use Kind. I have filed an issue to azure-sdk-for-go: Azure/azure-sdk-for-go#2182

Release note:

fix azure storage account creation failure

/kind bug
/sig azure
/assign @khenidak @feiskyer
cc @brendandburns

@karataliu

This comment has been minimized.

Copy link
Contributor

karataliu commented Jul 5, 2018

The document listed in code is for update. For creation, all 3 kinds are supported:
https://docs.microsoft.com/en-us/rest/api/storagerp/storageaccounts/create#kind

So according to the docs, If we're to make it StorageV2 in future, that'd be a decision we make, not limited by api. Can consider updating the comment in code.

@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented Jul 5, 2018

@karataliu original storage account is identical to StorageV1, and StorageV2 is recommended according to https://docs.microsoft.com/en-us/azure/storage/common/storage-account-options?toc=%2fazure%2fstorage%2fblobs%2ftoc.json, and BlobStorage is only for block blob, so let's switch to use StorageV2, paste some info here:

The three different storage account options are:
General-purpose v2 (GPv2) accounts
General-purpose v1 (GPv1) accounts
Blob storage accounts

  • General-purpose v2
    General-purpose v2 (GPv2) accounts are storage accounts that support all of the latest features for blobs, files, queues, and tables. GPv2 accounts support all APIs and features supported in GPv1 and Blob storage accounts. They also support the same durability, availability, scalability, and performance features in those account types. Pricing for GPv2 accounts has been designed to deliver the lowest per gigabyte prices, and industry competitive transaction prices.

You can upgrade your GPv1 or Blob storage account to a GPv2 account using Azure portal, PowerShell, or Azure CLI.

@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented Jul 5, 2018

/test pull-kubernetes-e2e-kops-aws

@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented Jul 5, 2018

BTW, this is a critical bug, storage account creation is broken on v1.11 and master branch

@@ -113,6 +113,8 @@ func (az *Cloud) ensureStorageAccount(accountName, accountType, location, genAcc
accountName, az.ResourceGroup, location, accountType)
cp := storage.AccountCreateParameters{
Sku: &storage.Sku{Name: storage.SkuName(accountType)},
// set as StorageV2 according to https://docs.microsoft.com/en-us/rest/api/storagerp/StorageAccounts/Update

This comment has been minimized.

@feiskyer

feiskyer Jul 5, 2018

Member

nit: Also add a comment: "Currently only StorageV2 supported" for updates?

This comment has been minimized.

@feiskyer

feiskyer Jul 5, 2018

Member

hmm, this is only for create, so all 3 kinds are supported here. So it's better changing the link to https://docs.microsoft.com/en-us/azure/storage/common/storage-account-options and add a comment of why StorageV2 is chosen

This comment has been minimized.

@karataliu

karataliu Jul 6, 2018

Contributor

This is exactly what i meant.
The comment should be 'StorageV2' is chosen by purpose, not limited by api.

This comment has been minimized.

@andyzhangx

andyzhangx Jul 6, 2018

Author Member

fixed by new comment

@feiskyer

This comment has been minimized.

Copy link
Member

feiskyer commented Jul 5, 2018

/priority critical-urgent

@ddebroy

This comment has been minimized.

Copy link
Member

ddebroy commented Jul 5, 2018

Will it make sense to add an e2e case for dynamic provisioning of AzureFile backed volumes to catch such issues (resulting from upgrading azure SDK or due to some other reason) in the future?

@andyzhangx andyzhangx force-pushed the andyzhangx:azure-storage-account-fix branch from 8e41b09 to b76a3a1 Jul 6, 2018

@andyzhangx
Copy link
Member Author

andyzhangx left a comment

@@ -113,6 +113,8 @@ func (az *Cloud) ensureStorageAccount(accountName, accountType, location, genAcc
accountName, az.ResourceGroup, location, accountType)
cp := storage.AccountCreateParameters{
Sku: &storage.Sku{Name: storage.SkuName(accountType)},
// set as StorageV2 according to https://docs.microsoft.com/en-us/rest/api/storagerp/StorageAccounts/Update

This comment has been minimized.

@andyzhangx

andyzhangx Jul 6, 2018

Author Member

fixed by new comment

@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented Jul 6, 2018

@ddebroy good question, I found this issue on Azure acs-engine regression test, while in upstream, there is no such plan to add Azure e2e test since cloud provider will be split to standalone repo in the near future.

@karataliu

This comment has been minimized.

Copy link
Contributor

karataliu commented Jul 6, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 6, 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 files:

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

@feiskyer

This comment has been minimized.

Copy link
Member

feiskyer commented Jul 6, 2018

Will it make sense to add an e2e case for dynamic provisioning of AzureFile backed volumes to catch such issues (resulting from upgrading azure SDK or due to some other reason) in the future?

Azure related e2e tests are adding to https://github.com/kubernetes/cloud-provider-azure. @karataliu is working on that part.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 6, 2018

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

@andyzhangx

This comment has been minimized.

Copy link
Member Author

andyzhangx commented Jul 6, 2018

/test pull-kubernetes-e2e-gce-device-plugin-gpu
/test pull-kubernetes-bazel-test

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 6, 2018

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

@k8s-github-robot k8s-github-robot merged commit 6ca53a0 into kubernetes:master Jul 6, 2018

15 of 17 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-bazel-test
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job triggered.
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-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

k8s-github-robot pushed a commit that referenced this pull request Jul 8, 2018

Kubernetes Submit Queue
Merge pull request #65893 from andyzhangx/automated-cherry-pick-of-#6…
…5846-upstream-release-1.11

Automatic merge from submit-queue.

Automated cherry pick of #65846: fix azure storage account creation failure

Cherry pick of #65846 on release-1.11.

#65846: fix azure storage account creation failure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.