Add option for ZSTD compression #2618

[StephenBrown2]

What a dev wants, a dev gets... At least when he can spare some time away from his crying child. Don't worry, she's OK.

Implemented the ZSTD option by hunting through and finding all the references to zlib and adding a zstd option next to it. I hope I found all the right places, that tooltip text sure could be a constant somewhere... I took the text from the btrfs rtd page: https://btrfs.readthedocs.io/en/latest/Compression.html but if we want to ahem compress it, that's fine as well.

Anywho, the results are good for the pool:

and shares:

Closes #2618

[phillxnet]

Hello again @StephenBrown2 This is a very welcome addition, and as per issue comments quite timely.

that tooltip text sure could be a constant somewhere...

Yes, that and a hundred other bits and bobs :) : focused pull requests always welcome.

There is however a little anomaly on the branch front with this pull request.

Would you mind re-doing the pull request against testing. That is where we do our development these days and on the timeliness front we have a little hick-up re Leap 15.3. It is end-of-life but in the next couple of days we are about to release our last rpm for that OS, and as you stated in the issue - it doesn't make the cut, kernel wise. In master branch (now used for our stable maintenance see: https://github.com/rockstor/rockstor-core/milestone/21) we try to do only fixes. And give the above 15.3 last-push, this would end up bringing incompatibilities. Where as our new testing phase/branch development has already begun and this would site very nicely there. And once I have some back-end odd-and-ends resolved we will begin again on publishing our next round/phase of testing - ergo new stuff, with the assumption that they will only be released for 15.4 and newer kernels.

Are you game? The two branches have diverged quite a bit given we have already begun our new testing phase. I know it's a little more work but this would be ideal for the testing branch - and we plan to have our next testing phase a great deal shorter than it ended up being last time.

Nice to hear from you again by the way.

[StephenBrown2]

Ah no worries, I'm no stranger to rebasing. I should have based it on testing to begin with, my bad.

Leap 15.3 is actually well within the kernel needs as well, as it ships with 5.3, and the minimum version for zstd is 4.14.

I'll get that rebased today or tomorrow hopefully and update this PR when done. Glad to be back contributing, though I probably still won't be regular for a while yet. :-)

[StephenBrown2]

Rebased on testing, should be all good now! I hope it makes the cut for your last 15.3 rpm!

[phillxnet]

Leap 15.3 is actually well within the kernel needs as well, as it ships with 5.3, and the minimum version for zstd is 4.14.

OK thank, that good to know - my mistake. But yes, I think we should still go with popping this into testing so thanks for your efforts here.

One last request before review - if you would: could you possibly squash your commits as we have the irrelevant history of your local merge that will very likely end up confusing me in the near or other future? Again my apologies for the fuss but it really helps to have only local commit history - which is plenty complex enough as it is :).

Oh and thanks for the test coverage here: we can always use more testing: much appreciated.

I hope it makes the cut for your last 15.3 rpm!

I don't think it will, given the next testing rpm is to be 15.4 onwards. And we are literally at the very end of our last testing/new stable run. But our following howto may be of interest re in-place update:

https://rockstor.com/docs/howtos/15-3_to_15-4.html

Hope that helps and my apologies for the run-around here. Otherwise this looks good on initial inspection - plus in the testing branch/channel, especially earlier on, we can be a little more adventurous.

[StephenBrown2]

One last request before review - if you would: could you possibly squash your commits as we have the irrelevant history of your local merge that will very likely end up confusing me in the near or other future? Again my apologies for the fuss but it really helps to have only local commit history - which is plenty complex enough as it is :).

Sure thing. I appreciate a clean commit history, and that was just to get the branch up to date with testing, habit from work.

Rebased again and pushed now, should be g2g. And hey, it'll give users a good reason to update to 15.4!

[phillxnet]

@StephenBrown2 We have a 5.0.0-0 testing release coming up very soon (beginning of next testing phase) as a result of our last 2 GitHub milestones. Directly there after I'm planning to pop this into the following release as we have a shiny new GitHub Action cascade by @FroggyFlox that needs some exercise (read me getting to grips with it) so a couple of quick releases in testing should work all around here hopefully.

[phillxnet]

@StephenBrown2 Had a quick look at this pr and we have 2 outstanding tests fail unfortunately:

poetry run django-admin test
.........................................................................................................F...............................F...................................................................................................................
======================================================================
FAIL: test_mount_options (rockstor.storageadmin.tests.test_pools.PoolTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/buildbot/worker/Poetry-Build-on-Leap15-4/rpmbuild/rockstor-core-5.0.0-2619/src/rockstor/storageadmin/tests/test_pools.py", line 680, in test_mount_options
    self.assertEqual(response.data[0], e_msg)
AssertionError: "compress-force is only allowed with ('lzo', 'zlib', 'zstd', 'no')." != "compress-force is only allowed with ('lzo', 'zlib', 'no')."
- compress-force is only allowed with ('lzo', 'zlib', 'zstd', 'no').
?                                                     --------
+ compress-force is only allowed with ('lzo', 'zlib', 'no').
======================================================================
FAIL: test_create (rockstor.storageadmin.tests.test_shares.ShareTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/buildbot/worker/Poetry-Build-on-Leap15-4/rpmbuild/rockstor-core-5.0.0-2619/src/rockstor/storageadmin/tests/test_shares.py", line 271, in test_create
    self.assertEqual(response3.data[0], e_msg2)
AssertionError: "Unsu[20 chars]algorithm (invalid). Use one of ('lzo', 'zlib', 'zstd', 'no')." != "Unsu[20 chars]algorithm (invalid). Use one of ('lzo', 'zlib', 'no')."
- Unsupported compression algorithm (invalid). Use one of ('lzo', 'zlib', 'zstd', 'no').
?                                                                         --------
+ Unsupported compression algorithm (invalid). Use one of ('lzo', 'zlib', 'no').
----------------------------------------------------------------------
Ran 253 tests in 23.449s
FAILED (failures=2)

So can't pop in for now, as is, unfortunately. Given our rpm builds require all existing tests to pass.

[StephenBrown2]

Shucks. Wish I'd seen this earlier. Hopefully fixed those tests, so maybe it'll make it into 5.0.2.

[StephenBrown2]

On that note, what all is needed to set up a test environment? Perhaps I could add a GitHub action to automatically run the tests on PRs?

[FroggyFlox]

Hi @StephenBrown2,

On that note, what all is needed to set up a test environment?

Very quickly on that one: we should have all the details you seek in our docs:

• section for developers: https://rockstor.com/docs/contribute/contribute.html#developers
• sub-section about running our test suite: https://rockstor.com/docs/contribute/contribute.html#code-test

Hope that helps,

[phillxnet]

@StephenBrown2 Thanks for seeing to those tests, I'll do another run here to see if all is OK.

so maybe it'll make it into 5.0.2.

That was my hope. But we would like this in a single commit again if possible. But to avoid the back-and-forth doing a local test run would be good, just in case there's something else we've missed in them.

Looks like @FroggyFlox has answered your developer setup question already.

Perhaps I could add a GitHub action to automatically run the tests on PRs?

Take a look as what we suggest folks do re development and see what you think, always good to have more eyes as it were.

Cheers.

[StephenBrown2]

@FroggyFlox Yes I was looking over that, but it appears that the docs assume you're running tests directly in the VM you're using.

To summarize, it looks like the needed steps in a generic runner would be:

1. Checkout the branch (into $WORKSPACE)
2. sh $WORKSPACE/build.sh
3. sudo systemctl start postgresql.service
4. cd $WORKSPACE/src/rockstor
5. export DJANGO_SETTINGS_MODULE=settings
6. poetry run django-admin test -v 2

(I'm not sure if the initrock script or the rockstor service are needed just for running tests)

@phillxnet Thanks, I've squashed and pushed again.

But we would like this in a single commit again if possible.

GitHub PRs can be configured to always squash when merging, and that is my preference as well, such that one does not always have to remember to squash when pushing. Just uncheck all other options:

And you may also want to enable linear history enforcement on the main and testing branches, as it appears that is the goal:

But to avoid the back-and-forth doing a local test run would be good, just in case there's something else we've missed in them.

I don't currently have a python environment set up, but if I get some time I'll do what I can. Working in the buildVM is a little constricting, and I'd like to get it automated if possible.

[StephenBrown2]

@FroggyFlox I got the tests passing in the buildvm following the instructions. They definitely require postgres, but django creates their own test database, so I'm thinking initrock and a running rockstor service aren't necessary.

[phillxnet]

They definitely require postgres, but django creates their own test database

Only if it is allowed to do so. We use HBA Host based authentication. Take a look first at how and what we start via our services, as we are a little more than a Django web app. And more like a home server with a Django admin interface. Hence our building each rpm on it's intended OS variants (Leap & TW) & version (15.4 & 15.5) across both architectures (x86_64 &aarch) with test installs of the resulting rpm. There are major differences between versions and architectures but mostly - thankfully, we have managed to maintain a single code base across them all. But we do use distro to change code paths. And we also have to account for varying kernel & btrfs versions. And occasionally differing service changes & interoperability. This is why we recommend using a VM as we are a systems management server - we manage the services on the system so that folks don't have to do it by-hand. But that need knowledge of OS version and variant & arch: ergo VM as we are then testing in the target environment.

As for Python environment we build our own, using Poetry, but depend upon the OS to provide some (fewer than we did) interfaces. And you commented recently on the forum re our Python versions issue. All a bit trickly and our unit test will never cover all these scenarios which is why we are plan to use end-to-end testing via openQA - and have an semi-automated testing-deployment backend inftastructure based on buildbot (used also to build python/chrome incidentally).

As to GitHub actions: niceties are good and I agree that a simply test runner would be nice (if on a relevant OS, not one preferred by a third party) but we entirely expect folks to test their submission in the VM as, being an appliance, we assume dominion over the OS - it's basically the definition of he project. But we try hard to not meddle too much, but it is is a design corner stone that we manage the system. But again we try to use modern systems such as systemd - which incidentally did not exist when we were first releasing Rockstor. We are older than we look.

So re stuff like HBA, which is not guaranteed - it changed a lot between leap versions incidentally, take a look at what rockstor-pre does. That is a good starter to seeing what we do - i.e. setting up certs and ssl access according to differences recently introduced in TW etc. We are not just a web app: ergo VM dev is minimal viable target. Real hardware follows shortly there-after via field testing int the testing channel releases - this all informs all our our repo developments such as rockstor-rpmbuild rockstor-installer rockstor-core rockstor-jslibs rockon-registry - and now your newely donated rockon-validator. All good stuff but we recommend stuff for a reason and we do not want to be dependant on closed source services that will ultimately hold us to ransom - see CircleCI who the btrfs folks used themselves and where then left high and dry one day when the free tier went away. So we do want to control carefully our dependencies so only nice to have - not critical stuff - in GitHub actions I think. Plus it doesn't have the scope of say buildbot. However out tests can - as you say - server us, and our more casual committers, in a usefull way. But untill something as been proven across all target OS variants we try not to publish. But smaller JS changes with simple back-ends are not too much of a problem on that font at it's basic web tec - not so much system management.

The project as a whole benefits from a variety of knowledge both in sysadmin as well as programming and infrastructure so it's to hear input on alternative approaches - but we are also only just forming our new dual branch arrangement - take a looks at @FroggyFlox release manager with automated PR etc. That sort of improvement is magic and our test run on every pr could also be a major win: but we do have our ways and means and some were hard won and not likely to change any time soon. Targeting a VM in development is critical to keeping us relevant to the OS we intend to help administer: and we do not support folks doing their own thing on that same system - we can't: the deviation is infinite. But folks do and we see where we can adapt within reason - all part of development via user interaction which is what our open docs on development and the testing channel is all about.

As to Git interactions I'm entirely happy with where we are now. It's a non trivial endeavour to manage two concurrent development paths but we have to do it this way to tackle our extreme technical dept - all while attempting (see our Open Collective) to achieve sustainability. It only takes a few co-ordinated folks to do a lot of the work - but they do have to be co-ordinates and not pulling in different directions.

Hope that helps, at least from the point of view of how I at least see the project: as the current years long co maintainer along with @FroggyFlox, learning some of our approach from the years long prior maintainer, and project founder @schakrava . We have our ways currently and I'm not keen on changing too much more than what we have on our hands currently. And folks should test their own stuff first is the essence of it. Otherwise it can represents a greater burden in the debugging there-after. If the first test run on new code is in GitHub actions! And likely on only one platform at that! But stuff always falls through the cracks. And many changes are trivial in nature - but I'm constantly surprised at how low the barrier for trivial actually is. The smallest of changes can end up erupting into something entirely 'else'. Such as this discussion for example :) .

And thanks again for your interest in all of this. Unfortunately as a very small team it is a massive distraction to tend to on-boarding - hence our docs. So when they are not read before hand - or are cast aside without due consideration, it can be frustrating. But we also have to keep them updated which is yet another challenge all around - see for example outstanding issues in all our many repos. So again, if you see anything a-miss do chip in as best you think. Though all are open to contributions of course. Discussion such as this belongs on the forum really. But there we go.

[StephenBrown2]

We have our ways currently and I'm not keen on changing too much more than what we have on our hands currently. And folks should test their own stuff first is the essence of it. Otherwise it can represents a greater burden in the debugging there-after.

Understood, you have your ways and of course contributors should follow them to reduce the burden on the maintainers, who are giving your own time just like others in the community. My apologies for not being a good contributor in that way, I should have put more effort in on my end before tossing things "over the wall".

we are plan to use end-to-end testing via openQA

That's great! I was going to suggest that in the GHA test runner PR.

Discussion such as this belongs on the forum really. But there we go.

Agreed, sorry for the long side-tracks here. Back to the OP, is there anything else you need from me before this can get merged?

[phillxnet]

@StephenBrown2 Re:

is there anything else you need from me before this can get merged?

It's basically awaiting review. And I'm currently doing a bit more work on our rpm spec file and plan to test the pending PR in that repo along with this pull request in core at the same time for the proof of rpm build and install. If all goes well this can then be merged. We try to test all pull requests through to rpmbuild (where the tests get run also) before they get merged. That way we can be mostly reassured that the result can be release to the relevant channel soon there after. Sorry for the delay - we have quite a few moving parts at the moment - what with the newly refreshed testing channel phase.

[phillxnet]

@StephenBrown2 Hello again.
Functional/cosmetic review only here as we can re-visit in follow-up issue-pr sets as and when.

Cosmetics

It would be nice to normalise our ordering across and within tooltips/selectors.

Share compression options

Take a look at for example our share detail where the tooltip order is not that of the selector. Not a show-stopper but a nice to have in some usability follow-up as I'm keen to get this functional extension of a PR in as-is given your work to date to get it working and roughly in shape.

• tooltip and selector
  

Tooltip ordering is consistent and I agree with putting the new zstd option at the end: sort of chronological ordering, at least for us. So we likely just need to re-order our share selector. But in another pr I think as only a cosmetic. On the same note we have existing issues re formatting etc. Note our missing space after "lzo:" in share and pool tooltips. Oh and thanks for throwing in some other little fixes re "it's" correction, that one was probably my mistake actually.

Pool compression options

Overview

• tooltip
  
• selector
  

Detail

• tooltip
  
• selector
  

As you noted in your PR we really need to abstract these out to a single source of truth, easier said than done but it would be another great addition - especially as this is not going to be our last addition here. That way future changes / corrections would be far more trivial and have consistency by design. But bit by bit and all good.

As you also noted, we have a reflection of the setting at the pool level in our active mount options:

Nice.

Thanks again for your effort here: much appreciated. Likely we need some infrastructure improvements in this area to support further enhancements but again: bit by bit.