-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[4.9+] CLOUDSTACK-9900: Fix high CPU deviation issues seen in metrics view #2075
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.
not going to crash planes or anything , but I'd rather see the "* 1.0"'s removed
public void setCpuMaxDeviation(final Double maxCpuUsagePercentage, final Double totalCpuUsedPercentage, final Long totalHosts) { | ||
if (maxCpuUsagePercentage != null && totalCpuUsedPercentage != null && totalHosts != null && totalHosts != 0) { | ||
final Double averageCpuUsagePercentage = totalCpuUsedPercentage / totalHosts; | ||
this.cpuMaxDeviation = String.format("%.2f%%", (maxCpuUsagePercentage - averageCpuUsagePercentage) * 1.0 / averageCpuUsagePercentage); |
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.
is the '* 1.0' needed when it is devided next? I would rather see a cast if it is, in this case
public void setCpuMaxDeviation(final Double maxCpuUsagePercentage, final Double totalCpuUsedPercentage, final Long totalHosts) { | ||
if (maxCpuUsagePercentage != null && totalCpuUsedPercentage != null && totalHosts != null && totalHosts != 0) { | ||
final Double averageCpuUsagePercentage = totalCpuUsedPercentage / totalHosts; | ||
this.cpuMaxDeviation = String.format("%.2f%%", (maxCpuUsagePercentage - averageCpuUsagePercentage) * 1.0 / averageCpuUsagePercentage); |
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.
but i think th '* 1.0' can be removed
HostStats returns cpu usage in percentage while memory usage in bytes. This fixes a regression in maximum CPU usage deviation that did not assume the values to be in percentage and multiple the final ratios with 100 which leads to 100x the actual deviation value. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
c81c088
to
efe7646
Compare
@DaanHoogland fixed thanks |
LGTM |
@blueorangutan package |
Thanks @rashmidixit for reviewing |
LGTM, I already approved but since we have this string in our policy... ;) |
LGTM, on code review. |
@karuturi can you consider merging this, thanks. |
@blueorangutan package |
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
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.
LGTM on code, will trigger smoketests now
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-702 |
@blueorangutan test |
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Trillian test result (tid-1063)
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-725 |
HostStats returns cpu usage in percentage while memory usage in bytes.
This fixes a regression in maximum CPU usage deviation that did not
assume the values to be in percentage and multiple the final ratios
with 100 which leads to 100x the actual deviation value.
Pinging for review - @karuturi @DaanHoogland @abhinandanprateek @borisstoyanov @rashmidixit
@blueorangutan package