date:20160506

[GitHub] cloudstack pull request: CLOUDSTACK-9199: Fixed deployVirtualMachi...

2016-05-06 Thread anshul1886

Github user anshul1886 commented on the pull request:

https://github.com/apache/cloudstack/pull/1280#issuecomment-217610090
  
@rhtyd @swill There will not be backward compatibility issues as with 
static offering those parameters were not taken into consideration. They were 
wrongly giving the impression that they are being used.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-217606327
  


### CI RESULTS

```
Tests Run: 85
  Skipped: 0
   Failed: 2
   Errors: 0
 Duration: 6h 11m 18s
```

**Summary of the problem(s):**
```
FAIL: Test redundant router internals
--
Traceback (most recent call last):
  File 
"/data/git/cs2/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_MN3MB5/results.txt
```

```
FAIL: Test redundant router internals
--
Traceback (most recent call last):
  File 
"/data/git/cs2/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_MN3MB5/results.txt
```



**Associated Uploads**

**`/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_34_18_ZVNY8R:`**
* 
[dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_34_18_ZVNY8R/dc_entries.obj)
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_34_18_ZVNY8R/failed_plus_exceptions.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_34_18_ZVNY8R/runinfo.txt)

**`/tmp/MarvinLogs/test_network_MN3MB5:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_network_MN3MB5/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_network_MN3MB5/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_network_MN3MB5/runinfo.txt)

**`/tmp/MarvinLogs/test_vpc_routers_DZWFXD:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_vpc_routers_DZWFXD/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_vpc_routers_DZWFXD/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_vpc_routers_DZWFXD/runinfo.txt)


Uploads will be available until `2016-07-07 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217606318
  


### CI RESULTS

```
Tests Run: 85
  Skipped: 0
   Failed: 1
   Errors: 0
 Duration: 6h 07m 49s
```

**Summary of the problem(s):**
```
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 
332, in performVPCTests
self.check_pvt_gw_connectivity(vm2, public_ip_2, vm1.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_FIVJNA/results.txt
```



**Associated Uploads**

**`/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_31_06_LPI7VW:`**
* 
[dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_31_06_LPI7VW/dc_entries.obj)
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_31_06_LPI7VW/failed_plus_exceptions.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_31_06_LPI7VW/runinfo.txt)

**`/tmp/MarvinLogs/test_network_FIVJNA:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/test_network_FIVJNA/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/test_network_FIVJNA/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/test_network_FIVJNA/runinfo.txt)

**`/tmp/MarvinLogs/test_vpc_routers_WUHEVI:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/test_vpc_routers_WUHEVI/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/test_vpc_routers_WUHEVI/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/test_vpc_routers_WUHEVI/runinfo.txt)


Uploads will be available until `2016-07-07 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9287 - Fix unique mac address ...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1483#issuecomment-217605374
  


### CI RESULTS

```
Tests Run: 85
  Skipped: 0
   Failed: 1
   Errors: 1
 Duration: 10h 26m 38s
```

**Summary of the problem(s):**
```
FAIL: Test destroy(expunge) Virtual Machine
--
Traceback (most recent call last):
  File 
"/data/git/cs1/cloudstack/test/integration/smoke/test_vm_life_cycle.py", line 
646, in test_09_expunge_vm
self.assertEqual(list_vm_response,None,"Check Expunged virtual machine 
is in listVirtualMachines response")
AssertionError: Check Expunged virtual machine is in listVirtualMachines 
response
--
Additional details in: /tmp/MarvinLogs/test_vpc_routers_LJEVIW/results.txt
```

```
ERROR: Test to verify access to loadbalancer haproxy admin stats page
--
Traceback (most recent call last):
  File 
"/data/git/cs1/cloudstack/test/integration/smoke/test_internal_lb.py", line 
854, in tearDown
raise Exception("Cleanup failed with %s" % e)
Exception: Cleanup failed with Job failed: {jobprocstatus : 0, created : 
u'2016-05-07T01:17:22+0200', jobresult : {errorcode : 530, errortext : u'Failed 
to delete network'}, cmd : 
u'org.apache.cloudstack.api.command.user.network.DeleteNetworkCmd', userid : 
u'ecdfa0ce-13af-11e6-929e-5254001daa61', jobstatus : 2, jobid : 
u'7ba837e5-36f2-4467-ab1b-69305fe3c897', jobresultcode : 530, jobresulttype : 
u'object', jobinstancetype : u'Network', accountid : 
u'ecdf8a0d-13af-11e6-929e-5254001daa61'}
--
Additional details in: /tmp/MarvinLogs/test_network_4LL71G/results.txt
```



**Associated Uploads**

**`/tmp/MarvinLogs/DeployDataCenter__May_06_2016_19_30_38_6T9JP6:`**
* 
[dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1483/tmp/MarvinLogs/DeployDataCenter__May_06_2016_19_30_38_6T9JP6/dc_entries.obj)
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1483/tmp/MarvinLogs/DeployDataCenter__May_06_2016_19_30_38_6T9JP6/failed_plus_exceptions.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1483/tmp/MarvinLogs/DeployDataCenter__May_06_2016_19_30_38_6T9JP6/runinfo.txt)

**`/tmp/MarvinLogs/test_network_4LL71G:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1483/tmp/MarvinLogs/test_network_4LL71G/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1483/tmp/MarvinLogs/test_network_4LL71G/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1483/tmp/MarvinLogs/test_network_4LL71G/runinfo.txt)

**`/tmp/MarvinLogs/test_vpc_routers_LJEVIW:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1483/tmp/MarvinLogs/test_vpc_routers_LJEVIW/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1483/tmp/MarvinLogs/test_vpc_routers_LJEVIW/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1483/tmp/MarvinLogs/test_vpc_routers_LJEVIW/runinfo.txt)


Uploads will be available until `2016-07-07 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Reimplement router.redundant.vrrp.interva...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1486#issuecomment-217605326
  


### CI RESULTS

```
Tests Run: 85
  Skipped: 0
   Failed: 0
   Errors: 0
 Duration: 8h 49m 25s
```



**Associated Uploads**

**`/tmp/MarvinLogs/DeployDataCenter__May_06_2016_20_02_11_2X0JYY:`**
* 
[dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1486/tmp/MarvinLogs/DeployDataCenter__May_06_2016_20_02_11_2X0JYY/dc_entries.obj)
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1486/tmp/MarvinLogs/DeployDataCenter__May_06_2016_20_02_11_2X0JYY/failed_plus_exceptions.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1486/tmp/MarvinLogs/DeployDataCenter__May_06_2016_20_02_11_2X0JYY/runinfo.txt)

**`/tmp/MarvinLogs/test_network_W1WTK6:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1486/tmp/MarvinLogs/test_network_W1WTK6/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1486/tmp/MarvinLogs/test_network_W1WTK6/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1486/tmp/MarvinLogs/test_network_W1WTK6/runinfo.txt)

**`/tmp/MarvinLogs/test_vpc_routers_CEKKSO:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1486/tmp/MarvinLogs/test_vpc_routers_CEKKSO/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1486/tmp/MarvinLogs/test_vpc_routers_CEKKSO/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1486/tmp/MarvinLogs/test_vpc_routers_CEKKSO/runinfo.txt)


Uploads will be available until `2016-07-07 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Convert patchviasocket to python (removes...

2016-05-06 Thread DaanHoogland

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1533#issuecomment-217569401
  
Sorry Will, Yes all passed, some after retest.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Convert patchviasocket to python (removes...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1533#issuecomment-217567369
  
I am missing some code review on this one, but otherwise this is looking 
good so far.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Convert patchviasocket to python (removes...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1533#issuecomment-217566881
  
@DaanHoogland can you post a 'summary' of the results when you post this.  
I don't want to have to download each of these files to know that they ran 
successfully.  Can you just say something like "CI Passed" or something like 
that?  "Here are the results" without telling me if it is a success for failure 
requires me to download and review every file.  I understand from your "dont 
bother with" comment that everything passed except that, but if you can just 
let me know that status, that will help me.  Thanks for testing this.  ð 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Convert patchviasocket to python (removes...

2016-05-06 Thread DaanHoogland

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1533#issuecomment-217560805
  
I hadn't looked into the code yet so for what it's worth and pending 
response to the comments:
CI RESULTS

[1533.results.internal_lb.txt](https://github.com/apache/cloudstack/files/253034/1533.results.internal_lb.txt)

[1533.results.network.txt](https://github.com/apache/cloudstack/files/253035/1533.results.network.txt)

[1533.results.vpc_routers.txt](https://github.com/apache/cloudstack/files/253037/1533.results.vpc_routers.txt)

[1533.results.vpc_vpn.txt](https://github.com/apache/cloudstack/files/253036/1533.results.vpc_vpn.txt)

I didn't bother with the known ping problem in 
test_02_redundant_VPC_default_routes


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Apply static routes on change to master s...

2016-05-06 Thread dmabry

Github user dmabry commented on the pull request:

https://github.com/apache/cloudstack/pull/1472#issuecomment-217552585
  
So, Si (@kiwiflyer) and I have been testing this functionality against VPCs 
in our lab and we verified that static routes are indeed loaded when a VR goes 
from BACKUP to MASTER.  The relevant logs showing that is works are below.  We 
are however still having problems with the static routes not getting added on 
demand to the current MASTER VR.  Has anyone else had this issue while running 
this commit?

```
2016-05-06 20:32:20,795 DEBUG{u'config': {u'router_id': u'2', 
u'baremetalnotificationapikey': 
u'QtWfDPFOrqddFe3iofWCS6Yhf6g2cCTwR6n3-L7TaaKl3ReBI4219jckJ0AfdX878jo2DpW4ohhNG_6Hlad62g',
 u'baremetalnotificationsecuritykey': 
u'B99mUf0oWKL7ATA5WF9iMb17nk1ePQ6kgg3YgWA0kJE2vCaGH52Z65EWDAyCaOYiZB1YPr40pWiwTlBt7_TTmg',
 u'name': u'r-28-VM', u'eth0mask': u'255.255.0.0', u'type': u'vpcrouter', 
u'dns1': u'207.191.191.90', u'privategateway': u'192.168.4.4', u'vpccidr': 
u'10.15.0.0/22', u'domain': u'cs2cloud.internal', u'redundant_state': 'BACKUP', 
u'host': u'10.150.0.201', u'disable_rp_filter': u'true', u'redundant_router': 
u'true', u'router_password': 
u'3838383966687309145665215760380045387957133810480803993986340699863084083818544695063990662780164604461468366180051318649230056294824943648327772148445755',
 u'redundant_master': False, u'port': u'8080', u'eth0ip': u'169.254.3.21', 
u'template': u'domP'}, u'id': u'cmdline'}
2016-05-06 20:32:20,796 DEBUGLoading data bag type ips
2016-05-06 20:32:20,796 DEBUGSetting router to master
2016-05-06 20:32:20,796 INFO Will proceed configuring device ==> eth2
2016-05-06 20:32:20,797 DEBUGExecuting: ip link set eth2 up
2016-05-06 20:32:20,800 INFO Bringing public interface eth2 up
2016-05-06 20:32:20,800 INFO Adding gateway ==> 192.168.4.1 to device 
==> eth2
2016-05-06 20:32:20,801 INFO Will proceed configuring device ==> eth1
2016-05-06 20:32:20,801 DEBUGExecuting: ip link set eth1 up
2016-05-06 20:32:20,804 INFO Bringing public interface eth1 up
2016-05-06 20:32:20,804 INFO Adding gateway ==> 10.152.0.1 to device 
==> eth1
2016-05-06 20:32:20,804 INFO Checking if default ipv4 route is present
2016-05-06 20:32:20,804 DEBUGExecuting: ip -4 route list 0/0
2016-05-06 20:32:20,807 WARNING  No default route found!
2016-05-06 20:32:20,807 INFO Adding default route
2016-05-06 20:32:20,807 DEBUGExecuting: ip route show default via 
10.152.0.1
2016-05-06 20:32:20,810 INFO Add default via 10.152.0.1
2016-05-06 20:32:20,810 DEBUGExecuting: ip route add default via 
10.152.0.1
2016-05-06 20:32:20,812 DEBUGConfiguring static routes
2016-05-06 20:32:20,813 DEBUGLoading data bag type staticroutes
2016-05-06 20:32:20,813 DEBUGProcessing CsStaticRoutes file ==> {u'id': 
u'staticroutes', u'10.87.0.0/16': {u'revoke': False, u'ip_address': 
u'192.168.4.4', u'gateway': u'192.168.4.1', u'network': u'10.87.0.0/16'}}
2016-05-06 20:32:20,813 DEBUGExecuting: ip route show | grep 
10.87.0.0/16 | awk '{print $1, $3}'
2016-05-06 20:32:20,818 DEBUGExecuting: ip route add 10.87.0.0/16 via 
192.168.4.1
2016-05-06 20:32:20,821 DEBUGExecuting: /usr/sbin/conntrackd -C 
/etc/conntrackd/conntrackd.conf -c
2016-05-06 20:32:20,824 DEBUGExecuting: /usr/sbin/conntrackd -C 
/etc/conntrackd/conntrackd.conf -f
2016-05-06 20:32:20,828 DEBUGExecuting: /usr/sbin/conntrackd -C 
/etc/conntrackd/conntrackd.conf -R
2016-05-06 20:32:20,831 DEBUGExecuting: /usr/sbin/conntrackd -C 
/etc/conntrackd/conntrackd.conf -B
```



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...

2016-05-06 Thread rafaelweingartner

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9348: Optimize NioTest and Nio...

2016-05-06 Thread asfgit

Github user asfgit closed the pull request at:

https://github.com/apache/cloudstack/pull/1534


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9348: Optimize NioTest and Nio...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1534#issuecomment-217521378
  
I am not concerned about the two failures.  One happens randomly in my 
environment and one is a cleanup issue between test runs which is not related 
to this PR.

Since `master` is currently broken due to some issues with #1493, I am 
going to merge this right away...


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9348: Optimize NioTest and Nio...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1534#issuecomment-217519767
  


### CI RESULTS

```
Tests Run: 85
  Skipped: 0
   Failed: 2
   Errors: 0
 Duration: 4h 31m 04s
```

**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_C0JJZR/results.txt
```

```
FAIL: test_02_vpc_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 
253, in test_02_vpc_privategw_static_routes
self.performVPCTests(vpc_off)
  File 
"/data/git/cs1/cloudstack/test/integration/smoke/test_privategw_acl.py", line 
304, in performVPCTests
privateGw_1 = self.createPvtGw(vpc_1, "10.0.3.100", "10.0.3.101", 
acl1.id, vlan_1)
  File 
"/data/git/cs1/cloudstack/test/integration/smoke/test_privategw_acl.py", line 
472, in createPvtGw
self.fail("Failed to create Private Gateway ==> %s" % e)
AssertionError: Failed to create Private Gateway ==> Execute cmd: 
createprivategateway failed, due to: errorCode: 431, errorText:Network with 
vlan vlan://100 already exists in zone 1
--
Additional details in: /tmp/MarvinLogs/test_network_C0JJZR/results.txt
```



**Associated Uploads**

**`/tmp/MarvinLogs/DeployDataCenter__May_06_2016_15_29_07_FHJDSL:`**
* 
[dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1534/tmp/MarvinLogs/DeployDataCenter__May_06_2016_15_29_07_FHJDSL/dc_entries.obj)
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1534/tmp/MarvinLogs/DeployDataCenter__May_06_2016_15_29_07_FHJDSL/failed_plus_exceptions.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1534/tmp/MarvinLogs/DeployDataCenter__May_06_2016_15_29_07_FHJDSL/runinfo.txt)

**`/tmp/MarvinLogs/test_network_C0JJZR:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1534/tmp/MarvinLogs/test_network_C0JJZR/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1534/tmp/MarvinLogs/test_network_C0JJZR/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1534/tmp/MarvinLogs/test_network_C0JJZR/runinfo.txt)

**`/tmp/MarvinLogs/test_vpc_routers_7C30C4:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1534/tmp/MarvinLogs/test_vpc_routers_7C30C4/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1534/tmp/MarvinLogs/test_vpc_routers_7C30C4/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1534/tmp/MarvinLogs/test_vpc_routers_7C30C4/runinfo.txt)


Uploads will be available until `2016-07-06 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217515255
  
Both this one and dynamic roles are queued up for CI as soon as I stabilize 
master.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CPU socket count reporting correction

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1520#issuecomment-217514394
  
I am comfortable not running CI on this one given the scope of the change 
and the manual tests that have been run.  This one is ready now.  We need this 
force pushed again though so we can get Jenkins green.  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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Addresses CLOUDSTACK-9300 where the MySQL...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1428#issuecomment-217513880
  
Ok, I will get CI run against this one to make sure that nothing else is 
broken.  This is ready pending the CI run...


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Addresses CLOUDSTACK-9300 where the MySQL...

2016-05-06 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1428#issuecomment-217507957
  
LGTM (code review only)


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CPU socket count reporting correction

2016-05-06 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1520#issuecomment-217507357
  
LGTM (code review)

tag:mergeready


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: systemvmtemplate: fix build and upgrade t...

2016-05-06 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1531#issuecomment-217506509
  
Here is a build log to prove that build succeeds using a Ubuntu 16.04 slave 
box with VirtualBox 5.0:


[systemvmtemplate-build-consoleText.txt](https://github.com/apache/cloudstack/files/252729/systemvmtemplate-build-consoleText.txt)

On existing environment, not using the latest Ruby version; the only change 
they will need to do is update ruby to 2.3.0 (using rvm or global).

Build steps are simply:
```
cd tools/appliance
bundle install
bash -l build.sh systemvm64template
```


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: systemvmtemplate: fix build and upgrade t...

2016-05-06 Thread rhtyd

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1531#discussion_r62361025
  
--- Diff: tools/appliance/definitions/systemvmtemplate/definition.rb ---
@@ -27,15 +27,15 @@
 architectures = {
 :i386 => {
 :os_type_id => 'Debian',
-:iso_file => 'debian-7.9.0-i386-netinst.iso',
-:iso_src => 
'http://cdimage.debian.org/cdimage/archive/7.9.0/i386/iso-cd/debian-7.9.0-i386-netinst.iso',
-:iso_md5 => 'e101a11ddb31f85acef542df1a49bf57',
+:iso_file => 'debian-7.10.0-i386-netinst.iso',
+:iso_src => 
'http://cdimage.debian.org/cdimage/archive/7.10.0/i386/iso-cd/debian-7.10.0-i386-netinst.iso',
+:iso_md5 => '7fa97966e31afc705b3e115906cbcaa5',
--- End diff --

Uses the latest/last Debian 7.x iso releases


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: systemvmtemplate: fix build and upgrade t...

2016-05-06 Thread rhtyd

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1531#discussion_r62360986
  
--- Diff: 
tools/appliance/definitions/systemvmtemplate/configure_systemvm_services.sh ---
@@ -19,7 +19,7 @@
 set -e
 set -x
 
-CLOUDSTACK_RELEASE=4.6.0
+CLOUDSTACK_RELEASE=4.9.0
--- End diff --

Bumps version to match current version ^^


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: systemvmtemplate: fix build and upgrade t...

2016-05-06 Thread rhtyd

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1531#discussion_r62360949
  
--- Diff: tools/appliance/build.sh ---
@@ -529,8 +534,6 @@ function vmware_export() {
 stage_vmx ${appliance_build_name}-vmware 
${appliance_build_name}-vmware.vmdk
 ovftool ${appliance_build_name}-vmware.vmx 
${appliance_build_name}-vmware.ova
   fi
-  bzip2 "${appliance_build_name}-vmware.vmdk"
-  mv "${appliance_build_name}-vmware.vmdk.bz2" dist/
--- End diff --

Notes: we're removing this as we don't need vmdks for VMware, but OVAs


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: systemvmtemplate: fix build and upgrade t...

2016-05-06 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1531#issuecomment-217504212
  
@swill you can use this PR on a branch and build systemvm templates. I've 
updated the README on how you can setup an environment to build yourself 
systemvmtemplates as well.

Here are two templates from two different PRs I built with this patch 
applied: https://packages.shapeblue.com/systemvmtemplate/custom/


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: systemvmtemplate: fix build and upgrade t...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1531#issuecomment-217502707
  
how do I test this PR?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217501592
  
@jburwell thanks, I'll sort them out. Meanwhile, if we can get the 
dynamic-roles PR merged today/tomorrow, I can use the annotations/validation 
usage in APIs and further improve the PR


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-06 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-217500553
  
Thanks @swill as soon as we can merge this one, I can rebase and use some 
of the annotations work and ListUtils from this PR into out-of-band management 
PR /cc @jburwell 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9351: Add ids parameter to res...

2016-05-06 Thread nvazquez

Github user nvazquez commented on the pull request:

https://github.com/apache/cloudstack/pull/1497#issuecomment-217499701
  
@koushik-das @rhtyd I finished writing marvin test for this feature, I copy 
my test results:


[root@ussarlabcsmgt41 cloudstack]# nosetests --with-marvin 
--marvin-config=setup/dev/advanced.cfg 
test/integration/smoke/test_list_ids_parameter.py

 Marvin Init Started 

=== Marvin Parse Config Successful ===

=== Marvin Setting TestData Successful===

 Log Folder Path: /tmp//MarvinLogs//May_06_2016_09_34_04_NHXZGI. All 
logs will be available here 

=== Marvin Init Logging Successful===

 Marvin Init Successful 
===final results are now copied to: 
/tmp//MarvinLogs/test_list_ids_parameter_TRKKN0===


[root@ussarlabcsmgt41 cloudstack]# cat 
/tmp//MarvinLogs/test_list_ids_parameter_TRKKN0/results.txt
Test listing Volumes using 'ids' parameter ... === TestName: 
test_01_list_volumes | Status : SUCCESS ===
ok
Test listing Templates using 'ids' parameter ... === TestName: 
test_02_list_templates | Status : SUCCESS ===
ok
Test listing Snapshots using 'ids' parameter ... === TestName: 
test_03_list_snapshots | Status : SUCCESS ===
ok
Test listing VMSnapshots using 'vmsnapshotids' parameter ... === TestName: 
test_04_list_vm_snapshots | Status : SUCCESS ===
ok

--
Ran 4 tests in 1187.175s

OK




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: zip the processed json files so we save d...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1421#issuecomment-217497179
  
any suggestions for how to test this?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Cloudstack 9339: Virtual Routers do not h...

2016-05-06 Thread dsclose

Github user dsclose commented on the pull request:

https://github.com/apache/cloudstack/pull/1519#issuecomment-217495575
  
@swill ok, thanks, good luck with the master.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Cloudstack 9339: Virtual Routers do not h...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1519#issuecomment-217491038
  
There is an issue currently on master which I am trying to get sorted out, 
but there should not be a problem on `4.7`.  It looks like Jenkins may be out 
of diskspace again looking at that error.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Cloudstack 9339: Virtual Routers do not h...

2016-05-06 Thread dsclose

Github user dsclose commented on the pull request:

https://github.com/apache/cloudstack/pull/1519#issuecomment-217490305
  
Same error over at #1519 - something introduced in an earlier 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Cloudstack 9339: Virtual Routers do not h...

2016-05-06 Thread dsclose

Github user dsclose commented on the pull request:

https://github.com/apache/cloudstack/pull/1519#issuecomment-217489945
  
I rebased against the latest 4.7 before force pushing. Has an error been 
introduced along the way?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CPU socket count reporting correction

2016-05-06 Thread dmabry

Github user dmabry commented on the pull request:

https://github.com/apache/cloudstack/pull/1520#issuecomment-217485432
  
We have rolled this patch in our lab and verified that it does indeed 
report the number of sockets correctly.  Based on testing this LGTM.

Please do a force push to kick off jenkins and travis 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: DAO: Hit the cache for entity flagged as ...

2016-05-06 Thread DaanHoogland

Github user DaanHoogland commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1532#discussion_r62349471
  
--- Diff: framework/db/src/com/cloud/utils/db/GenericDaoBase.java ---
@@ -969,7 +969,12 @@ public T findByUuidIncludingRemoved(final String uuid) 
{
 @Override
 @DB()
 public T findByIdIncludingRemoved(ID id) {
-return findById(id, true, null);
+if (_cache != null) {
+final Element element = _cache.get(id);
+return element == null ? findById(id, true, null) : 
(T)element.getObjectValue();
+} else {
--- End diff --

yes, but I think the submitted code suggest it is optional:
```
T result = findById(id, true, null)
if (_cache != null) {
final Element element = _cache.get(id);
if (element != null) {
result = (T)element.getObjectValue();
}
}
return result;
```
I have no problem with the proposed code if _cache is optional however.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Restore iptables at once using iptables-r...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1482#issuecomment-217482470
  
@remibergsma can you squash this and re-push.  I need one more code review 
on this one and we should be ready to merge it.  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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Addresses CLOUDSTACK-9300 where the MySQL...

2016-05-06 Thread dmabry

Github user dmabry commented on the pull request:

https://github.com/apache/cloudstack/pull/1428#issuecomment-217482136
  
tag:mergeready


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Taking fast and efficient volume snapshot...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1403#issuecomment-217481732
  
@jburwell can I get your final review on this?  I think we are probably 
pretty close 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9199: Fixed deployVirtualMachi...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1280#issuecomment-217480884
  
@anshul1886 can you answer @rhtyd's question so we can get his code review 
and get this moving forward.  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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8800 : Improved the listVirtua...

2016-05-06 Thread swill

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CPU socket count reporting correction

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1520#issuecomment-217475012
  
Can we get another code review on this?  Also, Jenkins seems to be hung, 
can we either close and reopen the PR or do a force push to kick it off?  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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9366: Capacity of one zone-wid...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1516#issuecomment-217472184
  
Can you force push again to get the jobs to kick off again.  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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9265 cleanup around httpclient...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1385#issuecomment-217471764
  
Can you do a force push again.  Jenkins has been in this state for 3 days 
now.  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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8970 Centos 6.{1,2,3,4,5} gues...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/956#issuecomment-217471716
  
Can you do a force push again.  Jenkins has been in this state for 3 days 
now.  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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: When no zone name is available display a ...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1477#issuecomment-217471150
  
Unless someone is able to verify this, I think we have to take the 
screenshots from @remibergsma as verification.  Not very many people have the 
ability to test regional templates, so we have a very limited group of users 
who can give us verification on this.  If we don't have any verification by 
Monday, I will likely merge this and count on @remibergsma's verification as 
proof that it works...


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-6975: Prevent dnsmasq from sta...

2016-05-06 Thread dsclose

Github user dsclose commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1514#discussion_r62344011
  
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsDhcp.py ---
@@ -54,7 +54,8 @@ def process(self):
 self.cloud.commit()
 
 # We restart DNSMASQ every time the configure.py is called in 
order to avoid lease problems.
-CsHelper.service("dnsmasq", "restart")
+if not self.cl.is_redundant() or self.cl.is_master():
--- End diff --

@jburwell Whilst that check might be convenient in some cases, I think that 
convenience would be a curse. For example "bring up dnsmasq on non-redundant or 
master routers" is a nice explicit declaration of behaviour. Simplifying that 
to "bring up dnsmasq on master routers" might do the same thing but it 
increases the learning curve for someone reading the code for the first time.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: [CLOUDSTACK-8973] Fix create template fro...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1424#issuecomment-217460449
  
Can I get one more code review on this one?  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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Re: Number of NICs in VMWare - CS 4.6 bug?

2016-05-06 Thread Octavian Popescu

Adding a bit to this – it looks like it’s a documented bug, fixed in CS-25155 
but unfortunately I couldn’t really find much about the fix other than vague 
references, any ideas how I could track it down?

Thanks,
Octavian

On 06/05/2016 10:28, "Octavian Popescu" 
> wrote:

Hello,

There seems to be a bug in CS 4.6 that prevents a VM running on VMWare from 
booting if it has more than 7 NICs – the NICs can be added just fine until you 
hit the VMWare limit (10) but on reboot it doesn’t start anymore until you 
remove them. Do you know if this was fixed in 4.7 or 4.8?

Thanks,
Octavian

[GitHub] cloudstack pull request: CLOUDSTACK-9099: SecretKey is returned fr...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1152#discussion_r62336714
  
--- Diff: 
api/src/org/apache/cloudstack/api/command/admin/user/GetUserKeysCmd.java ---
@@ -0,0 +1,74 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.api.command.admin.user;
+
+
+import com.cloud.user.Account;
+import com.cloud.user.User;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.response.RegisterResponse;
+import org.apache.cloudstack.api.response.UserResponse;
+
+import java.util.List;
+import java.util.logging.Logger;
+
+@APICommand(name = "getUserKeys",
+description = "This command allows the user to query the 
seceret and API keys for the account",
+responseObject = RegisterResponse.class,
+requestHasSensitiveInfo = false,
+responseHasSensitiveInfo = true)
--- End diff --

Please add the version annotation to indicate that this command was added 
for 4.9.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-217456452
  
I will get this CI'ed.  I have 4 new CI boxes in play now, but I am trying 
to figure out how to stabilize master, so I will run this as soon as I get 
master sorted out.  Sorry for the delay...


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9099: SecretKey is returned fr...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1152#discussion_r62337172
  
--- Diff: server/test/com/cloud/user/MockAccountManagerImpl.java ---
@@ -401,5 +403,24 @@ public Long finalyzeAccountId(String accountName, Long 
domainId, Long projectId,
 return null;
 }
 
+@Override
+public List getKeys(GetUserKeysCmd cmd) {
+return null;
+}
+
+@Override
+public void checkAccess(User user, ControlledEntity entity)
+throws PermissionDeniedException {
+
+}
+@Override
+public String getConfigComponentName() {
+return null;
+}
+
+@Override
+public ConfigKey[] getConfigKeys() {
+return null;
--- End diff --

Please return an empty array to avoid NPEs.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9099: SecretKey is returned fr...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1152#discussion_r62337077
  
--- Diff: server/test/com/cloud/user/MockAccountManagerImpl.java ---
@@ -401,5 +403,24 @@ public Long finalyzeAccountId(String accountName, Long 
domainId, Long projectId,
 return null;
 }
 
+@Override
+public List getKeys(GetUserKeysCmd cmd) {
+return null;
--- End diff --

Please return a ``Collections.emptyList()`` to avoid NPEs.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9099: SecretKey is returned fr...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1152#discussion_r62337130
  
--- Diff: server/test/com/cloud/user/MockAccountManagerImpl.java ---
@@ -401,5 +403,24 @@ public Long finalyzeAccountId(String accountName, Long 
domainId, Long projectId,
 return null;
 }
 
+@Override
+public List getKeys(GetUserKeysCmd cmd) {
+return null;
+}
+
+@Override
+public void checkAccess(User user, ControlledEntity entity)
+throws PermissionDeniedException {
+
+}
+@Override
+public String getConfigComponentName() {
+return null;
--- End diff --

Please return a blank string to avoid NPEs.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9099: SecretKey is returned fr...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1152#discussion_r62336941
  
--- Diff: server/src/com/cloud/user/AccountManager.java ---
@@ -198,4 +200,11 @@ void buildACLViewSearchCriteria(SearchCriteria s
 public static final String MESSAGE_ADD_ACCOUNT_EVENT = 
"Message.AddAccount.Event";
 
 public static final String MESSAGE_REMOVE_ACCOUNT_EVENT = 
"Message.RemoveAccount.Event";
+public static final ConfigKey UseSecretKeyInResponse = new 
ConfigKey(
+"Advanced",
+Boolean.class,
+"use.secret.key.in.response",
+"true",
--- End diff --

@kansal I agree with @DaanHoogland and @remibergsma -- it's about 
reasonable and secure defaults.  We should not configure a management server 
insecurely by default.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: DAO: Hit the cache for entity flagged as ...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1532#discussion_r62335611
  
--- Diff: framework/db/src/com/cloud/utils/db/GenericDaoBase.java ---
@@ -969,7 +969,12 @@ public T findByUuidIncludingRemoved(final String uuid) 
{
 @Override
 @DB()
 public T findByIdIncludingRemoved(ID id) {
-return findById(id, true, null);
+if (_cache != null) {
+final Element element = _cache.get(id);
+return element == null ? findById(id, true, null) : 
(T)element.getObjectValue();
+} else {
--- End diff --

@dahn if the ``_cache`` is not optional then I agree. It should probably be 
something like the following:

```
if (_cache == null) {
throw new IllegalStateException("The cache has not been properly 
initialized for DAO " + this.getClass().getSimpleName());
}

final Element element = _cache.get(id);
return element == null ? findById(id, true, null) : (T) 
element.getObjectValue();
```

Would you agree?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: [CLOUDSTACK-8973] Fix create template fro...

2016-05-06 Thread jburwell

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1424#issuecomment-217451440
  
LGTM based on code 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-06 Thread jburwell

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-217450750
  
@rhtyd @swill LGTM based on code 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Apply static routes on change to master s...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1472#discussion_r62331395
  
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsStaticRoutes.py 
---
@@ -0,0 +1,42 @@
+#!/usr/bin/python
+# -- coding: utf-8 --
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from CsDatabag import CsDataBag
+from CsRedundant import *
+
+
+class CsStaticRoutes(CsDataBag):
+
+def process(self):
+logging.debug("Processing CsStaticRoutes file ==> %s" % self.dbag)
+for item in self.dbag:
+if item == "id":
+continue
+self.__update(self.dbag[item])
+
+def __update(self, route):
+if route['revoke']:
+command = "ip route del %s via %s" % (route['network'], 
route['gateway'])
+CsHelper.execute(command)
--- End diff --

Should the execute be checked and error thrown and/or logged on failure?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Vmdk findbugs

2016-05-06 Thread jburwell

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1530#issuecomment-217442913
  
@DaanHoogland I am having surgery this afternoon, so I will be unable to 
get it to it today (6 May 2016).  I will try to put together a PR later this 
weekend or early next week.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9348: Optimize NioTest and Nio...

2016-05-06 Thread jburwell

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1534#issuecomment-217442413
  
LGTM based on code 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-6975: Prevent dnsmasq from sta...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1514#discussion_r62329838
  
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsDhcp.py ---
@@ -54,7 +54,8 @@ def process(self):
 self.cloud.commit()
 
 # We restart DNSMASQ every time the configure.py is called in 
order to avoid lease problems.
-CsHelper.service("dnsmasq", "restart")
+if not self.cl.is_redundant() or self.cl.is_master():
--- End diff --

Would is make sense for the ``is_redundant()`` check to be in the 
``is_master()`` method?  To my way of thinking, any non-redundant router is a 
de facto master.  

I understand such a change may be too far to be practical at this time, so 
we may want to table 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Convert patchviasocket to python (removes...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1533#discussion_r62329185
  
--- Diff: scripts/vm/hypervisor/kvm/test_patchviasocket.py ---
@@ -0,0 +1,142 @@
+#!/usr/bin/env python
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import patchviasocket
+
+import getpass
+import os
+import socket
+import tempfile
+import time
+import threading
+import unittest
+
+KEY_DATA = "I luv\nCloudStack\n"
+CMD_DATA = "/run/this-for-me --please=TRUE! very%quickly"
+NON_EXISTING_FILE = "must-not-exist"
+
+
+def write_key_file():
+tmpfile = tempfile.mktemp(".sck")
+with open(tmpfile, "w") as f:
+f.write(KEY_DATA)
+return tmpfile
+
+
+class SocketThread(threading.Thread):
+def __init__(self):
+super(SocketThread, self).__init__()
+self._data = ""
+self._file = tempfile.mktemp(".sck")
+self._ready = False
+
+def data(self):
+return self._data
+
+def file(self):
+return self._file
+
+def wait_until_ready(self):
+while not self._ready:
+time.sleep(0.050)
+
+def run(self):
+TIMEOUT = 0.314 # Very short time for tests that don't write 
to socket.
+MAX_SIZE = 10 * 1024
+
+s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+s.bind(self._file)
+s.listen(1)
+s.settimeout(TIMEOUT)
+try:
+self._ready = True
+client, address = s.accept()
+self._data = client.recv(MAX_SIZE)
+client.close()
+except socket.timeout:
+pass
+s.close()
+os.remove(self._file)
--- End diff --

Line 73-74 should be in a ``finally`` block to ensure the resources are 
properly closed when an exception occurs.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Convert patchviasocket to python (removes...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1533#discussion_r62328978
  
--- Diff: scripts/vm/hypervisor/kvm/test_patchviasocket.py ---
@@ -0,0 +1,142 @@
+#!/usr/bin/env python
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import patchviasocket
+
+import getpass
+import os
+import socket
+import tempfile
+import time
+import threading
+import unittest
+
+KEY_DATA = "I luv\nCloudStack\n"
+CMD_DATA = "/run/this-for-me --please=TRUE! very%quickly"
+NON_EXISTING_FILE = "must-not-exist"
+
+
+def write_key_file():
+tmpfile = tempfile.mktemp(".sck")
+with open(tmpfile, "w") as f:
+f.write(KEY_DATA)
+return tmpfile
+
+
+class SocketThread(threading.Thread):
+def __init__(self):
+super(SocketThread, self).__init__()
+self._data = ""
+self._file = tempfile.mktemp(".sck")
--- End diff --

According to the [Python API document for 
``tempfile.mktemp``](https://docs.python.org/2/library/tempfile.html), this 
method has been deprecated since Python 2.3.  ``tempfile.mkstemp`` should be 
used instead.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Convert patchviasocket to python (removes...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1533#discussion_r62328994
  
--- Diff: scripts/vm/hypervisor/kvm/test_patchviasocket.py ---
@@ -0,0 +1,142 @@
+#!/usr/bin/env python
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import patchviasocket
+
+import getpass
+import os
+import socket
+import tempfile
+import time
+import threading
+import unittest
+
+KEY_DATA = "I luv\nCloudStack\n"
+CMD_DATA = "/run/this-for-me --please=TRUE! very%quickly"
+NON_EXISTING_FILE = "must-not-exist"
+
+
+def write_key_file():
+tmpfile = tempfile.mktemp(".sck")
--- End diff --

According to the [Python API document for 
``tempfile.mktemp``](https://docs.python.org/2/library/tempfile.html), this 
method has been deprecated since Python 2.3.  ``tempfile.mkstemp`` should be 
used instead.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Convert patchviasocket to python (removes...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1533#discussion_r62328643
  
--- Diff: scripts/vm/hypervisor/kvm/test_patchviasocket.py ---
@@ -0,0 +1,142 @@
+#!/usr/bin/env python
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import patchviasocket
+
+import getpass
+import os
+import socket
+import tempfile
+import time
+import threading
+import unittest
+
+KEY_DATA = "I luv\nCloudStack\n"
+CMD_DATA = "/run/this-for-me --please=TRUE! very%quickly"
+NON_EXISTING_FILE = "must-not-exist"
+
+
+def write_key_file():
+tmpfile = tempfile.mktemp(".sck")
+with open(tmpfile, "w") as f:
+f.write(KEY_DATA)
+return tmpfile
+
+
+class SocketThread(threading.Thread):
+def __init__(self):
+super(SocketThread, self).__init__()
+self._data = ""
+self._file = tempfile.mktemp(".sck")
+self._ready = False
+
+def data(self):
+return self._data
+
+def file(self):
+return self._file
+
+def wait_until_ready(self):
--- End diff --

Should this be bounded by a maximum wait time?  For example, if the ulimits 
are exceeded, this function could turn into an infinite loop.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Convert patchviasocket to python (removes...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1533#discussion_r62328413
  
--- Diff: scripts/vm/hypervisor/kvm/patchviasocket.py ---
@@ -0,0 +1,80 @@
+#!/usr/bin/env python
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+#
+# This script connects to the system vm socket and writes the
+# authorized_keys and cmdline data to it. The system VM then
+# reads it from /dev/vport0p1 in cloud_early_config
+#
+
+import argparse
+import os
+import socket
+
+SOCK_FILE = "/var/lib/libvirt/qemu/{name}.agent"
+PUB_KEY_FILE = "/root/.ssh/id_rsa.pub.cloud"
+MESSAGE = "pubkey:{key}\ncmdline:{cmdline}\n"
+
+
+def read_pub_key(key_file):
+try:
+if os.path.isfile(key_file):
+with open(key_file, "r") as f:
+return f.read()
+except IOError:
+return None
+
+
+def send_to_socket(sock_file, key_file, cmdline):
+pub_key = read_pub_key(key_file)
+
+if not pub_key:
+print("ERROR: ssh public key not found on host at 
{0}".format(key_file))
--- End diff --

Why throw this error out of ``read_pub_key`` and consolidate the error 
handling/return in the exception handler on line 64?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Convert patchviasocket to python (removes...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1533#discussion_r62328214
  
--- Diff: scripts/vm/hypervisor/kvm/patchviasocket.py ---
@@ -0,0 +1,80 @@
+#!/usr/bin/env python
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+#
+# This script connects to the system vm socket and writes the
+# authorized_keys and cmdline data to it. The system VM then
+# reads it from /dev/vport0p1 in cloud_early_config
+#
+
+import argparse
+import os
+import socket
+
+SOCK_FILE = "/var/lib/libvirt/qemu/{name}.agent"
+PUB_KEY_FILE = "/root/.ssh/id_rsa.pub.cloud"
+MESSAGE = "pubkey:{key}\ncmdline:{cmdline}\n"
+
+
+def read_pub_key(key_file):
+try:
+if os.path.isfile(key_file):
+with open(key_file, "r") as f:
+return f.read()
+except IOError:
--- End diff --

Would it be helpful to print/log the details of the ``IOError`` for 
debugging purposes?  For example, understanding whether the file was not found 
or the user lacked sufficient permissions.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Notify listeners when a host has been add...

2016-05-06 Thread mike-tutkowski

Github user mike-tutkowski commented on the pull request:

https://github.com/apache/cloudstack/pull/816#issuecomment-217437287
  
Thanks for your time on the review, @miguelaferreira!


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread jburwell

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217434570
  
@rhtyd @swill we are very close.  I have a couple of outstanding comments 
-- particularly around potential thread explosion in ``ProcessRunner``.  
Ultimately, I think the issues are straightforward to address.

/cc @borisstoyanov


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62325927
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,111 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.List;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private String stdOutput;
+private String stdError;
+private int returnCode = -1;
+
+public String getStdOutput() {
+return stdOutput;
+}
+
+public void setStdOutput(String stdOutput) {
+this.stdOutput = stdOutput;
+}
+
+public String getStdError() {
+return stdError;
+}
+
+public void setStdError(String stdError) {
+this.stdError = stdError;
+}
+
+public int getReturnCode() {
+return returnCode;
+}
+
+public void setReturnCode(int returnCode) {
+this.returnCode = returnCode;
+}
+
+public static ProcessRunner executeCommands(List commands, 
long timeOutSeconds) {
+ProcessRunner result = new ProcessRunner();
+try {
+Process process = new 
ProcessBuilder().command(commands).start();
+StreamGobbler stdInputGobbler = new 
StreamGobbler(process.getInputStream());
+StreamGobbler stdErrorGobbler = new 
StreamGobbler(process.getErrorStream());
--- End diff --

The ``StreamGobbler`` idiom was replaced with ``ProcessBuilder.inheritIO`` 
in Java 1.7+.  Using it would simplify this code and allow the removal of the 
``StreamGobbler`` class.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62325553
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,111 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.List;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private String stdOutput;
+private String stdError;
+private int returnCode = -1;
+
+public String getStdOutput() {
+return stdOutput;
+}
+
+public void setStdOutput(String stdOutput) {
+this.stdOutput = stdOutput;
+}
+
+public String getStdError() {
+return stdError;
+}
+
+public void setStdError(String stdError) {
+this.stdError = stdError;
+}
+
+public int getReturnCode() {
+return returnCode;
+}
+
+public void setReturnCode(int returnCode) {
+this.returnCode = returnCode;
+}
+
+public static ProcessRunner executeCommands(List commands, 
long timeOutSeconds) {
+ProcessRunner result = new ProcessRunner();
+try {
+Process process = new 
ProcessBuilder().command(commands).start();
+StreamGobbler stdInputGobbler = new 
StreamGobbler(process.getInputStream());
+StreamGobbler stdErrorGobbler = new 
StreamGobbler(process.getErrorStream());
+stdInputGobbler.start();
+stdErrorGobbler.start();
+
+if (timeOutSeconds > 0) {
+ProcessWaitForThread worker = new 
ProcessWaitForThread(process);
--- End diff --

I am concerned about unbounded thread growth here.  Please consider using a 
bounded ``Executor`` and a 
[``ListenableFuture``](https://github.com/google/guava/wiki/ListenableFutureExplained)
 here.  It would also remove the need for the ``ProcessWaitForThread`` class.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62322683
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,111 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.List;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private String stdOutput;
+private String stdError;
+private int returnCode = -1;
+
+public String getStdOutput() {
+return stdOutput;
+}
+
+public void setStdOutput(String stdOutput) {
+this.stdOutput = stdOutput;
+}
+
+public String getStdError() {
+return stdError;
+}
+
+public void setStdError(String stdError) {
+this.stdError = stdError;
+}
+
+public int getReturnCode() {
+return returnCode;
+}
+
+public void setReturnCode(int returnCode) {
+this.returnCode = returnCode;
+}
+
+public static ProcessRunner executeCommands(List commands, 
long timeOutSeconds) {
--- End diff --

Consider adding ``Preconditions.checkArguments`` to verify that 
``commands`` is not ``null``.  

Also, consider using JodaTime's ``Duration`` to represent the timeout as 
number with a unit of measure.  This step is not required for LGTM, but would 
be a good refactoring if we fit it in.



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62322543
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,111 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.List;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private String stdOutput;
+private String stdError;
+private int returnCode = -1;
--- End diff --

Consider making this class immutable and/or creating a static, immutable 
inner class (e.g. ``ProcessResult``).  


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62322259
  
--- Diff: server/test/com/cloud/resource/MockResourceManagerImpl.java ---
@@ -172,6 +173,12 @@ public Cluster getCluster(final Long clusterId) {
 return null;
 }
 
+@Override
+public DataCenter getZone(Long zoneId) {
+// TODO Auto-generated method stub
--- End diff --

Because we have tools to search for TODOs, please remove the comment if 
there is nothing outstanding to be 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62321985
  
--- Diff: server/src/com/cloud/server/StatsCollector.java ---
@@ -412,6 +428,36 @@ protected void runInContext() {
 }
 }
 
+class HostOutOfBandManagementStatsCollector extends 
ManagedContextRunnable {
+@Override
+protected void runInContext() {
+try {
+s_logger.debug("HostOutOfBandManagementStatsCollector is 
running...");
+List outOfBandManagementHosts = 
outOfBandManagementDao.findAllByManagementServer(ManagementServerNode.getManagementServerId());
+if (outOfBandManagementHosts == null) {
+return;
+}
+for (OutOfBandManagement outOfBandManagementHost : 
outOfBandManagementHosts) {
+Host host = 
_hostDao.findById(outOfBandManagementHost.getHostId());
+if (host == null) {
+continue;
+}
+if 
(outOfBandManagementService.isOutOfBandManagementEnabled(host)) {
+
outOfBandManagementService.submitBackgroundPowerSyncTask(host);
+} else {
+if (outOfBandManagementHost.getPowerState() != 
OutOfBandManagement.PowerState.Disabled) {
+if 
(outOfBandManagementService.transitionPowerStateToDisabled(Collections.singletonList(host)))
 {
+s_logger.debug("Out-of-band management was 
disabled in zone/cluster/host, disabled power state for host id:" + 
host.getId());
--- End diff --

Please wrap in a ``if (LOGGER.isDebugEnabled)`` check.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62321963
  
--- Diff: server/src/com/cloud/server/StatsCollector.java ---
@@ -412,6 +428,36 @@ protected void runInContext() {
 }
 }
 
+class HostOutOfBandManagementStatsCollector extends 
ManagedContextRunnable {
+@Override
+protected void runInContext() {
+try {
+s_logger.debug("HostOutOfBandManagementStatsCollector is 
running...");
+List outOfBandManagementHosts = 
outOfBandManagementDao.findAllByManagementServer(ManagementServerNode.getManagementServerId());
+if (outOfBandManagementHosts == null) {
+return;
+}
+for (OutOfBandManagement outOfBandManagementHost : 
outOfBandManagementHosts) {
+Host host = 
_hostDao.findById(outOfBandManagementHost.getHostId());
+if (host == null) {
+continue;
+}
+if 
(outOfBandManagementService.isOutOfBandManagementEnabled(host)) {
+
outOfBandManagementService.submitBackgroundPowerSyncTask(host);
+} else {
+if (outOfBandManagementHost.getPowerState() != 
OutOfBandManagement.PowerState.Disabled) {
+if 
(outOfBandManagementService.transitionPowerStateToDisabled(Collections.singletonList(host)))
 {
--- End diff --

Collapse these two ``if`` blocks into a single ``else if`` on line 447 to 
improve readability.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62321858
  
--- Diff: server/src/com/cloud/server/StatsCollector.java ---
@@ -251,8 +262,9 @@ public boolean start() {
 }
 
 private void init(Map configs) {
-_executor = Executors.newScheduledThreadPool(4, new 
NamedThreadFactory("StatsCollector"));
+_executor = Executors.newScheduledThreadPool(6, new 
NamedThreadFactory("StatsCollector"));
--- End diff --

I understand that ``6`` is in the ``init`` method.  My question is how did 
we arrive at that number?  Why is 6 instead of 2 or 10?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62321576
  
--- Diff: 
api/src/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementService.java
 ---
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.dc.DataCenter;
+import com.cloud.host.Host;
+import com.cloud.org.Cluster;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.framework.config.ConfigKey;
+
+import java.util.List;
+
+public interface OutOfBandManagementService {
+
+ConfigKey OutOfBandManagementActionTimeout = new 
ConfigKey("Advanced", Long.class, "outofbandmanagement.action.timeout", 
"60",
+"The out of band management action timeout in seconds, 
configurable by cluster", true, ConfigKey.Scope.Cluster);
+
+ConfigKey OutOfBandManagementSyncThreadInterval = new 
ConfigKey("Advanced", Long.class, "outofbandmanagement.sync.interval", 
"30",
+"The interval (in milliseconds) when the out-of-band 
management background sync are retrieved", true, ConfigKey.Scope.Global);
+
+ConfigKey OutOfBandManagementSyncThreadPoolSize = new 
ConfigKey("Advanced", Integer.class, 
"outofbandmanagement.sync.poolsize", "50",
+"The out of band management background sync thread pool size", 
true, ConfigKey.Scope.Global);
+
+long getId();
+boolean isOutOfBandManagementEnabled(Host host);
+void submitBackgroundPowerSyncTask(Host host);
+boolean transitionPowerStateToDisabled(List hosts);
--- End diff --

@rhtyd the issue is that it is a wider type specification that is 
necessary.  It can also cause erasure compiler errors.  Therefore, we should 
only be using  when it is truly necessary.  In this case, the 
``OutOfBandManagementService`` only operates on instances of ``Host``.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62321148
  
--- Diff: 
api/src/org/apache/cloudstack/api/command/admin/outofbandmanagement/IssueOutOfBandManagementPowerActionCmd.java
 ---
@@ -0,0 +1,128 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.api.command.admin.outofbandmanagement;
+
+import com.cloud.event.EventTypes;
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.NetworkRuleConflictException;
+import com.cloud.exception.ResourceAllocationException;
+import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.host.Host;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiCommandJobType;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseAsyncCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.HostResponse;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.context.CallContext;
+import 
org.apache.cloudstack.outofbandmanagement.OutOfBandManagement.PowerOperation;
+import 
org.apache.cloudstack.outofbandmanagement.OutOfBandManagementService;
+
+import javax.inject.Inject;
+
+@APICommand(name = "issueOutOfBandManagementPowerAction", description = 
"Initiates the specified power action to the host's out-of-band management 
interface",
+responseObject = OutOfBandManagementResponse.class, 
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, authorized = 
{RoleType.Admin})
+public class IssueOutOfBandManagementPowerActionCmd extends BaseAsyncCmd {
+@Inject
+private OutOfBandManagementService outOfBandManagementService;
+
+/
+ API parameters /
+/
+
+@Parameter(name = ApiConstants.HOST_ID, type = CommandType.UUID, 
entityType = HostResponse.class, required = true, description = "the ID of the 
host")
+private Long hostId;
+
+@Parameter(name = ApiConstants.TIMEOUT, type = CommandType.LONG, 
description = "optional operation timeout in seconds that overrides the global 
or cluster-level out-of-band management timeout setting")
+private Long actionTimeout;
+
+@Parameter(name = ApiConstants.ACTION, type = CommandType.STRING, 
required = true, description = "out-of-band management power actions, valid 
actions are: ON, OFF, CYCLE, RESET, SOFT, STATUS")
+private String outOfBandManagementPowerOperation;
+
+/
+/// API Implementation///
+/
+
+@Override
+public String getCommandName() {
+return "issueoutofbandmanagementpoweractionresponse";
+}
+
+private void validateParams() {
+if (getHostId() == null || getHostId() < 1L) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR, 
"Invalid host ID: " + getHostId());
+}
+if (Strings.isNullOrEmpty(getOutOfBandManagementPowerOperation())) 
{
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR, 
"Invalid out-of-band management power action: " + 
getOutOfBandManagementPowerOperation());
+}
+}
+
+@Override
+public void execute() throws ResourceUnavailableException, 
InsufficientCapacityException, ServerApiException, 
ConcurrentOperationException, ResourceAllocationException,

Re: Any difference with the official rpms and the git 4.8 branch?

2016-05-06 Thread Linas Žilinskas

Ok, nevermind. I think I messed up something in the process of setting
up my dev env.

From what I saw, i probably ran the jetty instance with default configs
which do not use encryption. Then later I probably launched the instance
from packages, which used encryption and then the confusion occurred.

I updated the dev configs to use encryption and fixed the DB somewhat
and everything seems to be fine now. Oh yeah, and i ran them using
different users, so /tmp/tmpkey was locked under different user each
time, which caused permission issues.

Regards


On Thu, 2016-05-05 at 07:58 +, Linas Žilinskas wrote:
> Hello.
> 
> We're planning on setting up a local repo for our CS where we would store our 
> customized builds.
> I just wanted to clear up some things.
> 
> Currently when editing CS code and testing changes I use jetty. I noticed a 
> couple of differences compared to our test cluster, where the whole setup is 
> done from the official (?) packages.
> 
> First is that for some reason secondary.storage.vm setting was set to some 
> encoded string and treated as "false". Hence the secondary storage VM was 
> prevented from starting.
> I found the cause using this blog post: 
> http://blog.widodh.nl/2013/04/cloudstack-zone-x-is-is-not-ready-to-launch-console-proxy-yet/
>  . I set it through mysql to 'true' and it started working.
> 
> Second, SystemVMs were using some unknown ssh key. I found it in the database 
> in settings as well.  ssh.privatekey, ssh.publickey. Once i set it to my keys 
> (still do not know what the format(ing) should be for those) everything went 
> fine.
> But i wonder whos key it is (it was passworded) and why is my dev env using 
> those. I looked at our test env and the database contains some keys, but they 
> are not used in SystemVMs. So what's causing them to be (not-) used?
> 
> The final question I have is whether these issues I have found would appear 
> on our other environments or is this something maven config related and 
> therefore applies only to jetty or specific maven goals?
> 
> Some details on my setup:
> I'm not using maven developer profile. Simply building with 'maven clean 
> install' and running with 'maven -pl client jetty:run'. I'm still reading 
> about how maven works, so it might be my error at some step.
> HV (kvm) is running on the same box. HV is using the official repo and is 
> started through systemd.
> Management was first installed from official repos and initial setup was 
> done. Then I shut it down and started using maven/jetty. So the database init 
> was done from the official release rpms.
> All the work is being done on custom branches, branched out from the 4.8 
> release branch. (version is set to 4.8.1-SNAPSHOT).
> 
> I have this feeling that it might be related to database encryption keys (and 
> my mixed env - possibly mixed db.properties). But it would seems strange that 
> secondary.system.vm would be ecrypted anyway.
> 
> Regards
> 
> 
> Linas Žilinskas
> 
> Development Lead
> 
> [http://host1plus.com/images/h1p_e_sig/sig_logo.png]
> 
> website
> 
> facebook
> 
> twitter
> 
> linkedin
> 
> 
> 
> 
> Phone: +44 870 8200222
> 
> Fax: +44 870 8200222
> 
> Host1Plus is a division of Digital Energy Technologies Ltd.
> 
> 26 York Street, London W1U 6PZ, United Kingdom
> 
> 
>

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread borisstoyanov

Github user borisstoyanov commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217414515
  
I've just run final sanity tests on this PR, all the OOBM features are 
working and did not observed any side effects, so I'm happy with the latest 
changes getting merged.

LGTM!


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: systemvmtemplate: fix build and upgrade t...

2016-05-06 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1531#issuecomment-217413238
  
@swill this PR is ready for CI test run and merge, 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9348: Optimize NioTest and Nio...

2016-05-06 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1534#issuecomment-217413262
  
@swill this PR is ready for CI test run and merge, 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-06 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-217413190
  
@swill this PR is ready for CI test run and merge, 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

2016-05-06 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217413162
  
@swill this PR is ready for CI test run and merge, 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Strongswan vpn feature

2016-05-06 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/872#issuecomment-217410505
  
@jayapalu I've tried the client only one by one, one at a time. To share my 
environment, I'm on a 192.168.0.0/16 network from where all my clients try to 
connect to the router IP (192.168.50.12). I've checked and even tried dropping 
all iptables rules and firewall that may be blocking any connection; tcpdump 
confirmed that no ports/communication was blocked.

In the daemon/messages log, I get following logs every time a client 
connects and then it fails due to timeout; some relevant lines from the log:

remote host is behind NAT
no matching CHILD_SA config found
received retransmit of request with ID 1,, but no response to retransmit

I found that strongswan 5.x is much better at handling NAT traversals, so I 
tried to upgrade to that version but it still did not work out either. 
Strongswan 5.x failed with following kind of logs:

May  5 22:07:52 r-4-VM charon: 14[IKE] sending NAT-T (RFC 3947) vendor ID
May  5 22:07:52 r-4-VM charon: 14[ENC] generating ID_PROT response 0 [ SA V 
V V  
]
May  5 22:07:53 r-4-VM charon: 13[IKE] received retransmit of request with 
ID 0,,
 retransmitting response
May  5 22:07:54 r-4-VM charon: 15[IKE] received retransmit of request with 
ID 0,,
 retransmitting response
May  5 22:07:57 r-4-VM charon: 04[IKE] received retransmit of request with 
ID 0,,
 retransmitting response
May  5 22:08:22 r-4-VM charon: 16[JOB] deleting half open IKE_SA after 
timeout
May  5 22:08:22 r-4-VM charon: 16[IKE] IKE_SA (unnamed)[2] state change: 
CONNECTT
ING => DESTROYING


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Strongswan vpn feature

2016-05-06 Thread jayapalu

Github user jayapalu commented on the pull request:

https://github.com/apache/cloudstack/pull/872#issuecomment-217407342
  
@rhtyd 
Is single NAT client is not working for you ? 
Multiple clients at the same time from single NATed public ip will not 
work. 
Single nat client (windows, iOS) should work. 
Can you please post the /var/log/auth.log from the VR.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Convert patchviasocket to python (removes...

2016-05-06 Thread sverrirab

Github user sverrirab commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1533#discussion_r62309904
  
--- Diff: scripts/vm/hypervisor/kvm/patchviasocket.py ---
@@ -0,0 +1,80 @@
+#!/usr/bin/env python
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+#
+# This script connects to the system vm socket and writes the
+# authorized_keys and cmdline data to it. The system VM then
+# reads it from /dev/vport0p1 in cloud_early_config
+#
+
+import argparse
+import os
+import socket
+
+SOCK_FILE = "/var/lib/libvirt/qemu/{name}.agent"
+PUB_KEY_FILE = "/root/.ssh/id_rsa.pub.cloud"
+MESSAGE = "pubkey:{key}\ncmdline:{cmdline}\n"
+
+
+def read_pub_key(key_file):
+try:
+if os.path.isfile(key_file):
+with open(key_file, "r") as f:
+return f.read()
+except IOError:
+return None
+
+
+def send_to_socket(sock_file, key_file, cmdline):
+pub_key = read_pub_key(key_file)
+
+if not pub_key:
+print("ERROR: ssh public key not found on host at 
{0}".format(key_file))
+return 1
+
+# Keep old substitution from perl code:
+cmdline = cmdline.replace("%", " ")
+
+msg = MESSAGE.format(key=pub_key, cmdline=cmdline)
+
+if not os.path.exists(sock_file):
+print("ERROR: {0} socket not found".format(sock_file))
+return 1
+
+try:
+s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+s.connect(sock_file)
+s.sendall(msg)
+s.close()
+except IOError as e:
--- End diff --

IOError is the parent of socket.error so this will catch all socket related 
errors.
The code as it stands returns 1 on any error and 0 on success - so there is 
no benefit of adding a finally statement there that I can see.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Cloudstack 9339: Virtual Routers do not h...

2016-05-06 Thread dsclose

Github user dsclose commented on the pull request:

https://github.com/apache/cloudstack/pull/1519#issuecomment-217397801
  
Looks like the build environment isn't sufficient on jenkins-test-a20:

```
[ERROR] Java HotSpot(TM) 64-Bit Server VM warning: Insufficient space for 
shared memory file:
   30583
Try using the -Djava.io.tmpdir= option to select an alternate temp location.
```

Doing another force push.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-6975: Prevent dnsmasq from sta...

2016-05-06 Thread dsclose

Github user dsclose commented on the pull request:

https://github.com/apache/cloudstack/pull/1514#issuecomment-217392996
  
@DaanHoogland yes, the line `CsHelper.service("dnsmasq", "restart")` in 
`set_master` is in 
systemvm/patches/debian/config/opt/cloud/bin/cs/CsRedundant.py. When a backup 
router transitions to master, it successfully launches the dnsmasq service.

I should point out that I have only verified that the service is running. I 
have not confirmed that it is successfully configured.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: DAO: Hit the cache for entity flagged as ...

2016-05-06 Thread DaanHoogland

Github user DaanHoogland commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1532#discussion_r62303518
  
--- Diff: framework/db/src/com/cloud/utils/db/GenericDaoBase.java ---
@@ -969,7 +969,12 @@ public T findByUuidIncludingRemoved(final String uuid) 
{
 @Override
 @DB()
 public T findByIdIncludingRemoved(ID id) {
-return findById(id, true, null);
+if (_cache != null) {
+final Element element = _cache.get(id);
+return element == null ? findById(id, true, null) : 
(T)element.getObjectValue();
+} else {
--- End diff --

@jburwell if _cache == null a return needs to be there (or a throw of some 
sort)


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: DAO: Hit the cache for entity flagged as ...

2016-05-06 Thread jburwell

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1532#discussion_r62303317
  
--- Diff: framework/db/src/com/cloud/utils/db/GenericDaoBase.java ---
@@ -969,7 +969,12 @@ public T findByUuidIncludingRemoved(final String uuid) 
{
 @Override
 @DB()
 public T findByIdIncludingRemoved(ID id) {
-return findById(id, true, null);
+if (_cache != null) {
+final Element element = _cache.get(id);
+return element == null ? findById(id, true, null) : 
(T)element.getObjectValue();
+} else {
--- End diff --

This else clause is unnecessary due the return in preceding if clause.  
Please remove it to simplify the code.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Cloudstack 9339: Virtual Routers don't ha...

2016-05-06 Thread dsclose

Github user dsclose commented on the pull request:

https://github.com/apache/cloudstack/pull/1519#issuecomment-217383124
  
Squashed and force pushed. Tasks remaining:

- [ ] Pass CI
- [ ] Situational testing on VPC RvR
- [ ] Some automated tests

Regarding the automated tests for the routers, where should I look for 
these? If I see some examples I should be able to adjust/add to them.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Notify listeners when a host has been add...

2016-05-06 Thread miguelaferreira

Github user miguelaferreira commented on the pull request:

https://github.com/apache/cloudstack/pull/816#issuecomment-217370720
  
Hi @mike-tutkowski,

I trust you either addressed all my previous remarks, or have a good reason 
not to.
I've skimmed (very quickly through the diff) and the code looks good, with 
small bits, didn't see any commented out code, plus you added integration 
tests: seems like a good PR!

So ð 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Vmdk findbugs

2016-05-06 Thread DaanHoogland

GitHub user DaanHoogland reopened a pull request:

https://github.com/apache/cloudstack/pull/1530

Vmdk findbugs

replacing #1351 

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/DaanHoogland/cloudstack vmdk-findbugs

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/1530.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 #1530


commit 2e23d875637937fc1e4f910aeb6cb47980bc42da
Author: Daan Hoogland 
Date:   2016-01-19T15:55:05Z

findbugs: pre-fix reformat

commit a2b349d6c3ff49a0d758f1734213b3ab7200d0d2
Author: Daan Hoogland 
Date:   2016-01-19T15:56:20Z

findbugs: use explicit encoding




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Vmdk findbugs

2016-05-06 Thread DaanHoogland

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1530#issuecomment-217370549
  
@jburwell Even when out of scope, your comments may make sense. please send 
a pull request to my branch and we can discuss those.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Vmdk findbugs

2016-05-06 Thread DaanHoogland

Github user DaanHoogland closed the pull request at:

https://github.com/apache/cloudstack/pull/1530


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

97 matches

Mail list logo