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

Do not fail if 'total data bytes written' is zero #71

Merged
merged 5 commits into from
May 26, 2023

Conversation

Heidistein
Copy link
Contributor

It appears that complete unused NVME drives (at least SAMSUNG MZQL27T6HBLA-00A07) report just '0' if there is nothing ever written to the drive. With several hotspares this breaks. I have no idea what if ever anything else than '0' is returned, but this catches that too.

# for I in {0..6}; do smartctl -x /dev/nvme$I -d nvme| grep -F 'Data Units Written'; done
Data Units Written:                 10,478,811 [5.36 TB]
Data Units Written:                 2,938,151 [1.50 TB]
Data Units Written:                 2,943,329 [1.50 TB]
Data Units Written:                 0
Data Units Written:                 0
Data Units Written:                 0
Data Units Written:                 0

I'f like a little guidance on writing a unittest for this (and the PR69). Should I give the smartctl -x output?

It appears that complete unused NVME drives (at least SAMSUNG MZQL27T6HBLA-00A07) report just '0' if there is nothing ever written to the drive. With several hotspares this breaks. I have no idea what if ever anything else than '0' is returned, but this catches that too.

```
# for I in {0..6}; do smartctl -x /dev/nvme$I -d nvme| grep -F 'Data Units Written'; done
Data Units Written:                 10,478,811 [5.36 TB]
Data Units Written:                 2,938,151 [1.50 TB]
Data Units Written:                 2,943,329 [1.50 TB]
Data Units Written:                 0
Data Units Written:                 0
Data Units Written:                 0
Data Units Written:                 0
```

I'f like a little guidance on writing a unittest for this (and the PR69). Should I give the smartctl -x output?
I am an idiot, sorry.
@ralequi ralequi changed the base branch from master to develop May 23, 2023 08:31
@Heidistein Heidistein changed the title Do not failis total data bytes written is zero Do not fail if 'total data bytes written' is zero May 23, 2023
@ralequi
Copy link
Collaborator

ralequi commented May 23, 2023

For the tests, you can add a folder like tests/singletests/nvme_8

In that folder, you have to add a device.json and every call pysmart does to smartctl.

The device.json:

This file must have the following data:

{
    "name": "/dev/nvme0"
}

This will indicate tests that your device is called /dev/nvme0. However, it don't know its interface type so will try to perform a -d test call first to identify it. If you want to skip that call you can simply indicate the interface type like:

{
    "name": "/dev/nvme0",
    "interface": "nvme"
}

Nonetheless, this won't be enough to pass any test, just to interpret the folder data. Tests checks that pysmart reads correctly the data and for that there is another dictionary-field in device.json called "values": . You can check for examples and try to fill it on your own, but seriously, I don't recommend that.

Once you have filled the required fields adobe (name and maybe interface) you can run the file gen_devicejson.py located in the tests folder. This python will re-write the device.json accordingly with what pysmart interprets at some moment. It's on your side to manually verify (especially previous any commit) that the contents of the device.json is correct (and don't have a bunch of incorrect data) also, that other device.json from other tests haven't been incorrectly modified.

Output files

On each test, there should be a file for each virtually smartctl call. The format is simple:

  • For a call such as smartctl -d nvme --all /dev/nvme0 just replace any problematic character (such as whitespaces and vertical slashes /) with horizontal slash _.
  • For the last example, the file that we expect on the dataset should be _-d_nvme_--all__dev_nvme0

If you don't know which files do you require, you can try to run your test. If you need anything else, the test will raise an exception indicating that a file is required. This will generate a message with the expected filename but also the underlying expected smartctl call

ralequi added a commit that referenced this pull request May 23, 2023
@ralequi
Copy link
Collaborator

ralequi commented May 23, 2023

I've added the previous comment to a readme under tests folder: https://github.com/truenas/py-SMART/tree/develop/tests so it may help for future developers.

Please, ask me any question (and feel free to fix that readme if there is something unclear/wrong). Glad to help

@ralequi ralequi self-assigned this May 23, 2023
@Heidistein
Copy link
Contributor Author

I am terribly sorry... I have no idea how to get gen_devicejson.py to work.
I am out of ideas, about seven hours later I still am stuck at import errors... I think this is not going to work.

Just close this utter failure pull request of me. I'll patch and package it locally.

@Heidistein
Copy link
Contributor Author

So sorry to waste your time.

@ralequi
Copy link
Collaborator

ralequi commented May 24, 2023

Come on boy, helping devs it's not a waste of time ;-)
Don't be sad!

On the root path of py-SMART, you should be able to run python -m tests.gen_devicejson --folder tests/dataset/singletests/ --updateSubfolders

of course, a previous pip install .[dev] is recommended if you want to install the deps. Nothing else should be required

@ralequi
Copy link
Collaborator

ralequi commented May 24, 2023

If after a couple of tries you still couldn't add the tests, just put here your whole outputs and I'll include them. Your PR seems justified.
Good luck!

@ralequi
Copy link
Collaborator

ralequi commented May 26, 2023

Hi @Heidistein

I just added some artificial tests.

Thanks for your contrib and don't be sad! Is everybody work to make opensource projects better ;-)

See you soon with more ideas, issues & PRs!

@ralequi ralequi merged commit f0b2c03 into truenas:develop May 26, 2023
@Heidistein
Copy link
Contributor Author

Heidistein commented May 26, 2023 via email

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

Successfully merging this pull request may close these issues.

2 participants