GeezerVison

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

cloudstack-pull-rats #519 ABORTED

cloudstack-pull-rats #520 ABORTED

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

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

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

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

cloudstack-pull-analysis #454 ABORTED

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

cloudstack-pull-analysis #457 ABORTED

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

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

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

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

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

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

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

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

@bhaisaab @wido @remibergsma Plz review.

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

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

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>

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

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

@swill you can create a PR if you want.

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

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

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

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

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

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

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

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

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

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

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.

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