You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by remibergsma <gi...@git.apache.org> on 2015/09/25 00:26:51 UTC

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

GitHub user remibergsma opened a pull request:

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

    [BLOCKER] Combined PRs that fix VR issues

    Tonight I worked with @wilderrodrigues to figure out what is wrong with the virtual router. As we couldn't test single PRs any more (because of other issues with them causing tests to fail) we added all VR related PRs in a separate branch and started testing from there.
    
    We combined the following PRs into this PR:
    #836 #851 #867 #870 #881 #882 #842
    
    After that, one issue remains: the VPC does not get a default gateway. Which is strange, because we already solved it in PR #738. When I look back, it was fixed again in PR #784. It could very well be that either one fixed one specific case, but also breaking the other. We need to investigate this, and make sure there will be a fix that works both for VPCs and VRs.
    
    When we manually add the default gateway on the VPC, most tests pass and also spinning up two VPCs with one tier each, having a VM and them using s2s to VPN them together works fine. See for more details the report Wilder sent earlier.
    
    Tomorrow we'll try to figure out how to fix the default gateway and merge this. Then we should have a base to work from again. Any PR that fixes another blocker, should at least then be rebased against the fixed master so we can run the tests against the PR branch. I'm not saying everything is fixed, I'm just saying that we can spin up a cloud that has working VMs.
    
    When, in the mean time, someone has the time to checkout this branch and make the default route work for both VPC and VR that would be awesome. After that we should double check and verify the test results. 
    
    Pinging @karuturi to let her know the current status.
    
    Regards,
    Wilder / Remi

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

    $ git pull https://github.com/schubergphilis/cloudstack vr_fixes_combined

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

    https://github.com/apache/cloudstack/pull/887.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 #887
    
----
commit a15df0569fa0e56b14a9a119858c53e8ae6085c3
Author: Jayapal <ja...@apache.org>
Date:   2015-09-16T09:52:33Z

    CLOUDSTACK-8843: Fixed issue in default iptables rules on shared network VR

commit 56d4429500d0d3da7334b3f1c559d1eca8ee85a4
Author: SudharmaJain <su...@citrix.com>
Date:   2015-09-16T09:10:31Z

    CLOUDSTACK-8863: VM doesn't reconnect to internet post VR RESTART/STOP-START/RECREATE

commit 96c38bf491d81e41975dddbfc3c87716293c7bdf
Author: SudharmaJain <su...@citrix.com>
Date:   2015-09-19T18:10:21Z

    CLOUDSTACK-8864: Not able to add TCP port forwarding rule in VPN for specific ports

commit dbedfe2557839332c394c44deb6c376f386681d9
Author: Jayapal <ja...@apache.org>
Date:   2015-09-17T06:04:27Z

    Configured dnsmasq to listen on all interfaces so that vpn  client gets dns

commit 746a5dc48e01cc07cbd4b319755d45e414c49504
Author: Jayapal <ja...@apache.org>
Date:   2015-09-24T07:14:15Z

    CLOUDSTACK-8891: Fixed default iptables rules on VR  for guest traffic

commit 2bf7fb4b63932d80f641462073c751f07ab0c3ea
Author: Jayapal <ja...@apache.org>
Date:   2015-09-24T11:36:11Z

    CLOUDSTACK-8905: Fixed hooking egress rules

commit 40138d2e994458250b8db706be993d4b040f95ca
Author: Jayapal <ja...@apache.org>
Date:   2015-09-24T11:52:29Z

    CLOUDSTACK-8881: Fixed Static and PF configuration issue

commit 8367911ef7f502eb760ca57d0a018d96620fdbed
Author: Remi Bergsma <gi...@remi.nl>
Date:   2015-09-24T13:35:00Z

    Merge pull request #836 from SudharmaJain/cs-8863
    
    CLOUDSTACK-8863: VM doesn't reconnect to internet post VR RESTART/STOP-START/RECREATE
    
    The ongoing ICMP request reply session is broken when the VR is down, the expectation is that it would resume once the VR is up. Investigations revealed that the ongoing ICMP packets are sent out of eth2 without being NATed post VR stop/start or restart or recreate.
    
    TCPDUMP output from VR post restart/stop-start/recreate on eth2:
    
    root@r-4-VM:~# tcpdump -i eth2 icmp -n -vvv
    tcpdump: listening on eth2, link-type EN10MB (Ethernet), capture size 65535 bytes
    06:22:52.749770 IP (tos 0x0, ttl 63, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
        192.168.200.67 > 173.194.33.163: ICMP echo request, id 30996, seq 81, length 64
    06:22:53.749782 IP (tos 0x0, ttl 63, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
        192.168.200.67 > 173.194.33.163: ICMP echo request, id 30996, seq 82, length 64
    06:22:54.749771 IP (tos 0x0, ttl 63, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
        192.168.200.67 > 173.194.33.163: ICMP echo request, id 30996, seq 83, length 64
    06:22:55.749775 IP (tos 0x0, ttl 63, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
        192.168.200.67 > 173.194.33.163: ICMP echo request, id 30996, seq 84, length 64
    06:22:56.749765 IP (tos 0x0, ttl 63, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
        192.168.200.67 > 173.194.33.163: ICMP echo request, id 30996, seq 85, length 64
    06:22:57.749776 IP (tos 0x0, ttl 63, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
        192.168.200.67 > 173.194.33.163: ICMP echo request, id 30996, seq 86, length 64
    ^C
    6 packets captured
    6 packets received by filter
    0 packets dropped by kernel
    root@r-4-VM:~#
    root@r-4-VM:~# grep icmp /proc/net/ip_conntrack
    icmp     1 29 src=192.168.200.67 dst=173.194.33.163 type=8 code=0 id=30996 [UNREPLIED] src=173.194.33.163 dst=192.168.200.67 type=0 code=0 id=30996 mark=0 use=2
    
    This get fixed after flushing the conntrack table.
    
    Screenshots:
    
    Before fix (ping session doesn't resume, stop and starting the ping works, 120 packets lost):
    ![image](https://cloud.githubusercontent.com/assets/12229259/9897800/4de7488e-5c6a-11e5-98eb-3bd79cc3a8b1.png)
    
    After fix(ping session resumes, 27 packets lost):
    ![image](https://cloud.githubusercontent.com/assets/12229259/9897822/9112e866-5c6a-11e5-95b3-1b20600d2e44.png)
    
    * pr/836:
      CLOUDSTACK-8863: VM doesn't reconnect to internet post VR RESTART/STOP-START/RECREATE
    
    Signed-off-by: Remi Bergsma <gi...@remi.nl>

commit 7d5555429b90fcb9e1456ea858d5163b41ee41ab
Author: Remi Bergsma <gi...@remi.nl>
Date:   2015-09-24T13:35:16Z

    Merge pull request #851 from SudharmaJain/cs-8864
    
    CLOUDSTACK-8864: Not able to add TCP port forwarding rule in VPN for specific ports
    
    Setting port forwarding rules for port 500,1701 and 4500 after enabling VPN, gives the error message "The range specified, xxxx, conflicts with rule xxxx which has xxxx." This happens because the rules added for vpn doesn't have a matching condition to allow port forwarding rules.
    
    Added a unit test to verify the detectRulesConflict function of FirewallManagerImpl.
    
    * pr/851:
      CLOUDSTACK-8864: Not able to add TCP port forwarding rule in VPN for specific ports
    
    Signed-off-by: Remi Bergsma <gi...@remi.nl>

commit a5a5f612ea4fbdb37b6dc5c708fd042b00902f84
Author: Remi Bergsma <gi...@remi.nl>
Date:   2015-09-24T13:35:33Z

    Merge pull request #867 from jayapalu/CLOUDSTACK-8891
    
    CLOUDSTACK-8891: Fixed default iptables rules on VR  for guest trafficVR default iptables rules in INPUT chain are configured partially.
    In CsAddress.py rules are configured while configuring public interface, guest interface post configuration is missed. Fixed to configure guest post configuration so that iptables rules are configured.
    
    Testing:
    1. Deployed vm in the network.
    2.iptables rules on the VR configured correctly.
    3.VM got the dhcp ip address from the VR.
    
    * pr/867:
      CLOUDSTACK-8891: Fixed default iptables rules on VR  for guest traffic
    
    Signed-off-by: Remi Bergsma <gi...@remi.nl>

commit 4018d47ef8ea3be780f27d6558275f19b70e5ef0
Author: Remi Bergsma <gi...@remi.nl>
Date:   2015-09-24T13:35:48Z

    Merge pull request #870 from jayapalu/CLOUDSTACK-8874
    
    Configured dnsmasq to listen on all interfaces so that vpn  client gets dns1. Dnsmasq is not listening on the ppp+ interfaces due to this remote access vpn clients dns requests are  dropped.
    
    2. Configured the dnsmasq to listen on all the interfaces except public. There is firewall to allow only specific cidr to allow the dns requests.
    
    Tested from windows client nslookup.
    
    * pr/870:
      Configured dnsmasq to listen on all interfaces so that vpn  client gets dns
    
    Signed-off-by: Remi Bergsma <gi...@remi.nl>

commit 649a4bdc7633298ceba39d30857d147f17952a84
Author: Remi Bergsma <gi...@remi.nl>
Date:   2015-09-24T13:36:03Z

    Merge pull request #881 from jayapalu/CLOUDSTACK-8905
    
    CLOUDSTACK-8905: Fixed hooking egress rulesAdded hooking the FIREWALL_EGRESS_RULES chain into FW_OUTBOUND chain.
    With this egress rules will effective.
    
    * pr/881:
      CLOUDSTACK-8905: Fixed hooking egress rules
    
    Signed-off-by: Remi Bergsma <gi...@remi.nl>

commit 4420f48e3e0378c440806fad0a2dcebaaa17b511
Author: Remi Bergsma <gi...@remi.nl>
Date:   2015-09-24T13:36:18Z

    Merge pull request #882 from jayapalu/CLOUDSTACK-8881
    
    CLOUDSTACK-8881: Fixed Static and PF configuration issue1. For static nat filter rules are not configured in VR.
    2. Corrected vm ip in PF rule.
    
    * pr/882:
      CLOUDSTACK-8881: Fixed Static and PF configuration issue
    
    Signed-off-by: Remi Bergsma <gi...@remi.nl>

commit 4c8f4ac3417f60962abfc2cb0f1439bb78a44d4d
Author: Remi Bergsma <gi...@remi.nl>
Date:   2015-09-24T14:42:41Z

    Merge pull request #842 from jayapalu/shareNwVR
    
    CLOUDSTACK-8843: Fixed issue in default iptables rules on shared network VROn basic zone share network VR default iptables rules are not applied correctly. Due to this ssh to VR got failed.
    In shared network the VR type is 'dhcpsrvr' not router. So corrected it in the ''del_standard' method to select the correct type.
    
    Testing:
    1. VR is deployed correctly.
    2. Tested restart, stop, start VR.
    3. New VM deployment is success.
    4. ssh to VR from the host is successful.
    5. iptables rules on the VR came up correctly.
    below is the output from the VR:
    iptables -L INPUT -nv
    Chain INPUT (policy DROP 16 packets, 1056 bytes)
     pkts bytes target     prot opt in     out     source               destination
        0     0 ACCEPT     all  --  *      *       0.0.0.0/0            224.0.0.18
        0     0 ACCEPT     all  --  *      *       0.0.0.0/0            225.0.0.50
      104  9800 ACCEPT     all  --  eth0   *       0.0.0.0/0            0.0.0.0/0            state RELATED,ESTABLISHED
      281 36500 ACCEPT     all  --  eth1   *       0.0.0.0/0            0.0.0.0/0            state RELATED,ESTABLISHED
        0     0 ACCEPT     all  --  eth2   *       0.0.0.0/0            0.0.0.0/0            state RELATED,ESTABLISHED
        6   504 ACCEPT     icmp --  *      *       0.0.0.0/0            0.0.0.0/0
        0     0 ACCEPT     all  --  lo     *       0.0.0.0/0            0.0.0.0/0
        2   656 ACCEPT     udp  --  eth0   *       0.0.0.0/0            0.0.0.0/0            udp dpt:67
       13   780 ACCEPT     tcp  --  eth1   *       0.0.0.0/0            0.0.0.0/0            tcp dpt:3922 state NEW,ESTABLISHED
        0     0 ACCEPT     tcp  --  eth0   *       0.0.0.0/0            0.0.0.0/0            tcp dpt:80 state NEW
        0     0 ACCEPT     tcp  --  eth0   *       10.147.40.0/23       0.0.0.0/0            state NEW tcp dpt:8080
    
    * pr/842:
      CLOUDSTACK-8843: Fixed issue in default iptables rules on shared network VR
    
    Signed-off-by: Remi Bergsma <gi...@remi.nl>

----


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40450520
  
    --- Diff: server/src/com/cloud/network/firewall/FirewallManagerImpl.java ---
    @@ -426,7 +426,8 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
                     // we allow port forwarding rules with the same parameters but different protocols
                     boolean allowPf =
                         (rule.getPurpose() == Purpose.PortForwarding && newRule.getPurpose() == Purpose.PortForwarding && !newRule.getProtocol().equalsIgnoreCase(
    -                        rule.getProtocol()));
    +                        rule.getProtocol())) || (rule.getPurpose() == Purpose.Vpn && newRule.getPurpose() == Purpose.PortForwarding && !newRule.getProtocol().equalsIgnoreCase(
    +                            rule.getProtocol()));
    --- End diff --
    
    Shall we file an Jira issue for it to be improved later?


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40449738
  
    --- Diff: server/test/com/cloud/network/firewall/FirewallManagerTest.java ---
    @@ -151,4 +182,38 @@ public void testApplyFWRules() {
             }
         }
     
    +    @Test
    +    public void testDetectRulesConflict() {
    +        List<FirewallRuleVO> ruleList = new ArrayList<FirewallRuleVO>();
    +        FirewallRuleVO rule1 = spy(new FirewallRuleVO("rule1", 3, 500, "UDP", 1, 2, 1, Purpose.Vpn, null, null, null, null));
    +        FirewallRuleVO rule2 = spy(new FirewallRuleVO("rule2", 3, 1701, "UDP", 1, 2, 1, Purpose.Vpn, null, null, null, null));
    +        FirewallRuleVO rule3 = spy(new FirewallRuleVO("rule3", 3, 4500, "UDP", 1, 2, 1, Purpose.Vpn, null, null, null, null));
    +
    +        ruleList.add(rule1);
    +        ruleList.add(rule2);
    +        ruleList.add(rule3);
    +
    +        FirewallManagerImpl firewallMgr = (FirewallManagerImpl)_firewallMgr;
    +
    +        when(firewallMgr._firewallDao.listByIpAndPurposeAndNotRevoked(3,null)).thenReturn(ruleList);
    +        when(rule1.getId()).thenReturn(1L);
    +        when(rule2.getId()).thenReturn(2L);
    +        when(rule3.getId()).thenReturn(3L);
    +
    +        FirewallRule newRule1 = new FirewallRuleVO("newRule1", 3, 500, "TCP", 1, 2, 1, Purpose.PortForwarding, null, null, null, null);
    +        FirewallRule newRule2 = new FirewallRuleVO("newRule2", 3, 1701, "TCP", 1, 2, 1, Purpose.PortForwarding, null, null, null, null);
    +        FirewallRule newRule3 = new FirewallRuleVO("newRule3", 3, 4500, "TCP", 1, 2, 1, Purpose.PortForwarding, null, null, null, null);
    +
    +        try {
    +            firewallMgr.detectRulesConflict(newRule1);
    +            firewallMgr.detectRulesConflict(newRule2);
    +            firewallMgr.detectRulesConflict(newRule3);
    +        }
    +        catch (NetworkRuleConflictException ex) {
    +            Assert.fail();
    --- End diff --
    
    Although I did not write it, I would say that if the exception happens, and was not expected, the test should fail, as the code does it. Another way to do this would be to expect the exception and check if the exception thrown is the one expected, then the test would pass, given the expectation.
    
    Given the code above, it's okay to fail it. It's like the person who wrote is saying: if an exception happens, then fail it.


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40450962
  
    --- Diff: systemvm/patches/debian/config/opt/cloud/bin/vr_cfg.sh ---
    @@ -93,4 +93,15 @@ done < $cfg
     #remove the configuration file, log file should have all the records as well
     rm -f $cfg
     
    +# Flush kernel conntrack table
    +log_it "VR config: Flushing conntrack table"
    +conntrackd -d 2> /dev/null
    +if [ $? -eq 0 ]; then
    +    conntrackd -F
    +    conntrackd -k
    +else
    +   conntrackd -F
    --- End diff --
    
    Nice catch, will fix.


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40449936
  
    --- Diff: server/test/com/cloud/network/firewall/FirewallManagerTest.java ---
    @@ -71,6 +79,7 @@
     //        super.setUp();
     //    }
     
    +    @Ignore("Requires database to be set up")
    --- End diff --
    
    Because it says: "Requires database to be set up"
    
    So I'm assuming that the person who wrote it either did not know how to mock the DB layer or the ACS is just to messed up to a point where mocking DB stuff is almost impossible.


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:

    https://github.com/apache/cloudstack/pull/887#issuecomment-143545153
  
    Hi @remibergsma, @karuturi and @borisroman ,
    
    Finally got most of the ACS working as it was 4 weeks ago! Below the tests I executed manually and via Marvin. It all looks fine!
    
    However, there is still 1 thing that nobody noticed - because they don't test - which concerns the Redundant VPC routers: we cannot ssh into VM in a rVPC, unfortunately!
    
    @isoutham wrote a nice test for that (component/test_vpc_redundnat.py) which was working really fine. Unfortunately I tried it now and also some manual tests, but the rVPC no longer works.
    
    I will create an issue for that and try to get it fixed.
    
    I would like to suggest that for any router related changes the following tests are *always* executed:
    
    * component/test_vpc_redundant.py  - hardware=true
      * Feature currently broken.
    * component/test_routers_iptables_default_policy.py  - hardware=true
    * component/test_vpc_router_nics.py - hardware=true
    * smoke/test_network_acl.py - hardware=true
    * component/test_vpc_offerings.py - hardware=false
    * component/test_vpc_routers.py - hardware=false
    * smoke/test_routers.py - hardware=false
    * smoke/test_privategw_acl.py - hardware=false
    * smoke/test_reset_vm_on_reboot.py - hardware=false
    * smoke/test_vm_life_cycle.py - hardware=false
    * smoke/test_vpc_vpn.py - hardware=false
    * smoke/test_service_offerings.py - hardware=false
    
    The PR 887 LATM - Look AWESOME to Me!
    
    Cheers,
    Wilder 
    
    P.S.: I already executed all tests below, but I will continue for a bit more and will edit the PR comment later. Please, go ahead and merge it!
    
    * Environment
      * KVM on CentOS 7.1
      * Management Server on CentOS 7.1
      * Agent and Common packages built from source 
    
    *Manual tests*
    
    - Isolated network / VM / egress / FW / PF
    ````
    [root@cs1 integration]# ssh root@192.168.23.5
    The authenticity of host '192.168.23.5 (192.168.23.5)' can't be established.
    ECDSA key fingerprint is 52:b5:5e:21:78:d8:89:fc:95:1c:68:02:55:01:44:b7.
    Are you sure you want to continue connecting (yes/no)? yes
    Warning: Permanently added '192.168.23.5' (ECDSA) to the list of known hosts.
    root@192.168.23.5's password: 
    # ping 8.8.8.8
    PING 8.8.8.8 (8.8.8.8): 56 data bytes
    64 bytes from 8.8.8.8: seq=0 ttl=47 time=10.619 ms
    64 bytes from 8.8.8.8: seq=1 ttl=47 time=11.346 ms
    64 bytes from 8.8.8.8: seq=2 ttl=47 time=11.934 ms
    ^C
    --- 8.8.8.8 ping statistics ---
    3 packets transmitted, 3 packets received, 0% packet loss
    round-trip min/avg/max = 10.619/11.299/11.934 ms
    # 
    
    ```
    - VPC network / Tier / ACL / VM / PubIP / PF
    [wrodrigues@mct-wrodrigues-g9 ~]$ ssh root@192.168.23.6
    The authenticity of host '192.168.23.6 (192.168.23.6)' can't be established.
    ECDSA key fingerprint is bf:bb:32:27:b4:91:38:38:3e:dd:8f:e3:ad:9b:9f:26.
    Are you sure you want to continue connecting (yes/no)? yes
    Warning: Permanently added '192.168.23.6' (ECDSA) to the list of known hosts.
    root@192.168.23.6's password: 
    # ls -lart
    total 9
    -rw-r--r--    1 root     root            78 Dec  1  2014 .bash_profile
    -rw-r--r--    1 root     root           175 Dec  1  2014 .bash_logout
    -rw-r--r--    1 root     root             0 Dec  1  2014 .bash_history
    drwxr-xr-x   19 root     root          1024 Sep 27 00:06 ..
    -rw-------    1 root     root             9 Sep 27 00:10 .ash_history
    drwx------    2 root     root          1024 Sep 27 00:10 .
    # ping 8.8.8.8
    PING 8.8.8.8 (8.8.8.8): 56 data bytes
    64 bytes from 8.8.8.8: seq=0 ttl=47 time=10.768 ms
    64 bytes from 8.8.8.8: seq=1 ttl=47 time=11.036 ms
    64 bytes from 8.8.8.8: seq=2 ttl=47 time=13.196 ms
    ^C
    --- 8.8.8.8 ping statistics ---
    3 packets transmitted, 3 packets received, 0% packet loss
    round-trip min/avg/max = 10.768/11.666/13.196 ms
    
    "Marvin tests*
    
    ```
    Test router internal advanced zone ... === TestName: test_02_router_internal_adv | Status : SUCCESS ===
    ok
    Test restart network ... === TestName: test_03_restart_network_cleanup | Status : SUCCESS ===
    ok
    Test router basic setup ... === TestName: test_05_router_basic | Status : SUCCESS ===
    ok
    Test router advanced setup ... === TestName: test_06_router_advanced | Status : SUCCESS ===
    ok
    Test stop router ... === TestName: test_07_stop_router | Status : SUCCESS ===
    ok
    Test start router ... === TestName: test_08_start_router | Status : SUCCESS ===
    ok
    Test reboot router ... === TestName: test_09_reboot_router | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 7 tests in 615.524s
    
    OK
    /tmp//MarvinLogs/test_routers_PXXUZO/results.txt (END)
    
    
    Test start/stop of router after addition of one guest network ... === TestName: test_01_start_stop_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test reboot of router after addition of one guest network ... === TestName: test_02_reboot_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test to change service offering of router after addition of one guest network ... === TestName: test_04_chg_srv_off_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test destroy of router after addition of one guest network ... === TestName: test_05_destroy_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test to stop and start router after creation of VPC ... === TestName: test_01_stop_start_router_after_creating_vpc | Status : SUCCESS ===
    ok
    Test to reboot the router after creating a VPC ... === TestName: test_02_reboot_router_after_creating_vpc | Status : SUCCESS ===
    ok
    Tests to change service offering of the Router after ... === TestName: test_04_change_service_offerring_vpc | Status : SUCCESS ===
    ok
    Test to destroy the router after creating a VPC ... === TestName: test_05_destroy_router_after_creating_vpc | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 8 tests in 1474.059s
    
    OK
    /tmp//MarvinLogs/test_vpc_routers_QZKIAB/results.txt (END)
    
    
    Test VPN in VPC ... === TestName: test_vpc_remote_access_vpn | Status : SUCCESS ===
    ok
    Test VPN in VPC ... === TestName: test_vpc_site2site_vpn | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 2 tests in 936.917s
    
    OK
    /tmp//MarvinLogs/test_vpc_vpn_MHQ125/results.txt (END)
    
    
    VPN remote access user limit tests ... === TestName: test_01_VPN_user_limit | Status : SUCCESS ===
    ok
    Test create VPN when L2TP port in use ... === TestName: test_02_use_vpn_port | Status : SUCCESS ===
    ok
    Test create NAT rule when VPN when L2TP enabled ... === TestName: test_03_enable_vpn_use_port | Status : SUCCESS ===
    ok
    Test add new users to existing VPN ... === TestName: test_04_add_new_users | Status : SUCCESS ===
    ok
    Test add duplicate user to existing VPN ... === TestName: test_05_add_duplicate_user | Status : SUCCESS ===
    ok
    Test as global admin, add a new VPN user to an existing VPN entry ... === TestName: test_06_add_VPN_user_global_admin | Status : SUCCESS ===
    ok
    Test as domain admin, add a new VPN user to an existing VPN entry ... === TestName: test_07_add_VPN_user_domain_admin | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 7 tests in 2370.893s
    
    OK
    /tmp//MarvinLogs/test_vpn_users_BPZS7G/results.txt (END)
    
    
    test_privategw_acl (integration.smoke.test_privategw_acl.TestPrivateGwACL) ... === TestName: test_privategw_acl | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 1 test in 276.807s
    
    OK
    /tmp//MarvinLogs/test_privategw_acl_E1SFE8/results.txt (END)
    
    
    Test network ACL lists and items in VPC ... === TestName: test_network_acl | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 1 test in 347.838s
    
    OK
    /tmp//MarvinLogs/test_network_acl_GRU2PJ/results.txt (END)
    
    
    Create a vpc with two networks with two vms in each network ... === TestName: test_01_VPC_nics_after_destroy | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 1 test in 1246.437s
    
    OK
    /tmp//MarvinLogs/test_vpc_router_nics_W2BCYT/results.txt (END)
    
    
    Test to create service offering ... === TestName: test_01_create_service_offering | Status : SUCCESS ===
    ok
    Test to update existing service offering ... === TestName: test_02_edit_service_offering | Status : SUCCESS ===
    ok
    Test to delete service offering ... === TestName: test_03_delete_service_offering | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 3 tests in 463.478s
    
    OK
    /tmp//MarvinLogs/test_service_offerings_PYXAFK/results.txt (END)
    
    
    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
    
    ----------------------------------------------------------------------
    Ran 2 tests in 755.749s
    
    OK
    /tmp//MarvinLogs/test_routers_iptables_default_policy_PG0N56/results.txt (END)
    
    
    ```


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:

    https://github.com/apache/cloudstack/pull/887#issuecomment-143409077
  
    Update: we found a bug introduced in PR #784 (that we merge here) that needs resolving. Working on that with @wilderrodrigues as we speak so don't merge yet.


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40445955
  
    --- Diff: server/test/com/cloud/network/firewall/FirewallManagerTest.java ---
    @@ -100,9 +109,30 @@ public void testInjected() {
     
         }
     
    -    @Inject
    -    FirewallManager _firewallMgr;
    +    @Mock
    +    AccountManager _accountMgr;
    +    @Mock
    +    NetworkOrchestrationService _networkMgr;
    +    @Mock
    +    NetworkModel _networkModel;
    +    @Mock
    +    DomainManager _domainMgr;
    +    @Mock
    +    VpcManager _vpcMgr;
    +    @Mock
    +    IpAddressManager _ipAddrMgr;
    +    @Mock
    +    FirewallRulesDao _firewallDao;
    +
    +    @InjectMocks
    +    FirewallManager _firewallMgr = new FirewallManagerImpl();
    +
    +    @Before
    +    public void initMocks() {
    +        MockitoAnnotations.initMocks(this);
    +    }
     
    +    @Ignore("Requires database to be set up")
    --- End diff --
    
    Why have a test ignored?


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40450393
  
    --- Diff: server/test/com/cloud/network/firewall/FirewallManagerTest.java ---
    @@ -52,9 +60,9 @@
     import com.cloud.network.rules.FirewallRuleVO;
     import com.cloud.utils.component.ComponentContext;
     
    -@Ignore("Requires database to be set up")
    -@RunWith(SpringJUnit4ClassRunner.class)
    -@ContextConfiguration(locations = "classpath:/testContext.xml")
    +//@Ignore("Requires database to be set up")
    +@RunWith(MockitoJUnitRunner.class)
    +//@ContextConfiguration(locations = "classpath:/testContext.xml")
    --- End diff --
    
    I'll get rid of them in a moment, didn't like it either.


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

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


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40445748
  
    --- Diff: server/test/com/cloud/network/firewall/FirewallManagerTest.java ---
    @@ -151,4 +182,38 @@ public void testApplyFWRules() {
             }
         }
     
    +    @Test
    +    public void testDetectRulesConflict() {
    +        List<FirewallRuleVO> ruleList = new ArrayList<FirewallRuleVO>();
    +        FirewallRuleVO rule1 = spy(new FirewallRuleVO("rule1", 3, 500, "UDP", 1, 2, 1, Purpose.Vpn, null, null, null, null));
    +        FirewallRuleVO rule2 = spy(new FirewallRuleVO("rule2", 3, 1701, "UDP", 1, 2, 1, Purpose.Vpn, null, null, null, null));
    +        FirewallRuleVO rule3 = spy(new FirewallRuleVO("rule3", 3, 4500, "UDP", 1, 2, 1, Purpose.Vpn, null, null, null, null));
    +
    +        ruleList.add(rule1);
    +        ruleList.add(rule2);
    +        ruleList.add(rule3);
    +
    +        FirewallManagerImpl firewallMgr = (FirewallManagerImpl)_firewallMgr;
    +
    +        when(firewallMgr._firewallDao.listByIpAndPurposeAndNotRevoked(3,null)).thenReturn(ruleList);
    +        when(rule1.getId()).thenReturn(1L);
    +        when(rule2.getId()).thenReturn(2L);
    +        when(rule3.getId()).thenReturn(3L);
    +
    +        FirewallRule newRule1 = new FirewallRuleVO("newRule1", 3, 500, "TCP", 1, 2, 1, Purpose.PortForwarding, null, null, null, null);
    +        FirewallRule newRule2 = new FirewallRuleVO("newRule2", 3, 1701, "TCP", 1, 2, 1, Purpose.PortForwarding, null, null, null, null);
    +        FirewallRule newRule3 = new FirewallRuleVO("newRule3", 3, 4500, "TCP", 1, 2, 1, Purpose.PortForwarding, null, null, null, null);
    +
    +        try {
    +            firewallMgr.detectRulesConflict(newRule1);
    +            firewallMgr.detectRulesConflict(newRule2);
    +            firewallMgr.detectRulesConflict(newRule3);
    +        }
    +        catch (NetworkRuleConflictException ex) {
    +            Assert.fail();
    --- End diff --
    
    It's better to not have the try catch structure in a test. If a test throws an exceptions it will automatically be handled as 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:

    https://github.com/apache/cloudstack/pull/887#issuecomment-143524179
  
    @wilderrodrigues and I spent yesterday testing and fixing small issues. I just (force) pushed the commits we that we tested against over night.
    
    The following automated tests were run to verify the final change:
    
    ```
    smoke/test_routers.py
    
    Test router internal advanced zone ... === TestName: test_02_router_internal_adv | Status : SUCCESS ===
    ok
    Test restart network ... === TestName: test_03_restart_network_cleanup | Status : SUCCESS ===
    ok
    Test router basic setup ... === TestName: test_05_router_basic | Status : SUCCESS ===
    ok
    Test router advanced setup ... === TestName: test_06_router_advanced | Status : SUCCESS ===
    ok
    Test stop router ... === TestName: test_07_stop_router | Status : SUCCESS ===
    ok
    Test start router ... === TestName: test_08_start_router | Status : SUCCESS ===
    ok
    Test reboot router ... === TestName: test_09_reboot_router | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 7 tests in 1281.041s
    
    OK
    
    component/test_vpc_routers.py
    
    Test start/stop of router after addition of one guest network ... === TestName: test_01_start_stop_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test reboot of router after addition of one guest network ... === TestName: test_02_reboot_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test to change service offering of router after addition of one guest network ... === TestName: test_04_chg_srv_off_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test destroy of router after addition of one guest network ... === TestName: test_05_destroy_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test to stop and start router after creation of VPC ... === TestName: test_01_stop_start_router_after_creating_vpc | Status : SUCCESS ===
    ok
    Test to reboot the router after creating a VPC ... === TestName: test_02_reboot_router_after_creating_vpc | Status : SUCCESS ===
    ok
    Tests to change service offering of the Router after ... === TestName: test_04_change_service_offerring_vpc | Status : SUCCESS ===
    ok
    Test to destroy the router after creating a VPC ... === TestName: test_05_destroy_router_after_creating_vpc | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 8 tests in 3037.949s
    
    OK
    ```
    
    Next to that, we setup an environment where we created two VPCs, each with one tier and one VM in it. Then created a site2site VPN between them. Both VMs can now reach each other over their internal ip addresses (source code link mentioned above). This was done on both KVM and XenServer. 
    
    Note: if you test on KVM, please build agent rpm packages from this brancha nd install them prior to adding the KVM hypervisor. Hint: `cd packaging` and then `./package.sh -d {dist}`
    
    While Travis and Apache Jenkins do another build, we'll run some extra Marvin tests.
    
    Pinging @wilderrodrigues @borisroman to do their final reviews. Thanks!
    
    Goal is to merge it today.


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40445904
  
    --- Diff: server/test/com/cloud/network/firewall/FirewallManagerTest.java ---
    @@ -71,6 +79,7 @@
     //        super.setUp();
     //    }
     
    +    @Ignore("Requires database to be set up")
    --- End diff --
    
    Why have a test ignored?


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40450212
  
    --- Diff: server/test/com/cloud/network/firewall/FirewallManagerTest.java ---
    @@ -151,4 +182,38 @@ public void testApplyFWRules() {
             }
         }
     
    +    @Test
    +    public void testDetectRulesConflict() {
    +        List<FirewallRuleVO> ruleList = new ArrayList<FirewallRuleVO>();
    +        FirewallRuleVO rule1 = spy(new FirewallRuleVO("rule1", 3, 500, "UDP", 1, 2, 1, Purpose.Vpn, null, null, null, null));
    +        FirewallRuleVO rule2 = spy(new FirewallRuleVO("rule2", 3, 1701, "UDP", 1, 2, 1, Purpose.Vpn, null, null, null, null));
    +        FirewallRuleVO rule3 = spy(new FirewallRuleVO("rule3", 3, 4500, "UDP", 1, 2, 1, Purpose.Vpn, null, null, null, null));
    +
    +        ruleList.add(rule1);
    +        ruleList.add(rule2);
    +        ruleList.add(rule3);
    +
    +        FirewallManagerImpl firewallMgr = (FirewallManagerImpl)_firewallMgr;
    +
    +        when(firewallMgr._firewallDao.listByIpAndPurposeAndNotRevoked(3,null)).thenReturn(ruleList);
    +        when(rule1.getId()).thenReturn(1L);
    +        when(rule2.getId()).thenReturn(2L);
    +        when(rule3.getId()).thenReturn(3L);
    +
    +        FirewallRule newRule1 = new FirewallRuleVO("newRule1", 3, 500, "TCP", 1, 2, 1, Purpose.PortForwarding, null, null, null, null);
    +        FirewallRule newRule2 = new FirewallRuleVO("newRule2", 3, 1701, "TCP", 1, 2, 1, Purpose.PortForwarding, null, null, null, null);
    +        FirewallRule newRule3 = new FirewallRuleVO("newRule3", 3, 4500, "TCP", 1, 2, 1, Purpose.PortForwarding, null, null, null, null);
    +
    +        try {
    +            firewallMgr.detectRulesConflict(newRule1);
    +            firewallMgr.detectRulesConflict(newRule2);
    +            firewallMgr.detectRulesConflict(newRule3);
    +        }
    +        catch (NetworkRuleConflictException ex) {
    +            Assert.fail();
    --- End diff --
    
    It is syntactically correct. Though this way you remove the error message from the test failure. It just fails because it hits a fail(). If the exception is thrown without the try catch then if the test fails it will show why.


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40455108
  
    --- Diff: server/test/com/cloud/network/firewall/FirewallManagerTest.java ---
    @@ -71,6 +79,7 @@
     //        super.setUp();
     //    }
     
    +    @Ignore("Requires database to be set up")
    --- End diff --
    
    @wilderrodrigues @borisroman I cannot judge this. If it has no value, we can leave it out. Your call.


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40445206
  
    --- Diff: systemvm/patches/debian/config/opt/cloud/bin/vr_cfg.sh ---
    @@ -93,4 +93,15 @@ done < $cfg
     #remove the configuration file, log file should have all the records as well
     rm -f $cfg
     
    +# Flush kernel conntrack table
    +log_it "VR config: Flushing conntrack table"
    +conntrackd -d 2> /dev/null
    +if [ $? -eq 0 ]; then
    +    conntrackd -F
    +    conntrackd -k
    +else
    +   conntrackd -F
    --- End diff --
    
    Small indention 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40445960
  
    --- Diff: server/test/com/cloud/network/firewall/FirewallManagerTest.java ---
    @@ -123,6 +153,7 @@ public void testApplyRules() {
             }
         }
     
    +    @Ignore("Requires database to be set up")
    --- End diff --
    
    Why have a test ignored?


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:

    https://github.com/apache/cloudstack/pull/887#issuecomment-143543557
  
    Thanks for all the debugging and testing @remibergsma and @wilderrodrigues 
    looking forward to see a stable master once again.. 


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:

    https://github.com/apache/cloudstack/pull/887#issuecomment-143237173
  
    @wilderrodrigues and me fixed the default route on VPCs as well. Now doing final testing


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:

    https://github.com/apache/cloudstack/pull/887#issuecomment-143353564
  
    ping @remibergsma @karuturi @borisroman @miguelaferreira
    
    I executed a few Marvin tests but some manual tests. The changes are working fine and will fix quite a few issues.
    
    @remibergsma also executed his CloudMonkey VPN tests, which can be found here: https://github.com/schubergphilis/MCT-shared/tree/master/helper_scripts/cloudstack/vpn_tests
    
    All went fine and you can simply execute them as well!
    
    :+1:  LGTM
    
    Manual tests:
    
    * Create VM and isolated network
    * Add FW and PF rules
    * Try to ping 8.8.8.8 before opening egress:
       - Doens't workt
    * Open egress and it works!
    
    [root@cs1 integration]# ssh root@192.168.23.6
    The authenticity of host '192.168.23.6 (192.168.23.6)' can't be established.
    ECDSA key fingerprint is 44:1f:60:67:51:e5:c0:e1:65:6d:5d:dd:1f:eb:b0:3a.
    Are you sure you want to continue connecting (yes/no)? yes
    Warning: Permanently added '192.168.23.6' (ECDSA) to the list of known hosts.
    root@192.168.23.6's password: 
    # ls /
    bin         dev         home        lib64       lost+found  mnt         proc        run         sys         usr
    boot        etc         lib         linuxrc     media       opt         root        sbin        tmp         var
    # ping 8.8.8.8
    PING 8.8.8.8 (8.8.8.8): 56 data bytes
    ^C
    --- 8.8.8.8 ping statistics ---
    4 packets transmitted, 0 packets received, 100% packet loss
    # ping 8.8.8.8
    PING 8.8.8.8 (8.8.8.8): 56 data bytes
    64 bytes from 8.8.8.8: seq=0 ttl=47 time=10.100 ms
    64 bytes from 8.8.8.8: seq=1 ttl=47 time=9.648 ms
    64 bytes from 8.8.8.8: seq=2 ttl=47 time=9.155 ms
    ^C
    --- 8.8.8.8 ping statistics ---
    3 packets transmitted, 3 packets received, 0% packet loss
    round-trip min/avg/max = 9.155/9.634/10.100 ms
    # 
    
    * Create VPC
    * Add Tier, VM and public IP
    * Add PF rule
    * Try to ping 8.8.8.8 and it works!
    
    [root@cs1 integration]# ssh root@192.168.23.4
    The authenticity of host '192.168.23.4 (192.168.23.4)' can't be established.
    ECDSA key fingerprint is 6d:0d:71:3a:43:00:16:4a:0b:ee:2b:3e:4c:dc:d9:f9.
    Are you sure you want to continue connecting (yes/no)? yes
    Warning: Permanently added '192.168.23.4' (ECDSA) to the list of known hosts.
    root@192.168.23.4's password: 
    # ping 8.8.8.8
    PING 8.8.8.8 (8.8.8.8): 56 data bytes
    64 bytes from 8.8.8.8: seq=0 ttl=47 time=9.137 ms
    64 bytes from 8.8.8.8: seq=1 ttl=47 time=10.937 ms
    64 bytes from 8.8.8.8: seq=2 ttl=47 time=9.618 ms
    ^C
    --- 8.8.8.8 ping statistics ---
    3 packets transmitted, 3 packets received, 0% packet loss
    round-trip min/avg/max = 9.137/9.897/10.937 ms
    # 
    
    Automated tests executed:
    
    test_vpc_routers
    test_vpc_offerings
    test_vm_life_cycle
    test_vpc_vpn
    test_vpn_users
    test_routers_iptables_default_policy
    
    Test start/stop of router after addition of one guest network ... === TestName: test_01_start_stop_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test reboot of router after addition of one guest network ... === TestName: test_02_reboot_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test to change service offering of router after addition of one guest network ... === TestName: test_04_chg_srv_off_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test destroy of router after addition of one guest network ... === TestName: test_05_destroy_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test to stop and start router after creation of VPC ... === TestName: test_01_stop_start_router_after_creating_vpc | Status : SUCCESS ===
    ok
    Test to reboot the router after creating a VPC ... === TestName: test_02_reboot_router_after_creating_vpc | Status : SUCCESS ===
    ok
    Tests to change service offering of the Router after ... === TestName: test_04_change_service_offerring_vpc | Status : SUCCESS ===
    ok
    Test to destroy the router after creating a VPC ... === TestName: test_05_destroy_router_after_creating_vpc | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 8 tests in 1107.930s
    
    OK
    /tmp//MarvinLogs/test_vpc_routers_2UGRJR/results.txt (END)
    
    
    
    Test create VPC offering ... === TestName: test_01_create_vpc_offering | Status : SUCCESS ===
    ok
    Test VPC offering without load balancing service ... === TestName: test_03_vpc_off_without_lb | Status : SUCCESS ===
    ok
    Test VPC offering without static NAT service ... === TestName: test_04_vpc_off_without_static_nat | Status : SUCCESS ===
    ok
    Test VPC offering without port forwarding service ... === TestName: test_05_vpc_off_without_pf | Status : SUCCESS ===
    ok
    Test VPC offering with invalid services ... === TestName: test_06_vpc_off_invalid_services | Status : SUCCESS ===
    ok
    Test update VPC offering ... === TestName: test_07_update_vpc_off | Status : SUCCESS ===
    ok
    Test list VPC offering ... === TestName: test_08_list_vpc_off | Status : SUCCESS ===
    ok
    test_09_create_redundant_vpc_offering (integration.component.test_vpc_offerings.TestVPCOffering) ... === TestName: test_09_create_redundant_vpc_offering | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 8 tests in 1486.109s
    
    OK
    /tmp//MarvinLogs/test_vpc_offerings_68E5MJ/results.txt (END)
    
    
    
    Test advanced zone virtual router ... === TestName: test_advZoneVirtualRouter | Status : SUCCESS ===
    ok
    Test Deploy Virtual Machine ... === TestName: test_deploy_vm | Status : SUCCESS ===
    ok
    Test Multiple Deploy Virtual Machine ... === TestName: test_deploy_vm_multiple | Status : SUCCESS ===
    ok
    Test Stop Virtual Machine ... === TestName: test_01_stop_vm | Status : SUCCESS ===
    ok
    Test Start Virtual Machine ... === TestName: test_02_start_vm | Status : SUCCESS ===
    ok
    Test Reboot Virtual Machine ... === TestName: test_03_reboot_vm | Status : SUCCESS ===
    ok
    Test destroy Virtual Machine ... === TestName: test_06_destroy_vm | Status : SUCCESS ===
    ok
    Test recover Virtual Machine ... === TestName: test_07_restore_vm | Status : SUCCESS ===
    ok
    Test migrate VM ... SKIP: At least two hosts should be present in the zone for migration
    Test destroy(expunge) Virtual Machine ... === TestName: test_09_expunge_vm | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 10 tests in 1636.117s
    
    OK (SKIP=1)
    /tmp//MarvinLogs/test_vm_life_cycle_TK4597/results.txt (END)
    
    
    
    Test VPN in VPC ... === TestName: test_vpc_remote_access_vpn | Status : SUCCESS ===
    ok
    Test VPN in VPC ... === TestName: test_vpc_site2site_vpn | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 2 tests in 846.637s
    
    OK
    /tmp//MarvinLogs/test_vpc_vpn_73XROS/results.txt (END)
    
    
    
    VPN remote access user limit tests ... === TestName: test_01_VPN_user_limit | Status : SUCCESS ===
    ok
    Test create VPN when L2TP port in use ... === TestName: test_02_use_vpn_port | Status : SUCCESS ===
    ok
    Test create NAT rule when VPN when L2TP enabled ... === TestName: test_03_enable_vpn_use_port | Status : SUCCESS ===
    ok
    Test add new users to existing VPN ... === TestName: test_04_add_new_users | Status : SUCCESS ===
    ok
    Test add duplicate user to existing VPN ... === TestName: test_05_add_duplicate_user | Status : SUCCESS ===
    ok
    Test as global admin, add a new VPN user to an existing VPN entry ... === TestName: test_06_add_VPN_user_global_admin | Status : SUCCESS ===
    ok
    Test as domain admin, add a new VPN user to an existing VPN entry ... === TestName: test_07_add_VPN_user_domain_admin | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 7 tests in 2295.400s
    
    OK
    /tmp//MarvinLogs/test_vpn_users_QBYU77/results.txt (END)
    
    
    
    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
    
    ----------------------------------------------------------------------
    Ran 2 tests in 908.229s
    
    OK
    /tmp//MarvinLogs/test_routers_iptables_default_policy_KSMXG6/results.txt (END)


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40446163
  
    --- Diff: server/src/com/cloud/network/firewall/FirewallManagerImpl.java ---
    @@ -426,7 +426,8 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
                     // we allow port forwarding rules with the same parameters but different protocols
                     boolean allowPf =
                         (rule.getPurpose() == Purpose.PortForwarding && newRule.getPurpose() == Purpose.PortForwarding && !newRule.getProtocol().equalsIgnoreCase(
    -                        rule.getProtocol()));
    +                        rule.getProtocol())) || (rule.getPurpose() == Purpose.Vpn && newRule.getPurpose() == Purpose.PortForwarding && !newRule.getProtocol().equalsIgnoreCase(
    +                            rule.getProtocol()));
    --- End diff --
    
    Don't you think this gets a little hard to read and understand? Maybe reformat or split in understandable pieces.


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40454206
  
    --- Diff: server/test/com/cloud/network/firewall/FirewallManagerTest.java ---
    @@ -71,6 +79,7 @@
     //        super.setUp();
     //    }
     
    +    @Ignore("Requires database to be set up")
    --- End diff --
    
    True! Though, should that be a valid reason to push them to 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40451212
  
    --- Diff: server/test/com/cloud/network/firewall/FirewallManagerTest.java ---
    @@ -52,9 +60,9 @@
     import com.cloud.network.rules.FirewallRuleVO;
     import com.cloud.utils.component.ComponentContext;
     
    -@Ignore("Requires database to be set up")
    -@RunWith(SpringJUnit4ClassRunner.class)
    -@ContextConfiguration(locations = "classpath:/testContext.xml")
    +//@Ignore("Requires database to be set up")
    +@RunWith(MockitoJUnitRunner.class)
    +//@ContextConfiguration(locations = "classpath:/testContext.xml")
    --- End diff --
    
    When you look at the file, it's full of commented lines and blocks.. So doesn't really help in removing only these two :-s


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:

    https://github.com/apache/cloudstack/pull/887#issuecomment-143543608
  
    Yes, @rajan! It will be stable today!
    
    I'm running the last test as we speak and will add the complete test report on the PR, so everybody can see what we have worked on and what's now fixed and back on track!
    
    Please, stay around for the next 20min so you can have a look and give your LATM - Looks Awesome to Me - 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by runseb <gi...@git.apache.org>.
Github user runseb commented on the pull request:

    https://github.com/apache/cloudstack/pull/887#issuecomment-143140589
  
    @jayapalu @SudharmaJain can you check this asap, as it merges all your past PRs in one and rebases with master. thanks


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:

    https://github.com/apache/cloudstack/pull/887#issuecomment-143409187
  
    This PR also fixes CLOUDSTACK-8878 (https://issues.apache.org/jira/browse/CLOUDSTACK-8878): No route to host on VPC router after stop/start/reboot


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40454164
  
    --- Diff: server/src/com/cloud/network/firewall/FirewallManagerImpl.java ---
    @@ -426,7 +426,8 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
                     // we allow port forwarding rules with the same parameters but different protocols
                     boolean allowPf =
                         (rule.getPurpose() == Purpose.PortForwarding && newRule.getPurpose() == Purpose.PortForwarding && !newRule.getProtocol().equalsIgnoreCase(
    -                        rule.getProtocol()));
    +                        rule.getProtocol())) || (rule.getPurpose() == Purpose.Vpn && newRule.getPurpose() == Purpose.PortForwarding && !newRule.getProtocol().equalsIgnoreCase(
    +                            rule.getProtocol()));
    --- End diff --
    
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40445876
  
    --- Diff: server/test/com/cloud/network/firewall/FirewallManagerTest.java ---
    @@ -52,9 +60,9 @@
     import com.cloud.network.rules.FirewallRuleVO;
     import com.cloud.utils.component.ComponentContext;
     
    -@Ignore("Requires database to be set up")
    -@RunWith(SpringJUnit4ClassRunner.class)
    -@ContextConfiguration(locations = "classpath:/testContext.xml")
    +//@Ignore("Requires database to be set up")
    +@RunWith(MockitoJUnitRunner.class)
    +//@ContextConfiguration(locations = "classpath:/testContext.xml")
    --- End diff --
    
    Why have these commented 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40450011
  
    --- Diff: server/src/com/cloud/network/firewall/FirewallManagerImpl.java ---
    @@ -426,7 +426,8 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
                     // we allow port forwarding rules with the same parameters but different protocols
                     boolean allowPf =
                         (rule.getPurpose() == Purpose.PortForwarding && newRule.getPurpose() == Purpose.PortForwarding && !newRule.getProtocol().equalsIgnoreCase(
    -                        rule.getProtocol()));
    +                        rule.getProtocol())) || (rule.getPurpose() == Purpose.Vpn && newRule.getPurpose() == Purpose.PortForwarding && !newRule.getProtocol().equalsIgnoreCase(
    +                            rule.getProtocol()));
    --- End diff --
    
    I didn't do that or looked int it. We isolated our 35 hours of effort to get master up and running again. So, this can be fixed in a separate 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on the pull request:

    https://github.com/apache/cloudstack/pull/887#issuecomment-143545134
  
    Hi @wilderrodrigues @remibergsma,
    
    I've tested:
    component/test_vpc_routers.py
    component/test_vpc_offerings.py
    smoke/test_routers.py
    smoke/test_reset_vm_on_reboot.py
    smoke/test_vm_life_cycle.py
    smoke/test_vpc_vpn.py
    
    All returning success.
    
    Based on which I will :+1:  LGTM for this PR. Hopefully we will resolve the other bugs in the near future.



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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40449856
  
    --- Diff: server/test/com/cloud/network/firewall/FirewallManagerTest.java ---
    @@ -52,9 +60,9 @@
     import com.cloud.network.rules.FirewallRuleVO;
     import com.cloud.utils.component.ComponentContext;
     
    -@Ignore("Requires database to be set up")
    -@RunWith(SpringJUnit4ClassRunner.class)
    -@ContextConfiguration(locations = "classpath:/testContext.xml")
    +//@Ignore("Requires database to be set up")
    +@RunWith(MockitoJUnitRunner.class)
    +//@ContextConfiguration(locations = "classpath:/testContext.xml")
    --- End diff --
    
    I don't know. As @remibergsma mentioned, we just got the thing running with 7 PRs.
    
    It's very good to have you looking into it, @borisroman . I really mean it! :) It means that when those PRs were pushed by the original authors nobody looked at them at all!
    
    Thanks!


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

[GitHub] cloudstack pull request: [BLOCKER] Combined PRs that fix VR issues

Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/887#discussion_r40454232
  
    --- Diff: server/test/com/cloud/network/firewall/FirewallManagerTest.java ---
    @@ -52,9 +60,9 @@
     import com.cloud.network.rules.FirewallRuleVO;
     import com.cloud.utils.component.ComponentContext;
     
    -@Ignore("Requires database to be set up")
    -@RunWith(SpringJUnit4ClassRunner.class)
    -@ContextConfiguration(locations = "classpath:/testContext.xml")
    +//@Ignore("Requires database to be set up")
    +@RunWith(MockitoJUnitRunner.class)
    +//@ContextConfiguration(locations = "classpath:/testContext.xml")
    --- End diff --
    
    Maybe a second jira issue?


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