lvol - extending volumes with '+' only work for percentages #2267
Conversation
ansibullbot
added
affects_2.10
bug
There was a problem hiding this comment.
Can you please add a changelog fragment?
Also, is this really a bug, or more like a feature? I would tend to the latter.
|
@felixfontein Will add the changelog after implementing completely. Will also edit to change to feature. |
This comment has been minimized.
This comment has been minimized.
ansibullbot
added
ci_verified
plugins/modules/system/lvol.py
Outdated
| @@ -477,9 +502,9 @@ def main(): | |||
| size_requested = size_percent * this_vg['free'] / 100 | |||
|
|
|||
| # Round down to the next lowest whole physical extent | |||
| size_requested -= (size_requested % this_vg['ext_size']) | |||
| size_requested -= (size_requested % this_vg['ext_size']) # I'm not sure why we are trying to calculate the size instead of letting the tool do it and pass it the VG|PVS|FREE options. In my opinion it will only lead to confusion since the module logic might not match the tools and descreptencies might arise (not actually taking all the free space but an extent less or some such scenario). We can leave the test/error logic, but the tool already does that as well. Besides you're not doing any checks for size for the -L option either, it keeps the code simple and I preffer that approach. Do you dissagree, any suggestions? At the very least we should use the same approach for both -l and -L for consistency. Will implement after a discussion. | |||
There was a problem hiding this comment.
Would appreciate some input :)
There was a problem hiding this comment.
A more readable version (increases the chance that someone actually reads it):
I'm not sure why we are trying to calculate the size instead of letting the tool do it and pass it the VG|PVS|FREE options. In my opinion it will only lead to confusion since the module logic might not match the tools and descreptencies might arise (not actually taking all the free space but an extent less or some such scenario). We can leave the test/error logic, but the tool already does that as well. Besides you're not doing any checks for size for the -L option either, it keeps the code simple and I preffer that approach. Do you dissagree, any suggestions? At the very least we should use the same approach for both -l and -L for consistency. Will implement after a discussion.
(I'm not using this module and don't really know much about its internals, so I probably won't take part in this discussion.)
There was a problem hiding this comment.
No clue either - the only thing I know for sure is that this (long) comment is not helping at all.
There was a problem hiding this comment.
I added it as a comment since otherwise, I could not create the comment/review request on github on that line.
Since there wasn't any input I tried implementing the feature with the minimum amount of changes made. See latest commits.
ansibullbot
added
community_review
and removed
needs_revision
changelogs/fragments/2267-lvol_size_addition-subtraction_support.yaml
Outdated
Show resolved
Hide resolved
changelogs/fragments/2267-lvol_size_addition-subtraction_support.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Felix Fontein <[email protected]>
felixfontein
added
check-before-release
|
@zigaSRC @unkaputtbar112 thanks for implementing this! |
|
Yeah, thanks for officially implementing it :) I lost track of it, to be honest and yes @zigaSRC i was just starting with python and the calculations were stupid. I thought, someone else will probably pick it up and do it better anyway ;D |
| if size[-1].lower() in 'bskmgtpe': | ||
| size_unit = size[-1].lower() |
There was a problem hiding this comment.
#2360
Should not have been removed.
There was a problem hiding this comment.
Or better yet add .lower on the 406th line, where LVS command is being used.
SUMMARY
Recreating PR ansible/ansible#54402 from old repo.
Modified lvol.py to accept '+' on absolute values and also added simple recalculations from e.g. MebiByte to MegaByte (for Kilo, Giga, Terra,...) to have consistency between lvs --units M/m and given Size M/m to lvextend (K/k, G/g, E/e, ...)
Closes #1988
ISSUE TYPE
COMPONENT NAME
lvol
ADDITIONAL INFORMATION