fix: correctly parse resource config #3072
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
frrist
commented
Dec 6, 2023
Comment on lines
-240
to
-242
| // this will give us a numerator and denominator that should end up at the | ||
| // same 0.1 value that 100m means | ||
| // https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/managing_monitoring_and_updating_the_kernel/using-cgroups-v2-to-control-distribution-of-cpu-time-for-applications_managing-monitoring-and-updating-the-kernel#proc_controlling-distribution-of-cpu-time-for-applications-by-adjusting-cpu-bandwidth_using-cgroups-v2-to-control-distribution-of-cpu-time-for-applications |
There was a problem hiding this comment.
This is only relevant to the CPU test case above. Removed from here (it appears to have been copied)
frrist
force-pushed
the
frrist/resource-encodin
branch
from
6e4c252
to
70728fd
Compare
simonwo
approved these changes
Dec 6, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change modifies
models.ResourceConfigto usego-humanizeinstead ofdatasizefor more accurate parsing of human-readable strings into bytes.go-humanizecan parse mebibyte strings and megabytes strings (ditto that for the rest of the SI units). Also updated the 'String' method of 'ResourceConfig' to reflect these changes.I noticed this issue when
bacalhau config auto-resourcesproduced a config that failed to parse. The bacalhau config system usesgo-humanizefor parsing values, and had modifiedmodel.ResourceConfigto do the same. When #3061 landed it replaced usage ofmodel.ResourceConfigwithmodels.ResourceConfigwhich broke the behavior implemented in #3012. The change resolves this breakage.Additionally, when making this change I noticed the
models.ResourceConfig.Normalize()method erroneously converted Mebibytes (Mi/MiB) to Megabyte (MB) when parsing the string representation. Thedatasizepackage doesn't distinguish these types. Themodel.ResourceConfigimplementation did not have this behavior.go-humanizedoes distinguish between these types. I have removed the incorrect behavior frommodels.ResourceConfig.Normalize()and update the test for it.