Improve pool usage reporting #1460

sfranzen · 2016-10-05T20:23:48Z

Will fix #1454. Should now work for any pool configuration; test results to follow.

sfranzen · 2016-10-05T21:26:54Z

Test procedure

Create pool with four disks: 2x 2 GiB, 2x 1 GiB in given RAID configuration;
Fill pool with data: dd if=/dev/zero of=/mnt2/pool1/data bs=1k;
Verify that pool size is reported as here and usage is at (or very close to) 100%.

Test results

Fixed.

sfranzen · 2016-10-06T16:18:25Z

Found another issue when using an odd number of drives in a pool, because I had accidentally removed an important line at some point. Made a slightly more exotic test case for 3x 2 GiB and 2x 1 GiB disks.

Test results

@schakrava By the way, the current code spams quite a lot of debug messages that I used to verify the algorithm. This is not really useful for users; is there a nice way of enabling debug logging just for developers or should I just remove the logging output?

MFlyer · 2016-10-06T19:16:55Z

should I just remove the logging output?

Hi @sfranzen , just remove once you've completed your testing

sfranzen · 2016-10-08T16:47:58Z

Ok, found another slightly annoying issue: the usable size shown in the Add Pool view is still calculated wrongly because that is coded into the Backbone view. I don't like duplicate code, so I will:

Move the usage_bound function from the Pool model into the fs.btrfs module;
Create simple view function to accept a GET request with an array of disk sizes and a raid level and return the usage bound;
Rebase to fix my merge conflicts 😉
Replace current Add Pool size calculations with this result;

Partial fix of rockstor#1454. This algorithm should give an accurate report of the maximum available space in a storage pool, regardless of raid configuration and disk sizes. It will replace the current figure reported by btrfs.pool_usage.

Fixes the other part of rockstor#1454. New behaviour is to return only the used space value, but this now also includes space reserved by btrfs and unavailable for data.

>.>

Required in case of, for example, a 5-disk RAID10 setup.

Some refactoring was required to be able to use the new usage bound function, but I took the liberty of reorganising the rest of the code and hopefully make it more readable.

sfranzen · 2016-10-11T19:11:04Z

Ok, this is now ready for review/merging and depends on rockstor/rockstor-jslibs#7.

In the end I changed quite a few more things than I originally planned to, but even though they are not very noticeable, I'm quite proud of how the pool creation view looks and functions now. Please try it out and let me know what you think.

schakrava · 2016-10-13T02:17:29Z

src/rockstor/fs/btrfs.py

            block = fields[0][:-1].lower()
            raid = fields[1][:-1].lower()
-            if not block in raid_d and raid is not 'DUP':
+            if block not in raid_d and raid is not 'DUP':


nice catch :)

Wasn't me 😉 my editor does automatic PEP8 checking, it complains about a lot of small things.

schakrava · 2016-10-13T02:23:59Z

src/rockstor/fs/btrfs.py

+
+    new_bound = usage_bound(disk_sizes, bounding_q + 1, raid_level)
+
+    return bound * ((chunks / data_ratio) - parity) + new_bound


Nice work, thank you!

schakrava · 2016-10-13T15:01:30Z

@sfranzen to your question about logging, we CAN turn off debug messages by setting log level in settings.conf.in prior to building Rockstor in any environment. I think we should at some point not have debug level for production(rpm build) by default but can provide a simple script to turn it on to aid in troubleshooting. see #1478. and thanks for bringing this up.

schakrava · 2016-10-13T15:16:15Z

Thank you for improving code quality and detailed work log @sfranzen . Very nicely done. I am ok merging this pr, but I do like to ask a few questions and perhaps suggest some improvements. It's totally up to you to implement them in this pr or the next(my preference).

A minor issue. In the Create Pool form, the first click after selecting the raid level always triggers validation. So if I change the raid level from single to anything else and then proceed to select a checkbox to pick my first disk, it doesn't select the checkbox, but triggers validation and displays an error under the raid level dropdown. It doesn't matter where you click, but the first click after changing the raid level triggers the validation. This is a minor regression and I'll leave it up to you to fix in this pr or the next.
In Create pool again, as you select disks, each selection makes an api call to the backend. This is a bit expensive. I get your point about not duplicating usage calculation logic in the frontend, but it may actually be justified in this case. I don't feel strongly one way or the other about this, so I'll leave it up to you to improve it as you wish.
usage bound api as a function based api view in pools.py looks out of place. I like the simplicity of it though, something to think about as you tinker more with django to restful design.

schakrava · 2016-10-13T15:17:42Z

@sfranzen I'll wait for your reply on (1). If you like me to wait until you fix it, let me know. If not I'll merge this as is and you can submit another pr.

sfranzen · 2016-10-13T16:27:10Z

@schakrava Thanks for the feedback.

Good catch, I'll just fix that within this PR.
I thought about that, but since my main experience is application programming I don't really know what kind of expense a remote call carries. I think you're right that this may be pushing the "DRY" principle too far. 😉 It's no problem to do it client side, in fact the algorithm I translated is already in javascript. It will take a little more care than just reverting some commits though, so I'll do it in a future PR if you don't mind.
Yes it is, it probably should have gone in api/commands/usage_bound.

schakrava · 2016-11-10T02:40:15Z

Thanks @sfranzen. Nice work!

sfranzen force-pushed the pool_usage branch from cb73f42 to 624d8d2 Compare October 9, 2016 16:59

sfranzen added 10 commits October 9, 2016 19:06

Fix Python 3 compliance and style

58eff47

Add pool usage bound algorithm

285aa5f

Partial fix of rockstor#1454. This algorithm should give an accurate report of the maximum available space in a storage pool, regardless of raid configuration and disk sizes. It will replace the current figure reported by btrfs.pool_usage.

Modify pool_usage function and callers

4ab7f28

Fixes the other part of rockstor#1454. New behaviour is to return only the used space value, but this now also includes space reserved by btrfs and unavailable for data.

Fix typo

d1c27a6

>.>

Fix stripe calculation for RAID10

456ad76

Fix rounding of the number of chunks

6aacc39

Required in case of, for example, a 5-disk RAID10 setup.

Change logging level to debug

31166fb

Remove log spam and fix comment placement

0755f21

Also get rid of superfluous import

e960b61

Refactor usage_bound

3b4441a

sfranzen force-pushed the pool_usage branch from 624d8d2 to 3b4441a Compare October 9, 2016 17:06

sfranzen added 4 commits October 9, 2016 20:56

Add API view for pool usage bound

d1ccd5d

Update path to Underscore

b3e23f1

Fix missing argument to usage_bound

7353e86

Refactor add_pool and add usage bound calculation

602a673

Some refactoring was required to be able to use the new usage bound function, but I took the liberty of reorganising the rest of the code and hopefully make it more readable.

Fix missing var declarations

4cb745a

schakrava reviewed Oct 13, 2016

View reviewed changes

schakrava mentioned this pull request Oct 13, 2016

Turn off debug logs in Rockstor built by rpm(prod/testing builds) #1478

Closed

Revert to original form behaviour

89b84eb

schakrava merged commit 90c1694 into rockstor:master Nov 10, 2016

phillxnet mentioned this pull request Dec 6, 2016

Pool creation succeeds but refresh icon(ajax-loader) still exists #1563

Closed


		new_bound = usage_bound(disk_sizes, bounding_q + 1, raid_level)

		return bound * ((chunks / data_ratio) - parity) + new_bound

Improve pool usage reporting #1460

Improve pool usage reporting #1460

Uh oh!

Conversation

sfranzen commented Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sfranzen commented Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test procedure

Test results

Uh oh!

sfranzen commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test results

Uh oh!

MFlyer commented Oct 6, 2016

Uh oh!

sfranzen commented Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sfranzen commented Oct 11, 2016

Uh oh!

schakrava Oct 13, 2016

Choose a reason for hiding this comment

Uh oh!

sfranzen Oct 13, 2016

Choose a reason for hiding this comment

Uh oh!

schakrava Oct 13, 2016

Choose a reason for hiding this comment

Uh oh!

schakrava commented Oct 13, 2016

Uh oh!

schakrava commented Oct 13, 2016

Uh oh!

schakrava commented Oct 13, 2016

Uh oh!

sfranzen commented Oct 13, 2016

Uh oh!

schakrava commented Nov 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sfranzen commented Oct 5, 2016 •

edited

Loading

sfranzen commented Oct 5, 2016 •

edited

Loading

sfranzen commented Oct 6, 2016 •

edited

Loading

sfranzen commented Oct 8, 2016 •

edited

Loading