Merge pull request #1483 from remibergsma/pr1413-wilder-47

CLOUDSTACK-9287 - Fix unique mac address per rVPC routerThis is work by @wilderrodrigues, see PR #1413 It contains important fixes and I think it needs to be included so I send the PR again.

* pr/1483:
  CLOUDSTACK-9287 - Improve test by checking if pvt gw is removed and fix typos
  CLOUDSTACK-9287 - Fix RVR public interface
  CLOUDSTACK-9287 - Add integration test to cover the private gateway related changes
  CLOUDSTACK-9287 - Refactor the interface state configuration
  CLOUDSTACK-9287 - Check if the nic profile has already been removed from a certain router
  CLOUDSTACK-9287 - Bring up the private gw interface on state change to master
  CLOUDSTACK-9287 - Make sure private gw interface is not used for default gw
  CLOUDSTACK-9287 - Add integration test to cover the private gw interface/mac address issues
  CLOUDSTACK-9287 - Put private gateway interface down on backup router
  CLOUDSTACK-9287 - Generate new mac address if router is redundant and nic profile exists

Signed-off-by: Will Stevens <williamstevens@gmail.com>

server/src/com/cloud/network/element/VirtualRouterElement.java

@@ -24,18 +24,6 @@ import java.util.Set;
 
 import javax.inject.Inject;
 
-import org.apache.cloudstack.api.command.admin.router.ConfigureOvsElementCmd;
-import org.apache.cloudstack.api.command.admin.router.ConfigureVirtualRouterElementCmd;
-import org.apache.cloudstack.api.command.admin.router.CreateVirtualRouterElementCmd;
-import org.apache.cloudstack.api.command.admin.router.ListOvsElementsCmd;
-import org.apache.cloudstack.api.command.admin.router.ListVirtualRouterElementsCmd;
-import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
-import org.apache.cloudstack.network.topology.NetworkTopology;
-import org.apache.cloudstack.network.topology.NetworkTopologyContext;
-import org.apache.log4j.Logger;
-import org.cloud.network.router.deployment.RouterDeploymentDefinition;
-import org.cloud.network.router.deployment.RouterDeploymentDefinitionBuilder;
-
 import com.cloud.agent.api.to.LoadBalancerTO;
 import com.cloud.configuration.ConfigurationManager;
 import com.cloud.dc.DataCenter;
@@ -107,6 +95,18 @@ import com.cloud.vm.dao.DomainRouterDao;
 import com.cloud.vm.dao.UserVmDao;
 import com.google.gson.Gson;
 
+import org.apache.cloudstack.api.command.admin.router.ConfigureOvsElementCmd;
+import org.apache.cloudstack.api.command.admin.router.ConfigureVirtualRouterElementCmd;
+import org.apache.cloudstack.api.command.admin.router.CreateVirtualRouterElementCmd;
+import org.apache.cloudstack.api.command.admin.router.ListOvsElementsCmd;
+import org.apache.cloudstack.api.command.admin.router.ListVirtualRouterElementsCmd;
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.network.topology.NetworkTopology;
+import org.apache.cloudstack.network.topology.NetworkTopologyContext;
+import org.apache.log4j.Logger;
+import org.cloud.network.router.deployment.RouterDeploymentDefinition;
+import org.cloud.network.router.deployment.RouterDeploymentDefinitionBuilder;
+
 public class VirtualRouterElement extends AdapterBase implements VirtualRouterElementService, DhcpServiceProvider, UserDataServiceProvider, SourceNatServiceProvider,
 StaticNatServiceProvider, FirewallServiceProvider, LoadBalancingServiceProvider, PortForwardingServiceProvider, RemoteAccessVPNServiceProvider, IpDeployer,
 NetworkMigrationResponder, AggregatedCommandExecutor {
@@ -153,6 +153,8 @@ NetworkMigrationResponder, AggregatedCommandExecutor {
     IPAddressDao _ipAddressDao;
     @Inject
     DataCenterDao _dcDao;
+    @Inject
+    NetworkModel _networkModel;
 
     @Inject
     NetworkTopologyContext networkTopologyContext;

server/src/com/cloud/network/element/VpcVirtualRouterElement.java

@@ -25,13 +25,6 @@ import java.util.Set;
 
 import javax.inject.Inject;
 
-import org.apache.cloudstack.network.topology.NetworkTopology;
-import org.apache.log4j.Logger;
-import org.cloud.network.router.deployment.RouterDeploymentDefinition;
-import org.cloud.network.router.deployment.RouterDeploymentDefinitionBuilder;
-import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.beans.factory.annotation.Qualifier;
-
 import com.cloud.dc.DataCenter;
 import com.cloud.dc.DataCenterVO;
 import com.cloud.deploy.DeployDestination;
@@ -79,6 +72,13 @@ import com.cloud.vm.VirtualMachine;
 import com.cloud.vm.VirtualMachineManager;
 import com.cloud.vm.VirtualMachineProfile;
 
+import org.apache.cloudstack.network.topology.NetworkTopology;
+import org.apache.log4j.Logger;
+import org.cloud.network.router.deployment.RouterDeploymentDefinition;
+import org.cloud.network.router.deployment.RouterDeploymentDefinitionBuilder;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.beans.factory.annotation.Qualifier;
+
 public class VpcVirtualRouterElement extends VirtualRouterElement implements VpcProvider, Site2SiteVpnServiceProvider, NetworkACLServiceProvider {
 
     private static final Logger s_logger = Logger.getLogger(VpcVirtualRouterElement.class);
@@ -466,7 +466,7 @@ public class VpcVirtualRouterElement extends VirtualRouterElement implements Vpc
             }
         }
 
-        return result > 0 ? true : false;
+        return result == routers.size() ? true : false;
     }
 
     @Override
@@ -559,9 +559,16 @@ public class VpcVirtualRouterElement extends VirtualRouterElement implements Vpc
         final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
         final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
 
+        final Network privateNetwork = _networkModel.getNetwork(gateway.getNetworkId());
+
         boolean result = true;
         for (final DomainRouterVO domainRouterVO : routers) {
+            final NicProfile nicProfile = _networkModel.getNicProfile(domainRouterVO, privateNetwork.getId(), null);
+            if (nicProfile != null) {
                 result = result && networkTopology.applyNetworkACLs(network, rules, domainRouterVO, isPrivateGateway);
+            } else {
+                s_logger.warn("Nic Profile for router '" + domainRouterVO + "' has already been removed. Router is redundant = " + domainRouterVO.getIsRedundantRouter());
+            }
         }
         return result;
     }

server/src/com/cloud/network/router/CommandSetupHelper.java

@@ -58,6 +58,7 @@ import com.cloud.agent.api.to.FirewallRuleTO;
 import com.cloud.agent.api.to.IpAddressTO;
 import com.cloud.agent.api.to.LoadBalancerTO;
 import com.cloud.agent.api.to.NetworkACLTO;
+import com.cloud.agent.api.to.NicTO;
 import com.cloud.agent.api.to.PortForwardingRuleTO;
 import com.cloud.agent.api.to.StaticNatRuleTO;
 import com.cloud.agent.manager.Commands;
@@ -504,7 +505,8 @@ public class CommandSetupHelper {
             }
         }
 
-        final SetNetworkACLCommand cmd = new SetNetworkACLCommand(rulesTO, _networkHelper.getNicTO(router, guestNetworkId, null));
+        NicTO nicTO = _networkHelper.getNicTO(router, guestNetworkId, null);
+        final SetNetworkACLCommand cmd = new SetNetworkACLCommand(rulesTO, nicTO);
         cmd.setAccessDetail(NetworkElementCommand.ROUTER_IP, _routerControlHelper.getRouterControlIp(router.getId()));
         cmd.setAccessDetail(NetworkElementCommand.ROUTER_GUEST_IP, _routerControlHelper.getRouterIpInNetwork(guestNetworkId, router.getId()));
         cmd.setAccessDetail(NetworkElementCommand.GUEST_VLAN_TAG, guestVlan);

server/src/com/cloud/network/router/NicProfileHelperImpl.java

@@ -85,6 +85,11 @@ public class NicProfileHelperImpl implements NicProfileHelper {
                     new NicProfile(privateNic, privateNetwork, privateNic.getBroadcastUri(), privateNic.getIsolationUri(), _networkModel.getNetworkRate(
                             privateNetwork.getId(), router.getId()), _networkModel.isSecurityGroupSupportedInNetwork(privateNetwork), _networkModel.getNetworkTag(
                                     router.getHypervisorType(), privateNetwork));
+
+            if (router.getIsRedundantRouter()) {
+              String newMacAddress = NetUtils.long2Mac(NetUtils.createSequenceBasedMacAddress(ipVO.getMacAddress()));
+              privateNicProfile.setMacAddress(newMacAddress);
+            }
         } else {
             final String netmask = NetUtils.getCidrNetmask(privateNetwork.getCidr());
             final PrivateIpAddress ip =

server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java

@@ -26,9 +26,6 @@ import java.util.Map;
 import javax.inject.Inject;
 import javax.naming.ConfigurationException;
 
-import org.apache.log4j.Logger;
-import org.springframework.stereotype.Component;
-
 import com.cloud.agent.api.Answer;
 import com.cloud.agent.api.Command;
 import com.cloud.agent.api.Command.OnError;
@@ -91,6 +88,9 @@ import com.cloud.vm.VirtualMachineProfile;
 import com.cloud.vm.VirtualMachineProfile.Param;
 import com.cloud.vm.dao.VMInstanceDao;
 
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
 @Component
 public class VpcVirtualNetworkApplianceManagerImpl extends VirtualNetworkApplianceManagerImpl implements VpcVirtualNetworkApplianceManager {
     private static final Logger s_logger = Logger.getLogger(VpcVirtualNetworkApplianceManagerImpl.class);
@@ -531,16 +531,18 @@ public class VpcVirtualNetworkApplianceManagerImpl extends VirtualNetworkApplian
 
     @Override
     public boolean destroyPrivateGateway(final PrivateGateway gateway, final VirtualRouter router) throws ConcurrentOperationException, ResourceUnavailableException {
+        boolean result = true;
 
         if (!_networkModel.isVmPartOfNetwork(router.getId(), gateway.getNetworkId())) {
             s_logger.debug("Router doesn't have nic for gateway " + gateway + " so no need to removed it");
-            return true;
+            return result;
         }
 
         final Network privateNetwork = _networkModel.getNetwork(gateway.getNetworkId());
+        final NicProfile nicProfile = _networkModel.getNicProfile(router, privateNetwork.getId(), null);
 
         s_logger.debug("Releasing private ip for gateway " + gateway + " from " + router);
-        boolean result = setupVpcPrivateNetwork(router, false, _networkModel.getNicProfile(router, privateNetwork.getId(), null));
+        result = setupVpcPrivateNetwork(router, false, nicProfile);
         if (!result) {
             s_logger.warn("Failed to release private ip for gateway " + gateway + " on router " + router);
             return false;

server/src/com/cloud/network/vpc/NetworkACLManagerImpl.java

@@ -21,11 +21,6 @@ import java.util.List;
 
 import javax.inject.Inject;
 
-import org.apache.cloudstack.context.CallContext;
-import org.apache.cloudstack.framework.messagebus.MessageBus;
-import org.apache.cloudstack.framework.messagebus.PublishScope;
-import org.apache.log4j.Logger;
-
 import com.cloud.configuration.ConfigurationManager;
 import com.cloud.event.ActionEvent;
 import com.cloud.event.EventTypes;
@@ -52,6 +47,11 @@ import com.cloud.utils.db.TransactionCallback;
 import com.cloud.utils.db.TransactionStatus;
 import com.cloud.utils.exception.CloudRuntimeException;
 
+import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.framework.messagebus.MessageBus;
+import org.apache.cloudstack.framework.messagebus.PublishScope;
+import org.apache.log4j.Logger;
+
 public class NetworkACLManagerImpl extends ManagerBase implements NetworkACLManager {
     private static final Logger s_logger = Logger.getLogger(NetworkACLManagerImpl.class);
 
@@ -335,10 +335,10 @@ public class NetworkACLManagerImpl extends ManagerBase implements NetworkACLMana
 
     @Override
     public boolean revokeACLItemsForPrivateGw(final PrivateGateway gateway) throws ResourceUnavailableException {
-
+        final long networkACLId = gateway.getNetworkACLId();
-        final List<NetworkACLItemVO> aclItems = _networkACLItemDao.listByACL(gateway.getNetworkACLId());
+        final List<NetworkACLItemVO> aclItems = _networkACLItemDao.listByACL(networkACLId);
         if (aclItems.isEmpty()) {
-            s_logger.debug("Found no network ACL Items for private gateway  id=" + gateway.getId());
+            s_logger.debug("Found no network ACL Items for private gateway 'id=" + gateway.getId() + "'");
             return true;
         }
 

server/src/org/apache/cloudstack/network/topology/AdvancedNetworkTopology.java

@@ -19,11 +19,6 @@ package org.apache.cloudstack.network.topology;
 
 import java.util.List;
 
-import org.apache.log4j.Logger;
-import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.beans.factory.annotation.Qualifier;
-import org.springframework.stereotype.Component;
-
 import com.cloud.dc.DataCenter;
 import com.cloud.deploy.DeployDestination;
 import com.cloud.exception.ConcurrentOperationException;
@@ -52,6 +47,11 @@ import com.cloud.vm.NicProfile;
 import com.cloud.vm.VirtualMachine.State;
 import com.cloud.vm.VirtualMachineProfile;
 
+import org.apache.log4j.Logger;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.beans.factory.annotation.Qualifier;
+import org.springframework.stereotype.Component;
+
 @Component
 public class AdvancedNetworkTopology extends BasicNetworkTopology {
 
@@ -223,6 +223,7 @@ public class AdvancedNetworkTopology extends BasicNetworkTopology {
 
         final NetworkAclsRules aclsRules = new NetworkAclsRules(network, rules, isPrivateGateway);
 
-        return applyRules(network, router, typeString, isPodLevelException, podId, failWhenDisconnect, new RuleApplierWrapper<RuleApplier>(aclsRules));
+        final boolean result = applyRules(network, router, typeString, isPodLevelException, podId, failWhenDisconnect, new RuleApplierWrapper<RuleApplier>(aclsRules));
+        return result;
     }
 }

systemvm/patches/debian/config/opt/cloud/bin/configure.py

@@ -733,34 +733,34 @@ class CsForwardingRules(CsDataBag):
 
     #return the VR guest interface ip
     def getGuestIp(self):
-        ipr = []
+        interfaces = []
         ipAddr = None
-        for ip in self.config.address().get_ips():
+        for interface in self.config.address().get_interfaces():
-            if ip.is_guest():
+            if interface.is_guest():
-                ipr.append(ip)
+                interfaces.append(interface)
-            if len(ipr) > 0:
+            if len(interfaces) > 0:
-                ipAddr = sorted(ipr)[-1]
+                ipAddr = sorted(interfaces)[-1]
             if ipAddr:
                 return ipAddr.get_ip()
 
         return None
 
     def getDeviceByIp(self, ipa):
-        for ip in self.config.address().get_ips():
+        for interface in self.config.address().get_interfaces():
-            if ip.ip_in_subnet(ipa):
+            if interface.ip_in_subnet(ipa):
-                return ip.get_device()
+                return interface.get_device()
         return None
 
     def getNetworkByIp(self, ipa):
-        for ip in self.config.address().get_ips():
+        for interface in self.config.address().get_interfaces():
-            if ip.ip_in_subnet(ipa):
+            if interface.ip_in_subnet(ipa):
-                return ip.get_network()
+                return interface.get_network()
         return None
 
     def getGatewayByIp(self, ipa):
-        for ip in self.config.address().get_ips():
+        for interface in self.config.address().get_interfaces():
-            if ip.ip_in_subnet(ipa):
+            if interface.ip_in_subnet(ipa):
-                return ip.get_gateway()
+                return interface.get_gateway()
         return None
 
     def portsToString(self, ports, delimiter):

systemvm/patches/debian/config/opt/cloud/bin/cs/CsAddress.py

@@ -28,7 +28,6 @@ from CsRoute import CsRoute
 from CsRule import CsRule
 
 VRRP_TYPES = ['guest']
-PUBLIC_INTERFACE = ['eth1']
 
 class CsAddress(CsDataBag):
 
@@ -37,14 +36,14 @@ class CsAddress(CsDataBag):
             ip = CsIP(dev, self.config)
             ip.compare(self.dbag)
 
-    def get_ips(self):
+    def get_interfaces(self):
-        ret = []
+        interfaces = []
         for dev in self.dbag:
             if dev == "id":
                 continue
             for ip in self.dbag[dev]:
-                ret.append(CsInterface(ip, self.config))
+                interfaces.append(CsInterface(ip, self.config))
-        return ret
+        return interfaces
 
     def get_guest_if(self):
         """
@@ -52,13 +51,13 @@ class CsAddress(CsDataBag):
         """
         guest_interface = None
         lowest_device = 1000
-        for ip in self.get_ips():
+        for interface in self.get_interfaces():
-            if ip.is_guest() and ip.is_added():
+            if interface.is_guest() and interface.is_added():
-                device = ip.get_device()
+                device = interface.get_device()
                 device_suffix = int(''.join([digit for digit in device if digit.isdigit()]))
                 if device_suffix < lowest_device:
                     lowest_device = device_suffix
-                    guest_interface = ip
+                    guest_interface = interface
                     logging.debug("Guest interface will be set on device '%s' and IP '%s'" % (guest_interface.get_device(), guest_interface.get_ip()))
         return guest_interface
 
@@ -94,9 +93,9 @@ class CsAddress(CsDataBag):
         """
         Return the address object that has the control interface
         """
-        for ip in self.get_ips():
+        for interface in self.get_interfaces():
-            if ip.is_control():
+            if interface.is_control():
-                return ip
+                return interface
         return None
 
     def process(self):
@@ -117,6 +116,7 @@ class CsAddress(CsDataBag):
                 else:
                     logging.info(
                         "Address %s on device %s not configured", ip.ip(), dev)
+                    
                     if CsDevice(dev, self.config).waitfordevice():
                         ip.configure(address)
 
@@ -276,7 +276,7 @@ class CsIP:
             try:
                 logging.info("Configuring address %s on device %s", self.ip(), self.dev)
                 cmd = "ip addr add dev %s %s brd +" % (self.dev, self.ip())
-                subprocess.call(cmd, shell=True)
+                CsHelper.execute(cmd)
             except Exception as e:
                 logging.info("Exception occurred ==> %s" % e)
 
@@ -291,7 +291,10 @@ class CsIP:
             route.add_table(self.dev)
 
             CsRule(self.dev).addMark()
-            self.check_is_up()
+
+            interfaces = [CsInterface(address, self.config)]
+            CsHelper.reconfigure_interfaces(self.cl, interfaces)
+
             self.set_mark()
             self.arpPing()
 
@@ -306,7 +309,7 @@ class CsIP:
             # The code looks redundant here, but we actually have to cater for routers and
             # VPC routers in a different manner. Please do not remove this block otherwise
             # The VPC default route will be broken.
-            if self.get_type() in ["public"]:
+            if self.get_type() in ["public"] and address["device"] == CsHelper.PUBLIC_INTERFACES[self.cl.get_type()]:
                 gateway = str(address["gateway"])
                 route.add_defaultroute(gateway)
         else:
@@ -315,21 +318,6 @@ class CsIP:
             if(self.cl.get_gateway()):
                 route.add_defaultroute(self.cl.get_gateway())
 
-    def check_is_up(self):
-        """ Ensure device is up """
-        cmd = "ip link show %s | grep 'state DOWN'" % self.getDevice()
-        for i in CsHelper.execute(cmd):
-            if " DOWN " in i:
-                cmd2 = "ip link set %s up" % self.getDevice()
-                # If redundant only bring up public interfaces that are not eth1.
-                # Reason: private gateways are public interfaces.
-                # master.py and keepalived will deal with eth1 public interface.
-                if self.cl.is_redundant() and (not self.is_public() or self.getDevice() not in PUBLIC_INTERFACE):
-                    CsHelper.execute(cmd2)
-                # if not redundant bring everything up
-                if not self.cl.is_redundant():
-                    CsHelper.execute(cmd2)
-
     def set_mark(self):
         cmd = "-A PREROUTING -i %s -m state --state NEW -j CONNMARK --set-xmark %s/0xffffffff" % \
             (self.getDevice(), self.dnum)

systemvm/patches/debian/config/opt/cloud/bin/cs/CsHelper.py

@@ -27,6 +27,30 @@ import shutil
 from netaddr import *
 from pprint import pprint
 
+PUBLIC_INTERFACES = {"router" : "eth2", "vpcrouter" : "eth1"}
+
+STATE_COMMANDS = {"router" : "ip addr | grep eth0 | grep inet | wc -l | xargs bash -c  'if [ $0 == 2 ]; then echo \"MASTER\"; else echo \"BACKUP\"; fi'",
+                  "vpcrouter" : "ip addr | grep eth1 | grep state | awk '{print $9;}' | xargs bash -c 'if [ $0 == \"UP\" ]; then echo \"MASTER\"; else echo \"BACKUP\"; fi'"}
+
+def reconfigure_interfaces(router_config, interfaces):
+    for interface in interfaces:
+        cmd = "ip link show %s | grep 'state DOWN'" % interface.get_device()
+        for device in execute(cmd):
+            if " DOWN " in device:
+                cmd = "ip link set %s up" % interface.get_device()
+                # If redundant only bring up public interfaces that are not eth1.
+                # Reason: private gateways are public interfaces.
+                # master.py and keepalived will deal with eth1 public interface.
+
+                if router_config.is_redundant() and interface.is_public():
+                    state_cmd = STATE_COMMANDS[router_config.get_type()]
+                    logging.info("Check state command => %s" % state_cmd)
+                    state = execute(state_cmd)[0]
+                    logging.info("Route state => %s" % state)
+                    if interface.get_device() != PUBLIC_INTERFACES[router_config.get_type()] and state == "MASTER":
+                        execute(cmd)
+                else:
+                    execute(cmd)
 
 def is_mounted(name):
     for i in execute("mount"):

systemvm/patches/debian/config/opt/cloud/bin/cs/CsRedundant.py

@@ -41,7 +41,6 @@ from CsRoute import CsRoute
 import socket
 from time import sleep
 
-
 class CsRedundant(object):
 
     CS_RAMDISK_DIR = "/ramdisk"
@@ -88,7 +87,7 @@ class CsRedundant(object):
             self._redundant_off()
             return
 
-        interfaces = [interface for interface in self.address.get_ips() if interface.is_guest()]
+        interfaces = [interface for interface in self.address.get_interfaces() if interface.is_guest()]
         isDeviceReady = False
         dev = ''
         for interface in interfaces:
@@ -228,9 +227,9 @@ class CsRedundant(object):
         self.set_lock()
         logging.info("Router switched to fault mode")
 
-        ips = [ip for ip in self.address.get_ips() if ip.is_public()]
+        interfaces = [interface for interface in self.address.get_interfaces() if interface.is_public()]
-        for ip in ips:
+        for interface in interfaces:
-            CsHelper.execute("ifconfig %s down" % ip.get_device())
+            CsHelper.execute("ifconfig %s down" % interface.get_device())
 
         cmd = "%s -C %s" % (self.CONNTRACKD_BIN, self.CONNTRACKD_CONF)
         CsHelper.execute("%s -s" % cmd)
@@ -238,15 +237,18 @@ class CsRedundant(object):
         CsHelper.service("xl2tpd", "stop")
         CsHelper.service("dnsmasq", "stop")
 
-        ips = [ip for ip in self.address.get_ips() if ip.needs_vrrp()]
+        interfaces = [interface for interface in self.address.get_interfaces() if interface.needs_vrrp()]
-        for ip in ips:
+        for interface in interfaces:
-            CsPasswdSvc(ip.get_gateway()).stop()
+            CsPasswdSvc(interface.get_gateway()).stop()
 
         self.cl.set_fault_state()
         self.cl.save()
         self.release_lock()
         logging.info("Router switched to fault mode")
 
+        interfaces = [interface for interface in self.address.get_interfaces() if interface.is_public()]
+        CsHelper.reconfigure_interfaces(self.cl, interfaces)
+
     def set_backup(self):
         """ Set the current router to backup """
         if not self.cl.is_redundant():
@@ -257,28 +259,31 @@ class CsRedundant(object):
         logging.debug("Setting router to backup")
 
         dev = ''
-        ips = [ip for ip in self.address.get_ips() if ip.is_public()]
+        interfaces = [interface for interface in self.address.get_interfaces() if interface.is_public()]
-        for ip in ips:
+        for interface in interfaces:
-            if dev == ip.get_device():
+            if dev == interface.get_device():
                 continue
-            logging.info("Bringing public interface %s down" % ip.get_device())
+            logging.info("Bringing public interface %s down" % interface.get_device())
-            cmd2 = "ip link set %s down" % ip.get_device()
+            cmd2 = "ip link set %s down" % interface.get_device()
             CsHelper.execute(cmd2)
-            dev = ip.get_device()
+            dev = interface.get_device()
 
         cmd = "%s -C %s" % (self.CONNTRACKD_BIN, self.CONNTRACKD_CONF)
         CsHelper.execute("%s -d" % cmd)
         CsHelper.service("ipsec", "stop")
         CsHelper.service("xl2tpd", "stop")
 
-        ips = [ip for ip in self.address.get_ips() if ip.needs_vrrp()]
+        interfaces = [interface for interface in self.address.get_interfaces() if interface.needs_vrrp()]
-        for ip in ips:
+        for interface in interfaces:
-            CsPasswdSvc(ip.get_gateway()).stop()
+            CsPasswdSvc(interface.get_gateway()).stop()
         CsHelper.service("dnsmasq", "stop")
 
         self.cl.set_master_state(False)
         self.cl.save()
         self.release_lock()
+
+        interfaces = [interface for interface in self.address.get_interfaces() if interface.is_public()]
+        CsHelper.reconfigure_interfaces(self.cl, interfaces)
         logging.info("Router switched to backup mode")
 
     def set_master(self):
@@ -291,12 +296,12 @@ class CsRedundant(object):
         logging.debug("Setting router to master")
 
         dev = ''
-        ips = [ip for ip in self.address.get_ips() if ip.is_public()]
+        interfaces = [interface for interface in self.address.get_interfaces() if interface.is_public()]
         route = CsRoute()
-        for ip in ips:
+        for interface in interfaces:
-            if dev == ip.get_device():
+            if dev == interface.get_device():
                 continue
-            dev = ip.get_device()
+            dev = interface.get_device()
             logging.info("Will proceed configuring device ==> %s" % dev)
             cmd2 = "ip link set %s up" % dev
             if CsDevice(dev, self.config).waitfordevice():
@@ -304,8 +309,9 @@ class CsRedundant(object):
                 logging.info("Bringing public interface %s up" % dev)
 
                 try:
-                    gateway = ip.get_gateway()
+                    gateway = interface.get_gateway()
                     logging.info("Adding gateway ==> %s to device ==> %s" % (gateway, dev))
+                    if dev == CsHelper.PUBLIC_INTERFACES[self.cl.get_type()]:
                         route.add_defaultroute(gateway)
                 except:
                     logging.error("ERROR getting gateway from device %s" % dev)
@@ -320,14 +326,17 @@ class CsRedundant(object):
         CsHelper.execute("%s -B" % cmd)
         CsHelper.service("ipsec", "restart")
         CsHelper.service("xl2tpd", "restart")
-        ads = [o for o in self.address.get_ips() if o.needs_vrrp()]
+        interfaces = [interface for interface in self.address.get_interfaces() if interface.needs_vrrp()]
-        for o in ads:
+        for interface in interfaces:
-            CsPasswdSvc(o.get_gateway()).restart()
+            CsPasswdSvc(interface.get_gateway()).restart()
 
         CsHelper.service("dnsmasq", "restart")
         self.cl.set_master_state(True)
         self.cl.save()
         self.release_lock()
+
+        interfaces = [interface for interface in self.address.get_interfaces() if interface.is_public()]
+        CsHelper.reconfigure_interfaces(self.cl, interfaces)
         logging.info("Router switched to master mode")
 
     def _collect_ignore_ips(self):
@@ -353,23 +362,14 @@ class CsRedundant(object):
         that could function as a router and VPC router at the same time
         """
         lines = []
-        for ip in self.address.get_ips():
+        for interface in self.address.get_interfaces():
-            if ip.needs_vrrp():
+            if interface.needs_vrrp():
                 cmdline=self.config.get_cmdline_instance()
-                if not ip.is_added():
+                if not interface.is_added():
                     continue
                 if(cmdline.get_type()=='router'):
-                    str = "        %s brd %s dev %s\n" % (cmdline.get_guest_gw(), ip.get_broadcast(), ip.get_device())
+                    str = "        %s brd %s dev %s\n" % (cmdline.get_guest_gw(), interface.get_broadcast(), interface.get_device())
                 else:
-                    str = "        %s brd %s dev %s\n" % (ip.get_gateway_cidr(), ip.get_broadcast(), ip.get_device())
+                    str = "        %s brd %s dev %s\n" % (interface.get_gateway_cidr(), interface.get_broadcast(), interface.get_device())
                 lines.append(str)
         return lines
-
-    def check_is_up(self, device):
-        """ Ensure device is up """
-        cmd = "ip link show %s | grep 'state DOWN'" % device
-
-        for i in CsHelper.execute(cmd):
-            if " DOWN " in i:
-                cmd2 = "ip link set %s up" % device
-                CsHelper.execute(cmd2)

test/integration/smoke/test_privategw_acl.py

@@ -25,6 +25,7 @@ from marvin.lib.base import *
 from marvin.lib.common import *
 from nose.plugins.attrib import attr
 
+import time
 import logging
 
 class Services:
@@ -33,6 +34,13 @@ class Services:
 
     def __init__(self):
         self.services = {
+            "configurableData": {
+                "host": {
+                    "password": "password",
+                    "username": "root",
+                    "port": 22
+                }
+            },
             "account": {
                 "email": "test@test.com",
                 "firstname": "Test",
@@ -187,6 +195,7 @@ class TestPrivateGwACL(cloudstackTestCase):
 
     def setUp(self):
         self.apiclient = self.testClient.getApiClient()
+        self.hypervisor = self.testClient.getHypervisorInfo()
         
         self.logger.debug("Creating Admin Account for Domain ID ==> %s" % self.domain.id)
         self.account = Account.create(
@@ -262,7 +271,7 @@ class TestPrivateGwACL(cloudstackTestCase):
         self.logger.debug("Enabling the VPC offering created")
         vpc_off.update(self.apiclient, state='Enabled')
 
-        self.performVPCTests(vpc_off, True)
+        self.performVPCTests(vpc_off, restart_with_cleanup = True)
 
     @attr(tags=["advanced"], required_hardware="true")
     def test_04_rvpc_privategw_static_routes(self):
@@ -276,6 +285,18 @@ class TestPrivateGwACL(cloudstackTestCase):
 
         self.performVPCTests(vpc_off)
 
+    @attr(tags=["advanced"], required_hardware="true")
+    def _test_05_rvpc_privategw_check_interface(self):
+        self.logger.debug("Creating a Redundant VPC offering..")
+        vpc_off = VpcOffering.create(
+            self.apiclient,
+            self.services["redundant_vpc_offering"])
+
+        self.logger.debug("Enabling the Redundant VPC offering created")
+        vpc_off.update(self.apiclient, state='Enabled')
+
+        self.performPrivateGWInterfaceTests(vpc_off)
+
     def performVPCTests(self, vpc_off, restart_with_cleanup = False):
         self.logger.debug("Creating VPCs with  offering ID %s" % vpc_off.id)
         vpc_1 = self.createVPC(vpc_off, cidr = '10.0.1.0/24')
@@ -321,15 +342,117 @@ class TestPrivateGwACL(cloudstackTestCase):
         nat_rule_1 = self.create_natrule(vpc_1, vm1, public_ip_1, network_1)
         nat_rule_2 = self.create_natrule(vpc_2, vm2, public_ip_2, network_2)
 
-        self.check_pvt_gw_connectivity(vm1, public_ip_1, vm2.nic[0].ipaddress)
+        self.check_pvt_gw_connectivity(vm1, public_ip_1, [vm2.nic[0].ipaddress, vm1.nic[0].ipaddress])
-        self.check_pvt_gw_connectivity(vm2, public_ip_2, vm1.nic[0].ipaddress)
 
         if restart_with_cleanup:
-            self.reboot_vpc_with_cleanup(vpc_1, True)
+            self.reboot_vpc_with_cleanup(vpc_1, cleanup = restart_with_cleanup)
-            self.reboot_vpc_with_cleanup(vpc_2, True)
+            self.reboot_vpc_with_cleanup(vpc_2, cleanup = restart_with_cleanup)
 
-            self.check_pvt_gw_connectivity(vm1, public_ip_1, vm2.nic[0].ipaddress)
+            self.check_pvt_gw_connectivity(vm1, public_ip_1, [vm2.nic[0].ipaddress, vm1.nic[0].ipaddress])
-            self.check_pvt_gw_connectivity(vm2, public_ip_2, vm1.nic[0].ipaddress)
+
+    def performPrivateGWInterfaceTests(self, vpc_off):
+        self.logger.debug("Creating VPCs with  offering ID %s" % vpc_off.id)
+        vpc_1 = self.createVPC(vpc_off, cidr = '10.0.0.0/16')
+
+        self.cleanup = [vpc_1, vpc_off, self.account]
+
+        physical_networks = get_physical_networks(self.apiclient, self.zone.id)
+        if not physical_networks:
+            self.fail("No Physical Networks found!")
+
+        vlans = physical_networks[0].vlan.split('-')
+        vlan_1 = int(vlans[0])
+
+        net_offering_no_lb = "network_offering_no_lb"
+
+        network_1 = self.createNetwork(vpc_1, gateway = '10.0.0.1')
+        network_2 = self.createNetwork(vpc_1, net_offering = net_offering_no_lb, gateway = '10.0.1.1')
+        network_3 = self.createNetwork(vpc_1, net_offering = net_offering_no_lb, gateway = '10.0.2.1')
+        network_4 = self.createNetwork(vpc_1, net_offering = net_offering_no_lb, gateway = '10.0.3.1')
+
+        vm1 = self.createVM(network_1)
+        vm2 = self.createVM(network_2)
+        vm3 = self.createVM(network_3)
+        vm4 = self.createVM(network_4)
+
+        self.cleanup.insert(0, vm1)
+        self.cleanup.insert(0, vm2)
+        self.cleanup.insert(0, vm3)
+        self.cleanup.insert(0, vm4)
+
+        acl1 = self.createACL(vpc_1)
+        self.createACLItem(acl1.id, cidr = "0.0.0.0/0")
+        privateGw_1 = self.createPvtGw(vpc_1, "10.1.0.100", "10.1.0.101", acl1.id, vlan_1)
+        self.replacePvtGwACL(acl1.id, privateGw_1.id)
+
+        self.replaceNetworkAcl(acl1.id, network_1)
+        self.replaceNetworkAcl(acl1.id, network_2)
+        self.replaceNetworkAcl(acl1.id, network_3)
+        self.replaceNetworkAcl(acl1.id, network_4)
+
+        public_ip_1 = self.acquire_publicip(vpc_1, network_1)
+        nat_rule_1 = self.create_natrule(vpc_1, vm1, public_ip_1, network_1)
+        
+        self.check_private_gateway_interfaces()
+
+        self.check_pvt_gw_connectivity(vm1, public_ip_1, [vm2.nic[0].ipaddress, vm3.nic[0].ipaddress, vm4.nic[0].ipaddress])
+
+        self.reboot_vpc_with_cleanup(vpc_1, cleanup = True)
+        self.check_routers_state()
+
+        self.check_pvt_gw_connectivity(vm1, public_ip_1, [vm2.nic[0].ipaddress, vm3.nic[0].ipaddress, vm4.nic[0].ipaddress])
+
+        self.stop_router_by_type("MASTER")
+        self.check_routers_state()
+
+        self.check_private_gateway_interfaces()
+        self.check_pvt_gw_connectivity(vm1, public_ip_1, [vm2.nic[0].ipaddress, vm3.nic[0].ipaddress, vm4.nic[0].ipaddress])
+
+        self.start_routers()
+        self.check_routers_state()
+        self.check_private_gateway_interfaces()
+        self.check_pvt_gw_connectivity(vm1, public_ip_1, [vm2.nic[0].ipaddress, vm3.nic[0].ipaddress, vm4.nic[0].ipaddress])
+
+        self.deletePvtGw(privateGw_1.id)
+        self.check_private_gateway_interfaces(status_to_check = "DOWN")
+
+    def query_routers(self):
+        routers = list_routers(self.apiclient,
+                       account=self.account.name,
+                       domainid=self.account.domainid)
+
+        self.assertEqual(isinstance(routers, list), True,
+            "Check for list routers response return valid data")
+
+        self.assertEqual(len(routers), 2,
+            "Check for list routers size returned '%s' instead of 2" % len(routers))
+
+        return routers
+
+    def stop_router_by_type(self, redundant_state):
+        self.logger.debug('Stopping %s router' % redundant_state)
+        routers = self.query_routers()
+        for router in routers:
+            if router.redundantstate == redundant_state:
+                self.stop_router(router)
+                break
+
+    def start_routers(self):
+        self.logger.debug('Starting stopped routers')
+        routers = self.query_routers()
+        for router in routers:
+            self.logger.debug('Router %s has state %s' % (router.id, router.state))
+            if router.state == "Stopped":
+                self.logger.debug('Starting stopped router %s' % router.id)
+                cmd = startRouter.startRouterCmd()
+                cmd.id = router.id
+                self.apiclient.startRouter(cmd)
+
+    def stop_router(self, router):
+        self.logger.debug('Stopping router %s' % router.id)
+        cmd = stopRouter.stopRouterCmd()
+        cmd.id = router.id
+        self.apiclient.stopRouter(cmd)
 
     def createVPC(self, vpc_offering, cidr = '10.1.1.1/16'):
         try:
@@ -412,10 +535,10 @@ class TestPrivateGwACL(cloudstackTestCase):
         except Exception, e:
             self.fail('Unable to create ACL Item due to %s ' % e)
 
-    def createNetwork(self, vpc, gateway = '10.1.1.1'):
+    def createNetwork(self, vpc, net_offering = "network_offering", gateway = '10.1.1.1'):
         try:
             self.logger.debug('Create NetworkOffering')
-            net_offerring = self.services["network_offering"]
+            net_offerring = self.services[net_offering]
             net_offerring["name"] = "NET_OFF-%s" % gateway
             nw_off = NetworkOffering.create(
                 self.apiclient,
@@ -471,10 +594,23 @@ class TestPrivateGwACL(cloudstackTestCase):
         except Exception as e:
             self.fail("Failed to create Private Gateway ==> %s" % e)
 
-        self.assertIsNotNone(privateGw.id, "Failed to create ACL.")
+        self.assertIsNotNone(privateGw.id, "Failed to create Private Gateway.")
 
         return privateGw
 
+    def deletePvtGw(self, private_gw_id):
+        deletePrivateGatewayCmd = deletePrivateGateway.deletePrivateGatewayCmd()
+        deletePrivateGatewayCmd.id = private_gw_id
+
+        privateGwResponse = None
+        try:
+            privateGwResponse = self.apiclient.deletePrivateGateway(deletePrivateGatewayCmd)
+        except Exception as e:
+            self.fail("Failed to create Private Gateway ==> %s" % e)
+
+        self.assertIsNotNone(privateGwResponse, "Failed to Delete Private Gateway.")
+        self.assertTrue(privateGwResponse.success, "Failed to Delete Private Gateway.")
+
     def replaceNetworkAcl(self, aclId, network):
         self.logger.debug("Replacing Network ACL with ACL ID ==> %s" % aclId)
 
@@ -534,7 +670,11 @@ class TestPrivateGwACL(cloudstackTestCase):
 
         return nat_rule
 
-    def check_pvt_gw_connectivity(self, virtual_machine, public_ip, vm_ip):
+    def check_pvt_gw_connectivity(self, virtual_machine, public_ip, vms_ips):
+        sleep_time = 5
+        succeeded_pings = 0
+        minimum_vms_to_pass = 2
+        for vm_ip in vms_ips:
             ssh_command = "ping -c 3 %s" % vm_ip
 
             # Should be able to SSH VM
@@ -544,19 +684,24 @@ class TestPrivateGwACL(cloudstackTestCase):
 
                 ssh = virtual_machine.get_ssh_client(ipaddress=public_ip.ipaddress.ipaddress)
 
-            self.logger.debug("Ping to VM inside another VPC")
+                self.logger.debug("Sleeping for %s seconds in order to get the firewall applied..." % sleep_time)
+                time.sleep(sleep_time)
+                sleep_time += sleep_time
+
+                self.logger.debug("Ping to VM inside another Network Tier")
                 result = str(ssh.execute(ssh_command))
 
                 self.logger.debug("SSH result: %s; COUNT is ==> %s" % (result, result.count("3 packets received")))
             except Exception as e:
                 self.fail("SSH Access failed for %s: %s" % \
-                      (vmObj.get_ip(), e)
+                          (virtual_machine, e)
                           )
 
-        self.assertEqual(
+            succeeded_pings += result.count("3 packets received")
-                         result.count("3 packets received"),
+
-                         1,
+
-                         "Ping to outside world from VM should be successful"
+        self.assertTrue(succeeded_pings >= minimum_vms_to_pass,
+                        "Ping to VM on Network Tier N from VM in Network Tier A should be successful at least for 2 out of 3 VMs"
                        )
 
     def reboot_vpc_with_cleanup(self, vpc, cleanup = True):
@@ -569,3 +714,120 @@ class TestPrivateGwACL(cloudstackTestCase):
         cmd.makeredundant = False
         self.api_client.restartVPC(cmd)
 
+    def check_private_gateway_interfaces(self, status_to_check = "UP"):
+        routers = self.query_routers()
+
+        state_holder = {routers[0].linklocalip : {"state" : None, "mac" : None},
+                        routers[1].linklocalip : {"state" : None, "mac" : None}}
+        state = None
+        mac = None
+        for router in routers:
+            hosts = list_hosts(
+                    self.apiclient,
+                    zoneid=router.zoneid,
+                    type='Routing',
+                    state='Up',
+                    id=router.hostid)
+
+            self.assertEqual(
+                isinstance(hosts, list),
+                True,
+                "Check for list hosts response return valid data")
+
+            host = hosts[0]
+            host.user = self.services["configurableData"]["host"]["username"]
+            host.passwd = self.services["configurableData"]["host"]["password"]
+            host.port = self.services["configurableData"]["host"]["port"]
+
+            try:
+                state = get_process_status(
+                    host.ipaddress,
+                    host.port,
+                    host.user,
+                    host.passwd,
+                    router.linklocalip,
+                    "ip addr | grep eth6 | grep state | awk '{print $9;}'")
+
+                mac = get_process_status(
+                    host.ipaddress,
+                    host.port,
+                    host.user,
+                    host.passwd,
+                    router.linklocalip,
+                    "ip addr | grep link/ether | awk '{print $2;}' | sed -n 7p")
+            except KeyError:
+                self.skipTest("Provide a marvin config file with host credentials to run %s" % self._testMethodName)
+
+            state = str(state[0])
+            mac = str(mac[0])
+
+            self.logger.debug("Result from the Router on IP '%s' is -> state: '%s', mac: '%s'" % (router.linklocalip, state, mac))
+            state_holder[router.linklocalip]["state"] = str(state)
+            state_holder[router.linklocalip]["mac"] = str(mac)
+
+
+        if status_to_check == "UP":
+            check_state = state_holder[routers[0].linklocalip]["state"].count(state_holder[routers[1].linklocalip]["state"])
+            check_mac = state_holder[routers[0].linklocalip]["mac"].count(state_holder[routers[1].linklocalip]["mac"])
+
+            self.assertTrue(check_state == 0, "Routers private gateway interface should not be on the same state!")
+            self.assertTrue(check_mac == 0, "Routers private gateway interface should not have the same mac address!")
+        else:
+            self.assertTrue(check_state == 1, "Routers private gateway interface should should have been removed!")
+
+    def check_routers_state(self, status_to_check="MASTER", expected_count=1):
+        routers = self.query_routers()
+
+        vals = ["MASTER", "BACKUP", "UNKNOWN"]
+        cnts = [0, 0, 0]
+
+        result = "UNKNOWN"
+        for router in routers:
+            if router.state == "Running":
+                hosts = list_hosts(
+                    self.apiclient,
+                    zoneid=router.zoneid,
+                    type='Routing',
+                    state='Up',
+                    id=router.hostid
+                )
+                self.assertEqual(
+                    isinstance(hosts, list),
+                    True,
+                    "Check list host returns a valid list"
+                )
+                host = hosts[0]
+
+                if self.hypervisor.lower() in ('vmware', 'hyperv'):
+                        result = str(get_process_status(
+                            self.apiclient.connection.mgtSvr,
+                            22,
+                            self.apiclient.connection.user,
+                            self.apiclient.connection.passwd,
+                            router.linklocalip,
+                            "sh /opt/cloud/bin/checkrouter.sh ",
+                            hypervisor=self.hypervisor
+                        ))
+                else:
+                    try:
+                        host.user, host.passwd = get_host_credentials(
+                            self.config, host.ipaddress)
+                        result = str(get_process_status(
+                            host.ipaddress,
+                            22,
+                            host.user,
+                            host.passwd,
+                            router.linklocalip,
+                            "sh /opt/cloud/bin/checkrouter.sh "
+                        ))
+
+                    except KeyError:
+                        self.skipTest(
+                            "Marvin configuration has no host credentials to\
+                                    check router services")
+
+                if result.count(status_to_check) == 1:
+                    cnts[vals.index(status_to_check)] += 1
+
+        if cnts[vals.index(status_to_check)] != expected_count:
+            self.fail("Expected '%s' routers at state '%s', but found '%s'!" % (expected_count, status_to_check, cnts[vals.index(status_to_check)]))