VR: Fix Redundant VRouter guest network on wrong interface (#3847)

Author: weizhouapache

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

@@ -1783,17 +1783,11 @@ protected ExecutionResult prepareNetworkElementCommand(final IpAssocVpcCommand c
 
         try {
             conn = getLibvirtUtilitiesHelper().getConnectionByVmName(routerName);
-            final IpAddressTO[] ips = cmd.getIpAddresses();
-            Integer devNum = 0;
-            final List<InterfaceDef> pluggedNics = getInterfaces(conn, routerName);
-            final Map<String, Integer> macAddressToNicNum = new HashMap<>(pluggedNics.size());
-
-            for (final InterfaceDef pluggedNic : pluggedNics) {
-                final String pluggedVlan = pluggedNic.getBrName();
-                macAddressToNicNum.put(pluggedNic.getMacAddress(), devNum);
-                devNum++;
-            }
+            Pair<Map<String, Integer>, Integer> macAddressToNicNumPair = getMacAddressToNicNumPair(conn, routerName);
+            final Map<String, Integer> macAddressToNicNum = macAddressToNicNumPair.first();
+            Integer devNum = macAddressToNicNumPair.second();
 
+            final IpAddressTO[] ips = cmd.getIpAddresses();
             for (final IpAddressTO ip : ips) {
                 ip.setNicDevId(macAddressToNicNum.get(ip.getVifMacAddress()));
             }
@@ -1810,35 +1804,22 @@ public ExecutionResult prepareNetworkElementCommand(final IpAssocCommand cmd) {
         final String routerIp = cmd.getAccessDetail(NetworkElementCommand.ROUTER_IP);
         Connect conn;
         try {
-            conn = LibvirtConnection.getConnectionByVmName(routerName);
-            final List<InterfaceDef> nics = getInterfaces(conn, routerName);
-            final Map<String, Integer> broadcastUriAllocatedToVM = new HashMap<String, Integer>();
-            Integer nicPos = 0;
-            for (final InterfaceDef nic : nics) {
-                if (nic.getBrName().equalsIgnoreCase(_linkLocalBridgeName)) {
-                    broadcastUriAllocatedToVM.put("LinkLocal", nicPos);
-                } else {
-                    if (nic.getBrName().equalsIgnoreCase(_publicBridgeName) || nic.getBrName().equalsIgnoreCase(_privBridgeName) ||
-                            nic.getBrName().equalsIgnoreCase(_guestBridgeName)) {
-                        broadcastUriAllocatedToVM.put(BroadcastDomainType.Vlan.toUri(Vlan.UNTAGGED).toString(), nicPos);
-                    } else {
-                        final String broadcastUri = getBroadcastUriFromBridge(nic.getBrName());
-                        broadcastUriAllocatedToVM.put(broadcastUri, nicPos);
-                    }
-                }
-                nicPos++;
-            }
+            conn = getLibvirtUtilitiesHelper().getConnectionByVmName(routerName);
+            Pair<Map<String, Integer>, Integer> macAddressToNicNumPair = getMacAddressToNicNumPair(conn, routerName);
+            final Map<String, Integer> macAddressToNicNum = macAddressToNicNumPair.first();
+            Integer devNum = macAddressToNicNumPair.second();
+
             final IpAddressTO[] ips = cmd.getIpAddresses();
             int nicNum = 0;
             for (final IpAddressTO ip : ips) {
                 boolean newNic = false;
-                if (!broadcastUriAllocatedToVM.containsKey(ip.getBroadcastUri())) {
+                if (!macAddressToNicNum.containsKey(ip.getVifMacAddress())) {
                     /* plug a vif into router */
                     VifHotPlug(conn, routerName, ip.getBroadcastUri(), ip.getVifMacAddress());
-                    broadcastUriAllocatedToVM.put(ip.getBroadcastUri(), nicPos++);
+                    macAddressToNicNum.put(ip.getVifMacAddress(), devNum++);
                     newNic = true;
                 }
-                nicNum = broadcastUriAllocatedToVM.get(ip.getBroadcastUri());
+                nicNum = macAddressToNicNum.get(ip.getVifMacAddress());
                 networkUsage(routerIp, "addVif", "eth" + nicNum);
 
                 ip.setNicDevId(nicNum);
@@ -1860,39 +1841,21 @@ protected ExecutionResult cleanupNetworkElementCommand(final IpAssocCommand cmd)
         final String routerIp = cmd.getAccessDetail(NetworkElementCommand.ROUTER_IP);
         final String lastIp = cmd.getAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP);
         Connect conn;
-
-
-        try{
-            conn = LibvirtConnection.getConnectionByVmName(routerName);
-            final List<InterfaceDef> nics = getInterfaces(conn, routerName);
-            final Map<String, Integer> broadcastUriAllocatedToVM = new HashMap<String, Integer>();
-
-            Integer nicPos = 0;
-            for (final InterfaceDef nic : nics) {
-                if (nic.getBrName().equalsIgnoreCase(_linkLocalBridgeName)) {
-                    broadcastUriAllocatedToVM.put("LinkLocal", nicPos);
-                } else {
-                    if (nic.getBrName().equalsIgnoreCase(_publicBridgeName) || nic.getBrName().equalsIgnoreCase(_privBridgeName) ||
-                            nic.getBrName().equalsIgnoreCase(_guestBridgeName)) {
-                        broadcastUriAllocatedToVM.put(BroadcastDomainType.Vlan.toUri(Vlan.UNTAGGED).toString(), nicPos);
-                    } else {
-                        final String broadcastUri = getBroadcastUriFromBridge(nic.getBrName());
-                        broadcastUriAllocatedToVM.put(broadcastUri, nicPos);
-                    }
-                }
-                nicPos++;
-            }
+        try {
+            conn = getLibvirtUtilitiesHelper().getConnectionByVmName(routerName);
+            Pair<Map<String, Integer>, Integer> macAddressToNicNumPair = getMacAddressToNicNumPair(conn, routerName);
+            final Map<String, Integer> macAddressToNicNum = macAddressToNicNumPair.first();
+            Integer devNum = macAddressToNicNumPair.second();
 
             final IpAddressTO[] ips = cmd.getIpAddresses();
             int nicNum = 0;
             for (final IpAddressTO ip : ips) {
-
-                if (!broadcastUriAllocatedToVM.containsKey(ip.getBroadcastUri())) {
+                if (!macAddressToNicNum.containsKey(ip.getVifMacAddress())) {
                     /* plug a vif into router */
                     VifHotPlug(conn, routerName, ip.getBroadcastUri(), ip.getVifMacAddress());
-                    broadcastUriAllocatedToVM.put(ip.getBroadcastUri(), nicPos++);
+                    macAddressToNicNum.put(ip.getVifMacAddress(), devNum++);
                 }
-                nicNum = broadcastUriAllocatedToVM.get(ip.getBroadcastUri());
+                nicNum = macAddressToNicNum.get(ip.getVifMacAddress());
 
                 if (org.apache.commons.lang.StringUtils.equalsIgnoreCase(lastIp, "true") && !ip.isAdd()) {
                     // in isolated network eth2 is the default public interface. We don't want to delete it.
@@ -1914,6 +1877,19 @@ protected ExecutionResult cleanupNetworkElementCommand(final IpAssocCommand cmd)
         return new ExecutionResult(true, null);
     }
 
+
+    private Pair<Map<String, Integer>, Integer> getMacAddressToNicNumPair(Connect conn, String routerName) {
+        Integer devNum = 0;
+        final List<InterfaceDef> pluggedNics = getInterfaces(conn, routerName);
+        final Map<String, Integer> macAddressToNicNum = new HashMap<>(pluggedNics.size());
+        for (final InterfaceDef pluggedNic : pluggedNics) {
+            final String pluggedVlan = pluggedNic.getBrName();
+            macAddressToNicNum.put(pluggedNic.getMacAddress(), devNum);
+            devNum++;
+        }
+        return new Pair<Map<String, Integer>, Integer>(macAddressToNicNum, devNum);
+    }
+
     protected PowerState convertToPowerState(final DomainState ps) {
         final PowerState state = s_powerStatesTable.get(ps);
         return state == null ? PowerState.PowerUnknown : state;

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

@@ -674,7 +674,20 @@ public void createVpcAssociatePublicIPCommands(final VirtualRouter router, final
             vlanIpMap.put(vlanTag, ipList);
         }
 
+        Long guestNetworkId = null;
+        final List<NicVO> nics = _nicDao.listByVmId(router.getId());
+        for (final NicVO nic : nics) {
+            final NetworkVO nw = _networkDao.findById(nic.getNetworkId());
+            if (nw.getTrafficType() == TrafficType.Guest) {
+                guestNetworkId = nw.getId();
+                break;
+            }
+        }
+
+        Map<String, Boolean> vlanLastIpMap = getVlanLastIpMap(router.getVpcId(), guestNetworkId);
+
         for (final Map.Entry<String, ArrayList<PublicIpAddress>> vlanAndIp : vlanIpMap.entrySet()) {
+            final String vlanTagKey = vlanAndIp.getKey();
             final List<PublicIpAddress> ipAddrList = vlanAndIp.getValue();
 
             // Source nat ip address should always be sent first
@@ -732,6 +745,8 @@ public int compare(final PublicIpAddress o1, final PublicIpAddress o2) {
             final DataCenterVO dcVo = _dcDao.findById(router.getDataCenterId());
             cmd.setAccessDetail(NetworkElementCommand.ZONE_NETWORK_TYPE, dcVo.getNetworkType().toString());
 
+            setAccessDetailNetworkLastPublicIp(vlanLastIpMap, vlanTagKey, cmd);
+
             cmds.addCommand(ipAssocCommand, cmd);
         }
 
@@ -769,15 +784,25 @@ public void createRedundantAssociateIPCommands(final VirtualRouter router, final
 
         final List<NicVO> nics = _nicDao.listByVmId(router.getId());
         String baseMac = null;
+        Map<String, String> vlanMacAddress = new HashMap<String, String>();;
+        Long guestNetworkId = null;
         for (final NicVO nic : nics) {
             final NetworkVO nw = _networkDao.findById(nic.getNetworkId());
             if (nw.getTrafficType() == TrafficType.Public) {
-                baseMac = nic.getMacAddress();
-                break;
+                if (baseMac == null) {
+                    baseMac = nic.getMacAddress();
+                }
+                final String vlanTag = BroadcastDomainType.getValue(nic.getBroadcastUri());
+                vlanMacAddress.put(vlanTag, nic.getMacAddress());
+            } else if (nw.getTrafficType() == TrafficType.Guest && guestNetworkId == null) {
+                guestNetworkId = nw.getId();
             }
         }
 
+        Map<String, Boolean> vlanLastIpMap = getVlanLastIpMap(router.getVpcId(), guestNetworkId);
+
         for (final Map.Entry<String, ArrayList<PublicIpAddress>> vlanAndIp : vlanIpMap.entrySet()) {
+            final String vlanTagKey = vlanAndIp.getKey();
             final List<PublicIpAddress> ipAddrList = vlanAndIp.getValue();
             // Source nat ip address should always be sent first
             Collections.sort(ipAddrList, new Comparator<PublicIpAddress>() {
@@ -809,19 +834,16 @@ public int compare(final PublicIpAddress o1, final PublicIpAddress o2) {
                 final String vlanGateway = ipAddr.getGateway();
                 final String vlanNetmask = ipAddr.getNetmask();
                 String vifMacAddress = null;
-                // For non-source nat IP, set the mac to be something based on
-                // first public nic's MAC
-                // We cannot depend on first ip because we need to deal with
-                // first ip of other nics
-                if (router.getVpcId() != null) {
-                    //vifMacAddress = NetUtils.generateMacOnIncrease(baseMac, ipAddr.getVlanId());
-                    vifMacAddress = ipAddr.getMacAddress();
+                final String vlanTag = BroadcastDomainType.getValue(BroadcastDomainType.fromString(ipAddr.getVlanTag()));
+                if (vlanMacAddress.containsKey(vlanTag)) {
+                    vifMacAddress = vlanMacAddress.get(vlanTag);
                 } else {
-                    if (!sourceNat && ipAddr.getVlanId() != 0) {
+                    if (ipAddr.getVlanId() != 0) {
                         vifMacAddress = NetUtils.generateMacOnIncrease(baseMac, ipAddr.getVlanId());
                     } else {
                         vifMacAddress = ipAddr.getMacAddress();
                     }
+                    vlanMacAddress.put(vlanTag, vifMacAddress);
                 }
 
                 final IpAddressTO ip = new IpAddressTO(ipAddr.getAccountId(), ipAddr.getAddress().addr(), add, firstIP, sourceNat, vlanId, vlanGateway, vlanNetmask,
@@ -839,56 +861,59 @@ public int compare(final PublicIpAddress o1, final PublicIpAddress o2) {
                 }
             }
 
-            Long associatedWithNetworkId = ipAddrList.get(0).getAssociatedWithNetworkId();
-            if (associatedWithNetworkId == null || associatedWithNetworkId == 0) {
-                associatedWithNetworkId = ipAddrList.get(0).getNetworkId();
-            }
-
-            // for network if the ips does not have any rules, then only last ip
-            final List<IPAddressVO> userIps = _ipAddressDao.listByAssociatedNetwork(associatedWithNetworkId, null);
-            boolean hasSourceNat = false;
-            if (isVPC && userIps.size() > 0 && userIps.get(0) != null) {
-                // All ips should belong to a VPC
-                final Long vpcId = userIps.get(0).getVpcId();
-                final List<IPAddressVO> sourceNatIps = _ipAddressDao.listByAssociatedVpc(vpcId, true);
-                if (sourceNatIps != null && sourceNatIps.size() > 0) {
-                    hasSourceNat = true;
-                }
-            }
-
-            int ipsWithrules = 0;
-            int ipsStaticNat = 0;
-            for (IPAddressVO ip : userIps) {
-                if ( _rulesDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Active) > 0){
-                    ipsWithrules++;
-                }
-
-                // check onetoonenat and also check if the ip "add":false. If there are 2 PF rules remove and
-                // 1 static nat rule add
-                if (ip.isOneToOneNat() && ip.getRuleState() == null) {
-                    ipsStaticNat++;
-                }
+            final IpAssocCommand cmd;
+            if (router.getVpcId() != null) {
+                cmd = new IpAssocVpcCommand(ipsToSend);
+            } else {
+                cmd = new IpAssocCommand(ipsToSend);
             }
-
-            final IpAssocCommand cmd = new IpAssocCommand(ipsToSend);
             cmd.setAccessDetail(NetworkElementCommand.ROUTER_IP, _routerControlHelper.getRouterControlIp(router.getId()));
-            cmd.setAccessDetail(NetworkElementCommand.ROUTER_GUEST_IP, _routerControlHelper.getRouterIpInNetwork(associatedWithNetworkId, router.getId()));
+            cmd.setAccessDetail(NetworkElementCommand.ROUTER_GUEST_IP, _routerControlHelper.getRouterIpInNetwork(ipAddrList.get(0).getNetworkId(), router.getId()));
             cmd.setAccessDetail(NetworkElementCommand.ROUTER_NAME, router.getInstanceName());
             final DataCenterVO dcVo = _dcDao.findById(router.getDataCenterId());
             cmd.setAccessDetail(NetworkElementCommand.ZONE_NETWORK_TYPE, dcVo.getNetworkType().toString());
 
-            // if there is 1 static nat then it will be checked for remove at the resource
-            if (ipsWithrules == 0 && ipsStaticNat == 0 && !hasSourceNat) {
-                // there is only one ip address for the network.
-                cmd.setAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP, "true");
-            } else {
-                cmd.setAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP, "false");
-            }
+            setAccessDetailNetworkLastPublicIp(vlanLastIpMap, vlanTagKey, cmd);
 
             cmds.addCommand(ipAssocCommand, cmd);
         }
     }
 
+    private void setAccessDetailNetworkLastPublicIp(Map<String, Boolean> vlanLastIpMap, String vlanTagKey, IpAssocCommand cmd) {
+        // if public ip is the last ip in the vlan which is used for static nat or has active rules,
+        // it will be checked for remove at the resource
+        Boolean lastIp = vlanLastIpMap.get(vlanTagKey);
+        if (lastIp == null) {
+            cmd.setAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP, "true");
+        } else {
+            cmd.setAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP, "false");
+        }
+    }
+
+    private Map<String, Boolean> getVlanLastIpMap(Long vpcId, Long guestNetworkId) {
+        // for network if the ips does not have any rules, then only last ip
+        final Map<String, Boolean> vlanLastIpMap = new HashMap<String, Boolean>();
+        final List<IPAddressVO> userIps;
+        if (vpcId != null) {
+            userIps = _ipAddressDao.listByAssociatedVpc(vpcId, null);
+        } else {
+            userIps = _ipAddressDao.listByAssociatedNetwork(guestNetworkId, null);
+        }
+        for (IPAddressVO ip : userIps) {
+            String vlanTag = _vlanDao.findById(ip.getVlanId()).getVlanTag();
+            Boolean lastIp = vlanLastIpMap.get(vlanTag);
+            if (lastIp != null && !lastIp) {
+                continue;
+            }
+            if (ip.isSourceNat()
+                    || _rulesDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Active) > 0
+                    || (ip.isOneToOneNat() && ip.getRuleState() == null)) {
+                vlanLastIpMap.put(vlanTag, false);
+            }
+        }
+        return vlanLastIpMap;
+    }
+
     public void createStaticRouteCommands(final List<StaticRouteProfile> staticRoutes, final DomainRouterVO router, final Commands cmds) {
         final SetStaticRouteCommand cmd = new SetStaticRouteCommand(staticRoutes);
         cmd.setAccessDetail(NetworkElementCommand.ROUTER_IP, _routerControlHelper.getRouterControlIp(router.getId()));

systemvm/debian/etc/sysctl.conf

@@ -27,6 +27,10 @@ net.ipv4.conf.default.send_redirects = 0
 net.ipv4.conf.all.secure_redirects = 0
 net.ipv4.conf.default.secure_redirects = 0
 
+# Promote secondary ip to be primary if primary IP is removed
+net.ipv4.conf.all.promote_secondaries = 1
+net.ipv4.conf.default.promote_secondaries = 1
+
 # For smooth transition of the vip address in case of a keepalived failover
 net.ipv4.ip_nonlocal_bind = 1