search

truenas / py-SMART

Wrapper for smartctl (smartmontools)
GNU Lesser General Public License v2.1
79 stars 35 forks source link

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

Closed NicholasCJL closed 1 year ago

NicholasCJL commented 1 year ago

Fixed regex for this issue https://github.com/truenas/py-SMART/issues/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 commented 1 year ago

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

NicholasCJL commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

ralequi commented 1 year ago

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

NicholasCJL commented 1 year ago

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 commented 1 year ago

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.