feat: added support for NVMe disk controller for Azure VMs - #2071
feat: added support for NVMe disk controller for Azure VMs#2071dharapvj wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| for _, cap := range *sku.Capabilities { | ||
| if cap.Name != nil && *cap.Name == "DiskControllerTypes" && cap.Value != nil { | ||
| v := strings.ToLower(*cap.Value) | ||
| return v == "nvme" || (strings.Contains(v, "nvme") && !strings.Contains(v, "scsi")) |
There was a problem hiding this comment.
| return v == "nvme" || (strings.Contains(v, "nvme") && !strings.Contains(v, "scsi")) | |
| return strings.Contains(v, "nvme") && !strings.Contains(v, "scsi") |
if string equals to "nvme" then strings.Contains(v, "nvme") is also true.
There was a problem hiding this comment.
Pull request overview
Adds Azure VM disk controller selection support to unblock provisioning of v6+ Azure VM sizes that require NVMe controllers (per #1980), by allowing an explicit diskControllerType config and defaulting to NVMe for v6+ sizes when unspecified.
Changes:
- Introduces
diskControllerTypein the Azure provider spec (SDK RawConfig) and wires it into provider configuration. - Sets
StorageProfile.DiskControllerTypeduring VM creation, defaulting toNVMefor v6+ VM sizes when not explicitly configured. - Adds validation and unit tests for NVMe-detection helpers and disk controller type validation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sdk/cloudprovider/azure/types.go | Adds diskControllerType to the Azure machine provider spec schema. |
| pkg/cloudprovider/provider/azure/provider.go | Wires config + defaults DiskControllerType to NVMe for v6+; adds validation and SKU capability helpers. |
| pkg/cloudprovider/provider/azure/provider_test.go | Adds unit tests for NVMe heuristics and disk controller validation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func validateDiskControllerType(_ context.Context, c *config, sku compute.ResourceSku) error { | ||
| if c.DiskControllerType != nil { | ||
| if !slices.Contains(compute.PossibleDiskControllerTypesValues(), *c.DiskControllerType) { | ||
| return fmt.Errorf("invalid diskControllerType %q, valid values are: %v", *c.DiskControllerType, compute.PossibleDiskControllerTypesValues()) | ||
| } | ||
| if *c.DiskControllerType == compute.SCSI && skuRequiresNVMe(sku) { | ||
| return fmt.Errorf("VM size %q only supports NVMe disk controller, cannot use %q", c.VMSize, compute.SCSI) | ||
| } | ||
| } | ||
| return nil | ||
| } |
| if config.DiskControllerType != nil { | ||
| sp.DiskControllerType = *config.DiskControllerType | ||
| } else if vmSizeRequiresNVMe(config.VMSize) { | ||
| sp.DiskControllerType = compute.NVMe | ||
| } |
What this PR does / why we need it:
With
v6series of VMs, Azure has remove SCSI disk controller support and it now only provides NVMe disks. If we try to use any v6 or v7 machines e.g.Standard_E2as_v7we do not get new machines ready.This PR adds support to specify NVMe as diskcontroller type which will fix the issue.
Which issue(s) this PR fixes:
Fixes #1980
What type of PR is this?
/kind feature
/kind chore
Special notes for your reviewer:
I do not know how to do a real world test on Azure. So hopefully, we have e2e tests which will get fired with this PR to get such testing done.
Does this PR introduce a user-facing change? Then add your Release Note here:
Documentation: