add external resource group support for azure disk #64427

andyzhangx · 2018-05-29T07:51:24Z

What this PR does / why we need it:
add external resource group support for azure disk, specify resourcegroup in azure disk storage class so that user could specify external resource group

without this PR, user could only create dynamic azure disk in the same resource group as cluster
with this PR, user could specify external resource group in PVC:

kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
  name: hdd
provisioner: kubernetes.io/azure-disk
parameters:
  skuname: Standard_LRS
  kind: managed
  cachingmode: None
  resourcegroup: RESOURCE_GROUP_NAME

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

Special notes for your reviewer:
Pls note above config won't change resource group for azure disk forever, next time if user don't specify resource group, only default resource group will be used.

Release note:

add external resource group support for azure disk

/sig azure
/assign @feiskyer @karataliu
/cc @khenidak

feiskyer · 2018-05-29T08:14:38Z

pkg/cloudprovider/providers/azure/azure_managedDiskController.go

+	resourceGroup, err := getResourceGroupFromDiskURI(diskURI)
+	if err != nil {
+		glog.Errorf("azureDisk - getResourceGroupFromDiskURI(%s) returned error: %v", diskURI, err)
+		resourceGroup = c.common.resourceGroup


Shouldn't return error here? e.g. if there are disks with same name in different rg, this may delete the wrong disk?

correct, I have incorrect thoughts here, worried about data disk URI format change in the future...

feiskyer · 2018-05-29T08:16:43Z

pkg/volume/azure_dd/azure_provision.go

@@ -143,7 +147,11 @@ func (p *azureDiskProvisioner) Provision() (*v1.PersistentVolume, error) {
 	// create disk
 	diskURI := ""
 	if kind == v1.AzureManagedDisk {
-		diskURI, err = diskController.CreateManagedDisk(name, skuName, requestGB, *(p.options.CloudTags))
+		resourceGroup := ""


nit: default to c.common.resourceGroup

I set the default in diskController.CreateManagedDisk, mainly it's difficult to get c.common.resourceGroup in this code level

feiskyer · 2018-05-29T08:17:08Z

pkg/volume/azure_dd/azure_provision.go

+		if rg, found := p.options.PVC.Annotations[PVCAnnotationResourceGroup]; found {
+			resourceGroup = rg
+		}
+		diskURI, err = diskController.CreateManagedDisk(name, skuName, resourceGroup, requestGB, *(p.options.CloudTags))


check whether p.options.CloudTags is nil?

good catch, fixed

feiskyer · 2018-05-29T08:17:34Z

pkg/cloudprovider/providers/azure/azure_managedDiskController.go

@@ -131,3 +144,13 @@ func (c *ManagedDiskController) getDisk(diskName string) (string, string, error)

 	return "", "", err
 }
+
+// get resource group name from a managed disk URI, e.g. return {group-name} according to
+// 		/subscriptions/{sub-id}/resourcegroups/{group-name}/providers/microsoft.compute/disks/{disk-id}


nit: removing leading spaces

andyzhangx

@feiskyer PTAL

andyzhangx · 2018-05-29T08:29:02Z

pkg/cloudprovider/providers/azure/azure_managedDiskController.go

+	resourceGroup, err := getResourceGroupFromDiskURI(diskURI)
+	if err != nil {
+		glog.Errorf("azureDisk - getResourceGroupFromDiskURI(%s) returned error: %v", diskURI, err)
+		resourceGroup = c.common.resourceGroup


correct, I have incorrect thoughts here, worried about data disk URI format change in the future...

andyzhangx · 2018-05-29T08:29:18Z

pkg/cloudprovider/providers/azure/azure_managedDiskController.go

@@ -131,3 +144,13 @@ func (c *ManagedDiskController) getDisk(diskName string) (string, string, error)

 	return "", "", err
 }
+
+// get resource group name from a managed disk URI, e.g. return {group-name} according to
+// 		/subscriptions/{sub-id}/resourcegroups/{group-name}/providers/microsoft.compute/disks/{disk-id}


andyzhangx · 2018-05-29T08:30:36Z

pkg/volume/azure_dd/azure_provision.go

@@ -143,7 +147,11 @@ func (p *azureDiskProvisioner) Provision() (*v1.PersistentVolume, error) {
 	// create disk
 	diskURI := ""
 	if kind == v1.AzureManagedDisk {
-		diskURI, err = diskController.CreateManagedDisk(name, skuName, requestGB, *(p.options.CloudTags))
+		resourceGroup := ""


I set the default in diskController.CreateManagedDisk, mainly it's difficult to get c.common.resourceGroup in this code level

andyzhangx · 2018-05-29T08:34:21Z

pkg/volume/azure_dd/azure_provision.go

+		if rg, found := p.options.PVC.Annotations[PVCAnnotationResourceGroup]; found {
+			resourceGroup = rg
+		}
+		diskURI, err = diskController.CreateManagedDisk(name, skuName, resourceGroup, requestGB, *(p.options.CloudTags))


good catch, fixed

andyzhangx · 2018-05-29T11:36:55Z

/test pull-kubernetes-bazel-test
/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-integration

andyzhangx · 2018-06-03T13:51:30Z

/test pull-kubernetes-e2e-kops-aws

andyzhangx · 2018-06-03T13:53:06Z

@khenidak do you have any comments on this PR? We are very close to 1.11 release now, I would like to make this PR into v1.11 if there is no other issue. Thanks.

feiskyer

/lgtm
/approve

feiskyer · 2018-06-04T08:03:11Z

/retest

fix naming issue fix comments

andyzhangx · 2018-06-06T05:32:06Z

@feiskyer PTAL, and is it still possible to make this PR into v1.11?
rebased and specify external resource group in ResizeDisk, you may only need to look at this commit:
f6ffdaa

feiskyer · 2018-06-06T05:37:36Z

Since this is not a critical bug fix, we have missed v1.11 milestone.

fix issue

andyzhangx · 2018-06-06T05:43:22Z

@feiskyer thanks, PTAL anyway

andyzhangx · 2018-06-06T06:46:23Z

/test pull-kubernetes-bazel-test

andyzhangx · 2018-06-07T06:13:56Z

/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-e2e-gce-device-plugin-gpu

feiskyer

/lgtm
/approve

rootfs · 2018-06-18T13:56:22Z

pkg/cloudprovider/providers/azure/azure_managedDiskController.go

 	ctx, cancel := getContextWithCancel()
 	defer cancel()
-	_, err := c.common.cloud.DisksClient.CreateOrUpdate(ctx, c.common.resourceGroup, diskName, model)
+	_, err := c.common.cloud.DisksClient.CreateOrUpdate(ctx, resourceGroup, diskName, model)


how to ensure this rg can be accessed?

user should ensure current credentials should have access to the rg, otherwise there would be error returned.

rootfs · 2018-06-18T13:57:19Z

pkg/cloudprovider/providers/azure/azure_managedDiskController.go

+func getResourceGroupFromDiskURI(diskURI string) (string, error) {
+	fields := strings.Split(diskURI, "/")
+	if len(fields) != 9 {
+		return "", fmt.Errorf("disk URI(%s) is not expected", diskURI)


invalid disk URI

rootfs · 2018-06-18T14:00:38Z

pkg/cloudprovider/providers/azure/azure_managedDiskController.go

+	if len(fields) != 9 {
+		return "", fmt.Errorf("disk URI(%s) is not expected", diskURI)
+	}
+	return fields[4], nil


I am paranoid but I would verify if fields[3] == "resourceGroups"

rootfs · 2018-06-18T14:00:56Z

pkg/cloudprovider/providers/azure/azure_managedDiskController.go

@@ -173,3 +190,13 @@ func (c *ManagedDiskController) ResizeDisk(diskName string, oldSize resource.Qua

 	return newSizeQuant, nil
 }
+
+// get resource group name from a managed disk URI, e.g. return {group-name} according to
+// /subscriptions/{sub-id}/resourcegroups/{group-name}/providers/microsoft.compute/disks/{disk-id}


be more specific, add link https://docs.microsoft.com/en-us/rest/api/compute/disks/get

andyzhangx

@rootfs I have addressed your comments, PTAL.

andyzhangx · 2018-06-19T07:40:41Z

pkg/cloudprovider/providers/azure/azure_managedDiskController.go

 	ctx, cancel := getContextWithCancel()
 	defer cancel()
-	_, err := c.common.cloud.DisksClient.CreateOrUpdate(ctx, c.common.resourceGroup, diskName, model)
+	_, err := c.common.cloud.DisksClient.CreateOrUpdate(ctx, resourceGroup, diskName, model)


user should ensure current credentials should have access to the rg, otherwise there would be error returned.

andyzhangx · 2018-06-19T07:40:48Z

pkg/cloudprovider/providers/azure/azure_managedDiskController.go

@@ -173,3 +190,13 @@ func (c *ManagedDiskController) ResizeDisk(diskName string, oldSize resource.Qua

 	return newSizeQuant, nil
 }
+
+// get resource group name from a managed disk URI, e.g. return {group-name} according to
+// /subscriptions/{sub-id}/resourcegroups/{group-name}/providers/microsoft.compute/disks/{disk-id}


feiskyer · 2018-06-19T08:39:45Z

/cc @brendandburns

rootfs · 2018-06-19T12:26:08Z

/lgtm

k8s-ci-robot · 2018-06-19T12:26:19Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~pkg/cloudprovider/providers/azure/OWNERS~~ [andyzhangx,feiskyer]
~~pkg/volume/azure_dd/OWNERS~~ [andyzhangx,feiskyer,rootfs]

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

rootfs · 2018-06-19T12:29:17Z

@andyzhangx can you follow up to update azure disk examples?

andyzhangx · 2018-06-19T13:36:09Z

/kind bug

k8s-github-robot · 2018-06-20T11:23:22Z

Automatic merge from submit-queue (batch tested with PRs 65032, 63471, 64104, 64672, 64427). If you want to cherry-pick this change to another branch, please follow the instructions here.

…4427-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #64427: add external resource group support for azure disk Cherry pick of #64427 on release-1.8. #64427: add external resource group support for azure disk

…4427-upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #64427: add external resource group support for azure disk Cherry pick of #64427 on release-1.10. #64427: add external resource group support for azure disk

…4427-upstream-release-1.11 Automatic merge from submit-queue. Automated cherry pick of #64427: add external resource group support for azure disk Cherry pick of #64427 on release-1.11. #64427: add external resource group support for azure disk

…4427-upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #64427: add external resource group support for azure disk Cherry pick of #64427 on release-1.9. #64427: add external resource group support for azure disk

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 29, 2018

k8s-ci-robot assigned feiskyer and karataliu May 29, 2018

k8s-ci-robot added sig/azure 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 May 29, 2018

k8s-ci-robot requested a review from khenidak May 29, 2018 07:51

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 29, 2018

andyzhangx force-pushed the azuredisk-rg branch from ed1f0c4 to 7abc7ae Compare May 29, 2018 08:04

feiskyer reviewed May 29, 2018

View reviewed changes

andyzhangx commented May 29, 2018

View reviewed changes

feiskyer reviewed Jun 4, 2018

View reviewed changes

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

andyzhangx force-pushed the azuredisk-rg branch from b36bb4e to 95d408a Compare June 6, 2018 03:02

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 6, 2018

add external resource group support for azure disk

e6fdcb4

fix naming issue fix comments

andyzhangx force-pushed the azuredisk-rg branch from 95d408a to e6fdcb4 Compare June 6, 2018 05:19

specify external resource group in ResizeDisk

f533e37

fix issue

andyzhangx force-pushed the azuredisk-rg branch from f6ffdaa to f533e37 Compare June 6, 2018 05:40

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

feiskyer reviewed Jun 7, 2018

View reviewed changes

rootfs reviewed Jun 18, 2018

View reviewed changes

fix comments

ee290fe

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2018

andyzhangx commented Jun 19, 2018

View reviewed changes

k8s-ci-robot requested a review from brendandburns June 19, 2018 08:39

k8s-ci-robot assigned rootfs Jun 19, 2018

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

andyzhangx mentioned this pull request Jun 19, 2018

Automated cherry pick of #64427: add external resource group support for azure disk #65217

Closed

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 19, 2018

andyzhangx mentioned this pull request Jun 19, 2018

Automated cherry pick of #64427: add external resource group support for azure disk #65220

Merged

k8s-github-robot merged commit 3f581dc into kubernetes:master Jun 20, 2018

jsafrane mentioned this pull request Jun 25, 2018

azure: Move configuration of resource group in storage class. #65443

Merged

jsafrane mentioned this pull request Jul 10, 2018

Allow configuration of ResourceGroup in Azure StorageClass openshift/origin#20268

Closed

adam-resdiary mentioned this pull request Mar 19, 2019

Azure Managed Disk with Custom Resource Group Azure/AKS#873

Closed

add external resource group support for azure disk #64427

add external resource group support for azure disk #64427

Conversation

andyzhangx commented May 29, 2018 • edited

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

andyzhangx 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

andyzhangx commented May 29, 2018

andyzhangx commented Jun 3, 2018

andyzhangx commented Jun 3, 2018

feiskyer left a comment

Choose a reason for hiding this comment

feiskyer commented Jun 4, 2018

andyzhangx commented Jun 6, 2018

feiskyer commented Jun 6, 2018

andyzhangx commented Jun 6, 2018

andyzhangx commented Jun 6, 2018

andyzhangx commented Jun 7, 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

andyzhangx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feiskyer commented Jun 19, 2018

rootfs commented Jun 19, 2018

k8s-ci-robot commented Jun 19, 2018

rootfs commented Jun 19, 2018

andyzhangx commented Jun 19, 2018

k8s-github-robot commented Jun 20, 2018

andyzhangx commented May 29, 2018 •

edited