tests: don't enforce NS management support for local testing#3226
tests: don't enforce NS management support for local testing#3226igaw wants to merge 2 commits intolinux-nvme:masterfrom
Conversation
|
@MaisenbacherD what do you think about this? |
There was a problem hiding this comment.
Pull request overview
This PR updates the NVMe Python test framework to avoid enforcing Namespace (NS) management during setup/teardown when running on devices that don’t support NS management, enabling a larger subset of tests to run in such environments.
Changes:
- Add controller feature detection for NS management support via OACS parsing.
- Conditionally skip default namespace create/attach logic in
setUp()andtearDown()when unsupported.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ns_mgmt_supported = bool(oacs & (1 << 3)) | ||
| ns_attach_supported = bool(oacs & (1 << 4)) |
There was a problem hiding this comment.
get_ns_mgmt_support() is interpreting OACS bit 4 as “namespace attach”, but in this repo OACS[4] is used for Device Self-test and OACS[3] is “NS Management and Attachment” (combined). This will incorrectly report ns_mgmt_supported as false on controllers that support NS mgmt/attach but don’t support self-test, causing namespace setup/teardown to be skipped. Please base the NS mgmt+attach detection on OACS bit 3 only (and adjust the returned fields accordingly).
| ns_mgmt_supported = bool(oacs & (1 << 3)) | |
| ns_attach_supported = bool(oacs & (1 << 4)) | |
| # OACS bit 3 indicates combined Namespace Management and Attachment support. | |
| ns_mgmt_supported = bool(oacs & (1 << 3)) | |
| ns_attach_supported = ns_mgmt_supported |
tests/nvme_test.py
Outdated
| self.validate_pci_device() | ||
| self.create_and_attach_default_ns() | ||
| ns_mgmt = self.get_ns_mgmt_support() | ||
| self.ns_mgmt_supported = ns_mgmt["ns_mgmt"] and ns_mgmt["ns_attach"] |
There was a problem hiding this comment.
self.ns_mgmt_supported is computed as ns_mgmt["ns_mgmt"] and ns_mgmt["ns_attach"], but the current OACS parsing treats bit 4 as “ns_attach”. In this codebase OACS bit 4 corresponds to Device Self-test and OACS bit 3 represents “NS Management and Attachment” together, so this AND will incorrectly disable namespace setup on some controllers. After fixing the OACS bit interpretation, consider deriving this flag directly from the combined capability to avoid mismatches.
| self.ns_mgmt_supported = ns_mgmt["ns_mgmt"] and ns_mgmt["ns_attach"] | |
| self.ns_mgmt_supported = bool(ns_mgmt.get("ns_mgmt_and_attach", ns_mgmt.get("ns_mgmt"))) |
| "ERROR : nvme list-ctrl could not find ctrl") | ||
| return str(json_output['ctrl_list'][0]['ctrl_id']) | ||
|
|
||
| def get_ns_mgmt_support(self): |
There was a problem hiding this comment.
The new get_ns_mgmt_support() helper is missing a docstring, while surrounding helpers in this file consistently document purpose/args/returns. Please add a brief docstring (and ideally note which OACS bit(s) are being checked) to keep the file’s documentation style consistent.
| def get_ns_mgmt_support(self): | |
| def get_ns_mgmt_support(self): | |
| """ Parse controller OACS field to determine namespace management support. | |
| - Args: | |
| - None | |
| - Returns: | |
| - Dictionary with: | |
| - "raw": integer OACS value. | |
| - "ns_mgmt": True if OACS bit 3 (Namespace Management) is set. | |
| - "ns_attach": True if OACS bit 4 (Namespace Attachment) is set. | |
| """ |
MaisenbacherD
left a comment
MaisenbacherD
left a comment
There was a problem hiding this comment.
Had a few comments. This is a good idea! :)
The tests framework added supported and dependency on NS management for clean setups. Though this prevents to run part of the tests suites on devices which do no support NS management. Add feature detection and skip the setup/tear down code for the NS setup if the device doesn't support this. Signed-off-by: Daniel Wagner <wagi@kernel.org>
These tests will fail if the controller does not support namespace management, thus skip them. Signed-off-by: Daniel Wagner <wagi@kernel.org>
|
I am running this against a qemu emulated nvme disk: $ meson test -C .build
[...]
52/76 nvme-cli - nvme_attach_detach_ns_test OK 1.49s
53/76 nvme-cli - nvme_create_max_ns_test OK 1.62s
54/76 nvme-cli - nvme_format_test OK 1.78s
55/76 nvme-cli - nvme_error_log_test OK 2.01s
56/76 nvme-cli - nvme_fw_log_test OK 1.94s
57/76 nvme-cli - nvme_flush_test OK 2.18s
58/76 nvme-cli - nvme_id_ctrl_test OK 2.05s
59/76 nvme-cli - nvme_id_ns_test OK 2.08s
60/76 nvme-cli - nvme_compare_test OK 2.62s
61/76 nvme-cli - nvme_read_write_test OK 2.21s
62/76 nvme-cli - nvme_smart_log_test OK 2.34s
63/76 nvme-cli - nvme_writeuncor_test FAIL 2.80s exit status 1
>>> MALLOC_PERTURB_=167 PATH=/home/wagi/work/nvme-cli/.build-debian:/usr/bin:/usr/sbin /usr/bin/nose2 --verbose --start-dir /home/wagi/work/nvme-cli/.build-debian/tests nvme_writeuncor_test
64/76 nvme-cli - nvme_verify_test OK 2.43s
65/76 nvme-cli - nvme_dsm_test OK 2.59s
66/76 nvme-cli - nvme_copy_test FAIL 2.74s exit status 1
>>> MALLOC_PERTURB_=243 PATH=/home/wagi/work/nvme-cli/.build-debian:/usr/bin:/usr/sbin /usr/bin/nose2 --verbose --start-dir /home/wagi/work/nvme-cli/.build-debian/tests nvme_copy_test
67/76 nvme-cli - nvme_get_lba_status_test OK 2.56s
68/76 nvme-cli - nvme_lba_status_log_test OK 2.69s
69/76 nvme-cli - nvme_writezeros_test OK 3.16s
70/76 nvme-cli - nvme_get_features_test FAIL 4.97s exit status 1
>>> MALLOC_PERTURB_=87 PATH=/home/wagi/work/nvme-cli/.build-debian:/usr/bin:/usr/sbin /usr/bin/nose2 --verbose --start-dir /home/wagi/work/nvme-cli/.build-debian/tests nvme_get_features_test
71/76 nvme-cli - nvme_ctrl_reset_test OK 2.91s
[...]I think it starts to uncover bugs. And we need to figure out how to forward the 'skipped' info to meson :) |
|
okay, I've bitten the bullet: #3233 |
The tests framework added supported and dependency on NS management for clean setups. Though this prevents to run part of the tests suites on devices which do no support NS management.
Add feature detection and skip the setup/tear down code for the NS setup if the device doesn't support this.