Update GenVmSizes to query all Azure regions for comprehensive VM size list#16313
Merged
mitchdenny merged 2 commits intomainfrom Apr 20, 2026
Merged
Update GenVmSizes to query all Azure regions for comprehensive VM size list#16313mitchdenny merged 2 commits intomainfrom
mitchdenny merged 2 commits intomainfrom
Conversation
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 16313Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 16313" |
16 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the Azure Kubernetes VM size generation tool to query the full Azure Compute SKUs catalog (all regions) and regenerates the VM size constants to include region-specific sizes that were previously missing.
Changes:
- Switched
GenVmSizes.csfrom a hardcoded US-region loop to the unfilteredMicrosoft.Compute/skusendpoint withnextLinkpagination. - Added a public-cloud (AzureCloud) verification step and improved
azCLI path resolution for Windows. - Regenerated
AksNodeVmSizes.Generated.csto include the complete, API-derived VM size set and family-based groupings.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Aspire.Hosting.Azure.Kubernetes/tools/GenVmSizes.cs | Expands SKU discovery to all regions via paginated SKUs API; adds AzureCloud guard and az resolution changes. |
| src/Aspire.Hosting.Azure.Kubernetes/AksNodeVmSizes.Generated.cs | Regenerated VM size constants to reflect the new comprehensive SKU input and updated grouping scheme. |
| Console.WriteLine($"Using subscription: {subscriptionId}"); | ||
| // Verify we're targeting the public Azure cloud | ||
| var cloudName = await RunAzCommand("cloud show --query name -o tsv").ConfigureAwait(false); | ||
| cloudName = cloudName?.Trim(); |
There was a problem hiding this comment.
If RunAzCommand("cloud show ...") fails (e.g., az not installed / command error) cloudName will be null/empty, and this check reports a misleading "active cloud is ''" message instead of the underlying failure. Consider explicitly handling null/whitespace cloudName as a separate error (fail fast with a message like "Failed to determine active Azure cloud"), so users don’t get directed to switch clouds when the real issue is az execution/login.
Suggested change
| cloudName = cloudName?.Trim(); | |
| cloudName = cloudName?.Trim(); | |
| if (string.IsNullOrWhiteSpace(cloudName)) | |
| { | |
| Console.Error.WriteLine("Error: Failed to determine the active Azure cloud. Ensure the Azure CLI is installed and that you are logged in with 'az login'."); | |
| return 1; | |
| } |
Comment on lines
+158
to
+169
| // Try common names: 'az' on Linux/macOS, 'az.cmd' on Windows | ||
| var pathDirs = Environment.GetEnvironmentVariable("PATH")?.Split(Path.PathSeparator) ?? []; | ||
| string[] candidates = OperatingSystem.IsWindows() | ||
| ? ["az.cmd", "az.bat", "az.exe"] | ||
| : ["az"]; | ||
|
|
||
| foreach (var dir in pathDirs) | ||
| { | ||
| foreach (var candidate in candidates) | ||
| { | ||
| var fullPath = Path.Combine(dir, candidate); | ||
| if (File.Exists(fullPath)) |
There was a problem hiding this comment.
FindAzCli reimplements PATH lookup but is less robust than the repository’s existing PathLookupHelper.FindFullPathFromPath (e.g., it doesn’t use PATHEXT and doesn’t remove empty PATH entries). This increases the chance of subtle cross-platform lookup failures and duplicates logic already used elsewhere for resolving az. Consider reusing PathLookupHelper (or aligning this implementation with it) to keep command resolution consistent across the repo.
Suggested change
| // Try common names: 'az' on Linux/macOS, 'az.cmd' on Windows | |
| var pathDirs = Environment.GetEnvironmentVariable("PATH")?.Split(Path.PathSeparator) ?? []; | |
| string[] candidates = OperatingSystem.IsWindows() | |
| ? ["az.cmd", "az.bat", "az.exe"] | |
| : ["az"]; | |
| foreach (var dir in pathDirs) | |
| { | |
| foreach (var candidate in candidates) | |
| { | |
| var fullPath = Path.Combine(dir, candidate); | |
| if (File.Exists(fullPath)) | |
| const string commandName = "az"; | |
| var pathDirs = (Environment.GetEnvironmentVariable("PATH") ?? string.Empty) | |
| .Split(Path.PathSeparator, StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); | |
| string[] candidates; | |
| if (OperatingSystem.IsWindows()) | |
| { | |
| var pathExt = (Environment.GetEnvironmentVariable("PATHEXT") ?? ".COM;.EXE;.BAT;.CMD") | |
| .Split(';', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); | |
| candidates = | |
| [ | |
| commandName, | |
| .. pathExt.Select(ext => commandName + ext.ToLowerInvariant()), | |
| .. pathExt.Select(ext => commandName + ext.ToUpperInvariant()), | |
| ]; | |
| } | |
| else | |
| { | |
| candidates = [commandName]; | |
| } | |
| var visitedPaths = new HashSet<string>(OperatingSystem.IsWindows() ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal); | |
| foreach (var dir in pathDirs) | |
| { | |
| foreach (var candidate in candidates) | |
| { | |
| var fullPath = Path.Combine(dir, candidate); | |
| if (visitedPaths.Add(fullPath) && File.Exists(fullPath)) |
Comment on lines
22
to
+25
| /// <summary> | ||
| /// General purpose VM sizes optimized for balanced CPU-to-memory ratio. | ||
| /// VM sizes in the Standard NCASv3_T4 Family family. | ||
| /// </summary> | ||
| public static class GeneralPurpose | ||
| public static class StandardNCASv3T4 |
There was a problem hiding this comment.
The generated family summary repeats the word “family” when the Azure SKU family already contains “Family” (e.g., "Standard NCASv3_T4 Family family"). Consider adjusting the generator to strip a trailing "Family" from the displayed family name so the XML docs read cleanly.
mitchdenny
force-pushed
the
dev/update-aks-vm-sizes
branch
from
7895495 to
5140a13
Compare
mitchdenny
force-pushed
the
dev/update-aks-vm-sizes
branch
from
5140a13 to
5f3748e
Compare
…ilter The GenVmSizes tool previously only queried 9 US regions, missing VM sizes available exclusively in other regions. Updated to use the unfiltered Microsoft.Compute/skus API endpoint which returns all SKUs across all regions in a single paginated response. Additionally, the tool now filters to only AKS-compatible VM sizes per https://learn.microsoft.com/azure/aks/quotas-skus-regions#restricted-vm-sizes: - Requires minimum 2 vCPUs - Requires minimum 2 GB RAM - Excludes Basic tier VMs Changes: - Replace hardcoded US region list with unfiltered SKU API call - Add pagination support via nextLink - Add AzureCloud verification to prevent accidental use with gov/sovereign clouds - Add deterministic deduplication (sort by name+location before grouping) - Add AKS node pool compatibility filtering - Fix az CLI path resolution for cross-platform compatibility - Regenerated AksNodeVmSizes.Generated.cs: 1351 VM sizes (up from ~50) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mitchdenny
force-pushed
the
dev/update-aks-vm-sizes
branch
from
5f3748e to
c09342e
Compare
JamesNK
reviewed
Apr 20, 2026
Member
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
2 issues found: 1 stale reference (breaking code examples in XML docs and spec), 1 comment/code mismatch (Av1 filter documented but not implemented).
| /// </summary> | ||
| public static class GeneralPurpose | ||
| public static class StandardNCASv3T4 |
Member
There was a problem hiding this comment.
This PR removes the GpuAccelerated nested class and moves StandardNC6sV3 into StandardNCSv3. However, AzureKubernetesEnvironmentExtensions.cs (line 235) and docs/specs/aks-support.md (line 398) both contain XML doc / spec examples referencing the now-removed AksNodeVmSizes.GpuAccelerated.StandardNC6sV3. These should be updated to match the new class structure (e.g., AksNodeVmSizes.StandardNCSv3.StandardNC6sV3) so the example code compiles.
| // - VM sizes with fewer than 2 vCPUs are restricted | ||
| // - VM sizes with fewer than 2 GB RAM are restricted for user node pools | ||
| // - Basic tier VMs are not supported | ||
| // - Av1 series VMs are not recommended (deprecated) |
Member
There was a problem hiding this comment.
The comment above lists "Av1 series VMs are not recommended (deprecated)" as an AKS restriction, but IsAksCompatible does not implement any Av1 filter. Either remove this bullet from the comment (if it's intentionally not filtered) or add the missing filter to IsAksCompatible.
- Update stale references to AksNodeVmSizes.GpuAccelerated.StandardNC6sV3 to AksNodeVmSizes.StandardNCSv3.StandardNC6sV3 in extension XML docs and aks-support.md spec - Fix Av1 comment/code mismatch: clarify that Av1 is 'not recommended' (not restricted) and intentionally included in the output - Update spec checklist to reflect new family-based category structure Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
JamesNK
approved these changes
Apr 20, 2026
Contributor
|
🎬 CLI E2E Test Recordings — 72 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #24646365591 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The
GenVmSizes.cstool previously only queried 9 US regions for VM SKUs, resulting in theAksNodeVmSizes.Generated.csfile missing many VM sizes that are available exclusively in non-US regions.Changes
Tool (
GenVmSizes.cs):Microsoft.Compute/skusAPI endpoint, which returns all SKUs across all Azure regions in a single paginated responsenextLinkpagination support for the SKU API responseazCLI path resolution for cross-platform compatibility (Windows.cmdfiles weren't found byProcess.Start)usingdirective and suppressed CS1591 for the tool-only typesGenerated output (
AksNodeVmSizes.Generated.cs):StandardDSv5,StandardFSv2) instead of hand-curated namesChecklist
<remarks />and<code />elements on your triple slash comments?