Re: [pve-devel] [PATCH storage 1/2] smartctl: remove to-be-replaced disk_tests

[Oguz Bektas <o.bektas@proxmox.com>]

On Mon, May 10, 2021 at 02:15:37PM +0200, Dominik Csapak wrote:
> hi,
> 
> sorry for the long wait on a review, but better
> late than never ;)
> 
> so one high level comments:
> 
> why do you remove all disk tests ??
> they did not only test the smart output, but also the
> usage, ssd/hdd detection, vendor/model etc...
> 
> i understand that you do not have the original disks
> for the smartctl -j output, but it should not be
> *that* hard to fake it ;)

at the time i couldn't figure out a way to keep all the tests working
while also adding the json changes, so for proof of concept i went for
the simpler option.

thomas thought it would also be acceptable to incrementally
add the new tests over time, making sure that everything works with the
new changes.

i have to admit i haven't really tried to fake the outputs, mainly
because there were a lot of disk types that produce different kind of
results (one issue stefan noticed at an earlier version was that his
disk's smart result returned an array with the temperature sensors,
while the disks i've tested did not have these.)

all these vendor-specific quirks make it really hard to produce reliable
tests without the real hardware.

> 
> and even if it is, please change the tests so that
> only the smart part is not tested anymore, e.g. with a flag
> in the test itself....

that can be an option, indeed
> 
> so please find a way to keep the tests (as much as you can)
> 
> i'll comment more on the second patch
> 
>