address comments

Author: Pearl Dsilva

engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java

@@ -1856,11 +1856,22 @@ private Map<String, Boolean> getVlanToPersistenceMapForVM(long vmId) {
         return vlanToPersistenceMap;
     }
 
+    /**
+     *
+     * @param networkVO - the network object used to determine the vlanId from the broadcast URI
+     * @param isPersistent - indicates if the corresponding network's network offering is Persistent
+     *
+     * @return <VlanId, ShouldKVMBridgeBeDeleted> - basically returns the vlan ID which is used to determine the
+     * bridge name for KVM hypervisor and based on the network and isolation type and persistent setting of the offering
+     * we decide whether the bridge is to be deleted (KVM) if the last VM in that host is destroyed / migrated
+     */
     private Pair<String, Boolean> getVMNetworkDetails(NetworkVO networkVO, boolean isPersistent) {
         URI broadcastUri = networkVO.getBroadcastUri();
         String scheme = broadcastUri.getScheme();
         String vlanId = Networks.BroadcastDomainType.getValue(broadcastUri);
-        Boolean shouldDelete = !((networkVO.getGuestType() == Network.GuestType.L2 || networkVO.getGuestType() == Network.GuestType.Isolated) && scheme.equalsIgnoreCase("vlan") && isPersistent);
+        Boolean shouldDelete = !((networkVO.getGuestType() == Network.GuestType.L2 || networkVO.getGuestType() == Network.GuestType.Isolated) &&
+                (scheme.equalsIgnoreCase("vlan") || scheme.equalsIgnoreCase("vxlan"))
+                && isPersistent);
         return new Pair<>(vlanId, shouldDelete);
     }
 

engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java

@@ -1200,7 +1200,12 @@ Pair<NetworkGuru, NetworkVO> implementNetwork(final long networkId, final Deploy
         return implemented;
     }
 
-    private NicTO getNicTO(NetworkVO networkVO, NetworkOfferingVO networkOfferingVO) {
+    /**
+     *
+     * Creates a dummy NicTO object which is used by the respective hypervisors to setup network elements / resources
+     * - bridges(KVM), VLANs(Xen) and portgroups(VMWare) for L2 network
+     */
+    private NicTO createNicTOFromNetworkAndOffering(NetworkVO networkVO, NetworkOfferingVO networkOfferingVO) {
         NicTO to = new NicTO();
         to.setBroadcastType(networkVO.getBroadcastDomainType());
         to.setType(networkVO.getTrafficType());
@@ -1231,10 +1236,9 @@ private boolean isNtwConfiguredInCluster(HostVO hostVO, Map<Long, List<Long>> cl
     }
 
     private void setupPersistentNetwork(NetworkVO network, NetworkOfferingVO offering, Long dcId) throws AgentUnavailableException, OperationTimedoutException {
-        NicTO to = getNicTO(network, offering);
+        NicTO to = createNicTOFromNetworkAndOffering(network, offering);
         List<ClusterVO> clusterVOS = clusterDao.listClustersByDcId(dcId);
         List<HostVO> hosts = resourceManager.listAllUpAndEnabledHostsInOneZoneByType(Host.Type.Routing, dcId);
-        Collections.reverse(hosts);
         Map<Long, List<Long>> clusterToHostsMap = new HashMap<>();
         SetupPersistentNetworkCommand cmd = new SetupPersistentNetworkCommand(to);
         for (HostVO host : hosts) {
@@ -1247,13 +1251,11 @@ private void setupPersistentNetwork(NetworkVO network, NetworkOfferingVO offerin
                 s_logger.warn("Unable to get an answer to the SetupPersistentNetworkCommand from agent:" + host.getId());
                 clusterToHostsMap.get(host.getClusterId()).remove(host.getId());
                 continue;
-                // throw new CloudRuntimeException("Unable to get an answer to the SetupPersistentNetworkCommand from agent: " + host.getId());
             }
 
             if (!answer.getResult()) {
-                s_logger.warn("Unable to setup agent " + host.getId() + " due to " + answer.getDetails() );
-//                final String msg = "Incorrect Network setup on agent, Reinitialize agent after network names are setup, details : " + answer.getDetails();
-//                throw new CloudRuntimeException(msg);
+                s_logger.warn("Unable to setup agent " + host.getId() + " due to " + answer.getDetails());
+                clusterToHostsMap.get(host.getClusterId()).remove(host.getId());
                 continue;
             }
         }

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

@@ -282,7 +282,7 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
                 // each interface at this point, so inform all vif drivers
                 final List<VifDriver> allVifDrivers = libvirtComputingResource.getAllVifDrivers();
                 for (final VifDriver vifDriver : allVifDrivers) {
-                    vifDriver.unplug(iface, deleteBridge(vlanToPersistenceMap, vlanId));
+                    vifDriver.unplug(iface, shouldDeleteBridge(vlanToPersistenceMap, vlanId));
                 }
             }
         }
@@ -291,13 +291,17 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
     }
 
     private String getVlanIdFromBridgeName(String brName) {
-        if (brName != null) {
-            return brName.split("-")[1];
+        if (StringUtils.isNotBlank(brName)) {
+            String[] s = brName.split("-");
+            if (s.length > 1) {
+                return s[1];
+            }
+            return null;
         }
         return null;
     }
 
-    private boolean deleteBridge(Map<String, Boolean> vlanToPersistenceMap, String vlanId) {
+    private boolean shouldDeleteBridge(Map<String, Boolean> vlanToPersistenceMap, String vlanId) {
         if (MapUtils.isNotEmpty(vlanToPersistenceMap) && vlanId != null && vlanToPersistenceMap.containsKey(vlanId)) {
             return vlanToPersistenceMap.get(vlanId);
         }

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

@@ -30,6 +30,7 @@
 import com.cloud.utils.ssh.SshHelper;
 import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.collections.MapUtils;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.log4j.Logger;
 import org.libvirt.Connect;
 import org.libvirt.Domain;
@@ -129,7 +130,7 @@ public Answer execute(final StopCommand command, final LibvirtComputingResource
                         // We don't know which "traffic type" is associated with
                         // each interface at this point, so inform all vif drivers
                         for (final VifDriver vifDriver : libvirtComputingResource.getAllVifDrivers()) {
-                            vifDriver.unplug(iface, deleteBridge(vlanToPersistenceMap, vlanId));
+                            vifDriver.unplug(iface, shouldDeleteBridge(vlanToPersistenceMap, vlanId));
                         }
                     }
                 }
@@ -162,7 +163,7 @@ private void performAgentStopHook(String vmName, final LibvirtComputingResource
     }
 
     private String getVlanIdFromBridgeName(String brName) {
-        if (brName != null) {
+        if (StringUtils.isNotBlank(brName)) {
             String[] s = brName.split("-");
             if (s.length > 1) {
                 return s[1];
@@ -172,7 +173,7 @@ private String getVlanIdFromBridgeName(String brName) {
         return null;
     }
 
-    private boolean deleteBridge(Map<String, Boolean> vlanToPersistenceMap, String vlanId) {
+    private boolean shouldDeleteBridge(Map<String, Boolean> vlanToPersistenceMap, String vlanId) {
         if (MapUtils.isNotEmpty(vlanToPersistenceMap) && vlanId != null && vlanToPersistenceMap.containsKey(vlanId)) {
             return vlanToPersistenceMap.get(vlanId);
         }