Fix cpuallocated value in findHostsForMIgration api

[ravening]

Description

Fixes #4221

The findHostsForMigration api displays 0% always for
cpuallocated field which is wrong

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Screenshots (if appropriate):

How Has This Been Tested?

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
    }
  ]
}

[GabrielBrascher]

I hope to be helpful with the raised code enhancement suggestion.

Thanks for the PR @ravening.

[GabrielBrascher]

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)) + **"%"**
}

[ravening]

@GabrielBrascher I made the changes. Also can you look into the issue #4221 ?

[GabrielBrascher]

@ravening thanks +1 :)

Can you please take a look at line 314, I think that it could also benefit from the created method calculateResourceAllocatedPercentage.

[ravening]

@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

[GabrielBrascher]

@ravening sorry for the delay. I think that I was not clear about my point.

• the method that you extract L464:
  decimalFormat.format(((float)resource / resourceWithOverProvision * 100.0f)) + "%"

• line 314 that I mentioned:
  decimalFormat.format((float) mem / memWithOverprovisioning * 100.0f) +"%"

That is why I thought that it could also benefit from calculateResourceAllocatedPercentage.

[yadvr]

@blueorangutan package

[blueorangutan]

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result: ✔centos7 ✔debian. JID-1623

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[andrijapanicsb]

(tests failed, re-run after the weekend due to weekend maintenance)

[yadvr]

@blueorangutan test

[blueorangutan]

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[yadvr]

LGTM, did not manually test it

[ravening]

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

[DaanHoogland]

@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.

[GabrielBrascher]

@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.

[blueorangutan]

Trillian test result (tid-2253)
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

[yadvr]

@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

[ravening]

@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

[yadvr]

Alright I'll not revert it, thnx for replying.