-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 precision of cpu to millicore and memory to Mi #8409
Conversation
I think this should be p1 fix, but I cannot label. |
[test] |
Rationale for p1 fix is I will most likely fix the upstream code to fail validation if it gets fractional bytes for memory or sub-millicore precision on CPU |
@abhgupta - fyi |
LGTM. Note for the UI team this rounds memory to bytes; output will presumably still show full byte precision if it doesn't fall on binary values (1023 bytes won't round to 1Ki). |
we discussed this with @danmcp and this is going to be updated to round to On Fri, Apr 8, 2016 at 12:31 PM, Luke Meyer [email protected]
|
4675426
to
bd09e4f
Compare
@jwforres @danmcp @sosiouxme PTAL Fixes code to round down cpu to nearest millicore with a floor of 1m. |
@ncdc - this is a blocker bug fix, can you please review? |
Amount: multiply(inf.NewDec(int64(float64(memLimit.Value())*cpuBaseScaleFactor), 3), a.config.limitCPUToMemoryRatio), | ||
Format: resource.DecimalSI, | ||
// float math is necessary here as there is no way to create an inf.Dec to represent cpuBaseScaleFactor < 0.001 | ||
// cpu is measured in millicores, so we need to scale and round up the value to nearest millicore scale |
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.
comment says "round up" but you use RoundDown below?
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.
will fix comment, round down is correct, comment was missed in my update.
bd09e4f
to
ea48395
Compare
Comments updated. |
ea48395
to
a90ca8f
Compare
Evaluated for origin test up to a90ca8f |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2994/) |
Math is fun 😄 LGTM |
@smarterclayton - can I have approval and merge? |
I approve and [merge]! |
blast you AWS! [merge] already |
I am relentlessly [merge]ing! |
Evaluated for origin merge up to a90ca8f |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2994/) (Image: devenv-rhel7_3997) |
Fixes #8391
Fixes code to round down cpu to nearest millicore with a floor of 1m.
Fixes code to round down memory to nearest Mi with a floor of 1Mi.
@spadgett @jwforres @sspeiche @sosiouxme - PTAL