-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
fix: improve resource duration accuracy. Fixes #12468 #12492
fix: improve resource duration accuracy. Fixes #12468 #12492
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7c0adc1
to
6fc9d76
Compare
Thanks for taking a look @agilgur5 ! |
The `.Values()` function rounds cpu ressources up to an integer. As a result, containers with small cpu requests (like the sidecar that requests 10m cores) are counted as requiring one full core, making the resources duration computation quite inaccurate. This changes uses `.MilliValues()` instead. The `MilliValues()` documentation mentions that this function may overflow, but the return value is stored in an int64, so the overflow will happen if a container requests more than 9*10^(15) of a ressource. I assume this should be fine for a while. Signed-off-by: Aloÿs Augustin <aloys.augustin@aqemia.com>
6fc9d76
to
9d3bb42
Compare
It turns out removing the conversion of age into seconds caused an overflow as |
Oh, good find! I would not have expected an overflow and did not know that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a small style comment and a question for clarification
…rgoproj#12492) Signed-off-by: Aloÿs Augustin <aloys.augustin@aqemia.com>
…rgoproj#12492) Signed-off-by: Aloÿs Augustin <aloys.augustin@aqemia.com>
…rgoproj#12492) Signed-off-by: Aloÿs Augustin <aloys.augustin@aqemia.com> Signed-off-by: Isitha Subasinghe <isubasinghe@student.unimelb.edu.au>
…rgoproj#12492) Signed-off-by: Aloÿs Augustin <aloys.augustin@aqemia.com>
…rgoproj#12492) Signed-off-by: Aloÿs Augustin <aloys.augustin@aqemia.com>
…rgoproj#12492) Signed-off-by: Aloÿs Augustin <aloys.augustin@aqemia.com>
Fixes #12468
Motivation
Fix cpu ressources consumption estimation.
Modifications
Change the use of the
.Values()
function (which rounds ressources up to an integer) to.MilliValues()
which is more accurate for CPU resources, which can be fractional.The
MilliValues()
documentation mentions that this function may overflow, but the return value is stored in an int64, so the overflow will happen if a container requests more than 9*10^(15) of a ressource. I assume this should be fine for a while.Verification
Tested by submitting the workflow attached to the issue this PR fixes. The result is now what is expected: