Skip Azure Gen1-only VM family spec#2102
Conversation
Signed-off-by: Seokho Son <shsongist@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR implements filtering logic to skip Azure Gen1-only VM family specifications during the conversion process. The change addresses compatibility issues with Azure VM generation requirements by identifying and excluding VM specs that only support Gen1 virtual machines.
- Add filtering logic to skip Azure Gen1-only VM families during spec conversion
- Change error logging to debug level for skipped specs to reduce noise
- Update cloud image data to use Gen2-compatible Azure VM images
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/core/resource/spec.go | Implements Azure Gen1-only VM family detection and filtering logic |
| assets/cloudimage.csv | Updates Azure cloud images to use Gen2-compatible variants |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (strings.HasPrefix(lowerSpecName, "standard_d1") && !strings.Contains(lowerSpecName, "v")) || | ||
| (strings.HasPrefix(lowerSpecName, "standard_d2") && !strings.Contains(lowerSpecName, "v")) || | ||
| (strings.HasPrefix(lowerSpecName, "standard_d3") && !strings.Contains(lowerSpecName, "v")) || | ||
| (strings.HasPrefix(lowerSpecName, "standard_d4") && !strings.Contains(lowerSpecName, "v")) { |
There was a problem hiding this comment.
The logic for detecting original D-series VMs is incorrect. The condition !strings.Contains(lowerSpecName, "v") will incorrectly exclude legitimate Gen1-only specs like "standard_d1v2" which contains 'v' but is not a versioned spec. This should check for specific version patterns like "_v2", "_v3", etc. instead.
| (strings.HasPrefix(lowerSpecName, "standard_d4") && !strings.Contains(lowerSpecName, "v")) { | |
| if (strings.HasPrefix(lowerSpecName, "standard_d1") && | |
| !strings.Contains(lowerSpecName, "_v2") && | |
| !strings.Contains(lowerSpecName, "_v3") && | |
| !strings.Contains(lowerSpecName, "_v4") && | |
| !strings.Contains(lowerSpecName, "_v5")) || | |
| (strings.HasPrefix(lowerSpecName, "standard_d2") && | |
| !strings.Contains(lowerSpecName, "_v2") && | |
| !strings.Contains(lowerSpecName, "_v3") && | |
| !strings.Contains(lowerSpecName, "_v4") && | |
| !strings.Contains(lowerSpecName, "_v5")) || | |
| (strings.HasPrefix(lowerSpecName, "standard_d3") && | |
| !strings.Contains(lowerSpecName, "_v2") && | |
| !strings.Contains(lowerSpecName, "_v3") && | |
| !strings.Contains(lowerSpecName, "_v4") && | |
| !strings.Contains(lowerSpecName, "_v5")) || | |
| (strings.HasPrefix(lowerSpecName, "standard_d4") && | |
| !strings.Contains(lowerSpecName, "_v2") && | |
| !strings.Contains(lowerSpecName, "_v3") && | |
| !strings.Contains(lowerSpecName, "_v4") && | |
| !strings.Contains(lowerSpecName, "_v5")) { |
| // A-series: Only original A-series (without version) are Gen1-only | ||
| // Gen1-only: Standard_A1, Standard_A2, Basic_A1, etc. | ||
| // Gen2 supported: Standard_A1_v2, Standard_A2_v2, etc. | ||
| if strings.Contains(lowerSpecName, "_v2") { |
There was a problem hiding this comment.
The check for A-series v2 support only looks for "_v2" but misses other patterns. Azure A-series v2 VMs use naming patterns like "Standard_A1_v2", "Standard_A2m_v2", etc. The current logic may incorrectly classify some A-series v2 VMs as Gen1-only.
| if strings.Contains(lowerSpecName, "_v2") { | |
| if strings.HasSuffix(lowerSpecName, "_v2") { |
| // isAzureGen1OnlySpec checks if the given Azure VM spec belongs to a Gen1-only family | ||
| // Based on Microsoft documentation: https://learn.microsoft.com/en-us/azure/virtual-machines/generation-2 | ||
| // Gen2 supported families: B, D, E, F, L, M, NC, ND, NV, HB, HC, HX series | ||
| // Gen1-only families: A-series (classic), and some older generations |
There was a problem hiding this comment.
The function documentation references Microsoft documentation but the URL appears to be incomplete or potentially outdated. Consider adding the complete URL and verifying it's current, or reference the specific Azure documentation section about VM generations.
| // Gen1-only families: A-series (classic), and some older generations | |
| // isAzureGen1OnlySpec checks if the given Azure VM spec belongs to a Gen1-only family. | |
| // Reference: Microsoft Azure documentation on VM generations and supported families: | |
| // https://learn.microsoft.com/en-us/azure/virtual-machines/generation-2#supported-vm-sizes-and-series | |
| // Gen2 supported families (as of June 2024): B, D, E, F, L, M, NC, ND, NV, HB, HC, HX series. | |
| // Gen1-only families: A-series (classic), and some older generations. Please refer to the link above for the most up-to-date information. |
| // Gen2 supported: Standard_D2s_v3, Standard_D4s_v4, etc. | ||
| if strings.Contains(lowerSpecName, "_v2") || strings.Contains(lowerSpecName, "_v3") || | ||
| strings.Contains(lowerSpecName, "_v4") || strings.Contains(lowerSpecName, "_v5") || | ||
| strings.Contains(lowerSpecName, "s_") { |
There was a problem hiding this comment.
The version checking logic uses multiple string.Contains calls which could match unintended substrings. Consider using a more precise regex pattern or checking for the exact version suffix patterns to avoid false positives.
| strings.Contains(lowerSpecName, "s_") { | |
| // Use regex to match version suffixes at the end of the string | |
| versionSuffixRegex := regexp.MustCompile(`(_v2|_v3|_v4|_v5)$`) | |
| sTypeRegex := regexp.MustCompile(`^standard_d\d+s(_v\d+)?$`) | |
| if versionSuffixRegex.MatchString(lowerSpecName) || sTypeRegex.MatchString(lowerSpecName) { |
|
/approve |
No description provided.