You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by rafaelweingartner <gi...@git.apache.org> on 2016/03/19 20:43:05 UTC
[GitHub] cloudstack pull request: Worked around some possible runtime excep...
GitHub user rafaelweingartner opened a pull request:
https://github.com/apache/cloudstack/pull/1444
Worked around some possible runtime exceptions introduced in PR 780.
@Remi, @remibergsma I have created this PR to work out those possible problems introduce by PR #780 that may happen during runtime.
What do you think? Would it be better than reverting the PR?
During this PR, I have also removed some unused code from that class.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rafaelweingartner/cloudstack workAroundPR780
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1444.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1444
----
commit 6ba07b97477910526bfc9f13238c0ead76a5fa78
Author: weingartner <ra...@gmail.com>
Date: 2016-03-19T19:31:32Z
Worked around some possible runtime exceptions introduced in PR 780.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-217537499
@swill done ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216597727
That is a nice suggestion.
I have done that, what do you think now?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-218579831
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...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215109078
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???
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215833023
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215111745
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Worked around some possible runtime excep...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-198881402
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r62006414
--- Diff: 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
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216569013
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Worked around some possible runtime excep...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-199568119
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215829693
@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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216508737
@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 \u201c7ad3c5e\u201d from @maneesha-p, has the Jira ticket and a description.
And the commit \u201c6c3d8ca\u201d 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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1444
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-218579019
### 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:`**
* [dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/DeployDataCenter__May_11_2016_00_51_52_QTWNUF/dc_entries.obj)
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/DeployDataCenter__May_11_2016_00_51_52_QTWNUF/failed_plus_exceptions.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/DeployDataCenter__May_11_2016_00_51_52_QTWNUF/runinfo.txt)
**`/tmp/MarvinLogs/test_network_F298R0:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_network_F298R0/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_network_F298R0/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_network_F298R0/runinfo.txt)
**`/tmp/MarvinLogs/test_vpc_routers_6U8HTX:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_vpc_routers_6U8HTX/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_vpc_routers_6U8HTX/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_vpc_routers_6U8HTX/runinfo.txt)
Uploads will be available until `2016-07-11 02:00:00 +0200 CEST`
*Comment created by [`upr comment`](https://github.com/cloudops/upr).*
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-217798010
Thanks @rafaelweingartner for the updated PR.
LGTM based on code review and verification of PR on simulator.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Worked around some possible runtime excep...
Posted by eriweb <gi...@git.apache.org>.
Github user eriweb commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-198791923
Thanks for your PR @rafaelweingartner, for tracking purposes, would you mind creating a jira ticket and including the ticket id in the PR title?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Worked around some possible runtime excep...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-199500059
@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. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215554170
@rafaelweingartner please rebase as we currently have merge conflicts. Thanks...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r58375427
--- Diff: plugins/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
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r58316653
--- Diff: plugins/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
--- End diff --
@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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r61844330
--- Diff: 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
--- End diff --
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-217479698
Can we get another code review on this one. Also, can you force push to kick off Jenkins again? Thx...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Worked around some possible runtime excep...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r57194036
--- Diff: plugins/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
--- End diff --
You specify that memory must be greater than zero in this comment, but it uses `>=` in the actual logic. Can you explain?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Worked around some possible runtime excep...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r57208531
--- Diff: plugins/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
--- End diff --
I understand. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216264551
@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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215108116
### 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:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_usage_0CACSG/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_usage_0CACSG/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_usage_0CACSG/runinfo.txt)
**`/tmp/MarvinLogs/test_usage_46FO5S:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_usage_46FO5S/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_usage_46FO5S/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_usage_46FO5S/runinfo.txt)
Uploads will be available until `2016-06-27 02:00:00 +0200 CEST`
*Comment created by [`upr comment`](https://github.com/cloudops/upr).*
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215122425
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215822574
@swill conflicts solved
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Worked around some possible runtime excep...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r57202601
--- Diff: plugins/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
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216266457
I think the two commits are fine in this case, so just leave it as it is. \U0001f44d
Can we get some LGTM code reviews on this one? Thanks...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215847599
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...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215094461
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r58480834
--- Diff: plugins/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
--- End diff --
Ok, that does make sense. I will get this tested and we will go from there. Thanks for the review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1444#discussion_r61870927
--- Diff: 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
--- End diff --
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Worked around some possible runtime excep...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-198889972
@remibergsma I understand your point.
Anyways if we decide to revert that PR, I can get the changes it introduce and apply here; then we can follow the merge process by the letter.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Worked around some possible runtime excep...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-198797607
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216226029
@rafaelweingartner please rebase against master, and squash changes to a single commit
tag:easypr
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215832199
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...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-215090037
### 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:`**
* [dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/DeployDataCenter__Apr_27_2016_06_50_55_VQRZDI/dc_entries.obj)
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/DeployDataCenter__Apr_27_2016_06_50_55_VQRZDI/failed_plus_exceptions.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/DeployDataCenter__Apr_27_2016_06_50_55_VQRZDI/runinfo.txt)
**`/tmp/MarvinLogs/test_network_NAELYA:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_network_NAELYA/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_network_NAELYA/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_network_NAELYA/runinfo.txt)
**`/tmp/MarvinLogs/test_vpc_routers_Z0GM3S:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_vpc_routers_Z0GM3S/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_vpc_routers_Z0GM3S/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1444/tmp/MarvinLogs/test_vpc_routers_Z0GM3S/runinfo.txt)
Uploads will be available until `2016-06-27 02:00:00 +0200 CEST`
*Comment created by [`upr comment`](https://github.com/cloudops/upr).*
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216436317
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1444#issuecomment-216957591
@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 ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---