[WIP] feat: support changing cachingMode through VolumeAttributeClass by andyzhangx · Pull Request #3584 · kubernetes-sigs/azuredisk-csi-driver

andyzhangx · 2026-04-11T09:17:15Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds support for changing cachingMode via VolumeAttributeClass by validating the parameter in ControllerModifyVolume and updating the disk's caching mode on the VM in-place.

How it works

cachingMode is a VM-level data disk attachment property. When modified via VolumeAttributeClass:

ControllerModifyVolume validates the new cachingMode value
Enforces constraints: disks >= 4 TiB only support None; PremiumV2_LRS only supports None
If the disk is currently attached to a VM, calls UpdateDiskCachingMode which:
- Finds the node the disk is attached to via disk.ManagedBy
- Gets the disk's current LUN
- Issues an AttachDisk call with the existing LUN and new cachingMode to update the VM
If the disk is not attached, the update is a no-op — the new cachingMode will be applied on next attach via PV volumeAttributes

Changes

azure_controller_common.go: Added UpdateDiskCachingMode method on controllerCommon
controllerserver.go: Added cachingMode validation and update logic in ControllerModifyVolume
controllerserver_test.go: Added 4 unit test cases (3 valid modes + 1 invalid)

Example VolumeAttributeClass

apiVersion: storage.k8s.io/v1beta1
kind: VolumeAttributesClass
metadata:
  name: caching-readonly
driverName: disk.csi.azure.com
parameters:
  cachingMode: ReadOnly

Which issue(s) this PR fixes:

Fixes #3471

Special notes for your reviewer:

Supported cachingMode values: None, ReadOnly, ReadWrite
For attached disks, the change is applied in-place via VM update (no pod restart required)
For unattached disks, the new cachingMode takes effect on next attach

k8s-ci-robot · 2026-04-11T09:17:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx

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

The pull request process is described here

Details

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

In ControllerModifyVolume: - Validate the cachingMode parameter - Enforce caching limits (disks >= 4 TiB support only None) - Enforce PremiumV2_LRS constraint (only None) - Pass cachingMode to ModifyDisk via ManagedDiskOptions In ModifyDisk: - Accept CachingMode in ManagedDiskOptions - Log cachingMode update (applied on next ControllerPublishVolume attach) Note: cachingMode is a VM-level data disk attachment property, not a disk resource property. The new value is persisted in PV volumeAttributes via VolumeAttributeClass and takes effect on next ControllerPublishVolume. Fixes: kubernetes-sigs#3471

Add modify-volume-params.yaml with cachingmode: ReadOnly and pass --csi.testvolumemutableparameters to csi-sanity so ModifyVolume tests exercise the real cachingMode code path instead of empty parameters.

k8s-ci-robot · 2026-04-11T17:49:48Z

@andyzhangx: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-azuredisk-csi-driver-e2e-capz	`7d82151`	link	true	`/test pull-azuredisk-csi-driver-e2e-capz`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

Copilot

Pull request overview

Adds initial wiring to accept cachingMode as a mutable parameter (intended for Kubernetes VolumeAttributesClass) and exercises it via CSI sanity tests.

Changes:

Pass --csi.testvolumemutableparameters into the sanity test runner and add a sample params YAML.
Validate cachingMode in ControllerModifyVolume (including PremiumV2_LRS and >= 4 TiB constraints) and plumb it into ManagedDiskOptions.
Add unit tests for valid/invalid cachingMode values.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/sanity/run-test.sh	Enables csi-sanity mutable-parameter test input file.
test/sanity/modify-volume-params.yaml	Provides a mutable-params YAML containing `cachingmode: ReadOnly`.
pkg/azuredisk/controllerserver.go	Validates `cachingMode` in `ControllerModifyVolume` and passes it into disk modification options.
pkg/azuredisk/controllerserver_test.go	Adds `ControllerModifyVolume` unit test cases for caching mode parsing.
pkg/azuredisk/azure_managedDiskController.go	Extends `ManagedDiskOptions` with `CachingMode` and updates ModifyDisk logging/flow.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-12T03:09:41Z

@@ -588,6 +612,7 @@ func (d *Driver) ControllerModifyVolume(ctx context.Context, req *csi.Controller
 		StorageAccountType: skuName,
 		SourceResourceID:   diskURI,
 		SourceType:         consts.SourceVolume,
+		CachingMode:        cachingMode,
 	}


ControllerModifyVolume validates cachingMode and passes it through ManagedDiskOptions, but ManagedDiskController.ModifyDisk does not apply caching mode anywhere (it only logs). Since caching mode is a VM data-disk attachment property (not a disk resource property), this change currently has no effect for attached volumes and does not reliably affect future attaches either. Implement the intended behavior by updating the VM attachment when currentDisk.ManagedBy is set (e.g., re-attach the disk on the VM with the existing LUN and new caching mode), and keep the no-op behavior for unattached disks.

Copilot · 2026-04-12T03:09:41Z

 	// PerformancePlus - Set this flag to true to get a boost on the performance target of the disk deployed
 	PerformancePlus *bool
+	// CachingMode - The caching mode of the disk when attached to a VM.
+	// This is a VM-level property stored in the PV volumeAttributes, applied on next ControllerPublishVolume.


The new CachingMode field comment says the value is stored in PV volumeAttributes and applied on next ControllerPublishVolume, but this code path does not persist caching mode anywhere (and ModifyDisk doesn’t apply it either). Either wire this field into an actual VM-attachment update path (for attached disks) and/or clarify in the comment what component is responsible for persisting and re-supplying the value on future attaches (e.g., Kubernetes VAC).

Suggested change

// This is a VM-level property stored in the PV volumeAttributes, applied on next ControllerPublishVolume.

// This is a VM attachment-level setting supplied to publish/attach operations; it is not persisted by

// ManagedDiskOptions or by managed disk modification. If the value must survive detach/reattach cycles,

// a higher-level component must persist and re-supply it (for example via Kubernetes PV volumeAttributes/VAC).

Copilot · 2026-04-12T03:09:41Z

 	}
+
+	if options.CachingMode != "" {
+		klog.V(2).Infof("azureDisk - cachingMode(%s) updated for disk(%s), will take effect on next attach", options.CachingMode, diskName)


ModifyDisk logs that cachingMode was updated and will take effect on next attach, but there is no actual update performed (and cachingMode isn’t persisted on the disk resource). This log is misleading as-is. Either perform the VM attachment update here/elsewhere (for attached disks), or change the log to reflect that only validation occurred and the new value must be provided again via volume_context on a future attach.

Suggested change

klog.V(2).Infof("azureDisk - cachingMode(%s) updated for disk(%s), will take effect on next attach", options.CachingMode, diskName)

klog.V(2).Infof("azureDisk - cachingMode(%s) for disk(%s) was not persisted by ModifyDisk; provide it again via volume_context on a future attach", options.CachingMode, diskName)

Copilot · 2026-04-12T03:09:42Z

+		{
+			desc: "success with valid cachingMode ReadOnly",
+			req: &csi.ControllerModifyVolumeRequest{
+				VolumeId: testVolumeID,
+				MutableParameters: map[string]string{
+					consts.CachingModeField: "ReadOnly",
+				},
+			},
+			oldSKU:                 to.Ptr(armcompute.DiskStorageAccountTypesPremiumLRS),
+			expectedResp:           &csi.ControllerModifyVolumeResponse{},
+			expectMigrationStarted: false,
+		},
+		{
+			desc: "success with valid cachingMode None",
+			req: &csi.ControllerModifyVolumeRequest{
+				VolumeId: testVolumeID,
+				MutableParameters: map[string]string{
+					consts.CachingModeField: "None",
+				},
+			},
+			oldSKU:                 to.Ptr(armcompute.DiskStorageAccountTypesPremiumLRS),
+			expectedResp:           &csi.ControllerModifyVolumeResponse{},
+			expectMigrationStarted: false,
+		},
+		{
+			desc: "success with valid cachingMode ReadWrite",
+			req: &csi.ControllerModifyVolumeRequest{
+				VolumeId: testVolumeID,
+				MutableParameters: map[string]string{
+					consts.CachingModeField: "ReadWrite",
+				},
+			},
+			oldSKU:                 to.Ptr(armcompute.DiskStorageAccountTypesPremiumLRS),
+			expectedResp:           &csi.ControllerModifyVolumeResponse{},
+			expectMigrationStarted: false,
+		},
+		{
+			desc: "fail with invalid cachingMode",
+			req: &csi.ControllerModifyVolumeRequest{
+				VolumeId: testVolumeID,
+				MutableParameters: map[string]string{
+					consts.CachingModeField: "WriteOnly",
+				},
+			},
+			expectedResp:           nil,
+			expectedErrCode:        codes.InvalidArgument,
+			expectMigrationStarted: false,
+		},


The added cachingMode test cases only cover “valid/invalid value” parsing. The new behavior in ControllerModifyVolume also enforces size (>= 4 TiB) and PremiumV2_LRS constraints, and (per PR intent) should update caching mode in-place for attached disks; none of that is covered here. Add unit tests that (1) assert InvalidArgument for non-None caching on >=4TiB and PremiumV2_LRS, and (2) verify the VM attachment update path is invoked when the disk is attached (e.g., ManagedBy set) and is a no-op when unattached.

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. labels Apr 11, 2026

k8s-ci-robot requested review from landreasyan and nearora-msft April 11, 2026 09:17

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 11, 2026

andyzhangx force-pushed the feat/support-cachingmode-in-modify-volume branch from 53abd35 to 5af5d6f Compare April 11, 2026 09:30

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 11, 2026

andyzhangx force-pushed the feat/support-cachingmode-in-modify-volume branch 2 times, most recently from 5f0ea93 to 9c1c857 Compare April 11, 2026 13:45

andyzhangx changed the title ~~feat: support changing cachingMode through VolumeAttributeClass~~ [WIP] feat: support changing cachingMode through VolumeAttributeClass Apr 11, 2026

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2026

andyzhangx force-pushed the feat/support-cachingmode-in-modify-volume branch from 9c1c857 to 0048abf Compare April 11, 2026 13:58

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 11, 2026

test: add cachingMode to sanity ModifyVolume test

7d82151

Add modify-volume-params.yaml with cachingmode: ReadOnly and pass --csi.testvolumemutableparameters to csi-sanity so ModifyVolume tests exercise the real cachingMode code path instead of empty parameters.

andyzhangx requested a review from Copilot April 12, 2026 03:05

Copilot started reviewing on behalf of andyzhangx April 12, 2026 03:06 View session

Copilot AI reviewed Apr 12, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] feat: support changing cachingMode through VolumeAttributeClass#3584

[WIP] feat: support changing cachingMode through VolumeAttributeClass#3584
andyzhangx wants to merge 2 commits intokubernetes-sigs:masterfrom
andyzhangx:feat/support-cachingmode-in-modify-volume

andyzhangx commented Apr 11, 2026 •

edited

Loading

Uh oh!

k8s-ci-robot commented Apr 11, 2026

Uh oh!

k8s-ci-robot commented Apr 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 12, 2026

Uh oh!

Copilot AI Apr 12, 2026

Uh oh!

Copilot AI Apr 12, 2026

Uh oh!

Copilot AI Apr 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-	// This is a VM-level property stored in the PV volumeAttributes, applied on next ControllerPublishVolume.
+	// This is a VM attachment-level setting supplied to publish/attach operations; it is not persisted by
+	// ManagedDiskOptions or by managed disk modification. If the value must survive detach/reattach cycles,
+	// a higher-level component must persist and re-supply it (for example via Kubernetes PV volumeAttributes/VAC).

	klog.V(2).Infof("azureDisk - cachingMode(%s) updated for disk(%s), will take effect on next attach", options.CachingMode, diskName)
	klog.V(2).Infof("azureDisk - cachingMode(%s) for disk(%s) was not persisted by ModifyDisk; provide it again via volume_context on a future attach", options.CachingMode, diskName)

Conversation

andyzhangx commented Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

How it works

Changes

Example VolumeAttributeClass

Which issue(s) this PR fixes:

Special notes for your reviewer:

Uh oh!

k8s-ci-robot commented Apr 11, 2026

Uh oh!

k8s-ci-robot commented Apr 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andyzhangx commented Apr 11, 2026 •

edited

Loading