test: add ModifyVolume sanity test with storage account type change#3587
test: add ModifyVolume sanity test with storage account type change#3587andyzhangx wants to merge 1 commit intokubernetes-sigs:masterfrom
Conversation
|
@andyzhangx: The label(s) DetailsIn response to this:
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. |
k8s-ci-robot
added
size/XS
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
andyzhangx
force-pushed
the
test/add-modify-volume-sanity-test
branch
from
a5777d4 to
acaa447
Compare
There was a problem hiding this comment.
Pull request overview
Adds CSI sanity coverage for ControllerModifyVolume by supplying non-empty mutable parameters to csi-sanity, aiming to exercise the Azure Disk ModifyVolume code path during sanity runs.
Changes:
- Add a mutable-parameters YAML file for ModifyVolume sanity tests.
- Pass
--csi.testvolumemutableparameterstocsi-sanityin the sanity runner script.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/sanity/run-test.sh | Adds --csi.testvolumemutableparameters argument when invoking csi-sanity. |
| test/sanity/modify-volume-params.yaml | Introduces mutable parameters (currently skuName: StandardSSD_LRS) for ModifyVolume tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1 @@ | |||
| skuName: StandardSSD_LRS | |||
There was a problem hiding this comment.
The chosen mutable parameter skuName: StandardSSD_LRS is the driver’s default SKU when no account type is specified (NormalizeStorageAccountType defaults to StandardSSDLRS). Since csi-sanity CreateVolume in this script does not pass any volume parameters, the test volume will likely already be StandardSSD_LRS, and ModifyDisk will log “no modification needed” without calling diskClient.Patch. To actually exercise the ModifyVolume SKU-change path, use a different target SKU than the CreateVolume default (or also pass a --csi.testvolumeparameters file to create with one SKU and modify to another).
| skuName: StandardSSD_LRS | |
| skuName: Premium_LRS |
| echo 'Begin to run sanity test...' | ||
| readonly CSI_SANITY_BIN='csi-sanity' | ||
| "$CSI_SANITY_BIN" --ginkgo.v --csi.endpoint="$endpoint" --ginkgo.skip='should work|should fail when volume does not exist on the specified path|should be idempotent|pagination should detect volumes added between pages and accept tokens when the last volume from a page is deleted|should remove target path|should return appropriate capabilities' | ||
| "$CSI_SANITY_BIN" --ginkgo.v --csi.endpoint="$endpoint" --csi.testvolumemutableparameters="test/sanity/modify-volume-params.yaml" --ginkgo.skip='should work|should fail when volume does not exist on the specified path|should be idempotent|pagination should detect volumes added between pages and accept tokens when the last volume from a page is deleted|should remove target path|should return appropriate capabilities' |
There was a problem hiding this comment.
PR title/description mention exercising ModifyVolume with a cachingMode parameter, but the added mutable-parameters file only sets skuName. Either update the mutable-parameters file to include the intended parameter(s) or adjust the PR title/description to match what’s actually being passed to csi-sanity.
Previously the csi-sanity ModifyVolume test ran with empty mutable parameters, making ControllerModifyVolume effectively a no-op. Add modify-volume-params.yaml with skuName: Premium_LRS and pass --csi.testvolumemutableparameters to csi-sanity so ModifyVolume tests exercise the real SKU change code path (ParseDiskParameters -> NormalizeStorageAccountType -> ModifyDisk -> diskClient.Patch). Use Premium_LRS instead of StandardSSD_LRS since StandardSSD_LRS is the default SKU for CreateVolume, which would result in no-op.
andyzhangx
force-pushed
the
test/add-modify-volume-sanity-test
branch
from
acaa447 to
c21dc98
Compare
andyzhangx
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@andyzhangx: The following test failed, say
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. DetailsInstructions 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. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The existing csi-sanity
ModifyVolumetest runs with empty mutable parameters, makingControllerModifyVolumeeffectively a no-op. This PR adds amodify-volume-params.yamlwithskuName: Premium_LRSand passes--csi.testvolumemutableparameterstocsi-sanity, so the ModifyVolume tests exercise the real SKU change code path (ParseDiskParameters→NormalizeStorageAccountType→ModifyDisk→diskClient.Patch).Uses
Premium_LRSinstead ofStandardSSD_LRSsinceStandardSSD_LRSis the default SKU for CreateVolume, which would result in a no-op.Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Two files changed:
test/sanity/modify-volume-params.yaml— new file withskuName: Premium_LRStest/sanity/run-test.sh— added--csi.testvolumemutableparametersflag