CLOUDSTACK-8800 : Improved the listVirtualMachines API call to include memory utilization information for a VM by maneesha-p · Pull Request #780 · apache/cloudstack

 } 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 <kishan@apache.org>

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

 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


+ 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 <williamstevens@gmail.com>

* Remember tab on page change * fix for resourceview * Fix instance tab * Fix vpc tab * Fix kubernetes tab Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

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

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

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

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

karuturi reviewed Sep 8, 2015
View reviewed changes

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

maneesha-p force-pushed the pull-19 branch from 9978eb2 to 78fa786Compare 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 732a852Compare March 16, 2016 10:51

asfgit merged commit 732a852 into apache:masterMar 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#780from maneesha-p/pull-19" due code qua…
c48b6c3
…lity issues and lack of CI results. This reverts commit dc0ba6b, reversing changes made to 63f58dd.

yadvr 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 <rohit.yadav@shapeblue.com>

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!

yadvr commented Sep 8, 2015

Uh oh!

karuturiSep 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!

karuturiSep 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!

kishankavalaMar 16, 2016

Choose a reason for hiding this comment

Uh oh!

maneesha-pMar 16, 2016

Choose a reason for hiding this comment

Uh oh!

kishankavala commented Mar 16, 2016

Uh oh!

swillMar 17, 2016

Choose a reason for hiding this comment

Uh oh!

swill commented Mar 17, 2016

Uh oh!

rafaelweingartnerMar 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!

remibergsma commented Mar 18, 2016

Uh oh!