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

maneesha-p · 2015-09-07T06:44:36Z

for xenserver,kvm and for vmware.

asfbot · 2015-09-07T06:59:25Z

cloudstack-pull-rats #518 SUCCESS
This pull request looks good

asfbot · 2015-09-07T07:02:25Z

cloudstack-pull-rats #519 ABORTED

asfbot · 2015-09-07T07:17:53Z

cloudstack-pull-rats #520 ABORTED

asfbot · 2015-09-07T07:27:48Z

cloudstack-pull-rats #521 SUCCESS
This pull request looks good

asfbot · 2015-09-07T08:11:31Z

cloudstack-pull-analysis #451 SUCCESS
This pull request looks good

asfbot · 2015-09-07T08:11:33Z

cloudstack-pull-analysis #452 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-09-07T08:11:35Z

cloudstack-pull-analysis #453 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-09-07T08:12:26Z

cloudstack-pull-analysis #454 ABORTED

asfbot · 2015-09-07T08:48:17Z

cloudstack-pull-rats #523 SUCCESS
This pull request looks good

asfbot · 2015-09-07T08:52:18Z

cloudstack-pull-analysis #457 ABORTED

asfbot · 2015-09-07T09:58:40Z

cloudstack-pull-analysis #456 UNSTABLE
Looks like there's a problem with this pull request

asfbot · 2015-09-07T10:01:39Z

cloudstack-pull-rats #524 SUCCESS
This pull request looks good

asfbot · 2015-09-07T11:12:08Z

cloudstack-pull-analysis #458 SUCCESS
This pull request looks good

rohityadavcloud · 2015-09-08T11:29:36Z

LGTM (though not tested in an environment), anyone wants to review @wido @remibergsma @wilderrodrigues ?

karuturi · 2015-09-08T12:00:46Z

plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

What does 2 mean? Can you use a constant and probably add a comment?

asfbot · 2015-09-09T11:03:17Z

cloudstack-pull-rats #556 SUCCESS
This pull request looks good

asfbot · 2015-09-09T11:51:03Z

cloudstack-pull-rats #557 SUCCESS
This pull request looks good

asfbot · 2015-09-09T12:03:06Z

cloudstack-pull-analysis #491 UNSTABLE
Looks like there's a problem with this pull request

asfbot · 2015-09-09T12:07:59Z

cloudstack-pull-analysis #492 FAILURE
Looks like there's a problem with this pull request

karuturi · 2015-09-10T03:54:25Z

plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

Can you make this private static final and probably use caps for the constants?

maneesha-p · 2016-01-29T10:26:19Z

@bhaisaab @wido @remibergsma Plz review.

wido · 2016-01-29T11:00:56Z

This looks good to me. Hard to test though :(

maneesha-p · 2016-02-24T09:55:12Z

@remibergsma @wido @bhaisaab 2 LGTMs can it be merged?

kishankavala · 2016-03-16T10:33:34Z

...ns/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java

                } else if (param.matches("vbd_.*_write")) {
                    vmStatsAnswer.setDiskWriteKBs(vmStatsAnswer.getDiskWriteKBs() + getDataAverage(dataNode, col, numRows) / 1000);
+                } else if (param.contains("memory_internal_free")) {
+                    vmStatsAnswer.setIntFreeMemoryKBs(vmStatsAnswer.getIntFreeMemoryKBs() + getDataAverage(dataNode, col, numRows) / 1000);


Bytes to KB conversion should use 1024 instead of 1000

Updated.Thank you.

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

kishankavala · 2016-03-16T11:03:16Z

Tested Manually. memorykbs, memoryintfreekbs,memorytargetkbs are listed correctly.
LGTM.

<?xml version="1.0" encoding="UTF-8"?><listvirtualmachinesresponse cloud-stack-version="4.9.0-SNAPSHOT"><count>1</count><virtualmachine><id>a574e9d1-8057-4195-a8b3-2117e9059652</id><name>VM-a574e9d1-8057-4195-a8b3-2117e9059652</name><displayname>VM-a574e9d1-8057-4195-a8b3-2117e9059652</displayname><account>admin</account><userid>ee621684-952a-11e5-abd2-d4ae52cb9a54</userid><username>admin</username><domainid>ee5bfd6f-952a-11e5-abd2-d4ae52cb9a54</domainid><domain>ROOT</domain><created>2015-11-27T23:30:46+0530</created><state>Running</state><haenable>false</haenable><zoneid>b5503a30-f55b-4889-94c2-32a73c6b4166</zoneid><zonename>KK</zonename><hostid>57382ec5-87e1-4c0b-9df1-02b2cba04971</hostid><hostname>Rack3Pod1Host44</hostname><templateid>ee3b1574-952a-11e5-abd2-d4ae52cb9a54</templateid><templatename>CentOS 5.6(64-bit) no GUI (XenServer)</templatename><templatedisplaytext>CentOS 5.6(64-bit) no GUI (XenServer)</templatedisplaytext><passwordenabled>false</passwordenabled><serviceofferingid>1d88488d-6651-4a63-b865-a3a5f4115dd6</serviceofferingid><serviceofferingname>custom</serviceofferingname><cpunumber>4</cpunumber><cpuspeed>512</cpuspeed><memory>128</memory><cpuused>0.04%</cpuused><networkkbsread>0</networkkbsread><networkkbswrite>0</networkkbswrite><diskkbsread>0</diskkbsread><diskkbswrite>11</diskkbswrite><memorykbs>131072</memorykbs><memoryintfreekbs>15</memoryintfreekbs><memorytargetkbs>131072</memorytargetkbs><diskioread>0</diskioread><diskiowrite>0</diskiowrite><guestosid>ee4c163d-952a-11e5-abd2-d4ae52cb9a54</guestosid><rootdeviceid>0</rootdeviceid><rootdevicetype>ROOT</rootdevicetype><nic><id>66347c35-f51f-4527-bb59-50a976d90927</id><networkid>d97370e1-7fb3-44be-8af5-9515c1d624a2</networkid><networkname>n1</networkname><netmask>255.255.255.0</netmask><gateway>10.1.1.1</gateway><ipaddress>10.1.1.14</ipaddress><isolationuri>vlan://514</isolationuri><broadcasturi>vlan://514</broadcasturi><traffictype>Guest</traffictype><type>Isolated</type><isdefault>true</isdefault><macaddress>02:00:66:75:00:01</macaddress></nic><hypervisor>XenServer</hypervisor><instancename>i-2-3-VM</instancename><details>{hypervisortoolsversion=xenserver56}</details><displayvm>true</displayvm><isdynamicallyscalable>true</isdynamicallyscalable><ostypeid>142</ostypeid></virtualmachine></listvirtualmachinesresponse>

CLOUDSTACK-8800 : Improved the listVirtualMachines API call to include memory utilization information for a VMfor xenserver,kvm and for vmware. * 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: Kishan Kavala <[email protected]>

swill · 2016-03-17T13:55:31Z

plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

            newStat._bytesRead = bytes_rd;
            newStat._bytesWrote = bytes_wr;
            newStat._timestamp = now;
+            newStat._intmemfree = mems[0].getValue();


Is this guaranteed to have a result? This looks like it is prime to introduce a Null Pointer Exception...

swill · 2016-03-17T13:58:38Z

I see this was merged into master this morning. Hmmm...

rafaelweingartner · 2016-03-17T14:05:23Z

plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java


+            stats.setMemoryKBs(info.maxMem);
+            stats.setTargetMemoryKBs(info.memory);
+            stats.setIntFreeMemoryKBs((double) mems[0].getValue());


the same problem pointed out by @swill at line 3128, may happen here.

swill · 2016-03-17T14:08:01Z

should we revert this merge in favor of fixing these issues?

ustcweizhou · 2016-03-17T14:32:40Z

@swill you can create a PR if you want.

swill · 2016-03-17T14:56:22Z

@ustcweizhou the point is more that there are things in this PR that I am not comfortable with and as the 4.9 RM, I need to be comfortable with the code that goes into master.

rafaelweingartner · 2016-03-17T15:29:04Z

@swill, I understand your position; as you, I also think that there was room for improvements in this PR (especially to avoid possible runtime exception). However, reverting a PR is always something complicated. I believe we should wait to hear feedback from others, before taking any decision.

swill · 2016-03-17T15:44:06Z

@rafaelweingartner I don't mind waiting a bit, but runtime exceptions need to be taken seriously. As the 4.9 RM, it is my responsibility to make sure this type of stuff is sorted out BEFORE it gets merged into master. I welcome others to give feedback, but I would recommend that we do a git revert on this merge and at the very least code defensively around the runtime exceptions.

remibergsma · 2016-03-18T19:49:49Z

@swill Please do proceed with revert, I totally agree.

git revert -m 1 dc0ba6bd1a774d3ff4bc4a4dcc00e1434ab1f6e3 will do. Let me know if you need help.

DaanHoogland · 2016-03-21T10:08:26Z

@swill I would not allow any other merge before this is fixed or reverted!

kishankavala · 2016-03-21T10:49:00Z

@swill @remibergsma @DaanHoogland PR was open since Sep 2015. Review from @swill came after the PR was merged on Mar 17 2016. By then, there were code reviews and tests done with 3 LGTMs. I merged the PR as per the current process.
Unfortunately all reviewers missed the issued mentioned by @swill
Thanks to @rafaelweingartner #1444 addresses the runtime exception issues.
Unless there are issues with PR 1444, I do not see a need to revert this merge. Can we all please review PR 1444 and close this issue?

DaanHoogland · 2016-03-21T11:10:02Z

@kishankavala you are formally correct and we had not closed master for release so I don't blame you for the merge. The issue as brought up by @swill is not to blame anybody for the merge but to preserve the quality of master before doing so. I am just saying that we should close master now and not merge anything before this is fixed or reverted, whatever.

koushik-das · 2016-03-21T12:39:18Z

@DaanHoogland I don't see any reason for closing master in this case. If an issue is noticed after a merge, it definitely needs to be tracked and fixed. Releasing a branch can definitely be blocked if the issues (particularly regressions) are not addressed but I don't see any reason to close master for other commits. I think that closing master makes sense only if build is broken or there is a blocking regression introduced after the last release.

remibergsma · 2016-03-21T13:17:37Z

Guys, LGTMs or not, there were no integration test results posted so it shouldn't have been merged.

swill · 2016-03-21T21:36:29Z

Hey Guys, Sorry I have been MIA on this. I have been traveling and have limited connectivity.

I am going to revert this merge. There are visible problems with this merge and there were no integration tests run. @rafaelweingartner has offered to merge these changes into #1444, which fix the issues in this commit and allow us to run integration tests on this commit before merging it in.

Sorry for the delay following up on this and getting a clear resolution in place.

…lity issues and lack of CI results. This reverts commit dc0ba6b, reversing changes made to 63f58dd.

swill · 2016-03-23T18:14:16Z

Can we continue the conversation at #1444 please. This commit has been reverted and has been pulled into #1444 to continue QA of the code. Please come contribute at #1444 and continue getting this code ready for production. Thx...

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 <[email protected]>

* Remember tab on page change * fix for resourceview * Fix instance tab * Fix vpc tab * Fix kubernetes tab Signed-off-by: Rohit Yadav <[email protected]>

maneesha-p force-pushed the pull-19 branch 2 times, most recently from 6fde3d1 to 58f4bbb Compare September 7, 2015 06:52

maneesha-p force-pushed the pull-19 branch from 58f4bbb to fa4731f Compare September 7, 2015 07:14

maneesha-p force-pushed the pull-19 branch from fa4731f to aa41d58 Compare September 7, 2015 08:39

maneesha-p force-pushed the pull-19 branch from aa41d58 to e3f8cb9 Compare September 7, 2015 09:47

karuturi reviewed Sep 8, 2015
View reviewed changes

maneesha-p force-pushed the pull-19 branch from e3f8cb9 to 9978eb2 Compare September 9, 2015 10:14

maneesha-p force-pushed the pull-19 branch from 9978eb2 to 78fa786 Compare September 9, 2015 11:38

karuturi reviewed Sep 10, 2015
View reviewed changes

kishankavala reviewed Mar 16, 2016
View reviewed changes

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

732a852

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

maneesha-p force-pushed the pull-19 branch from 262782c to 732a852 Compare March 16, 2016 10:51

asfgit merged commit 732a852 into apache:master Mar 17, 2016

swill reviewed Mar 17, 2016
View reviewed changes

rafaelweingartner reviewed Mar 17, 2016
View reviewed changes

rafaelweingartner mentioned this pull request Mar 19, 2016

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

Merged

asfgit pushed a commit that referenced this pull request Mar 21, 2016

Revert "Merge pull request #780 from maneesha-p/pull-19" due code qua…

c48b6c3

…lity issues and lack of CI results. This reverts commit dc0ba6b, reversing changes made to 63f58dd.

rohityadavcloud pushed a commit that referenced this pull request Jan 20, 2021

ui: Remember tab on page change (#780)

87c1950

* Remember tab on page change * fix for resourceview * Fix instance tab * Fix vpc tab * Fix kubernetes tab Signed-off-by: Rohit Yadav <[email protected]>

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

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

Uh oh!

Conversation

maneesha-p commented Sep 7, 2015

Uh oh!

asfbot commented Sep 7, 2015

Uh oh!

asfbot commented Sep 7, 2015

Uh oh!

asfbot commented Sep 7, 2015

Uh oh!

asfbot commented Sep 7, 2015

Uh oh!

asfbot commented Sep 7, 2015

Uh oh!

asfbot commented Sep 7, 2015

Uh oh!

asfbot commented Sep 7, 2015

Uh oh!

asfbot commented Sep 7, 2015

Uh oh!

asfbot commented Sep 7, 2015

Uh oh!

asfbot commented Sep 7, 2015

Uh oh!

asfbot commented Sep 7, 2015

Uh oh!

asfbot commented Sep 7, 2015

Uh oh!

asfbot commented Sep 7, 2015

Uh oh!

rohityadavcloud commented Sep 8, 2015

Uh oh!

karuturi Sep 8, 2015

Choose a reason for hiding this comment

Uh oh!

asfbot commented Sep 9, 2015

Uh oh!

asfbot commented Sep 9, 2015

Uh oh!

asfbot commented Sep 9, 2015

Uh oh!

asfbot commented Sep 9, 2015

Uh oh!

karuturi Sep 10, 2015

Choose a reason for hiding this comment

Uh oh!

maneesha-p commented Jan 29, 2016

Uh oh!

wido commented Jan 29, 2016

Uh oh!

maneesha-p commented Feb 24, 2016

Uh oh!

kishankavala Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

maneesha-p Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

kishankavala commented Mar 16, 2016

Uh oh!

swill Mar 17, 2016

Choose a reason for hiding this comment

Uh oh!

swill commented Mar 17, 2016

Uh oh!

rafaelweingartner Mar 17, 2016

Choose a reason for hiding this comment

Uh oh!

swill commented Mar 17, 2016

Uh oh!

ustcweizhou commented Mar 17, 2016

Uh oh!

swill commented Mar 17, 2016

Uh oh!

rafaelweingartner commented Mar 17, 2016

Uh oh!

swill commented Mar 17, 2016

Uh oh!