CLOUDSTACK-8800 : Improved the listVirtualMachines API call to include memory utilization information for a VM #1444

rafaelweingartner · 2016-03-19T19:43:02Z

This PR introduces the changes proposed in PR #780 with some work to make the code null safe.

During this PR, I have also removed some unused code.

terbolous · 2016-03-19T21:51:32Z

Thanks for your PR @rafaelweingartner, for tracking purposes, would you mind creating a jira ticket and including the ticket id in the PR title?

rafaelweingartner · 2016-03-19T22:18:22Z

@eriweb sure I can.
I will just wait for @swill and @remibergsma feedback.
If we decide to revert #780, then I will close this PR; otherwise, we can use it.

remibergsma · 2016-03-20T09:26:34Z

This PR has some nice improvements, so that's great @rafaelweingartner!

Still, I'm clueless why people merge without running a single integration test. When doing so, you risk making master unstable. You simply don't know if you broke something else unintentionally. When you run integration tests on another PR later on and that fails, you don't know whether it's that PR or it was master. We've been there, it's a pain. I'll leave it up to @swill to decide as I'm no longer RM. I'd have reverted it, for sure.

rafaelweingartner · 2016-03-20T10:27:56Z

@remibergsma I understand your point.
Anyways if we decide to revert that PR, I can get the changes it introduces and apply here; then we can follow the merge process by the letter.

swill · 2016-03-21T21:43:56Z

@rafaelweingartner, thank you for offering to merge in the changes from #780. I have reverted that merge, if you can merge it here and we can run integration tests against it, that would be greatly appreciated. Thank you sir. :)

rafaelweingartner · 2016-03-22T01:06:58Z

@remibergsma, @swill I had cherry-picked the changes of PR #780 into this PR and then I applied my changes to solve those problems that were reported there.

swill · 2016-03-23T16:45:54Z

...ins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java

@@ -484,6 +489,10 @@ public DomainBlockStats answer(final InvocationOnMock invocation) throws Throwab
        // IO traffic as generated by the logic above, must be greater than zero
        Assert.assertTrue(vmStat.getDiskReadKBs() > 0);
        Assert.assertTrue(vmStat.getDiskWriteKBs() > 0);
+        // Memory limit of VM must be greater than zero


You specify that memory must be greater than zero in this comment, but it uses >= in the actual logic. Can you explain?

I have no idea; this code is not mine. I just cherry-picked the commit “92b99714a03c238bcda6ac0c7a57b44cf4890e65” and worked around the possible runtime exception that it might cause.

I expected the code to be ok, giving that it had already received LGTMs and it seemed that it was only reverted because of those points we lifted up in the PR #780.

The code I introduced here can be seen with commit “70d0d1f8c0615fd3b0e6484b7818bb5d14b089b3”. For every single bit there I can provide you an explanation, for the other code introduced with “92b99714a03c238bcda6ac0c7a57b44cf4890e65” I sadly cannot.

I understand. Thanks.

@maneesha-p: can you shed some light on this?

You specify that memory must be greater than zero in this comment, but it uses >= in the actual logic. Can you explain?

I have read and given some time to think about this test. It is very big and complicated, but those are other issues that are due to the method itself that is being tested.

About the test coditional, I believe that the “>=” is right. Because if you are checking the freeMemory, that method can return zero (0) in cases that you are using all of the memory that is being allocated to the VM. I think that the comment in the test case is misleading us.

Ok, that does make sense. I will get this tested and we will go from there. Thanks for the review.

swill · 2016-04-27T13:51:55Z

CI RESULTS

Tests Run: 85
  Skipped: 0
   Failed: 3
   Errors: 0

Summary of the problem(s):

FAIL: Test redundant router internals
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/git/cs1/cloudstack/test/integration/smoke/test_routers_network_ops.py", line 290, in test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true
    "Attempt to retrieve google.com index page should be successful!"
AssertionError: Attempt to retrieve google.com index page should be successful!
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_NAELYA/results.txt

FAIL: test_03_vpc_privategw_restart_vpc_cleanup (integration.smoke.test_privategw_acl.TestPrivateGwACL)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/git/cs1/cloudstack/test/integration/smoke/test_privategw_acl.py", line 265, in test_03_vpc_privategw_restart_vpc_cleanup
    self.performVPCTests(vpc_off, True)
  File "/data/git/cs1/cloudstack/test/integration/smoke/test_privategw_acl.py", line 324, in performVPCTests
    self.check_pvt_gw_connectivity(vm1, public_ip_1, vm2.nic[0].ipaddress)
  File "/data/git/cs1/cloudstack/test/integration/smoke/test_privategw_acl.py", line 559, in check_pvt_gw_connectivity
    "Ping to outside world from VM should be successful"
AssertionError: Ping to outside world from VM should be successful
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_NAELYA/results.txt

FAIL: test_04_rvpc_privategw_static_routes (integration.smoke.test_privategw_acl.TestPrivateGwACL)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/git/cs1/cloudstack/test/integration/smoke/test_privategw_acl.py", line 277, in test_04_rvpc_privategw_static_routes
    self.performVPCTests(vpc_off)
  File "/data/git/cs1/cloudstack/test/integration/smoke/test_privategw_acl.py", line 324, in performVPCTests
    self.check_pvt_gw_connectivity(vm1, public_ip_1, vm2.nic[0].ipaddress)
  File "/data/git/cs1/cloudstack/test/integration/smoke/test_privategw_acl.py", line 559, in check_pvt_gw_connectivity
    "Ping to outside world from VM should be successful"
AssertionError: Ping to outside world from VM should be successful
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_NAELYA/results.txt

Associated Uploads

/tmp/MarvinLogs/DeployDataCenter__Apr_27_2016_06_50_55_VQRZDI:

/tmp/MarvinLogs/test_network_NAELYA:

/tmp/MarvinLogs/test_vpc_routers_Z0GM3S:

Uploads will be available until 2016-06-27 02:00:00 +0200 CEST

Comment created by upr comment.

swill · 2016-04-27T14:05:35Z

The issues that we see here are environment issues that sometimes show up. They are not related to this PR.

I am running another set of usage specific integration tests. I am not sure of the state of these tests, so I will post the results when they are done and we can go from there.

swill · 2016-04-27T14:49:50Z

CI RESULTS

Tests Run: 18
  Skipped: 0
   Failed: 0
   Errors: 2

Summary of the problem(s):

ERROR: Test Create/Delete a manual snap shot and verify
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/git/cs1/cloudstack/test/integration/component/test_project_usage.py", line 1376, in test_01_snapshot_usage
    snapshot = Snapshot.create(self.apiclient, volumes[0].id)
  File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 1014, in create
    return Snapshot(apiclient.createSnapshot(cmd).__dict__)
  File "/usr/lib/python2.7/site-packages/marvin/cloudstackAPI/cloudstackAPIClient.py", line 797, in createSnapshot
    response = self.connection.marvinRequest(command, response_type=response, method=method)
  File "/usr/lib/python2.7/site-packages/marvin/cloudstackConnection.py", line 379, in marvinRequest
    raise e
CloudstackAPIException: Execute cmd: createsnapshot failed, due to: errorCode: 530, errorText:KVM Snapshot is not supported: 1
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_usage_0CACSG/results.txt

ERROR: Test Create/Delete a manual snap shot and verify
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/git/cs1/cloudstack/test/integration/component/test_usage.py", line 1300, in test_01_snapshot_usage
    snapshot = Snapshot.create(self.apiclient, volumes[0].id)
  File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 1014, in create
    return Snapshot(apiclient.createSnapshot(cmd).__dict__)
  File "/usr/lib/python2.7/site-packages/marvin/cloudstackAPI/cloudstackAPIClient.py", line 797, in createSnapshot
    response = self.connection.marvinRequest(command, response_type=response, method=method)
  File "/usr/lib/python2.7/site-packages/marvin/cloudstackConnection.py", line 379, in marvinRequest
    raise e
CloudstackAPIException: Execute cmd: createsnapshot failed, due to: errorCode: 530, errorText:KVM Snapshot is not supported: 1
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_usage_0CACSG/results.txt

Associated Uploads

/tmp/MarvinLogs/test_usage_0CACSG:

/tmp/MarvinLogs/test_usage_46FO5S:

Uploads will be available until 2016-06-27 02:00:00 +0200 CEST

Comment created by upr comment.

swill · 2016-04-27T14:52:56Z

Ran the usage tests to make sure everything is passing. Don't worry about the snapshot issue, it is not related to this PR.

I think this PR is ready if I count the LGTM votes from the previous PR. Any final thoughts???

rafaelweingartner · 2016-04-27T15:01:06Z

@swill, I am so sorry, I had forgotten this PR. I opened the email to help in a review thinking that this was from someone else.

Are you sure we can count the LGTMs from other PR here?
We have not received any LGTMs. I do not know if the code was reviewed though.

swill · 2016-04-27T15:33:17Z

I would like to get at least one LGTM in this PR. The code is basically the same as the previous one other than the fixes to the potential NPE issues. I think we should get some reviews here regardless.

swill · 2016-04-28T20:34:24Z

@rafaelweingartner please rebase as we currently have merge conflicts. Thanks...

…e memory utilization information for a VM for xenserver,kvm and for vmware.

rafaelweingartner · 2016-04-29T17:26:31Z

@swill conflicts solved

rafaelweingartner · 2016-04-29T17:54:19Z

@swill Jenkins is complaining about a file called "testsmallfileinactive", but I have not introduced any sort of file like that. Is that some sort of trash that was left behind on the Jenkins VM?

swill · 2016-04-29T18:04:15Z

Just do a force push again. Jenkins is being a bit of a princess recently, so we just have to keep trying. This has been happening to a lot of PRs and it is almost never related to the PR. Maybe I should get my hands dirty and see if I can improve the Jenkins implementation, but I just don't have time right now...

rafaelweingartner · 2016-04-29T18:07:48Z

Ok, I will do that.
I can help youwith that (If we get access to the VM).
It would be nice, something like this plugin "https://github.com/janinko/ghprb", then we could use sentences such "test this please" or "retest this please". I have used it, and it is very interesting.

swill · 2016-04-29T18:54:19Z

ya, that is interesting. a couple people have mentioned similar tools. once I get the repo moved to the new github org I can start look into adding such tooling...

rohityadavcloud · 2016-05-02T12:47:17Z

@rafaelweingartner please rebase against master, and squash changes to a single commit

tag:easypr

rafaelweingartner · 2016-05-02T15:17:15Z

@rhtyd I think it would not be a good idea to squash these commits. We will lose the history if we do it here.

I only cherry picked the changes done by @maneesha-p and were reverted a while back. Then, I fixed some of the issues that commit introduced.

Also, there is no conflict with the master version, do we really need to rebase again?

swill · 2016-05-02T15:24:33Z

I think the two commits are fine in this case, so just leave it as it is. 👍

Can we get some LGTM code reviews on this one? Thanks...

rohityadavcloud · 2016-05-03T05:10:20Z

@rafaelweingartner for a project with thousands of commits, splitting the commits for a PR or bug that solves for the same logical issue results in fragmented history. Both commits can individually improve the commit messages, at least they can be amended to including the JIRA bug ID, detail description (what it is, what it solves etc.). Those things are important and useful when we look back in future. If commits are split into multiple ones, it creates issue to track them, to port them etc. I suggest you squash them into one commit.

koushik-das · 2016-05-03T07:05:00Z

server/src/com/cloud/api/query/dao/UserVmJoinDaoImpl.java

@@ -205,14 +205,21 @@ public UserVmResponse newUserVmResponse(ResponseView view, String objectName, Us

                userVmResponse.setNetworkKbsWrite((long)vmStats.getNetworkWriteKBs());

-                if ((userVm.getHypervisorType() != null) && (userVm.getHypervisorType().equals(HypervisorType.KVM) || userVm.getHypervisorType().equals(HypervisorType.XenServer))) { // support KVM and XenServer only util 2013.06.25
+                if ((userVm.getHypervisorType() != null) && (userVm.getHypervisorType().equals(HypervisorType.KVM) || userVm.getHypervisorType().equals(HypervisorType.XenServer) || userVm.getHypervisorType().equals(HypervisorType.VMware))) { // support KVM and XenServer only util 2013.06.25 . supporting Vmware from 2015.09.02


@maneesha-p @rafaelweingartner
Is there any need for all the HV checks? In case these properties are not supported in some HV, the values can be defaulted to something like 0 or -1.
Due to the checks it cannot be run in simulator.

@koushik-das, that is a great question.
I have no idea why we need to check the hypervisor type. This is something that probably comes way before @maneesha-p changes, giving that she/he just added the check for VMware.

I removed the HV checks and ran it on a simulator and worked as expected, returned 0 for the newly added fields in response. Since this is supposed to work for XS, KVM, VMware anyways, and simulator works as well, guess the check can be removed.

rafaelweingartner · 2016-05-03T12:09:16Z

@rhtyd, I understand and I agree with your concerns. The point here is that I am not fragmenting commits, I just cherry picked a commit that was reverted and then I fixed the issues it introduced.

About the commit message, both commits are pretty clear what they do, not?

The commit “7ad3c5e” from @maneesha-p, has the Jira ticket and a description.

And the commit “6c3d8ca” that I introduced, has only a title, but it is pretty clear what it is doing (it fix issues that were introduced by a PR). Do you mean that it should have some Jira ticket opened?

rohityadavcloud · 2016-05-03T15:36:23Z

@rafaelweingartner just checked that the first commit indeed is by someone else, though I think it would be still valid to note in your commit (the 2nd one) the JIRA ID (same as the first one) and summarize details on what extra work you're doing to improve (i.e. why we need it, how we are doing it etc.). Thanks.

It was worked around some possible runtime exceptions introduced by the changes that were added by the PR 780. Basically, the points in which a null pointer exception could happen, we added safety checks to avoid them. It was create a specific method do that, all together test cases were created for this newly method that was added.

rafaelweingartner · 2016-05-03T17:07:34Z

That is a nice suggestion.
I have done that, what do you think now?

rafaelweingartner · 2016-05-04T18:29:04Z

@koushik-das, You are right.
Those variables that were introduced are all primitives; if they have not being loaded, their default value will be zero. We can indeed remove this check.
Thanks for the review and help ;)

swill · 2016-05-06T15:44:22Z

Can we get another code review on this one. Also, can you force push to kick off Jenkins again? Thx...

rafaelweingartner · 2016-05-06T19:29:44Z

@swill done ;)

koushik-das · 2016-05-09T07:50:05Z

Thanks @rafaelweingartner for the updated PR.
LGTM based on code review and verification of PR on simulator.

swill · 2016-05-11T20:24:45Z

CI RESULTS

Tests Run: 85
  Skipped: 0
   Failed: 1
   Errors: 0
 Duration: 9h 12m 09s

Summary of the problem(s):

FAIL: Test redundant router internals
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/git/cs1/cloudstack/test/integration/smoke/test_routers_network_ops.py", line 483, in test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false
    "Attempt to retrieve google.com index page should be successful once rule is added!"
AssertionError: Attempt to retrieve google.com index page should be successful once rule is added!
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_F298R0/results.txt

Associated Uploads

/tmp/MarvinLogs/DeployDataCenter__May_11_2016_00_51_52_QTWNUF:

/tmp/MarvinLogs/test_network_F298R0:

/tmp/MarvinLogs/test_vpc_routers_6U8HTX:

Uploads will be available until 2016-07-11 02:00:00 +0200 CEST

Comment created by upr comment.

swill · 2016-05-11T20:27:42Z

I think this one is pretty much ready. I have one code review on this one. Can I get one more person to look over this for me? Thanks...

CLOUDSTACK-8800 : Improved the listVirtualMachines API call to include memory utilization information for a VMThis PR introduces the changes proposed in PR #780 with some work to make the code null safe. During this PR, I have also removed some unused code. * pr/1444: Removed unnecessary check when creating the “userVmResponse” object. Fixed issues from CLOUDSTACK-8800 that were introduced in PR 780 CLOUDSTACK-8800 : Improved the listVirtualMachines API call to include memory utilization information for a VM for xenserver,kvm and for vmware. Signed-off-by: Will Stevens <williamstevens@gmail.com>

kishankavala mentioned this pull request Mar 21, 2016

CLOUDSTACK-8800 : Improved the listVirtualMachines API call to include memory utilization information for a VM #780

Merged

rafaelweingartner force-pushed the workAroundPR780 branch from 6ba07b9 to 70d0d1f Compare March 22, 2016 01:05

swill reviewed Mar 23, 2016
View reviewed changes

rafaelweingartner changed the title ~~Worked around some possible runtime exceptions introduced in PR 780.~~ CLOUDSTACK-8800 : Improved the listVirtualMachines API call to include memory utilization information for a VM Mar 24, 2016

CLOUDSTACK-8800 : Improved the listVirtualMachines API call to includ…

7ad3c5e

…e memory utilization information for a VM for xenserver,kvm and for vmware.

rafaelweingartner force-pushed the workAroundPR780 branch from 70d0d1f to 6f659e6 Compare April 29, 2016 17:26

rafaelweingartner force-pushed the workAroundPR780 branch from 6f659e6 to 6c3d8ca Compare April 29, 2016 18:54

koushik-das reviewed May 3, 2016
View reviewed changes

rafaelweingartner force-pushed the workAroundPR780 branch from 6c3d8ca to 866ac7b Compare May 3, 2016 17:06

Removed unnecessary check when creating the “userVmResponse” object.

417d9a5

rafaelweingartner force-pushed the workAroundPR780 branch from 1114165 to 417d9a5 Compare May 6, 2016 18:17

asfgit merged commit 417d9a5 into apache:master May 12, 2016

CLOUDSTACK-8800 : Improved the listVirtualMachines API call to include memory utilization information for a VM #1444

CLOUDSTACK-8800 : Improved the listVirtualMachines API call to include memory utilization information for a VM #1444

Conversation

rafaelweingartner commented Mar 19, 2016

terbolous commented Mar 19, 2016

rafaelweingartner commented Mar 19, 2016

remibergsma commented Mar 20, 2016

rafaelweingartner commented Mar 20, 2016 • edited Loading

swill commented Mar 21, 2016

rafaelweingartner commented Mar 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swill commented Apr 27, 2016

CI RESULTS

swill commented Apr 27, 2016

swill commented Apr 27, 2016

CI RESULTS

swill commented Apr 27, 2016

rafaelweingartner commented Apr 27, 2016

swill commented Apr 27, 2016

swill commented Apr 28, 2016

rafaelweingartner commented Apr 29, 2016

rafaelweingartner commented Apr 29, 2016

swill commented Apr 29, 2016

rafaelweingartner commented Apr 29, 2016

swill commented Apr 29, 2016

rohityadavcloud commented May 2, 2016

rafaelweingartner commented May 2, 2016

swill commented May 2, 2016

rohityadavcloud commented May 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelweingartner commented May 3, 2016

rohityadavcloud commented May 3, 2016

rafaelweingartner commented May 3, 2016

rafaelweingartner commented May 4, 2016

swill commented May 6, 2016

rafaelweingartner commented May 6, 2016

koushik-das commented May 9, 2016

swill commented May 11, 2016

CI RESULTS

swill commented May 11, 2016

rafaelweingartner commented Mar 20, 2016 •

edited

Loading