[GitHub] cloudstack pull request: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-172362547
  
This PR has already been merged by @remibergsma . For same weird reason it 
remained open here and now says it has conflicts. Closing it.

Thanks, Remi!


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues closed the pull request at:

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


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[remibergsma]

Github user remibergsma commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-172359841
  
Run the tests again, all fine.

```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a 
tags=advanced,required_hardware=true \
component/test_vpc_redundant.py \
component/test_routers_iptables_default_policy.py \
component/test_routers_network_ops.py \
component/test_vpc_router_nics.py \
smoke/test_loadbalance.py \
smoke/test_internal_lb.py \
smoke/test_ssvm.py \
smoke/test_network.py

```

Result:

```
Check the password file in the Router VM ... === TestName: 
test_isolate_network_password_server | Status : SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network ... 
=== TestName: test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | Status : 
SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network and 
check default routes ... === TestName: test_02_redundant_VPC_default_routes | 
Status : SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network ... 
=== TestName: 
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | Status : 
SUCCESS ===
ok
Create a redundant VPC with 1 Tier, 1 VM, 1 ACL, 1 PF and test Network GC 
Nics ... === TestName: test_04_rvpc_network_garbage_collector_nics | Status : 
SUCCESS ===
ok
Create a redundant VPC with 1 Tier, 1 VM, 1 ACL, 1 PF and test Network GC 
Nics ... === TestName: test_05_rvpc_multi_tiers | Status : SUCCESS ===
ok
Test iptables default INPUT/FORWARD policy on RouterVM ... === TestName: 
test_02_routervm_iptables_policies | Status : SUCCESS ===
ok
Test iptables default INPUT/FORWARD policies on VPC router ... === 
TestName: test_01_single_VPC_iptables_policies | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: 
test_01_isolate_network_FW_PF_default_routes_egress_true | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: 
test_02_isolate_network_FW_PF_default_routes_egress_false | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: 
test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: 
test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: 
test_03_RVR_Network_check_router_state | Status : SUCCESS ===
ok
Create a VPC with two networks with one VM in each network and test nics 
after destroy ... === TestName: test_01_VPC_nics_after_destroy | Status : 
SUCCESS ===
ok
Create a VPC with two networks with one VM in each network and test default 
routes ... === TestName: test_02_VPC_default_routes | Status : SUCCESS ===
ok
Check that the /etc/dhcphosts.txt doesn't contain duplicate IPs ... === 
TestName: test_router_dhcphosts | Status : SUCCESS ===
ok
Test to create Load balancing rule with source NAT ... === TestName: 
test_01_create_lb_rule_src_nat | Status : SUCCESS ===
ok
Test to create Load balancing rule with non source NAT ... === TestName: 
test_02_create_lb_rule_non_nat | Status : SUCCESS ===
ok
Test for assign & removing load balancing rule ... === TestName: 
test_assign_and_removal_lb | Status : SUCCESS ===
ok
Test create, assign, remove of an Internal LB with roundrobin http traffic 
to 3 vm's in a Single VPC ... === TestName: 
test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 | Status : SUCCESS ===
ok
Test create, assign, remove of an Internal LB with roundrobin http traffic 
to 3 vm's in a Redundant VPC ... === TestName: 
test_02_internallb_roundrobin_1RVPC_3VM_HTTP_port80 | Status : SUCCESS ===
ok
Test to verify access to loadbalancer haproxy admin stats page ... === 
TestName: test_03_vpc_internallb_haproxy_stats_on_all_interfaces | Status : 
SUCCESS ===
ok
Test to verify access to loadbalancer haproxy admin stats page ... === 
TestName: test_04_rvpc_internallb_haproxy_stats_on_all_interfaces | Status : 
SUCCESS ===
ok
Test SSVM Internals ... === TestName: test_03_ssvm_internals | Status : 
SUCCESS ===
ok
Test CPVM Internals ... === TestName: test_04_cpvm_internals | Status : 
SUCCESS ===
ok
Test stop SSVM ... === TestName: test_05_stop_ssvm | Status : SUCCESS ===
ok
Test stop CPVM ... === TestName: test_06_stop_cpvm | Status : SUCCESS ===
ok
Test reboot SSVM ... === TestName: test_07_reboot_ssvm | Status : SUCCESS 
===
ok
Test reboot CPVM ... === TestName: test_08_reboot_cpvm | Status : SUCCESS 
===
ok
Test destroy SSVM ... === TestName: test_09_destroy_ssvm | Status : SUCCESS 
===
ok
Test destroy CPVM ... === TestName: test_10_destroy_cpvm | Status : SUCCESS 
===
ok
Test 

[GitHub] cloudstack pull request: [4.7] Critical VPCVR issues fixed: CLOUDS...

[remibergsma]

Github user remibergsma commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-172361223
  
PR is merged but not properly picked up by Github or so. Checked all 
commits, they are in 4.7 just fine. @wilderrodrigues please close this PR, 
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-172176953
  
Ping @remibergsma 

If you get a second LGTM, feel free to merge.

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

GitHub user wilderrodrigues reopened a pull request:

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

[4.7] Critical VPCVR issues fixed: CLOUDSTACK-9154; CLOUDSTACK-9187; and 
CLOUDSTACK-9188

This PR applies the same fixes as in the PR #1259, but against branch 4.7.

Please refer to PR #1259 for the tests results and all the comments already 
made there.

Issues fixed are:

* CLOUDSTACK-9154: rVPC doesn't recover from cleaning up of network garbage 
collector
* CLOUDSTACK-9187: rVPC routers in Master/Master due to concurrency problem 
when writing the keepalivd.conf
* CLOUDSTACK-9188: NetworkGarbageCollector is not using gc.interval and 
gc.wait from settings

Those changes have been covered by 2 new tests added to 
```smoke/test_vpc_redundant.py```:

* test_04_rvpc_network_garbage_collector_nics
* test_05_rvpc_multi_tiers

The test ```test_04_rvpc_network_garbage_collector_nics``` depends on the 
global settings for the network.gc.interval and gc.wait. If one wants the test 
to run quicker, please change the settings (default is 600 seconds for each) 
and restart the Management Server before running the tests. I would suggest to 
set it to 60 seconds.

In addition, the NetworkGarbageCollector was redefining the settings above 
mentioned and not reading their values through ConfigDao. Due to that, the 
settings were not being applied properly and the test was waiting to long to 
check the VPC routers.

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

$ git pull https://github.com/ekholabs/cloudstack 
fix/4.7-rvpc-net-gc-CLOUDSTACK-9154

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

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


commit 7988f51ac07782ad9b9873c32438fd7cdb24edba
Author: Wilder Rodrigues 
Date:   2015-12-17T13:20:59Z

CLOUDSTACK-9154 - Returns the guest iterface that is marked as added

   - Force a restart of keepalived if conntrackd is not running or 
configuration has changed

commit 749ac2e2242d8af9f05977951dd1b4855c1f6f08
Author: Wilder Rodrigues 
Date:   2015-12-18T09:37:13Z

CLOUDSTACK-9154 - Adds test to cover nics state after GC

commit b1e421068280fe0acc427baf3e9f07dd2d610803
Author: Wilder Rodrigues 
Date:   2015-12-18T17:32:29Z

CLOUDSTACK-9187 - Adds test to cover multiple nics and nic removal

commit c99d6f18c9fccdc44698a30af1b20701a2e85df4
Author: Wilder Rodrigues 
Date:   2015-12-18T17:36:02Z

CLOUDSTACK-9187 - Fixes interface allocation to VRRP instances

commit 3e1cc4ecef716670a9c8953c791189b22d5c9a9e
Author: Wilder Rodrigues 
Date:   2016-01-08T08:22:55Z

CLOUDSTACK-9188 - Formatting the code

commit 638a818a5a6de6cabd2daa8f17325e9475ca0c81
Author: Wilder Rodrigues 
Date:   2016-01-09T09:41:34Z

CLOUDSTACK-9187 - Makes code ready for more something like eth, if we 
ever get that far

commit 6c7ef4b713855002f5ba7a675a315f2723bbf680
Author: Wilder Rodrigues 
Date:   2016-01-09T09:41:50Z

CLOUDSTACK-9154 - Sets the pub interface down when all guest nets are gone

   - Refactors the set_backup, set_master and set_fault methods to have 
better names for the variable
   - Increase the sleep on the test in order to wait for the routers to be 
ready. It's now 3 times the GC settings




---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[remibergsma]

Github user remibergsma commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-172246077
  
@koushik-das @DaanHoogland You both reviewed this PR. What do we need to do 
to get it in? Those are critical fixes I want in the next releases. 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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-172250734
  
@remibergsma @DaanHoogland 

This PR doesn't contain the commit that @koushik-das voted -1. So, it's 
good to go.

I just double checked in the branch and also in the file itself - via 
github. The changes on the GC class are not present and my last test report was 
fine.

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[remibergsma]

Github user remibergsma commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-172250789
  
Thanks @wilderrodrigues happy we can move on now! 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: [4.7] Critical VPCVR issues fixed: CLOUDS...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-172260184
  
looked at the code. python LGTM. The reformat seems unneeded, it seems the 
java code does not contain any change but reformat and making stuff final.


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[remibergsma]

Github user remibergsma commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-172057412
  
@wilderrodrigues Please reopen this PR, it needs to get in as those are 
important fixes. 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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-171572729
  
If those changes are needed by the community in the futures, please let me 
know and I will reopen the PR. One can also feel free to create a PR on top of 
this one and push back to the ACS repo.

Cheers,
Wilder 


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues closed the pull request at:

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


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-171597154
  
:( indeed, @remibergsma. We have a policy against having our fork on github 
(tell you over a beer someday) Otherwise I think it would be merged in our 
private branch ;) After I see a successful integration run as well, btw.


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-171591896
  
To bad this is taking to much effort. We or someone will run into this and 
need it. hopefully we get it in then or someday, @wilderrodrigues. Thanks for 
trying anyway. 


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[remibergsma]

Github user remibergsma commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-171593083
  
@DaanHoogland This PR addresses serious issues. Without it we couldn't run 
a reliable production services. It's a shame we can't get it merged. :-(


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170508275
  
Hi @koushik-das 

Your questions were rhetorical and hence not valid because I did not 
disagree with you in terms of refactor. I asked you to help testing, which you 
partially did. But not having a test environment and relying on a simulator for 
everything seems a bit naive for me. You might remember that we had many 
problems when people did LGTM PRs based on the Travis results, which are not 
reliable.

As I wrote on Saturday, I was going to test again - with the changes - and 
post the result, which I did. I will now retest in a clean DC without the 
commit - GC related -  and post the results. We all expect it to pass, of 
course. But if it doesn't we need to find out why the "agnostic" nature of the 
implementation is broken.

We could talk for hours and disagree only on things being agnostic within 
ACS. I have a few cases in my sleeve. But let's not start again.

Don't give up and say it's useless. At the end we will learn something 
anyway, even from rants on github.

By the way, the ```smoke/test_routers.py``` tests were executed by 
@remibergsma as well.

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[koushik-das]

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170483420
  
@wilderrodrigues My questions and comments are very valid. If you don't see 
any value in them too bad. Looks like you don't want to understand the point I 
am trying to make. Here is some more data if you are really interested. I also 
don't have time for the useless comments you are making.

There is a set of VR tests which validates network gc (check out 
test/integration/component/test_routers.py). I ran it on latest master (without 
any PRs) using a simulator setup and it worked as expected.

nosetests-2.7 --with-marvin --marvin-config=setup/dev/advanced.cfg   
--with-xunit  --xunit-file=out.xml  
test/integration/component/test_routers.py:TestRouterServices -a 
tags=advanced,required_hardware=false  --zone=Sandbox-simulator  
--hypervisor=simulator

Test advanced zone router services ... === TestName: 
test_01_AdvancedZoneRouterServices | Status : SUCCESS ===
ok
Test network garbage collection ... === TestName: 
test_02_NetworkGarbageCollection | Status : SUCCESS ===
ok
Test router start on VM deploy ... === TestName: 
test_03_RouterStartOnVmDeploy | Status : SUCCESS ===
ok

--
Ran 3 tests in 150.061s

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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170535918
  
Let's all move on, @koushik-das. If we don't, we will be talking about this 
for weeks. We have been saying the same thing all the time. I got your points 
and if I could not make it clear, sorry: I could not put more time on writing 
comments here to make it clearer.

The scheduler code is pure Java, it will work even with Unit Tests - not 
even a running management server is needed. Unfortunately, it's been 20 das 
since I create the PR. So, I can't remember what went wrong, that's why I'm 
retesting without the commit. Last time I did, on Saturday, could even be 
something in my environment: that's why I'm doing again in a clean DC.

All the time we wasted typing here could have been used to test the changes 
with[out] the commit you reviewed. Testing against hardware, with the proper 
integration tests. I know when real hardware is needed. However, we don't trust 
the simulator.

If the test passes, do you - representing the community - want my 
contribution or not? If you don't, I will just close the PR because I don't 
want to keep writing here every 20 minutes.

Perhaps saying your questions were rhetorical was not right, but way better 
than using other words. 

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170558660
  
@DaanHoogland @remibergsma @koushik-das 

It worked here without the commit mentioned by @koushik-das .

```
[root@cs1 integration]# less 
/tmp//MarvinLogs/test_vpc_redundant_ABA7FW/results.txt 
Create a redundant VPC with two networks with two VMs in each network ... 
=== TestName: test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | Status : 
SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network and 
check default routes ... === TestName: test_02_redundant_VPC_default_routes | 
Status : SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network ... 
=== TestName: 
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | Status : 
SUCCESS ===
ok
Create a redundant VPC with 1 Tier, 1 VM, 1 ACL, 1 PF and test Network GC 
Nics ... === TestName: test_04_rvpc_network_garbage_collector_nics | Status : 
SUCCESS ===
ok
Create a redundant VPC with 1 Tier, 1 VM, 1 ACL, 1 PF and test Network GC 
Nics ... === TestName: test_05_rvpc_multi_tiers | Status : SUCCESS ===
ok

--
Ran 5 tests in 6109.473s

OK
/tmp//MarvinLogs/test_vpc_redundant_ABA7FW/results.txt (END)
```

If you think it should be added, I will push the PR without the commit 
21da4cf518fb0d6858d6ce07ab1ead156345995a

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[koushik-das]

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170520102
  
@wilderrodrigues My questions were not in the context of refactor at all. 
The reason for asking the questions was to highlight the point that for testing 
config parameters you don't need real hardware to test. And based on that the 
testing I did was sufficient to verify the specific commit. So you see the 
questions were not rhetorical at all. Based on your comments it looks like you 
never understood what I was trying to say.

About testing with real hardware, it is only needed when you want to verify 
something by actually accessing the VM OR some HV specific scenario needs to be 
tested. For e.g. ssh to VM for validating certain properties of the VM OR 
verifying if some n/w rules etc. are programmed properly in VR. Anything else 
can be run in both real hardware as well as simulator.





---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: [4.7] Critical VPCVR issues fixed: CLOUDS...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170542781
  
I got two failures. egress and an exception VPC_nics_after_destroy. No time 
to investigate today.

[1277.vpc.results.txt](https://github.com/apache/cloudstack/files/85753/1277.vpc.results.txt)

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



---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170340809
  
* Results

```
[root@cs1 integration]# less 
/tmp//MarvinLogs/test_vpc_redundant_DIBS05/results.txt 
Create a redundant VPC with two networks with two VMs in each network ... 
=== TestName: test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | Status : 
SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network and 
check default routes ... === TestName: test_02_redundant_VPC_default_routes | 
Status : SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network ... 
=== TestName: 
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | Status : 
SUCCESS ===
ok
Create a redundant VPC with 1 Tier, 1 VM, 1 ACL, 1 PF and test Network GC 
Nics ... === TestName: test_04_rvpc_network_garbage_collector_nics | Status : 
SUCCESS ===
ok
Create a redundant VPC with 1 Tier, 1 VM, 1 ACL, 1 PF and test Network GC 
Nics ... === TestName: test_05_rvpc_multi_tiers | Status : SUCCESS ===
ok

--
Ran 5 tests in 6092.459s

OK
/tmp//MarvinLogs/test_vpc_redundant_DIBS05/results.txt (END)
```

I'll give it another chance without the commit you -1, but only tomorrow. 
Going to cycle now.

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170343091
  
@wilderrodrigues I will start this in my old bubble and do some 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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170352604
  
never mind, should have tried with -b 4.7


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170352576
  
got an error during the install:
```
/data/git/cs1/cloudstack
Done.
HEAD is now at 2a9927a Merge pull request #1315 from pavanb018/master
Switched to branch 'master'
Branch master set up to track remote branch master from origin.
Already up-to-date.
Deleted branch try/1277 (was 2a9927a).
Switched to branch 'try/1277'
WARNING: You used --force to merge to 'try/1277' while this PR is for 
branch '4.7'.
INFO: Using remote repository 'origin' to fetch PR (should point to 
github.com/apache/cloudstack.git)
INFO: PR #1277 against branch '4.7' from 'wilderrodrigues': '[4.7] Critical 
VPCVR issues fixed: CLOUDSTACK-9154; CLOUDSTACK-9187; and CLOUDSTACK-9188'
INFO: has state 'open' and mergable state 'clean', about to be merged in 
branch 'try/1277'.
ATTENTION: Merging pull request #1277 from 
ekholabs/fix/4.7-rvpc-net-gc-CLOUDSTACK-9154 into 'try/1277' branch in 5 
seconds. CTRL+c to abort..
5 4 3 2 1 0
INFO: Executing the merge now.. Git output below:
INFO: 
***
From https://github.com/apache/cloudstack
 * [new ref] refs/pull/1277/head -> pr/1277
Auto-merging 
engine/orchestration/src/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
CONFLICT (content): Merge conflict in 
engine/orchestration/src/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
Automatic merge failed; fix conflicts and then commit the result.
ERROR: Merge failed, aborting.

ERROR: Merge failed!
```
my check script uses the ```git pr``` command from the cloudstack repo


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[remibergsma]

Github user remibergsma commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170360300
  
@DaanHoogland Indeed, that should work :-)


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[koushik-das]

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170240228
  
@wilderrodrigues
- Do you agree that gc thread should run every gc.interval seconds whether 
it has to cleanup any network or not?
- Do you agree that the gc thread starts running as soon as the MS starts 
up?
- Since the network gc logic is hypervisor agnostic do you agree that 
simulator and real hardware shouldn't make a difference here at least in terms 
of config values?

Since you are only seeing this with some specific VPC test cases, I am 
suspecting it has more to do with the specific test scenario rather than the 
config values.


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170236373
  
@koushik-das 

You will have to run the tests on your environment. How did you check that 
the Network GC was running every 10 seconds? After removing the commit with my 
changes the tests are now failing.

In order to hold your -1 you will have to run the tests and prove they are 
working fine.

```
Create a redundant VPC with 1 Tier, 1 VM, 1 ACL, 1 PF and test Network GC 
Nics ... === TestName: test_04_rvpc_network_garbage_collector_nics | Status : 
FAILED ===
FAIL
Create a redundant VPC with 1 Tier, 1 VM, 1 ACL, 1 PF and test Network GC 
Nics ... === TestName: test_05_rvpc_multi_tiers | Status : FAILED ===
FAIL
```

I will put my commit back and rerun the tests.

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170237820
  
Hi @koushik-das ,

Do you have real hardware to test?

Well I have 2 tests running agains real hardware. @remibergsma also 
tested it and all passed. I don't really need to attache a debugger.

You actually need to test it, to have a VPC, Tier and VM; do the removal; 
etc. Like I have suggested.

I'm running the tests again, with the new code. By the way, I was also 
using 10 seconds as interval.

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[koushik-das]

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170238553
  
@wilderrodrigues
The GC thread execution frequency shouldn't depend on real hardware or 
simulator. It will always execute at gc.interval seconds as soon as the MS is 
started. So whether validated with real hardware or simulator shouldn't matter 
at least for validating config values.

I am not sure what kind of work the GC thread is doing for the tests that 
are failing in your environment.
With real hardware one of the possibility is:
- GC thread may take a longer time to complete (> 10 seconds that you have 
configured). In this case the next instance of the thread will only run when 
the previous one is completed. So this needs to be ruled out.


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170239651
  
Are you not sure what it should do once real machines are gone and a 
network is still present? 

The tests do:

1. Deploy a DC
2. Create a VPC + Tier + VM
3. Change the GC settings for 10 seconds, for example
4. Restart the Management Server
5. Remove the VM from the tier

After that, the tests sleeps the time needed (based on GC interval/wait) in 
order to check if the routers are in the correct state once the guest networks 
are gone.

I will run the tests with the changes to see how it goes.

Cheers,
Wilder



---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[koushik-das]

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170237613
  
@wilderrodrigues I simply ran the steps mentioned against latest master in 
a simulator setup and attached a debugger. The network GC thread was running 
every 10 secs. The GC thread wasn't doing any actual work as there were no 
network. I simply verified that the config parameters are working correctly.

Can you just attach a debugger and put a breakpoint in the GC code and see 
if it is running every gc.interval seconds?



---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170242223
  
Hi @koushik-das 

It's not only with specific VPC tests, @koushik-das. That's why I asked you 
to do it manually, but unfortunately you don't have a proper test environment.

I would be really happy to see the tests failing again (but this time with 
my changes, which would be the first time!). I will post the results here and 
if that's all fine I will close the PR. Unless you want to help for real. I 
mean, if you want to identify why it doesn't work with the previous 
implementation.

If the only help you can give is making rhetorical questions, then I'm done 
with it. I don't have time for pissing contest with code. So long, so long and 
thanks for all the fish.

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170218395
  
@remibergsma @koushik-das @DaanHoogland 

I removed the commit you mentioned. But now we have to retest it. Do you 
have a test environment? I need your help us running the new integration tests. 
Could you also do it?

You have to run the ```smoke/test_vpc_redundant.py``` test using an advance 
zone setup.

The command is:

```
nosetests --with-marvin --marvin-config=[YOUR_CONFIG_HERE] -s -a 
tags=advanced,required_hardware=true \
component/test_vpc_redundant.py 
```

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-169928856
  
Hi @DaanHoogland and @koushik-das 

Now I see your point. Thanks for the screenshot, @DaanHoogland. I checked 
the Github diff but did not notice that the file was omitted, hence my question.

I'm splitting the NetworkOrchestrator related commits (formatting and 
fixing) so you can analyse the changes.

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-169939093
  
@DaanHoogland and @koushik-das 

I split the commits and pushed the changes. You now should be able to 
review the actual changes I have applied to NetworkOchestrator file. The commit 
ID is a0a230af7b1eb1bf24da8ed0d9e2d1a4fc6a5fe4

I built the source, including unit tests execution, to make sure all 
compiles and I also did a diff between the old file and the 1 changes after the 
2 commits.

* Build result
```
[INFO] 

[INFO] BUILD SUCCESS
[INFO] 

[INFO] Total time: 17:44 min
[INFO] Finished at: 2016-01-08T09:49:24+01:00
[INFO] Final Memory: 121M/947M
[INFO] 

```

* Diff output
```
sbpltk1zffh04:asf_cloudstack wrodrigues$ diff ~/dev/gc_net.java 
engine/orchestration/src/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
3352d3351
< 
```

As you can see, the difference is irrelevant.

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[remibergsma]

Github user remibergsma commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170098946
  
LGTM based on these tests:

```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a 
tags=advanced,required_hardware=true \
component/test_vpc_redundant.py \
component/test_routers_iptables_default_policy.py \
component/test_routers_network_ops.py \
component/test_vpc_router_nics.py \
smoke/test_loadbalance.py \
smoke/test_internal_lb.py \
smoke/test_ssvm.py \
smoke/test_network.py

```

Result:

```
Create a redundant VPC with two networks with two VMs in each network ... 
=== TestName: test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | Status : 
SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network and 
check default routes ... === TestName: test_02_redundant_VPC_default_routes | 
Status : SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network ... 
=== TestName: 
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | Status : 
SUCCESS ===
ok
Test iptables default INPUT/FORWARD policy on RouterVM ... === TestName: 
test_02_routervm_iptables_policies | Status : SUCCESS ===
ok
Test iptables default INPUT/FORWARD policies on VPC router ... === 
TestName: test_01_single_VPC_iptables_policies | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: 
test_01_isolate_network_FW_PF_default_routes_egress_true | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: 
test_02_isolate_network_FW_PF_default_routes_egress_false | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: 
test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: 
test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false | Status : SUCCESS ===
ok
Create a VPC with two networks with one VM in each network and test nics 
after destroy ... === TestName: test_01_VPC_nics_after_destroy | Status : 
SUCCESS ===
ok
Create a VPC with two networks with one VM in each network and test default 
routes ... === TestName: test_02_VPC_default_routes | Status : SUCCESS ===
ok
Check the password file in the Router VM ... === TestName: 
test_isolate_network_password_server | Status : SUCCESS ===
ok
Check that the /etc/dhcphosts.txt doesn't contain duplicate IPs ... === 
TestName: test_router_dhcphosts | Status : SUCCESS ===
ok
Test to create Load balancing rule with source NAT ... === TestName: 
test_01_create_lb_rule_src_nat | Status : SUCCESS ===
ok
Test to create Load balancing rule with non source NAT ... === TestName: 
test_02_create_lb_rule_non_nat | Status : SUCCESS ===
ok
Test for assign & removing load balancing rule ... === TestName: 
test_assign_and_removal_lb | Status : SUCCESS ===
ok
Test to verify access to loadbalancer haproxy admin stats page ... === 
TestName: test02_internallb_haproxy_stats_on_all_interfaces | Status : SUCCESS 
===
ok
Test create, assign, remove of an Internal LB with roundrobin http traffic 
to 3 vm's ... === TestName: test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 
| Status : SUCCESS ===
ok
Test SSVM Internals ... === TestName: test_03_ssvm_internals | Status : 
SUCCESS ===
ok
Test CPVM Internals ... === TestName: test_04_cpvm_internals | Status : 
SUCCESS ===
ok
Test stop SSVM ... === TestName: test_05_stop_ssvm | Status : SUCCESS ===
ok
Test stop CPVM ... === TestName: test_06_stop_cpvm | Status : SUCCESS ===
ok
Test reboot SSVM ... === TestName: test_07_reboot_ssvm | Status : SUCCESS 
===
ok
Test reboot CPVM ... === TestName: test_08_reboot_cpvm | Status : SUCCESS 
===
ok
Test destroy SSVM ... === TestName: test_09_destroy_ssvm | Status : SUCCESS 
===
ok
Test destroy CPVM ... === TestName: test_10_destroy_cpvm | Status : SUCCESS 
===
ok
Test Remote Access VPN in VPC ... === TestName: test_vpc_remote_access_vpn 
| Status : SUCCESS ===
ok
Test VPN in VPC ... === TestName: test_vpc_site2site_vpn | Status : SUCCESS 
===
ok
Test for port forwarding on source NAT ... === TestName: 
test_01_port_fwd_on_src_nat | Status : SUCCESS ===
ok
Test for port forwarding on non source NAT ... === TestName: 
test_02_port_fwd_on_non_src_nat | Status : SUCCESS ===
ok
Test for reboot router ... === TestName: test_reboot_router | Status : 
SUCCESS ===
ok
Test for Router rules for network rules on acquired public IP ... === 
TestName: test_network_rules_acquired_public_ip_1_static_nat_rule | Status : 
SUCCESS ===
ok
Test for Router rules for network rules on acquired public IP ... === 
TestName: test_network_rules_acquired_public_ip_2_nat_rule | Status : SUCCESS 
===

[GitHub] cloudstack pull request: [4.7] Critical VPCVR issues fixed: CLOUDS...

[koushik-das]

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170202865
  
On latest master (without any fixes) I see that the config values 
(network.gc.interval and network.gc.wait) are read correctly. Did the following:
- Started MS
- Changed the config values to 10 secs.
- Restarted MS
- Values read correctly and GC thread was running every 10 secs. Inside the 
GC thread correct value was shown for gc.wait

@wilderrodrigues On which branch did you notice this issue? Or is it 
possible that the issue is something else?
-1 for commit a0a230a/CLOUDSTACK-9188 based on the test on master.

Regarding these network gc configs, the only cleanup that can be done on 
the code is to remove the entries from Config.java as they are already defined 
in NetworkOrchestrator.java. This can go as a separate PR as well.


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170017549
  
Try something, @koushik-das:

```
Do not use this code for the steps below, but Master instead.
```

1. Deploy a DC
2. Create a VPC + Tier + VM
3. Change the GC settings for 10 seconds, for example
4. Restart the Management Server
5. Remove the VM from the tier
6. Observe if the Network GC is actually running every 10 seconds - as now 
defined in the DB - or if it is still using the 600 seconds hardcoded in the 
nested Enum.

That's why a changed it. I needed to make sure that the changes applied in 
the global settings would be retrieved from the Database. But don't bother, 
it's already deployed in our production here and working really nice.

Concerning gradually refactoring code, I agree with that - I'm not an 
idiot. Actually, I have done a lot of refactoring on ACS since I started. I 
haven't been adding glue to things or not writing tests. But in that case, the 
whole configuration is messed and I don't agree with someone adding separate 
things to classes and letting them be for years.

If for some reason I did not get the way it should work, you can do one of 
two things:

1. Mark the PR with a -1; or
2. Checkout the code; fix the way you want; make sure the tests passes; and 
push a PR on top of this one.

Have a happy coding.

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[koushik-das]

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-169979032
  
As a general cleanup, all configs defined in Config.java should be 
gradually moved to the appropriate java classes from where they are used. We 
should do away with the old way of managing configs and switch to the new 
mechanism.



---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-169996476
  
Do you agree that it is a completely different issue you are trying to 
address?

ACS needs refactor in each corner of its code, but we cannot work on a 
issue and do a massive refactor in the whole configuration model.

There should be a group of people defining the new design for the 
configuration and work on it, not scattered people around the world doing 
things at their will.

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[koushik-das]

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-170011469
  
I have clearly mentioned that the refactoring needs to be done gradually. 
But in the changes you have reverted to using the config defined in Config.java 
when existing code is already using the config defined in 
NetworkOrchestrator.java. Why are you changing back to the old config mechanism 
when the configs are already defined in NetworkOrchestrator?


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-169609249
  
@wilderrodrigues as @koushik-das says, the diff for 
engine/orchestration/src/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
 does not show in github. 
![screen shot 2016-01-07 at 10 32 
49](https://cloud.githubusercontent.com/assets/2486961/12167023/16b8dca6-b52a-11e5-9824-608b23458c3a.png)

I will try to find time to review but I am not familiar enough with this 
code so it will need to be a lot of 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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-169304485
  
Ping @remibergsma @DaanHoogland @koushik-das @bhaisaab 

Is there anyone caring about getting those fixes in? If not, I will close 
the PR.

@koushik-das: I don't have any problems seeing the changes on github. I 
haven't got any problem with github showing the diff.

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[remibergsma]

Github user remibergsma commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-166881123
  
@koushik-das Github cannot display large diffs. This is how you can show it 
on your local checkout:

```
prId=1277
git checkout master
git fetch origin pull/${prId}/head:pr/${prId}
git checkout pr/${prId}
git show 2aab4c142d47b77e7bbc584927a80b8ba180934e 
engine/orchestration/src/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
```

Hope this helps.


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[koushik-das]

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-166884395
  
@remibergsma What you are suggesting is to pull the commit locally and then 
view it using git show. But then we are loosing the benefit of viewing and 
in-place review of the code where you can put some comment. Would it help to 
break these bigger changes into multiple commits to keep github happy?


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

GitHub user wilderrodrigues opened a pull request:

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

[4.7] Critical VPCVR issues fixed: CLOUDSTACK-9154; CLOUDSTACK-9187; and 
CLOUDSTACK-9188

This PR applies the same fixes as in the PR #1259, but against branch 4.7.

Please refer to PR #1259 for the tests results and all the comments already 
made there.

Issues fixed are:

* CLOUDSTACK-9154: rVPC doesn't recover from cleaning up of network garbage 
collector
* CLOUDSTACK-9187: rVPC routers in Master/Master due to concurrency problem 
when writing the keepalivd.conf
* CLOUDSTACK-9188: NetworkGarbageCollector is not using gc.interval and 
gc.wait from settings

Those changes have been covered by 2 new tests added to 
```smoke/test_vpc_redundant.py```:

* test_04_rvpc_network_garbage_collector_nics
* test_05_rvpc_multi_tiers

The test ```test_04_rvpc_network_garbage_collector_nics``` depends on the 
global settings for the network.gc.interval and gc.wait. If one wants the test 
to run quicker, please change the settings (default is 600 seconds for each) 
and restart the Management Server before running the tests. I would suggest to 
set it to 60 seconds.

In addition, the NetworkGarbageCollector was redefining the settings above 
mentioned and not reading their values through ConfigDao. Due to that, the 
settings were not being applied properly and the test was waiting to long to 
check the VPC routers.

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

$ git pull https://github.com/ekholabs/cloudstack 
fix/4.7-rvpc-net-gc-CLOUDSTACK-9154

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

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


commit 7988f51ac07782ad9b9873c32438fd7cdb24edba
Author: Wilder Rodrigues 
Date:   2015-12-17T13:20:59Z

CLOUDSTACK-9154 - Returns the guest iterface that is marked as added

   - Force a restart of keepalived if conntrackd is not running or 
configuration has changed

commit 749ac2e2242d8af9f05977951dd1b4855c1f6f08
Author: Wilder Rodrigues 
Date:   2015-12-18T09:37:13Z

CLOUDSTACK-9154 - Adds test to cover nics state after GC

commit b1e421068280fe0acc427baf3e9f07dd2d610803
Author: Wilder Rodrigues 
Date:   2015-12-18T17:32:29Z

CLOUDSTACK-9187 - Adds test to cover multiple nics and nic removal

commit c99d6f18c9fccdc44698a30af1b20701a2e85df4
Author: Wilder Rodrigues 
Date:   2015-12-18T17:36:02Z

CLOUDSTACK-9187 - Fixes interface allocation to VRRP instances

commit 2aab4c142d47b77e7bbc584927a80b8ba180934e
Author: Wilder Rodrigues 
Date:   2015-12-18T18:56:06Z

CLOUDSTACK-9188 -  Reads network GC interval and wait from configDao

commit f5a6dee8dd6b89ef954d39b4ade91f8f898cc026
Author: Wilder Rodrigues 
Date:   2015-12-18T19:18:24Z

CLOUDSTACK-9187 - Makes code ready for more something like eth, if we 
ever get that far

   - Adds log info to NetworkOrchestrator in order to make the work of the 
Net-Scavenger more visible.

commit 5ef3144fdf65922a3da99357c6f90f3811b231d8
Author: Wilder Rodrigues 
Date:   2015-12-19T10:21:18Z

CLOUDSTACK-9154 - Sets the pub interface down when all guest nets are gone

   - Refactors the set_backup, set_master and set_fault methods to have 
better names for the variable
   - Increase the sleep on the test in order to wait for the routers to be 
ready. It's now 3 times the GC settings




---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[wilderrodrigues]

Github user wilderrodrigues commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-13159
  
Ping @remibergsma @miguelaferreira @borisroman @michaelandersen

PR created against 4.7. Please check all tests I ran and results in the 
previous PR.

Cheers,
Wilder


---
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: [4.7] Critical VPCVR issues fixed: CLOUDS...

[koushik-das]

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1277#issuecomment-166820236
  
Github is not showing the diff for the below file as there are too many 
changes. Is there any way to display it?


engine/orchestration/src/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
476 additions, 468 deletions not shown


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