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

Updated code for Device creation for nvme drives following smartmontools update #76

Merged
merged 13 commits into from
Oct 25, 2023

Conversation

NicholasCJL
Copy link
Contributor

Fixed regex for this issue #75 for smartmontools release 7.4

Also updated testentry.py and device.py to capture and show new output format for nvme self-test logs.

@ralequi ralequi self-assigned this Oct 17, 2023
@ralequi
Copy link
Collaborator

ralequi commented Oct 17, 2023

This PR --as is-- breaks retrocompatibility.
We have to add new tests and ensure it works for current 7.4 and older versions

@NicholasCJL
Copy link
Contributor Author

Yep, realised the new regex will break the old one. I'll modify the code so it stays backwards compatible with older versions.

What new tests should I add? I'm not sure how to replicate the new output from the new version of smartmontools.

@ralequi
Copy link
Collaborator

ralequi commented Oct 17, 2023

I've issues explaining how to add tests, so don't worry about that yet.
Simply make the current tests work and I'll add the new test-case

@NicholasCJL
Copy link
Contributor Author

Got it, will work on the code in a separate branch until the tests pass so I stop triggering reruns of the tests.

@ralequi
Copy link
Collaborator

ralequi commented Oct 17, 2023

after some issues with gh tool, i've pushed f76917d to your branch. It should have a test for the new 7.4 version.

@ralequi ralequi linked an issue Oct 17, 2023 that may be closed by this pull request
@NicholasCJL
Copy link
Contributor Author

As the tests are currently, the test_device_iface_attributes test will fail on the assertion dev_if_attributes == if_attributes due to the new attribute seg being reported and not accounted for in the old nvme tests.

There are basically two options here, either modify the old tests to report seg as None, or not report seg to maintain backwards compatibility despite the new smartctl version reporting it as an entry. What should we do here?

@ralequi ralequi merged commit 82e8d0c into truenas:develop Oct 25, 2023
12 checks passed
@ralequi
Copy link
Collaborator

ralequi commented Oct 25, 2023

Thank you @NicholasCJL !

Yeah, setting the seg = null is the correct way, as is a new field that would be in any nvme tests from now on.

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.

pySMART fails to create Device for nvme drives
2 participants