feat: Implement KEP3751 ("ControllerModifyVolume") #2344

umagnus · 2024-06-07T03:50:27Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

feat: Implement KEP3751 ("ControllerModifyVolume")
Implements support for modifying volumes via KEP3751 (https://kep.k8s.io/3751)

Which issue(s) this PR fixes:

Fixes #

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Special notes for your reviewer:

I0627 02:52:59.490914       1 utils.go:105] GRPC call: /csi.v1.Controller/ControllerModifyVolume
I0627 02:52:59.490952       1 utils.go:106] GRPC request: {"mutable_parameters":{"DiskIOPSReadWrite":"5000","DiskMBpsReadWrite":"1200"},"volume_id":"/subscriptions/8ecadfc9-d1a3-4ea4-b844-0d9f87e4d7c8/resourceGroups/MC_aks-yxytest_group_aks-yxytest_eastus/providers/Microsoft.Compute/disks/pvc-526c35c8-87d2-4a67-8cee-99e3b6ce3b3f"}
I0627 02:52:59.875553       1 controllerserver.go:437] begin to modify azure disk() account type() rg() location()
I0627 02:52:59.875586       1 azure_managedDiskController.go:420] azureDisk - modifying managed Name:pvc-526c35c8-87d2-4a67-8cee-99e3b6ce3b3f, StorageAccountType:, DiskIOPSReadWrite:5000, DiskMBpsReadWrite:1200
I0627 02:53:03.174198       1 azure_managedDiskController.go:486] azureDisk - modified new MD Name:pvc-526c35c8-87d2-4a67-8cee-99e3b6ce3b3f StorageAccountType:
I0627 02:53:03.174248       1 controllerserver.go:466] modify azure disk() account type() rg() location() successfully
I0627 02:53:03.174608       1 azure_metrics.go:115] "Observed Request Latency" latency_seconds=3.298678755 request="azuredisk_csi_driver_controller_modify_volume" resource_group="mc_aks-yxytest_group_aks-yxytest_eastus" subscription_id="8ecadfc9-d1a3-4ea4-b844-0d9f87e4d7c8" source="disk.csi.azure.com" volumeid="/subscriptions/8ecadfc9-d1a3-4ea4-b844-0d9f87e4d7c8/resourceGroups/MC_aks-yxytest_group_aks-yxytest_eastus/providers/Microsoft.Compute/disks/pvc-526c35c8-87d2-4a67-8cee-99e3b6ce3b3f" result_code="succeeded"

I0627 06:58:30.405618       1 utils.go:105] GRPC call: /csi.v1.Controller/ControllerModifyVolume
I0627 06:58:30.405664       1 utils.go:106] GRPC request: {"mutable_parameters":{"cachingMode":"None","skuName":"StandardSSD_LRS"},"volume_id":"/subscriptions/8ecadfc9-d1a3-4ea4-b844-0d9f87e4d7c8/resourceGroups/MC_aks-yxytest_group_aks-yxytest_eastus/providers/Microsoft.Compute/disks/pvc-11b511bb-c767-42c9-9fae-cf31c641ab4a"}
I0627 06:58:30.533276       1 controllerserver.go:437] begin to modify azure disk() account type(StandardSSD_LRS) rg() location()
I0627 06:58:30.533333       1 azure_managedDiskController.go:420] azureDisk - modifying managed Name:pvc-11b511bb-c767-42c9-9fae-cf31c641ab4a, StorageAccountType:StandardSSD_LRS, DiskIOPSReadWrite:, DiskMBpsReadWrite:
I0627 06:58:33.598201       1 azure_managedDiskController.go:486] azureDisk - modified new MD Name:pvc-11b511bb-c767-42c9-9fae-cf31c641ab4a StorageAccountType:StandardSSD_LRS
I0627 06:58:33.598251       1 controllerserver.go:466] modify azure disk() account type(StandardSSD_LRS) rg() location() successfully
I0627 06:58:33.598305       1 azure_metrics.go:115] "Observed Request Latency" latency_seconds=3.064952723 request="azuredisk_csi_driver_controller_modify_volume" resource_group="mc_aks-yxytest_group_aks-yxytest_eastus" subscription_id="8ecadfc9-d1a3-4ea4-b844-0d9f87e4d7c8" source="disk.csi.azure.com" volumeid="/subscriptions/8ecadfc9-d1a3-4ea4-b844-0d9f87e4d7c8/resourceGroups/MC_aks-yxytest_group_aks-yxytest_eastus/providers/Microsoft.Compute/disks/pvc-11b511bb-c767-42c9-9fae-cf31c641ab4a" result_code="succeeded"

I0627 07:14:17.551830       1 utils.go:105] GRPC call: /csi.v1.Controller/ControllerModifyVolume
I0627 07:14:17.551909       1 utils.go:106] GRPC request: {"mutable_parameters":{"skuName":"Premium_LRS"},"volume_id":"/subscriptions/8ecadfc9-d1a3-4ea4-b844-0d9f87e4d7c8/resourceGroups/MC_aks-yxytest_group_aks-yxytest_eastus/providers/Microsoft.Compute/disks/pvc-11b511bb-c767-42c9-9fae-cf31c641ab4a"}
I0627 07:14:17.634946       1 controllerserver.go:437] begin to modify azure disk() account type(Premium_LRS) rg() location()
I0627 07:14:17.635000       1 azure_managedDiskController.go:420] azureDisk - modifying managed Name:pvc-11b511bb-c767-42c9-9fae-cf31c641ab4a, StorageAccountType:Premium_LRS, DiskIOPSReadWrite:, DiskMBpsReadWrite:
I0627 07:14:20.141664       1 azure_managedDiskController.go:486] azureDisk - modified new MD Name:pvc-11b511bb-c767-42c9-9fae-cf31c641ab4a StorageAccountType:Premium_LRS
I0627 07:14:20.141707       1 controllerserver.go:466] modify azure disk() account type(Premium_LRS) rg() location() successfully
I0627 07:14:20.141760       1 azure_metrics.go:115] "Observed Request Latency" latency_seconds=2.506722411 request="azuredisk_csi_driver_controller_modify_volume" resource_group="mc_aks-yxytest_group_aks-yxytest_eastus" subscription_id="8ecadfc9-d1a3-4ea4-b844-0d9f87e4d7c8" source="disk.csi.azure.com" volumeid="/subscriptions/8ecadfc9-d1a3-4ea4-b844-0d9f87e4d7c8/resourceGroups/MC_aks-yxytest_group_aks-yxytest_eastus/providers/Microsoft.Compute/disks/pvc-11b511bb-c767-42c9-9fae-cf31c641ab4a" result_code="succeeded"
I0627 07:14:20.141795       1 utils.go:112] GRPC response: {}

Release note:

none

k8s-ci-robot · 2024-06-07T03:50:36Z

Hi @umagnus. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

andyzhangx · 2024-06-07T04:56:46Z

/ok-to-test

umagnus · 2024-06-26T01:46:53Z

/retest

pkg/azuredisk/azure_managedDiskController.go

pkg/azuredisk/controllerserver.go

andyzhangx · 2024-06-27T03:57:19Z

pkg/azuredisk/azure_managedDiskController.go

+	}
+
+	model := armcompute.DiskUpdate{
+		SKU: &armcompute.DiskSKU{


are these two parameters necessary in DiskUpdate struct? IMO, if only sku is needed, then only update sku, otherwise only update diskProperties

in L432, there is result, err := diskClient.Get(ctx, rg, options.DiskName) operation, is this necessary if disk sku is not going to be modified?

I move model.SKU = &armcompute.DiskSKU{ Name: to.Ptr(diskSku), } and model.Properties = &diskProperties in update needed. But I think result, err := diskClient.Get(ctx, rg, options.DiskName) is still need because we need old disk sku in L443 and L454

pkg/azuredisk/controllerserver.go

pkg/azuredisk/controllerserver_v2.go

umagnus · 2024-06-28T01:09:59Z

/retest

umagnus · 2024-06-28T05:47:51Z

/retest

pkg/azuredisk/azure_managedDiskController.go

andyzhangx · 2024-06-28T06:11:56Z

pkg/azuredisk/controllerserver.go

+		skuName = ""
+	}
+
+	if _, err := azureutils.NormalizeCachingMode(diskParams.CachingMode); err != nil {


why there is cachingmode check here since cachingmode change is not allowed

remove it since azuredisk cannot change PremiumV2

andyzhangx · 2024-06-28T06:12:19Z

pkg/azuredisk/controllerserver.go

+	}
+	if skuName == armcompute.DiskStorageAccountTypesPremiumV2LRS {
+		// PremiumV2LRS only supports None caching mode
+		azureutils.SetKeyValueInMap(diskParams.VolumeContext, consts.CachingModeField, string(v1.AzureDataDiskCachingNone))


so this change is also not necessary?

andyzhangx · 2024-06-28T06:12:48Z

pkg/azuredisk/controllerserver.go

+		azureutils.SetKeyValueInMap(diskParams.VolumeContext, consts.CachingModeField, string(v1.AzureDataDiskCachingNone))
+	}
+
+	metricsRequest := "controller_modify_volume"


not necessary to define a new var here since it's just used once

andyzhangx · 2024-06-28T06:13:26Z

pkg/azuredisk/controllerserver_v2.go

+		mc.ObserveOperationWithResult(isOperationSucceeded, consts.VolumeID, diskURI)
+	}()
+
+	err = localDiskController.ModifyDisk(ctx, volumeOptions)


pls make same change as v1

andyzhangx · 2024-06-28T06:18:12Z

pkg/azuredisk/controllerserver_v2.go

+		return nil, status.Error(codes.NotFound, fmt.Sprintf("Volume not found, failed with error: %v", err))
+	}
+
+	mutableParams := req.GetMutableParameters()


remove this mutableParams var define, just use req.GetMutableParameters() in L327

andyzhangx · 2024-06-28T06:18:27Z

pkg/azuredisk/controllerserver.go

+		return nil, status.Error(codes.NotFound, fmt.Sprintf("Volume not found, failed with error: %v", err))
+	}
+
+	mutableParams := req.GetMutableParameters()


remove this mutableParams var define, just use req.GetMutableParameters() in L327

andyzhangx · 2024-06-28T06:21:01Z

pkg/azuredisk/azure_managedDiskController.go

+		}
+	}
+
+	if _, err := diskClient.Patch(ctx, rg, options.DiskName, model); err != nil {


only run diskClient.Patch if model.SKU or model.Properties is not nil

andyzhangx · 2024-06-28T11:04:12Z

/retest

umagnus · 2024-07-03T01:00:56Z

/retest

umagnus · 2024-07-04T01:02:28Z

/retest

andyzhangx · 2024-07-15T02:21:23Z

pkg/azuredisk/azure_managedDiskController.go

+		klog.V(4).Infof("azureDisk - no modification needed for disk(%s)", options.DiskName)
+	}
+
+	klog.V(2).Infof("azureDisk - modified new MD Name:%s StorageAccountType:%s", options.DiskName, options.StorageAccountType)


remove this line since it's not necessary

andyzhangx · 2024-07-15T02:40:25Z

pkg/azureutils/azure_disk_utils.go

 			diskParams.ResourceGroup = v
 		case consts.DiskIOPSReadWriteField:
+			if _, err = strconv.Atoi(v); err != nil {
+				return diskParams, fmt.Errorf("parse %s failed with error: %v", v, err)


pls also provide the key name in error msg, e.g. (parsing %s:%s failed with error: %v, consts.DiskIOPSReadWriteField, v, err)

andyzhangx · 2024-07-15T02:40:34Z

pkg/azureutils/azure_disk_utils.go

 			diskParams.DiskIOPSReadWrite = v
 		case consts.DiskMBPSReadWriteField:
+			if _, err = strconv.Atoi(v); err != nil {
+				return diskParams, fmt.Errorf("parse %s failed with error: %v", v, err)


same as the above comments

andyzhangx · 2024-07-15T02:43:09Z

pkg/azuredisk/controllerserver.go

+	}
+	diskURI := volumeID
+
+	if err := azureutils.IsValidDiskURI(diskURI); err != nil {


not necessary here since since azureutils.GetDiskName(diskURI) already checks whether diskURI is valid

andyzhangx · 2024-07-15T02:45:51Z

pkg/azuredisk/controllerserver.go

+		ResourceGroup:      diskParams.ResourceGroup,
+		SubscriptionID:     diskParams.SubscriptionID,
+		StorageAccountType: skuName,
+		SourceResourceID:   volumeID,


use diskURI even it's the same

pkg/azuredisk/azure_managedDiskController.go

test/sanity/run-test.sh

andyzhangx · 2024-07-15T03:09:07Z

pls also add yamllint check in the PR, refer to kubernetes-sigs/blob-csi-driver#1484

andyzhangx · 2024-07-15T07:52:06Z

/test pull-azuredisk-csi-driver-sanity

deploy/rbac-csi-azuredisk-controller.yaml

andyzhangx · 2024-07-16T03:14:33Z

deploy/example/modifyvolume/pod-modifyvolume.yaml

@@ -0,0 +1,49 @@
+---


pls use existing sc/pvc/pod config yaml files, this yaml file is not necessary

But the existing sc has no yaml files to use PremiumV2_LRS which supports DiskIOPSReadWrite and DiskMBpsReadWrite parameters as an example.

then only create a new storage class

andyzhangx

/lgtm

k8s-ci-robot · 2024-07-16T05:57:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, umagnus

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:

~~OWNERS~~ [andyzhangx]

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

andyzhangx · 2024-07-16T05:58:25Z

pls cherrypick to release-1.30, thx

k8s-ci-robot · 2024-07-16T06:01:40Z

New changes are detected. LGTM label has been removed.

andyzhangx · 2024-07-16T08:00:00Z

/retest

andyzhangx · 2024-07-17T01:56:52Z

/retest

umagnus · 2024-07-17T05:50:34Z

/retest

andyzhangx · 2024-07-17T07:47:45Z

/retest

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 7, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 7, 2024

k8s-ci-robot requested review from ZeroMagic and andyzhangx June 7, 2024 03:50

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 7, 2024

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 25, 2024

umagnus force-pushed the modify-volume branch from 0493d59 to b7764f3 Compare June 25, 2024 07:43

umagnus force-pushed the modify-volume branch from cf3860b to 9d14c43 Compare June 27, 2024 02:19

andyzhangx reviewed Jun 27, 2024

View reviewed changes

pkg/azuredisk/controllerserver_v2.go Show resolved Hide resolved

andyzhangx reviewed Jun 28, 2024

View reviewed changes

umagnus force-pushed the modify-volume branch from 667b436 to 2ae6dc8 Compare July 2, 2024 09:09

andyzhangx reviewed Jul 15, 2024

View reviewed changes

umagnus force-pushed the modify-volume branch from 21ecf73 to da751a5 Compare July 16, 2024 03:10

andyzhangx reviewed Jul 16, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2024

umagnus force-pushed the modify-volume branch from 751da05 to bfe21bf Compare July 16, 2024 03:32

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2024

andyzhangx approved these changes Jul 16, 2024

View reviewed changes

k8s-ci-robot assigned andyzhangx Jul 16, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2024

andyzhangx changed the title ~~[draft] feat: Implement KEP3751 ("ControllerModifyVolume")~~ feat: Implement KEP3751 ("ControllerModifyVolume") Jul 16, 2024

umagnus force-pushed the modify-volume branch from c1e58ce to 45747c4 Compare July 16, 2024 06:01

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2024

umagnus mentioned this pull request Jul 16, 2024

[release-1.30] feat: Implement KEP3751 ("ControllerModifyVolume") #2411

Merged

4 tasks

add modify volume

63a446f

umagnus force-pushed the modify-volume branch from 45747c4 to 63a446f Compare July 16, 2024 06:08

andyzhangx merged commit 71740c6 into kubernetes-sigs:master Jul 17, 2024

dfajmon mentioned this pull request Nov 15, 2024

OCPBUGS-43823: Rebase to upstream v1.31.1 for OCP 4.18 openshift/azure-disk-csi-driver#87

Closed

feat: Implement KEP3751 ("ControllerModifyVolume") #2344

feat: Implement KEP3751 ("ControllerModifyVolume") #2344

Uh oh!

Conversation

umagnus commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 7, 2024

Uh oh!

andyzhangx commented Jun 7, 2024

Uh oh!

umagnus commented Jun 26, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

umagnus commented Jun 28, 2024

Uh oh!

umagnus commented Jun 28, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyzhangx commented Jun 28, 2024

Uh oh!

umagnus commented Jul 3, 2024

Uh oh!

umagnus commented Jul 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

umagnus commented Jun 7, 2024 •

edited

Loading