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.

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

NVMe and resource disc testcase fixes #3432

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SRIKKANTH
Copy link
Collaborator

@SRIKKANTH SRIKKANTH commented Sep 20, 2024

NVMe and resource disc testcase fixes

  1. Added new testcase "Nvme.verify_nvme_function_unpartitioned".
  2. NVMe test case fixes for disc controller type NVMe.
  3. Resource disc testcases fixes for VMs with Local NVMEs.
  4. "Nvme.verify_nvme_fstrim" - use fstrim test file size based on available space.

@SRIKKANTH
Copy link
Collaborator Author

SRIKKANTH commented Sep 20, 2024

This PR has a dependency on #3420

Please merge #3420 before accepting this PR so that lspci changes are part of different PR. and not clubbed with NVMe changes

@SRIKKANTH SRIKKANTH force-pushed the smyakam/nvme_test_fixes_15_09_2024 branch from c7e4a17 to bd7d0e2 Compare September 24, 2024 04:20
@SRIKKANTH SRIKKANTH marked this pull request as draft October 23, 2024 03:28
@SRIKKANTH SRIKKANTH force-pushed the smyakam/nvme_test_fixes_15_09_2024 branch 2 times, most recently from f76263a to 5855bb7 Compare October 23, 2024 03:42
@SRIKKANTH SRIKKANTH force-pushed the smyakam/nvme_test_fixes_15_09_2024 branch 5 times, most recently from b402609 to b29f14e Compare November 12, 2024 11:39
@SRIKKANTH SRIKKANTH marked this pull request as ready for review November 12, 2024 11:45
@SRIKKANTH SRIKKANTH changed the title NVMe testcase fixes for ASAP VMs NVMe and resource disc testcase fixes Nov 12, 2024
lisa/features/nvme.py Outdated Show resolved Hide resolved
f"'WARNING: THIS IS A TEMPORARY DISK' should be present in {file_path}",
).contains("WARNING: THIS IS A TEMPORARY DISK")
node_disc = node.features[Disk]
resource_disks = node_disc.get_resource_disks()
Copy link
Member

Choose a reason for hiding this comment

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

Add a method in Disk feature, called like get_disk_controller_type, and it accept mount point as a parameter. Please also reduce duplicate code with existing get_os_disk_controller_type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed it.

Copy link
Member

Choose a reason for hiding this comment

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

Please refine get_os_disk_controller_type to use the same logic. You can see the get_os_disk_controller_type contains logic for FreeBSD too. If you implement another get_resource_disk_type. The FreeBSD needs to fix the new failure again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added new method "get_disc_type" to re-use in both get_os_disk_controller_type and get_resource_disk_type

Copy link
Member

Choose a reason for hiding this comment

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

Please use disk instead of disc to keep consistent.

resource_disks = node_disc.get_resource_disks()
if not resource_disks:
raise LisaException("Resource disk not found")
if "nvme" in resource_disks[0]:
Copy link
Member

Choose a reason for hiding this comment

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

As above comments, use the type enum, instead of checking by string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Introduced new constants for NVMe and SCSI resource disc types.

Copy link
Member

Choose a reason for hiding this comment

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

New constants are right, but enum is needed too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed as suggested

@SRIKKANTH SRIKKANTH marked this pull request as draft November 20, 2024 12:11
NVMe and resource disc testcase fixes

1. Added new testcase "Nvme.verify_nvme_function_unpartitioned".
2. NVMe test case fixes for disc controller type NVMe.
3. Resource disc testcases fixes for VMs with Local NVMEs.
4. "Nvme.verify_nvme_fstrim" - use fstrim testfile size based on available space.
@SRIKKANTH SRIKKANTH force-pushed the smyakam/nvme_test_fixes_15_09_2024 branch from cda3e20 to 0756836 Compare November 20, 2024 19:32
@SRIKKANTH SRIKKANTH marked this pull request as ready for review November 20, 2024 19:42
return []

def get_resource_disk_type(self) -> str:
return ""
Copy link
Member

Choose a reason for hiding this comment

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

It should be RESOURCE_DISK_TYPE_SCSI by default. The empty string is confusing. Actually, an enumeration is better to prevent misuse of a type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants