-
Notifications
You must be signed in to change notification settings - Fork 1.3k
metrics: fix hostsmetricsresponse for zero cpu, locale #5329
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,10 +17,14 @@ | |
|
|
||
| package org.apache.cloudstack.response; | ||
|
|
||
| import java.text.DecimalFormat; | ||
| import java.text.ParseException; | ||
|
|
||
| import org.apache.cloudstack.api.response.HostResponse; | ||
| import org.apache.cloudstack.outofbandmanagement.OutOfBandManagement; | ||
|
|
||
| import com.cloud.serializer.Param; | ||
| import com.cloud.utils.exception.CloudRuntimeException; | ||
| import com.google.gson.annotations.SerializedName; | ||
|
|
||
| public class HostMetricsResponse extends HostResponse { | ||
|
|
@@ -118,7 +122,7 @@ public void setCpuTotal(final Integer cpuNumber, final Long cpuSpeed) { | |
|
|
||
| public void setCpuUsed(final String cpuUsed, final Integer cpuNumber, final Long cpuSpeed) { | ||
| if (cpuUsed != null && cpuNumber != null && cpuSpeed != null) { | ||
| this.cpuUsed = String.format("%.2f Ghz", Double.valueOf(cpuUsed.replace("%", "")) * cpuNumber * cpuSpeed / (100.0 * 1000.0)); | ||
| this.cpuUsed = String.format("%.2f Ghz", parseCPU(cpuUsed) * cpuNumber * cpuSpeed / (100.0 * 1000.0)); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -130,10 +134,14 @@ public void setLoadAverage(final Double loadAverage) { | |
|
|
||
| public void setCpuAllocated(final String cpuAllocated, final Integer cpuNumber, final Long cpuSpeed) { | ||
| if (cpuAllocated != null && cpuNumber != null && cpuSpeed != null) { | ||
| this.cpuAllocated = String.format("%.2f Ghz", Double.valueOf(cpuAllocated.replace("%", "")) * cpuNumber * cpuSpeed / (100.0 * 1000.0)); | ||
| this.cpuAllocated = String.format("%.2f Ghz", parseCPU(cpuAllocated) * cpuNumber * cpuSpeed / (100.0 * 1000.0)); | ||
| } | ||
| } | ||
|
|
||
| public String getCpuAllocatedGhz() { | ||
| return cpuAllocated; | ||
| } | ||
|
|
||
| public void setMemTotal(final Long memTotal) { | ||
| if (memTotal != null) { | ||
| this.memTotal = String.format("%.2f GB", memTotal / (1024.0 * 1024.0 * 1024.0)); | ||
|
|
@@ -166,25 +174,25 @@ public void setNetworkWrite(final Long networkWriteKbs) { | |
|
|
||
| public void setCpuUsageThreshold(final String cpuUsed, final Double threshold) { | ||
| if (cpuUsed != null && threshold != null) { | ||
| this.cpuThresholdExceeded = Double.valueOf(cpuUsed.replace("%", "")) > (100.0 * threshold); | ||
| this.cpuThresholdExceeded = parseCPU(cpuUsed) > (100.0 * threshold); | ||
| } | ||
| } | ||
|
|
||
| public void setCpuUsageDisableThreshold(final String cpuUsed, final Float threshold) { | ||
| if (cpuUsed != null && threshold != null) { | ||
| this.cpuDisableThresholdExceeded = Double.valueOf(cpuUsed.replace("%", "")) > (100.0 * threshold); | ||
| this.cpuDisableThresholdExceeded = parseCPU(cpuUsed) > (100.0 * threshold); | ||
| } | ||
| } | ||
|
|
||
| public void setCpuAllocatedThreshold(final String cpuAllocated, final Double threshold) { | ||
| if (cpuAllocated != null && threshold != null) { | ||
| this.cpuAllocatedThresholdExceeded = Double.valueOf(cpuAllocated.replace("%", "")) > (100.0 * threshold ); | ||
| this.cpuAllocatedThresholdExceeded = parseCPU(cpuAllocated) > (100.0 * threshold ); | ||
| } | ||
| } | ||
|
|
||
| public void setCpuAllocatedDisableThreshold(final String cpuAllocated, final Float threshold) { | ||
| if (cpuAllocated != null && threshold != null) { | ||
| this.cpuAllocatedDisableThresholdExceeded = Double.valueOf(cpuAllocated.replace("%", "")) > (100.0 * threshold); | ||
| this.cpuAllocatedDisableThresholdExceeded = parseCPU(cpuAllocated) > (100.0 * threshold); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -212,4 +220,13 @@ public void setMemoryAllocatedDisableThreshold(final Long memAllocated, final Lo | |
| } | ||
| } | ||
|
|
||
| private Double parseCPU(String cpu) { | ||
| DecimalFormat decimalFormat = new DecimalFormat("#.##"); | ||
| try { | ||
| return decimalFormat.parse(cpu).doubleValue(); | ||
| } catch (ParseException e) { | ||
| throw new CloudRuntimeException(e); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about logging the exception and handling the exception? Either here or in the usage of this function. For example, the character in the PR description can cause the API to still fail:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nvazquez as the ParseException is thrown as CloudRuntimeException, it will get logged: And API will fail in this case: PR description shows API output. With the change (similar to main branch), |
||
| } | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| package org.apache.cloudstack.response; | ||
|
|
||
| import org.junit.Assert; | ||
| import org.junit.Test; | ||
|
|
||
| import com.cloud.utils.exception.CloudRuntimeException; | ||
|
|
||
| public class HostMetricsResponseTest { | ||
|
|
||
| @Test | ||
| public void testSetCpuAllocatedWithZeroCpu() { | ||
| final HostMetricsResponse hostResponse = new HostMetricsResponse(); | ||
| hostResponse.setCpuAllocated("50.25%", 0, 1000L); | ||
| Assert.assertEquals("0.00 Ghz", hostResponse.getCpuAllocatedGhz()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSetCpuAllocatedWithInfiniteCpuAllocated() { | ||
| final HostMetricsResponse hostResponse = new HostMetricsResponse(); | ||
| hostResponse.setCpuAllocated("β%", 10, 1000L); | ||
| Assert.assertEquals("Infinity Ghz", hostResponse.getCpuAllocatedGhz()); | ||
| } | ||
|
|
||
| @Test(expected = CloudRuntimeException.class) | ||
| public void testSetCpuAllocatedWithInvalidCpu() { | ||
| final HostMetricsResponse hostResponse = new HostMetricsResponse(); | ||
| hostResponse.setCpuAllocated("abc", 10, 1000L); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSetCpuAllocatedWithValidCpu() { | ||
| final HostMetricsResponse hostResponse = new HostMetricsResponse(); | ||
| hostResponse.setCpuAllocated("50.25%", 10, 1000L); | ||
| Assert.assertEquals("5.03 Ghz", hostResponse.getCpuAllocatedGhz()); | ||
| } | ||
|
|
||
| } |
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.
Can we add some unit tests for it?
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.
Added test