Refactor lsblk, resolves issue re leading space in values #2907

[cellisten]

scan_disks() raised ValueError when lsblk values had leading spaces refactored to strip leading and trailing spaces

Fixes #2907

[cellisten]

Realised I hadn't added the updated test case in the right place. Now I have (hence the latest force push)

[phillxnet]

@cellisten Hello again, and thanks for this submission. Great to have more developer input.

It does look like what we have previously handled, and submissions to this area of the code must come with accompanying tests to reproduce an original issue, and thus prove the proposed fix. Take a look at the PR & linked issue I put against this PR's issue (I'll add in this PR's text shortly). I did leave links in @Hooverdan96 issue for this PR. There you should see how to run all the unit tests, it should be relatively simple to add your reproducer lsblk output as you presented on the forum to an existing test or to a new dedicated one. This code area is so low, it's always a worry to make changes.

I'm looking forward to having a closer look at what you have presented here however. And there is for-sure more robustness to be had. But I'd also like to try and keep stuff readable. Take a look at the issue linked PR which last fixed something similar and presented a new test. That should help to see who one runs/extends tests in this area.

Thanks again for this submission: as always much appreciated. And my apologies for not being able to take a proper look right-away.

[phillxnet]

@cellisten Opps we've had a comment overlap. Re:

Realised I hadn't added the updated test case in the right place. Now I have (hence the latest force push)

That modifies an existing test. That would potentially loose the regression that tested against. Have another look at the test structure, it's possible to add an entire new set (potentially constrained to smallest possible) to an existing test. That way we preserve the prior tests proven work. And add another set of data that may show a slightly different behaviour that it looks like you have approached (leading space). My prior comment hints as that being outstanding: so this is a nice addition.

See if you can add a new test data set, rather than modify an existing one. To preserve the original test. Which was for something a little different. Each test ideally tests for a specific failure.

Again apologies I can't focus just yet on this PR, but again this is much appreciated.

[cellisten]

The test I changed was supposed to check for leading spaces to begin with however... That's why I changed it.
Quoting from the test case:

Testing releases 5.0.9-0 & 5.0.10-0 failed lsblk output parsing when values began with a space.

However, the test case had no values that began with a space. If it had, the error would've been caught earlier
I will add a second test set instead if that is preferable :)

[phillxnet]

@cellisten Those comments are a little unclear addimittely. It failed before my changes with the indicated data. And passed there-after. To prove those particular changes. What we need is another set of data on that same test to initially fail with 5.0.14, and then pass post your changes. All other tests must also pass.

However, the test case had no values that began with a space. If it had, the error would've been caught earlier

And my comments acknowledge this very short-comming. We are defintely getting there on this one :) . I addressed a different failure detailed in the associated issue and that test proved what I was fixing to be fixed. We need another test to prove a similar fix presented here. I like what you've done by the way, from only another quick look.

Changing existing tests means we risk loosing what they were designed to fix. We recently had to completely re-vamp who we do this part of the code. So many things were changed. And we have a number of tests to cover this whole area. We need to ensure we have some continuity from how we worked before. The test, as may of our are, are intended to be extended with specific minimum reproducers. But often it's just real life data, which you have. Plug in your reproducer data and see it fail with another loop over the new set of data. And then we gain two instances of specific failure. Regression testing. Sometimes a test does not quite test what it was intended to test agreed. But that test failed then passed with the changes made to address (not) leading space - but trailing. We still failed on leading space: as indicated in my comment quotes in the associated issue to help focus the issue itself.

[phillxnet]

rockstor-core/src/rockstor/system/tests/test_osi.py

Lines 1653 to 1657 in 658291b

	# E.g 'VENDOR=" "' failed with: "ValueError: not enough values to unpack (expected 2, got 1)"
	# We use dictionary comprehension to parse lsblk output and rely on: 'line.strip().split('" ')'
	# this breaks when a disk property begins with a space.
	# See: https://github.com/rockstor/rockstor-core/issues/2853
	# Reported/encountered/addressed failure concerns values consisting solely of whitespace.

[cellisten]

No problem at all, I agree that existing regressions need to be tested. Was a bit naive in thinking that testing both in the same would be ok. I have now added an extra line in the existing test case, hope that is more suitable.

[cellisten]

Took the liberty to also remove the previous comment regarding leading whitespace that is no longer valid, hope that is ok.

[phillxnet]

I've got a few older pending issues to address before this one (replication & restore related etc), so I'm unlikely to get to review this properly for a little while. But others may beat me to it. And I'm hoping to get this all sorted (which it may be already) so we can have it as part of the 5.0.15-0 rpm release.

[cellisten]

Thank you very much for the fast feedback loop and nice welcome! As a new contributor it is heart warming to get this much contact so fast. Looking forward to any comments when do you do get time for a proper review!

[phillxnet]

@cellisten Thank for seeing to this outstanding failure, much appreciated.
My apologies for taking so long to get back to this.

I've now, finally, gotten around to doing a test rpm build and this in turn runs all our tests, and all looks good on that front. From the forum discussion re running tests etc we have the following doc entry: https://rockstor.com/docs/contribute/contribute.html#code-test
and:

cd /opt/rockstor/src/rockstor/
poetry run django-admin test -v 2

on an rpm based install, looks happy, ending in :

----------------------------------------------------------------------
Ran 285 tests in 36.991s

OK

Thank for the quick responses re earlier feedback by the way, and for the test work. My intention with expanding that test was to add an entire other system data set, with its own expected output, to the existing test loop, expressing the leading space reproducer: but the added drive entry approach is similar. We should still via failure log be able to see which parsing failed. I just like whole system data sets as they can end up being expressive of something that was not noticed before - and the more whole-system datasets the better. Hence us having loads of test data seemingly redundant: but never-the-less the end target is whole systems after all. This is also the reason for ending the mock output of lsblk as it actually ends, with the empty line. The existing data we had on this front was super useful for the last refactoring. And hopefully we can in-turn refactor later once all our OS bases can give us json to hand-off this tedious parsing to the underlying command itself. And then bring in via json to dict.

On a further note re tests/unit-tests nomenclature, as per a Brian Okken podcast comment/advice, I think we should now just call these 'tests': i.e. what even is a 'unit' these days. But we do still try to keep them small. We hope in-time to have what I regard as more meaningful end-to-end tests, using openQA: but again all down to developer resources. We once, years ago now, has a selenium test set. But that has long since faded into an unmaintained state and I don't want to depend on Java for any part of our infrastructure.

I'm chuffed to have this fix in, as it bothered me that I'd not found a solution when last there - and this does look to be a relatively elegant approach. And thanks for the in-code comments: much appreciated. This code recently received a major re-vamp so some new bugs were inevitable. But as-way it had become almost unworkable obscure/convoluted. I look forward to our underlying tools being able to present us with JSON, but all in good time I guess.

Thanks again for all your efforts here, and do keep an eye out on the forum for more bugs in this area as there are few folks who dabble this low in the disk management, and the more the better really.

This patch will end-up being released in our pending 5.0.15-0 rpm.

@Hooverdan96 Thanks for helping with the creation of the original issue here.