Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add verify_supports_nvme #3122

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add verify_supports_nvme #3122

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 66 additions & 1 deletion microsoft/testsuites/core/storage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,24 @@
from lisa.node import Node
from lisa.operating_system import BSD, Posix, Windows
from lisa.schema import DiskControllerType, DiskOptionSettings, DiskType
from lisa.search_space import SetSpace
from lisa.sut_orchestrator import AZURE
from lisa.sut_orchestrator.azure.common import AzureNodeSchema, VhdSchema
from lisa.sut_orchestrator.azure.features import AzureDiskOptionSettings, AzureFileShare
from lisa.sut_orchestrator.azure.platform_ import AzurePlatform
from lisa.sut_orchestrator.azure.tools import Waagent
from lisa.tools import Blkid, Cat, Dmesg, Echo, Lsblk, Mount, NFSClient, Swap, Sysctl
from lisa.tools import (
Blkid,
Cat,
Dmesg,
Echo,
Lsblk,
Lsinitrd,
Mount,
NFSClient,
Swap,
Sysctl,
)
from lisa.tools.blkid import PartitionInfo
from lisa.tools.journalctl import Journalctl
from lisa.tools.kernel_config import KernelConfig
Expand Down Expand Up @@ -285,6 +299,57 @@ def verify_scsi_disk_controller_type(self, node: RemoteNode) -> None:
def verify_nvme_disk_controller_type(self, node: RemoteNode) -> None:
self._verify_disk_controller_type(node, DiskControllerType.NVME)

@TestCaseMetadata(
description="""
This test verifies the nvme drivers are either built in or present in initramfs.
This test can be used to verify an image before it is tagged as NVMe capable.
squirrelsc marked this conversation as resolved.
Show resolved Hide resolved
""",
priority=1,
requirement=simple_requirement(
supported_platform_type=[AZURE],
),
)
def verify_supports_nvme(self, node: RemoteNode, environment: Environment) -> None:
has_nvme_core = node.tools[KernelConfig].is_built_in("CONFIG_NVME_CORE") or (
node.tools[KernelConfig].is_built_as_module("CONFIG_NVME_CORE")
and node.tools[Lsinitrd].has_module("nvme-core.ko")
)
has_nvme = node.tools[KernelConfig].is_built_in("CONFIG_BLK_DEV_NVME") or (
node.tools[KernelConfig].is_built_as_module("CONFIG_BLK_DEV_NVME")
and node.tools[Lsinitrd].has_module("nvme.ko")
)

# Pass condition
if has_nvme_core and has_nvme:
return

assert isinstance(environment.platform, AzurePlatform)
node_runbook: AzureNodeSchema = node.capability.get_extended_runbook(
AzureNodeSchema, AZURE
)
image = node_runbook.image
# Failure conditions
if image and not isinstance(image, VhdSchema):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VHD should be able to set in the runbook to specify it supports nvme.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't rely on people manually specifying things in the runbook because it causes confusion. This means we need a good default value. If we don't include NVMe in VHD default, people will be confused why they can't deploy VHD with NVMe disk controller type. On the other hand, if default value has NVMe (as it currently does) then there will be confusing failures on this test case. The test case will assume VHD supports NVMe by default when this isn't necessarily true.
If someone wants to check if their VHD supports VHD all they have to do is confirm that this test case is pass and not skip. VHDs aren't tagged with any info so there shouldn't be an expectation that LISA will check if they are correctly tagged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you list the situations in a table? With current logic, the test case is always disabled for vhd.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marketplace SIG / CGI VHD
Has drivers PASS PASS PASS
Missing drivers, not tagged SKIP SKIP SKIP
Missing drivers, tagged capable FAIL FAIL VHD is never tagged

Copy link
Collaborator Author

@kamalca kamalca May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main purpose of this test case is "Tag the image if and only if the test case passes" to be used before the image is published publicly to the marketplace. We don't really care about the skip vs. failure condition unless an image published in the marketplace is tagged but doesn't have drivers. This test case does not really care about VHDs because they aren't tagged.

Copy link
Member

@squirrelsc squirrelsc May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It maybe a case, that we need to verify a VHD is good with the tag before it's published. If the runbook is not modified, the vhd can be skipped to verify. It needs to be capable to verify vhd support that or not by adding tag in runbook. How about create unsupported NVMe capable for vhd by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is how I see the options

Deploying VHD on NVMe Running verify_support_nvme
Default support NVMe Works! Why is test case failing? :(
Default don't support NVMe Doesn't deploy :( Catches Failures

I could add a tag which it separate from disk_controller_type for the image, but it would be quite niche use case. It would be confusing and I don't think it would really be used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add a test variable for the test case, can it solve the problem?

image.load_from_platform(environment.platform)
if isinstance(image.disk_controller_type, DiskControllerType):
assert DiskControllerType.NVME != image.disk_controller_type, (
"Image is tagged as NVMe capable, but NVMe drivers are missing."
f"nvme-core: {has_nvme_core}, nvme: {has_nvme}"
)
elif isinstance(image.disk_controller_type, SetSpace):
assert (
DiskControllerType.NVME not in image.disk_controller_type.items
), (
"Image is tagged as NVMe capable, but NVMe drivers are missing."
f"nvme-core: {has_nvme_core}, nvme: {has_nvme}"
)

# Skip if image is not tagged as NVMe capable
raise SkippedException(
"NVME drivers are not present. "
f"nvme-core: {has_nvme_core}, nvme: {has_nvme}"
)

@TestCaseMetadata(
description="""
This test will verify that identifier of root partition matches
Expand Down
Loading