nvme basic test fix for disk count match#4437
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the NVMe test suite to validate Azure NVMe disk counts using Azure SKU capabilities (via NvmeSettings.disk_count) instead of a hardcoded vCPU-based heuristic, improving correctness across VM families.
Changes:
- Replace the vCPU/8-based expected NVMe count logic with
NvmeSettings.disk_countfrom feature settings. - Remove unused
math/Lscpuusage and update related test descriptions/messages.
| # Copyright (c) Microsoft Corporation. | ||
| # Licensed under the MIT license. | ||
| import math | ||
| from typing import cast |
There was a problem hiding this comment.
The PR description content appears to be entirely inside an HTML comment in the PR template, so reviewers won’t see the rationale or the before/after behavior. Please add a visible description explaining what this PR changes and why.
| 4. Azure platform only, nvme devices count should equal to | ||
| actual vCPU count / 8. | ||
| expected disk count from SKU capabilities. |
There was a problem hiding this comment.
This step says "nvme devices count" but the implementation uses the namespace list length as the count being asserted later. Please clarify the wording to refer to namespaces/disks (e.g., /dev/nvme*n*) to avoid confusion about controller vs namespace counts.
| # 4. Azure platform only, nvme devices count should equal to | ||
| # actual vCPU count / 8. | ||
| # expected disk count from SKU capabilities. |
There was a problem hiding this comment.
This comment says "nvme devices count" but the assertion below is on the NVMe namespaces list length. Please update the comment to match what’s being checked (namespaces/disks vs controllers/devices).
| expected_count = nvme_settings.disk_count | ||
| assert_that(nvme_namespace).described_as( | ||
| "nvme devices count should be equal to [vCPU/8]." | ||
| f"nvme devices count should be equal to expected disk count" |
There was a problem hiding this comment.
The assertion is checking nvme_namespace length, but the described_as message says "nvme devices count". Please align the message with the actual subject being asserted (namespaces/disks), so failures are easier to interpret.
| f"nvme devices count should be equal to expected disk count" | |
| f"nvme namespace count should be equal to expected disk count" |
kanchansenlaskar
changed the title
✅ AI Test Selection — PASSED5 test case(s) selected (view run) Marketplace image: canonical 0001-com-ubuntu-server-jammy 22_04-lts-gen2 latest
Test case details
|
Description
Replaced import math with from typing import cast (math is no longer needed).
Replaced the hardcoded vCPU/8 formula in _verify_nvme_disk() step 4 with a query to the Azure SKU capabilities:
Before: expected_count = math.ceil(thread_count / 8)
After: expected_count = cast(NvmeSettings, node.features[Nvme].get_settings()).disk_count
The Azure platform already computes the correct NVMe disk count from NvmeDiskSizeInMiB // NvmeSizePerDiskInMiB in the SKU capabilities and stores it in NvmeSettings.disk_count. This is the authoritative source for expected disk count and works correctly across all VM families (including v6-series with different NVMe-to-vCPU ratios).
Improved the assertion message to include the actual expected count and its source for easier debugging.
Related Issue
Type of Change
Checklist
Test Validation
Key Test Cases:
verify_nvme_basic
Impacted LISA Features:
Tested Azure Marketplace Images:
Test Results