Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Size tests fix #399

Merged
merged 2 commits into from May 2, 2016
Merged

Conversation

vpodzime
Copy link
Contributor

This should fix the recent failures of tests because of libbytesize's translations being finally in place.

The translations are provided by libbytesize.
@dwlehman
Copy link
Contributor

dwlehman commented Apr 29, 2016

Wow, how did I not notice that libbytesize doesn't use ROUND_HALF_UP (or even support it)?

My brain struggles mightily with the idea that round_to_nearest('10.3') would return 11 as opposed to 10 given that 10.3 is more than twice as close to 10 than to 11.

Since we are dealing with size quantities here I can see an argument for rounding up in certain situations, but it is still a mental hurdle for me. If the size represents a quantity used, rounding up seems to make sense (so that we do not under-estimate how much we have taken). But what if the size represents a quantity of free space? Rounding up seems to be a bad idea for this (because it leads us to over-estimate how much we have available). And what if we're relating two complementary sizes? ROUND_HALF_UP makes those kinds of relations sensible in general even when rounding.

Consider a 100 GiB drive with a 44.2 GiB partition and 55.8 GiB of free space. If someone wanted to get a general feel for the situation, for example to display in a UI, here's what we'd see:

With ROUND_UP:

  Disk Size: 100 GiB
  Used: 45 GiB
  Free: 56 GiB

With ROUND_HALF_UP:

  Disk Size: 100 GiB
  Used: 44 GiB
  Free: 56 GiB

Which one do you think does a better job of representing the truth?

@vpodzime
Copy link
Contributor Author

vpodzime commented May 2, 2016

Originally I didn't want to set any default because with storage, it's always a bug to rely on some default rounding. That's also why we always specify the rounding mode in Blivet.

As for the ROUND_HALF_UP I wasn't able to come up with an example of where it could be useful as using such value would always lead to issues. Even when showing to the user in the example above -- the user could tell themselves "Okay, I have 56 GiB of free space so I'm gonna create a 56GiB partition." which would fail. Thus the sizes should have been rounded up (used) and down (free) to prevent such issues.

So what I'm gonna do is:

  • add ROUND_HALF_UP to rounding modes in libbytesize
  • remove the default rounding mode in round_to_nearest() in libbytesize
  • set the default rounding mode to ROUND_HALF_UP in round_to_nearest() in Blivet and add a warning to the docstring that relying on the default rounding mode is always a bug

Rounding half up is the most natural rounding mode. It however doesn't make much
sense in most of the calculations related to storage so we should try to explain
developers that they should think about and specify the rounding mode. Removing
the default would change the API.
@vpodzime
Copy link
Contributor Author

vpodzime commented May 2, 2016

The tests are gonna fail here because they require the new libbytesize. I'll build and install it on our jenkins slaves once storaged-project/libbytesize#15 gets merged.

@dwlehman
Copy link
Contributor

dwlehman commented May 2, 2016

I would be okay with making rounding mode a positional/mandatory argument for blivet-3.0.

@vpodzime
Copy link
Contributor Author

vpodzime commented May 2, 2016

I would be okay with making rounding mode a positional/mandatory argument for blivet-3.0.

I had the very same idea.

@dwlehman dwlehman added the ACK label May 2, 2016
@vpodzime
Copy link
Contributor Author

vpodzime commented May 2, 2016

Jenkins, please test this.

@vpodzime
Copy link
Contributor Author

vpodzime commented May 2, 2016

Jenkins, please test this again.

@vpodzime
Copy link
Contributor Author

vpodzime commented May 2, 2016

Ran the check manually on the Jenkins slave and all tests passed.

@vpodzime vpodzime merged commit f6ca57a into storaged-project:2.0-devel May 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants