You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/07/22 10:04:30 UTC
[GitHub] [cloudstack] ravening opened a new pull request #4220: Fix cpuallocated value in findHostsForMIgration api
ravening opened a new pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220
## Description
The findHostsForMigration api displays 0% always for
cpuallocated field which is wrong
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [X] Bug fix (non-breaking change which fixes an issue)
## Screenshots (if appropriate):
## How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
Tested from cloudmonkey api
Before the fix
```
local) > find hostsformigration virtualmachineid=01f155-fffc-493f-95b8-85a76414 filter=cpuallocated,suitableformigration
{
"count": 221,
"host": [
{
"cpuallocated": "0%",
"suitableformigration": true
},
{
"cpuallocated": "0%",
"suitableformigration": true
},
{
"cpuallocated": "0%",
"suitableformigration": true
},
```
After the fix
```
(local) mgt01 > list hosts virtualmachineid=95b4270f-1354-4703-99cb-a769400b2d30 filter=cpuallocated,suitableformigration,name
{
"count": 3,
"host": [
{
"cpuallocated": "2.27%",
"name": "node32",
"suitableformigration": true
},
{
"cpuallocated": "1.14%",
"name": "node33",
"suitableformigration": true
}
]
}
```
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#discussion_r459883908
##########
File path: server/src/main/java/com/cloud/api/query/dao/HostJoinDaoImpl.java
##########
@@ -330,10 +330,10 @@ public HostForMigrationResponse newHostForMigrationResponse(HostJoinVO host, Enu
hostResponse.setHypervisorVersion(host.getHypervisorVersion());
- Float cpuWithOverprovisioning = new Float(host.getCpus() * host.getSpeed() * ApiDBUtils.getCpuOverprovisioningFactor(host.getClusterId()));
- String cpuAlloc = decimalFormat.format(((float)cpu / cpuWithOverprovisioning * 100f)).toString() + "%";
+ float cpuWithOverprovisioning = new Float(host.getCpus() * host.getSpeed() * ApiDBUtils.getCpuOverprovisioningFactor(host.getClusterId()));
+ String cpuAlloc = decimalFormat.format(((float)cpu / cpuWithOverprovisioning * 100f)) + "%";
Review comment:
@GabrielBrascher I didn't make changes in line 314 because it has different behavior compared to line 165.
I have mentioned the same in issue #4221 also. list hosts api returns absolute value where as findhostsformigration returns percentage. If we decided what to return then I can make changes in these two places also
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#discussion_r459287236
##########
File path: server/src/main/java/com/cloud/api/query/dao/HostJoinDaoImpl.java
##########
@@ -330,10 +330,10 @@ public HostForMigrationResponse newHostForMigrationResponse(HostJoinVO host, Enu
hostResponse.setHypervisorVersion(host.getHypervisorVersion());
- Float cpuWithOverprovisioning = new Float(host.getCpus() * host.getSpeed() * ApiDBUtils.getCpuOverprovisioningFactor(host.getClusterId()));
- String cpuAlloc = decimalFormat.format(((float)cpu / cpuWithOverprovisioning * 100f)).toString() + "%";
+ float cpuWithOverprovisioning = new Float(host.getCpus() * host.getSpeed() * ApiDBUtils.getCpuOverprovisioningFactor(host.getClusterId()));
+ String cpuAlloc = decimalFormat.format(((float)cpu / cpuWithOverprovisioning * 100f)) + "%";
Review comment:
@GabrielBrascher I made the changes. Also can you look into the issue #4221 ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#issuecomment-665569219
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#issuecomment-668395440
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#issuecomment-669060689
@ravening sorry I missed your comment, and merged based on @GabrielBrascher 's `I am good with the code as it is`. Do you need to do more work on this PR, should we revert? cc @DaanHoogland
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#issuecomment-668505963
> LGTM, did not manually test it
@rhtyd @DaanHoogland its not yet ready for merge. Im waiting for @GabrielBrascher reply for my question.
If you can also reply it then it will be helpful
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#discussion_r459838472
##########
File path: server/src/main/java/com/cloud/api/query/dao/HostJoinDaoImpl.java
##########
@@ -330,10 +330,10 @@ public HostForMigrationResponse newHostForMigrationResponse(HostJoinVO host, Enu
hostResponse.setHypervisorVersion(host.getHypervisorVersion());
- Float cpuWithOverprovisioning = new Float(host.getCpus() * host.getSpeed() * ApiDBUtils.getCpuOverprovisioningFactor(host.getClusterId()));
- String cpuAlloc = decimalFormat.format(((float)cpu / cpuWithOverprovisioning * 100f)).toString() + "%";
+ float cpuWithOverprovisioning = new Float(host.getCpus() * host.getSpeed() * ApiDBUtils.getCpuOverprovisioningFactor(host.getClusterId()));
+ String cpuAlloc = decimalFormat.format(((float)cpu / cpuWithOverprovisioning * 100f)) + "%";
Review comment:
@ravening thanks **+1** :)
Can you please take a look at line [314](https://github.com/apache/cloudstack/blob/7febf17aa8c8ba6403deeb160a4af5f6dda5111c/server/src/main/java/com/cloud/api/query/dao/HostJoinDaoImpl.java#L314), I think that it could also benefit from the created method `calculateResourceAllocatedPercentage`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#discussion_r458826571
##########
File path: server/src/main/java/com/cloud/api/query/dao/HostJoinDaoImpl.java
##########
@@ -330,10 +330,10 @@ public HostForMigrationResponse newHostForMigrationResponse(HostJoinVO host, Enu
hostResponse.setHypervisorVersion(host.getHypervisorVersion());
- Float cpuWithOverprovisioning = new Float(host.getCpus() * host.getSpeed() * ApiDBUtils.getCpuOverprovisioningFactor(host.getClusterId()));
- String cpuAlloc = decimalFormat.format(((float)cpu / cpuWithOverprovisioning * 100f)).toString() + "%";
+ float cpuWithOverprovisioning = new Float(host.getCpus() * host.getSpeed() * ApiDBUtils.getCpuOverprovisioningFactor(host.getClusterId()));
+ String cpuAlloc = decimalFormat.format(((float)cpu / cpuWithOverprovisioning * 100f)) + "%";
Review comment:
I see `100f` a couple of times on the code, there is also `100.0f`.
What do you think of extracting these _magic_ `100f`/`100.0f` into a constant?
Another approach that would be even nicer would be to have these pieces extracted to a method like `calculateResourceAllocatedPercentage`, and add a few unit test case methods.
```
public String calculateResourceAllocatedPercentage(float resource, float resourceWithOverprovisioning) {
return decimalFormat.format(((float)resource / resourceWithOverprovisioning * 100f)) + **"%"**
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#discussion_r465263348
##########
File path: server/src/main/java/com/cloud/api/query/dao/HostJoinDaoImpl.java
##########
@@ -330,10 +330,10 @@ public HostForMigrationResponse newHostForMigrationResponse(HostJoinVO host, Enu
hostResponse.setHypervisorVersion(host.getHypervisorVersion());
- Float cpuWithOverprovisioning = new Float(host.getCpus() * host.getSpeed() * ApiDBUtils.getCpuOverprovisioningFactor(host.getClusterId()));
- String cpuAlloc = decimalFormat.format(((float)cpu / cpuWithOverprovisioning * 100f)).toString() + "%";
+ float cpuWithOverprovisioning = new Float(host.getCpus() * host.getSpeed() * ApiDBUtils.getCpuOverprovisioningFactor(host.getClusterId()));
+ String cpuAlloc = decimalFormat.format(((float)cpu / cpuWithOverprovisioning * 100f)) + "%";
Review comment:
@ravening sorry for the delay. I think that I was clear about my point.
- the method that you extract [L464](https://github.com/apache/cloudstack/blob/7febf17aa8c8ba6403deeb160a4af5f6dda5111c/server/src/main/java/com/cloud/api/query/dao/HostJoinDaoImpl.java#L464):
`decimalFormat.format(((float)resource / resourceWithOverProvision * 100.0f)) + "%"`
- line [314](https://github.com/apache/cloudstack/blob/7febf17aa8c8ba6403deeb160a4af5f6dda5111c/server/src/main/java/com/cloud/api/query/dao/HostJoinDaoImpl.java#L314) that I mentioned:
`decimalFormat.format((float) mem / memWithOverprovisioning * 100.0f) +"%"`
That is why I thought that it could also benefit from `calculateResourceAllocatedPercentage`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#issuecomment-668764505
@ravening if your question is whether a percentage or an absolute number is to be returned i would really not care. I don't think that the two API have to return the same type of value. I do think that the difference should be clear from the name of the parameter and hence both could be returned if so desired.
if that was not your question please correct me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#issuecomment-665569508
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#issuecomment-669061434
> @ravening sorry I missed your comment, and merged based on @GabrielBrascher 's `I am good with the code as it is`. Do you need to do more work on this PR, should we revert? cc @DaanHoogland
Since it's ok for them that two functions returns different value, it's fine with me as well
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#issuecomment-665856561
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#issuecomment-669082226
Alright I'll not revert it, thnx for replying.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#issuecomment-668395892
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] andrijapanicsb commented on pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
andrijapanicsb commented on pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#issuecomment-667150067
(tests failed, re-run after the weekend due to weekend maintenance)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#issuecomment-668837688
<b>Trillian test result (tid-2253)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 54624 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4220-t2253-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_certauthority_root.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
Smoke tests completed. 82 look OK, 1 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_add_delete_kubernetes_supported_version | `Error` | 1807.11 | test_kubernetes_supported_versions.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd merged pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#issuecomment-668773172
@DaanHoogland @ravening I am good with the code as it is, I just saw that maybe the method could be also used in the other context considering how similar those lines are.
Note that even `DecimalFormat decimalFormat = new DecimalFormat("#.##");` is duplicated and used on the same way for `cpuAllocated` and `memoryAllocated`. However I don't want to stall this PR with that observation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#issuecomment-665856282
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4220: Fix cpuallocated value in findHostsForMIgration api
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4220:
URL: https://github.com/apache/cloudstack/pull/4220#discussion_r465263348
##########
File path: server/src/main/java/com/cloud/api/query/dao/HostJoinDaoImpl.java
##########
@@ -330,10 +330,10 @@ public HostForMigrationResponse newHostForMigrationResponse(HostJoinVO host, Enu
hostResponse.setHypervisorVersion(host.getHypervisorVersion());
- Float cpuWithOverprovisioning = new Float(host.getCpus() * host.getSpeed() * ApiDBUtils.getCpuOverprovisioningFactor(host.getClusterId()));
- String cpuAlloc = decimalFormat.format(((float)cpu / cpuWithOverprovisioning * 100f)).toString() + "%";
+ float cpuWithOverprovisioning = new Float(host.getCpus() * host.getSpeed() * ApiDBUtils.getCpuOverprovisioningFactor(host.getClusterId()));
+ String cpuAlloc = decimalFormat.format(((float)cpu / cpuWithOverprovisioning * 100f)) + "%";
Review comment:
@ravening sorry for the delay. I think that I was not clear about my point.
- the method that you extract [L464](https://github.com/apache/cloudstack/blob/7febf17aa8c8ba6403deeb160a4af5f6dda5111c/server/src/main/java/com/cloud/api/query/dao/HostJoinDaoImpl.java#L464):
`decimalFormat.format(((float)resource / resourceWithOverProvision * 100.0f)) + "%"`
- line [314](https://github.com/apache/cloudstack/blob/7febf17aa8c8ba6403deeb160a4af5f6dda5111c/server/src/main/java/com/cloud/api/query/dao/HostJoinDaoImpl.java#L314) that I mentioned:
`decimalFormat.format((float) mem / memWithOverprovisioning * 100.0f) +"%"`
That is why I thought that it could also benefit from `calculateResourceAllocatedPercentage`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org