Please put this onto a single line instead of using a block since there is only one constant.

reuse method provided in mounter

discussed above.

IF that's the case, it should be addressed in pkg/util/mount.

As mentioned, this format hang issue was root caused here

Submit an issue if you see other symptoms or root cause.

I'd really rather not fix this problem in this PR. We can TODO it, but this PR has a bunch of fixes I'd like to see merged. Can we address the refactor/merge into pkg/util/mount in a separate PR?

2017 and for rest of the new files

actually it is 2016 in all the files i have checked. Wouldn't that break Bazel if changed?

No, 2017 for all new files should work.

This is a regression. We use location to create a disk at the same Azure region with compute instance.

Users does not need to provide location, location is picked up from the cloud provider and disks are created in the same resource group. The current implementation does validate location = current (AFAIK) hence is subject to situation where users create disks in different location than the cluster. PVCs and PVs provisioned already will have no impact with this change. only new PVC/PVs

Address this.

Why this is not supported?

because the user has 2 options when doing blob disk, either PVC (dedicated or shared) using a set of storage accounts managed by the plugin or PV in this case the user provides diskuri which includes storage account

Use case for storage account is for a multi tenant setup, providing separate storage account for each tenant.

storageAccountType is assigned twice for skuname and storageaccounttype, what does it really mean?

because sku is "arm" label not end user label, all Azure documentation and portal refer to it "storage account type". Only when you interact with arm templates you deal with sku. Hence, This should use "storage account type" to avoid confusion, but since there is no risk (as described below) in leaving sku parameter it is left for this version until we remove it from all samples then retire it

This is not what I read from https://docs.microsoft.com/en-us/rest/api/storagerp/storageaccounts
"Note that in older versions, sku name was called accountType."

skuname and storageAccountType has same meaning here, it's an alias:
Note that in older versions, sku name was called accountType.
type SkuName { Standard_LRS, Standard_GRS, Standard_RAGRS, Standard_ZRS, Premium_LRS }

this is mostly identical (except the order of command) with /pkg/util/mount but bypass mounter and directly call exec.New to obtain a runner. This is problematic as we are refactoring mounters.

If there is an issue with default mounter, refactor at /pkg/util/mount, but don't bypass mounter interface.

as discussed, there is an issue that hangs the format/mount sequence as its is in K8s (it takes 4 mins+ to execute, which is basically a hang). It waits on IO (process in D sate) on Azure. Until we identify exactly what is wrong (either in k8s) & until a fix is applied and taste (either on Azure/k8s or both) we will have to go with this approach since this approach consistently works with predictable format/mount time.

This issue is now discussed in #47158

follow the file name convention: the file that implement the plugin is named with the plugin's name.

the file is named azure_dd.go, as for mount and attacher i didn't find a consistent naming concision across the plugins. I can prefix all files with azure_dd if that will help.

filename prefixed with azure_ for log parsing.
For new files, copyright year stay with 2017

keep it as we are going to refactor disk utilities among plugins.

this has moved into the common file of the disk controllers. Since this only needed by Blob Disk + Managed Disk (and will always be, since any block device based disks will always use AzureDiskVolumeSource type with the addition of Kind parameter) also they have nothing to do with "vhd" per-se

Missed Managed

added

this is for ownership change.

removed the comment

Upper case words have their meanings in golang. Use other alias e.g. libstrings

done

IF that's the case, it should be addressed in pkg/util/mount.

As mentioned, this format hang issue was root caused here

Submit an issue if you see other symptoms or root cause.

This is not safe, it will panic on unexpected vm stream. Run unit test below and you'll get a panic.

 	vm := []byte("foo")
	if err := json.Unmarshal(vm, &vmData); err != nil {
               return 
	}
        // panic happens!
  	fragment := vmData.(map[string]interface{})

thanks for the check, I have used below code to avoid such conversion issue:
fragment, ok := vmData.(map[string]interface{})
if !ok {
return attached, lun, fmt.Errorf("convert vmData to map error")
}
Also do the checks for the proceeding code.

I have movoed all the error handling code of json format in func: ExtractVMData, ExtractDiskData in azure_util.go

This is not accurate. You have to choose among core.windows.net, core.usgovcloudapi.net, etc....

thanks for the hint, I have used az.Environment.StorageEndpointSuffix

This apiversion is not supported in every azure region
Azure/azure-xplat-cli#3513

For ARM, The supported api-versions are '2016-04-30-preview, 2017-03-30' in global azure. So for other regions, like azure china, just wait for the api version update or do customerization if want to use it ASAP.

keep vhd_util.go file and its unit test.

cc @colemickens

As advised by Joseph Romeo by email: "Avoid looking at VHD blob leases to decide a disk is attached or detached. This can lead you to unexpected failures or worse-case get into a bad state."

nodeName != InstanceID
refer to https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/azure_dd/attacher.go#L76

hi, we use nodeName to get the vm object, see in AttachManagedDisk func:
vm, err := c.common.getArmVM(nodeName)

How to deal with race condition?

I have used lock in this func

see #37801

@jsafrane