Improve pool usage reporting #1460

Merged
merged 16 commits into from Nov 10, 2016

Projects

None yet

3 participants

@sfranzen
Contributor
sfranzen commented Oct 5, 2016 edited

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

@sfranzen
Contributor
sfranzen commented Oct 5, 2016 edited

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

  • Single: 6/6 GB
  • RAID0: 6/6 GB
  • RAID1: 3/3 GB
  • RAID10: 2/2 GB
  • RAID5: 4/4 GB
  • RAID6: 2/2 GB

Fixed.

@sfranzen
Contributor
sfranzen commented Oct 6, 2016 edited

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

  • Single: 8/8 GiB
  • RAID0: 8/8 GiB
  • RAID1: 4/4 GiB
  • RAID10: 4/4 GiB
  • RAID5: 6/6 GiB
  • RAID6: 4/4 GiB

@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
Member
MFlyer commented Oct 6, 2016

should I just remove the logging output?

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

@sfranzen
Contributor
sfranzen commented Oct 8, 2016 edited

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;
sfranzen added some commits Sep 29, 2016
@sfranzen sfranzen Fix Python 3 compliance and style 58eff47
@sfranzen sfranzen Add pool usage bound algorithm
Partial fix of #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.
285aa5f
@sfranzen sfranzen Modify pool_usage function and callers
Fixes the other part of #1454.

New behaviour is to return only the used space value, but this now also
includes space reserved by btrfs and unavailable for data.
4ab7f28
@sfranzen sfranzen Fix typo
>.>
d1c27a6
@sfranzen sfranzen Fix stripe calculation for RAID10 456ad76
@sfranzen sfranzen Fix rounding of the number of chunks
Required in case of, for example, a 5-disk RAID10 setup.
6aacc39
@sfranzen sfranzen Change logging level to debug 31166fb
@sfranzen sfranzen Remove log spam and fix comment placement 0755f21
@sfranzen sfranzen Also get rid of superfluous import e960b61
@sfranzen sfranzen Refactor usage_bound 3b4441a
@sfranzen sfranzen Add API view for pool usage bound d1ccd5d
@sfranzen sfranzen Update path to Underscore b3e23f1
@sfranzen sfranzen Fix missing argument to usage_bound 7353e86
@sfranzen sfranzen Refactor add_pool and add usage bound calculation
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.
602a673
@sfranzen
Contributor

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.

@sfranzen sfranzen Fix missing var declarations 4cb745a
@@ -127,7 +127,7 @@ def pool_raid(mnt_pt):
if (len(fields) > 1):
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':
@schakrava
schakrava Oct 13, 2016 Member

nice catch :)

@sfranzen
sfranzen Oct 13, 2016 Contributor

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

+
+ new_bound = usage_bound(disk_sizes, bounding_q + 1, raid_level)
+
+ return bound * ((chunks / data_ratio) - parity) + new_bound
@schakrava
schakrava Oct 13, 2016 Member

Nice work, thank you!

@schakrava
Member

@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
Member

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).

  1. 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.
  2. 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.
  3. 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
Member

@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
Contributor

@schakrava Thanks for the feedback.

  1. Good catch, I'll just fix that within this PR.
  2. 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.
  3. Yes it is, it probably should have gone in api/commands/usage_bound.
@sfranzen sfranzen Revert to original form behaviour 89b84eb
@schakrava
Member

Thanks @sfranzen. Nice work!

@schakrava schakrava merged commit 90c1694 into rockstor:master Nov 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment