You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by wilderrodrigues <gi...@git.apache.org> on 2015/12/05 19:36:24 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
GitHub user wilderrodrigues opened a pull request:
https://github.com/apache/cloudstack/pull/1179
CLOUDSTACK-9106 - As a Developer I want the Redundant VPC private gateway feature fixed
This PR fixes the Private Gateway feature when using Redundant VPCs.
In order to get it to work, I had to refactor some of the Java code in order to reduce the number of iterations we had with the routers list. It caused an issue when trying to configure ACL rules in a router (the backup one) that did not have the interface setup yet.
The rVPC Pvt GW integration test is not being skipped anymore and is 100% green!
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ekholabs/cloudstack fix/rvpc-pvtgw-CLOUDSTACK-9106
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1179.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 #1179
----
commit ae325f601605c2dec285db621660bacdc2959d4b
Author: Wilder Rodrigues <wr...@schubergphilis.com>
Date: 2015-12-04T17:41:03Z
CLOUDSTACK-9106 - Reduces the amount of iterations through the routers of a VPC
- It was causing problems because Nics were expected to be plugged before they actually exist. Only in rVPC cases.
- Applies ACL items to routers only after the Pvt GW is setup.
commit 78e15d1cfa27937935f1269c9e9b6ec5a91db08f
Author: Wilder Rodrigues <wr...@schubergphilis.com>
Date: 2015-12-05T13:03:50Z
CLOUDSTACK-9106 - Refactor the createPrivateNicProfileForGateway() method
- Use the router to retrieve the instance ID
- Check if the VPC is redundant in order to reuse the private gateway address.
- Brings the private gateways interfaces up.
commit 6dab3613e475aac524e448f0d52fe575fbdf5e23
Author: Wilder Rodrigues <wr...@schubergphilis.com>
Date: 2015-12-05T16:33:55Z
CLOUDSTACK-9106 - Enables private gateway tests on Redundant VPCs
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1179#issuecomment-162287415
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 ===
ok
Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_3_Load_Balancer_Rule | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 34 tests in 15999.825s
OK
```
And:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a tags=advanced,required_hardware=false \
smoke/test_routers.py \
smoke/test_network_acl.py \
smoke/test_privategw_acl.py \
smoke/test_reset_vm_on_reboot.py \
smoke/test_vm_life_cycle.py \
smoke/test_vpc_vpn.py \
smoke/test_service_offerings.py \
component/test_vpc_offerings.py \
component/test_vpc_routers.py
```
Result:
```
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
Test reset virtual machine on reboot ... === TestName: test_01_reset_vm_on_reboot | Status : SUCCESS ===
ok
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 ... === TestName: test_08_migrate_vm | Status : SUCCESS ===
ok
Test destroy(expunge) Virtual Machine ... === TestName: test_09_expunge_vm | Status : SUCCESS ===
ok
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
Test for delete account ... === TestName: test_delete_account | Status : SUCCESS ===
ok
Test for Associate/Disassociate public IP address for admin account ... === TestName: test_public_ip_admin_account | Status : SUCCESS ===
ok
Test for Associate/Disassociate public IP address for user account ... === TestName: test_public_ip_user_account | Status : SUCCESS ===
ok
Test for release public IP address ... === TestName: test_releaseIP | Status : SUCCESS ===
ok
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
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 41 tests in 7804.791s
OK
```
Will also run the test you mentioned above!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46761916
--- Diff: plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java ---
@@ -468,19 +470,23 @@ public boolean applyStaticNats(final Network network, final List<? extends Stati
return true;
}
- DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
- NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
-
- return networkTopology.applyStaticNats(network, rules, routers);
+ final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
+ final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
+ boolean result = false;
+ for (final DomainRouterVO domainRouterVO : routers) {
+ result = networkTopology.applyStaticNats(network, rules, domainRouterVO);
--- End diff --
Good point. Thought about that when I was coding, but not on the Ovs one. If you look, I did the same on other places. To be honest, I just forgot to change. When I changed first time and thought about that, I just gave it a run because I was trying to narrow down the problem. 6 hours later, when I got it working, I forgot to change.
I can change that tomorrow... os later tonight. Or we can get this one through and fix it on Monday, since the PR/Feature will be already in. I just want to get through the feature release.
Spent the whole day behind the mac. If I do 30min tomorrow, wife will kill me. :D
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1179#issuecomment-162243159
Wilder can this one be on 4.6 please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46761996
--- Diff: server/src/com/cloud/network/element/VpcVirtualRouterElement.java ---
@@ -479,48 +482,47 @@ public boolean applyIps(final Network network, final List<? extends PublicIpAddr
break;
}
}
+ boolean result = false;
if (canHandle) {
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
if (routers == null || routers.isEmpty()) {
s_logger.debug(getName() + " element doesn't need to associate ip addresses on the backend; VPC virtual " + "router doesn't exist in the network "
+ network.getId());
- return true;
+ return result;
}
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
- return networkTopology.associatePublicIP(network, ipAddress, routers);
- } else {
- return false;
+ for (final DomainRouterVO domainRouterVO : routers) {
+ result = networkTopology.associatePublicIP(network, ipAddress, domainRouterVO);
+ }
}
+ return result;
}
@Override
public boolean applyNetworkACLs(final Network network, final List<? extends NetworkACLItem> rules) throws ResourceUnavailableException {
+ boolean result = true;
if (canHandle(network, Service.NetworkACL)) {
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
if (routers == null || routers.isEmpty()) {
s_logger.debug("Virtual router elemnt doesn't need to apply firewall rules on the backend; virtual " + "router doesn't exist in the network " + network.getId());
- return true;
+ return result;
}
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
- try {
- if (!networkTopology.applyNetworkACLs(network, rules, routers, false)) {
- return false;
- } else {
- return true;
+ for (final DomainRouterVO domainRouterVO : routers) {
+ try {
+ result = networkTopology.applyNetworkACLs(network, rules, domainRouterVO, false);
--- End diff --
result &=
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1179#issuecomment-162324443
@remibergsma I think I am being captain obvious (as my new colleagues like to call each other) but let's add them to the standard run.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46769789
--- Diff: plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java ---
@@ -488,50 +494,54 @@ public boolean applyPFRules(final Network network, final List<PortForwardingRule
return true;
}
- DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
- NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
-
- return networkTopology.applyFirewallRules(network, rules, routers);
+ final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
+ final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
+ for (final DomainRouterVO domainRouterVO : routers) {
+ result = networkTopology.applyFirewallRules(network, rules, domainRouterVO);
--- End diff --
Yep... I know. That's what I said when you first mentioned it. I will change it now, but we have to revisit many places in the Java side as well, and not only the classes I changed, but the VpcManagerImpl.java, for instance.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46761808
--- Diff: plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java ---
@@ -445,13 +446,14 @@ public boolean applyIps(final Network network,
return true;
}
- DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
- NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
+ final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
+ final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
- return networkTopology.associatePublicIP(network, ipAddress, routers);
- } else {
- return false;
+ for (final DomainRouterVO domainRouterVO : routers) {
+ result = networkTopology.associatePublicIP(network, ipAddress, domainRouterVO);
--- End diff --
will we forget all intermediate results and only return the last one?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46765484
--- Diff: server/src/com/cloud/network/element/VirtualRouterElement.java ---
@@ -751,13 +757,15 @@ public boolean savePassword(final Network network, final NicProfile nic, final V
@Override
public boolean saveSSHKey(final Network network, final NicProfile nic, final VirtualMachineProfile vm, final String sshPublicKey) throws ResourceUnavailableException {
+ boolean result = false;
if (!canHandle(network, null)) {
- return false;
+ return result;
}
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
if (routers == null || routers.isEmpty()) {
s_logger.debug("Can't find virtual router element in network " + network.getId());
- return true;
+ result = true;
--- End diff --
Because I do not want to return a literal. And the same for the other comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46761947
--- Diff: server/src/com/cloud/network/element/VirtualRouterElement.java ---
@@ -656,20 +660,22 @@ public static String getHAProxyStickinessCapability() {
@Override
public boolean applyStaticNats(final Network network, final List<? extends StaticNat> rules) throws ResourceUnavailableException {
+ boolean result = true;
if (canHandle(network, Service.StaticNat)) {
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
if (routers == null || routers.isEmpty()) {
s_logger.debug("Virtual router elemnt doesn't need to apply static nat on the backend; virtual " + "router doesn't exist in the network " + network.getId());
- return true;
+ return result;
}
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
- return networkTopology.applyStaticNats(network, rules, routers);
- } else {
- return true;
+ for (final DomainRouterVO domainRouterVO : routers) {
+ result = networkTopology.applyStaticNats(network, rules, domainRouterVO);
--- End diff --
&=
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46761965
--- Diff: server/src/com/cloud/network/element/VirtualRouterElement.java ---
@@ -765,18 +773,23 @@ public boolean saveSSHKey(final Network network, final NicProfile nic, final Vir
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
- return networkTopology.saveSSHPublicKeyToRouter(network, nic, uservm, routers, sshPublicKey);
+ for (final DomainRouterVO domainRouterVO : routers) {
+ result = networkTopology.saveSSHPublicKeyToRouter(network, nic, uservm, domainRouterVO, sshPublicKey);
--- End diff --
&=
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46761989
--- Diff: server/src/com/cloud/network/element/VpcVirtualRouterElement.java ---
@@ -479,48 +482,47 @@ public boolean applyIps(final Network network, final List<? extends PublicIpAddr
break;
}
}
+ boolean result = false;
if (canHandle) {
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
if (routers == null || routers.isEmpty()) {
s_logger.debug(getName() + " element doesn't need to associate ip addresses on the backend; VPC virtual " + "router doesn't exist in the network "
+ network.getId());
- return true;
+ return result;
}
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
- return networkTopology.associatePublicIP(network, ipAddress, routers);
- } else {
- return false;
+ for (final DomainRouterVO domainRouterVO : routers) {
+ result = networkTopology.associatePublicIP(network, ipAddress, domainRouterVO);
--- End diff --
result &= ...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1179#issuecomment-162342338
Run this test: `nosetests --with-marvin --marvin-config=${marvinCfg} -s -a tags=advanced,required_hardware=true smoke/test_privategw_acl.py`
Result:
```
test_01_vpc_privategw_acl (integration.smoke.test_privategw_acl.TestPrivateGwACL) ... === TestName: test_01_vpc_privategw_acl | Status : SUCCESS ===
ok
test_02_vpc_privategw_static_routes (integration.smoke.test_privategw_acl.TestPrivateGwACL) ... === TestName: test_02_vpc_privategw_static_routes | Status : SUCCESS ===
ok
test_03_rvpc_privategw_static_routes (integration.smoke.test_privategw_acl.TestPrivateGwACL) ... === TestName: test_03_rvpc_privategw_static_routes | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 3 tests in 2057.520s
OK
```
Nice work @wilderrodrigues !
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1179#issuecomment-162460207
@remibergsma @DaanHoogland
I'm closing this PR and will create a new one against 4.6.
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: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46761973
--- Diff: server/src/com/cloud/network/element/VirtualRouterElement.java ---
@@ -841,24 +857,26 @@ public VirtualRouterProvider addElement(final Long nspId, final Type providerTyp
@Override
public boolean applyPFRules(final Network network, final List<PortForwardingRule> rules) throws ResourceUnavailableException {
+ boolean result = false;
if (canHandle(network, Service.PortForwarding)) {
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
if (routers == null || routers.isEmpty()) {
s_logger.debug("Virtual router elemnt doesn't need to apply firewall rules on the backend; virtual " + "router doesn't exist in the network " + network.getId());
- return true;
+ result = true;
--- End diff --
return true; would work wouldn't it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46771839
--- Diff: server/src/com/cloud/network/element/VirtualRouterElement.java ---
@@ -841,24 +857,26 @@ public VirtualRouterProvider addElement(final Long nspId, final Type providerTyp
@Override
public boolean applyPFRules(final Network network, final List<PortForwardingRule> rules) throws ResourceUnavailableException {
+ boolean result = false;
if (canHandle(network, Service.PortForwarding)) {
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
if (routers == null || routers.isEmpty()) {
s_logger.debug("Virtual router elemnt doesn't need to apply firewall rules on the backend; virtual " + "router doesn't exist in the network " + network.getId());
- return true;
+ result = true;
--- End diff --
I agree with your consideration and I am not forcing you to change to win my lgtm. I just want to make sure we discuss the consideration here for posterity and change only if a simple good alternative comes up.
I noticed line 874 and wondered about this and the &= thingy upstairs. I would use a combination of first trying all of them and then throw the exception if any of them failed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1179#issuecomment-162237710
I like the scrum style title but just to be nitpicking: isn't this more of a network engineer feature instead of a developer tool?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1179#issuecomment-162262512
More tests... On the same environment, but with hardware TRUE.
```
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 ===
ok
Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_3_Load_Balancer_Rule | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 31 tests in 13956.667s
OK
(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: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46761835
--- Diff: plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java ---
@@ -468,19 +470,23 @@ public boolean applyStaticNats(final Network network, final List<? extends Stati
return true;
}
- DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
- NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
-
- return networkTopology.applyStaticNats(network, rules, routers);
+ final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
+ final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
+ boolean result = false;
+ for (final DomainRouterVO domainRouterVO : routers) {
+ result = networkTopology.applyStaticNats(network, rules, domainRouterVO);
--- End diff --
how about &= ? (goes for the comment above 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46761967
--- Diff: server/src/com/cloud/network/element/VirtualRouterElement.java ---
@@ -751,13 +757,15 @@ public boolean savePassword(final Network network, final NicProfile nic, final V
@Override
public boolean saveSSHKey(final Network network, final NicProfile nic, final VirtualMachineProfile vm, final String sshPublicKey) throws ResourceUnavailableException {
+ boolean result = false;
if (!canHandle(network, null)) {
- return false;
+ return result;
}
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
if (routers == null || routers.isEmpty()) {
s_logger.debug("Can't find virtual router element in network " + network.getId());
- return true;
+ result = true;
--- End diff --
huh? why not return true;?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46761920
--- Diff: server/src/com/cloud/network/element/VirtualRouterElement.java ---
@@ -509,10 +512,11 @@ public boolean applyIps(final Network network, final List<? extends PublicIpAddr
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
- return networkTopology.associatePublicIP(network, ipAddress, routers);
- } else {
- return false;
+ for (final DomainRouterVO domainRouterVO : routers) {
+ result = networkTopology.associatePublicIP(network, ipAddress, domainRouterVO);
--- End diff --
&= these constructs only return the last result.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46769848
--- Diff: server/src/com/cloud/network/element/VirtualRouterElement.java ---
@@ -841,24 +857,26 @@ public VirtualRouterProvider addElement(final Long nspId, final Type providerTyp
@Override
public boolean applyPFRules(final Network network, final List<PortForwardingRule> rules) throws ResourceUnavailableException {
+ boolean result = false;
if (canHandle(network, Service.PortForwarding)) {
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
if (routers == null || routers.isEmpty()) {
s_logger.debug("Virtual router elemnt doesn't need to apply firewall rules on the backend; virtual " + "router doesn't exist in the network " + network.getId());
- return true;
+ result = true;
--- End diff --
Yes, it would. But I won't change because I don't agree with the approach of returning true/false in several places.
1. A method that returns something should have only one point where it actually returns. So those several returns is a bad practice. A way to make it not so bad is to assign the return to a variable, so people looking at the code in the future won't miss a hidden "return true" somewhere. I did not change all of it because in some places it requires a better refactor.
2. If a method execution cannot proceed due to some condition, it should fail - exception. A return false should be used when one wants to check is something is valid/exists or not.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46765516
--- Diff: plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java ---
@@ -488,50 +494,54 @@ public boolean applyPFRules(final Network network, final List<PortForwardingRule
return true;
}
- DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
- NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
-
- return networkTopology.applyFirewallRules(network, rules, routers);
+ final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
+ final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
+ for (final DomainRouterVO domainRouterVO : routers) {
+ result = networkTopology.applyFirewallRules(network, rules, domainRouterVO);
--- End diff --
I already said why I didn't do it and want to change the code. So, what's the point in going on and comment on every thing? We could just accept this and improve during the 4.7.0 release cycle.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1179#issuecomment-162233742
Ping @remibergsma @DaanHoogland @bhaisaab @karuturi @borisroman @miguelaferreira
Could you please review this PR? I will execute more tests now.
* Environment
* Hardware required: TRUE
* Management Server + MySQL on CentOS 7.1
* One KVM Host on CentOS 7.1
* Agent + Common RPMs built from source
* Test executed
```
nosetests --with-marvin --marvin-config=/data/shared/marvin/mct-zone2-kvm2-ISOLATED.cfg -s -a tags=advanced,required_hardware=true smoke/test_privategw_acl.py
```
* Results
```
test_01_vpc_privategw_acl (integration.smoke.test_privategw_acl.TestPrivateGwACL) ... === TestName: test_01_vpc_privategw_acl | Status : SUCCESS ===
ok
test_02_vpc_privategw_static_routes (integration.smoke.test_privategw_acl.TestPrivateGwACL) ... === TestName: test_02_vpc_privategw_static_routes | Status : SUCCESS ===
ok
test_03_rvpc_privategw_static_routes (integration.smoke.test_privategw_acl.TestPrivateGwACL) ... === TestName: test_03_rvpc_privategw_static_routes | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 3 tests in 2493.111s
OK
/tmp//MarvinLogs/test_privategw_acl_V6KWGZ/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: CLOUDSTACK-9106 - As a Developer I want t...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1179#issuecomment-162341294
@DaanHoogland Yes, sir! See linked PR above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46767881
--- Diff: plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java ---
@@ -488,50 +494,54 @@ public boolean applyPFRules(final Network network, final List<PortForwardingRule
return true;
}
- DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
- NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
-
- return networkTopology.applyFirewallRules(network, rules, routers);
+ final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
+ final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
+ for (final DomainRouterVO domainRouterVO : routers) {
+ result = networkTopology.applyFirewallRules(network, rules, domainRouterVO);
--- End diff --
Sorry, i did not notice. Where did you say that?
I saw some other places where you bail on the first fail. I don't like that solution:
If the first fails nothing is applied.
If the second fails the first router has the rules applied.
with &= it is symmetrical.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46761845
--- Diff: plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java ---
@@ -488,50 +494,54 @@ public boolean applyPFRules(final Network network, final List<PortForwardingRule
return true;
}
- DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
- NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
-
- return networkTopology.applyFirewallRules(network, rules, routers);
+ final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
+ final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
+ for (final DomainRouterVO domainRouterVO : routers) {
+ result = networkTopology.applyFirewallRules(network, rules, domainRouterVO);
--- End diff --
result &= ...?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1179#discussion_r46761941
--- Diff: plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java ---
@@ -468,19 +470,23 @@ public boolean applyStaticNats(final Network network, final List<? extends Stati
return true;
}
- DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
- NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
-
- return networkTopology.applyStaticNats(network, rules, routers);
+ final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
+ final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
+ boolean result = false;
+ for (final DomainRouterVO domainRouterVO : routers) {
+ result = networkTopology.applyStaticNats(network, rules, domainRouterVO);
--- End diff --
don't worry, this one is a fix and will go in. do it on monday
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9106 - As a Developer I want t...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1179#issuecomment-162237998
@DaanHoogland
Agree... ;) Sometimes I find difficult to wear different hats, although I could do it just fine. Should I change the title here and on the issue?
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: CLOUDSTACK-9106 - As a Developer I want t...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues closed the pull request at:
https://github.com/apache/cloudstack/pull/1179
---
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.
---