Clean up persistent VXLAN network bridges on all hosts on network delete

Author: MitchDrage

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

@@ -1515,9 +1515,10 @@ private void setupPersistentNetwork(NetworkVO network, NetworkOfferingVO offerin
         }
     }
 
-    private boolean networkMeetsPersistenceCriteria(NetworkVO network, NetworkOfferingVO offering, boolean cleanup) {
-        boolean criteriaMet = offering.isPersistent() &&
-                (network.getBroadcastUri() != null && BroadcastDomainType.getSchemeValue(network.getBroadcastUri()) == BroadcastDomainType.Vlan);
+    protected boolean networkMeetsPersistenceCriteria(NetworkVO network, NetworkOfferingVO offering, boolean cleanup) {
+        BroadcastDomainType broadcastDomainType = BroadcastDomainType.getSchemeValue(network.getBroadcastUri());
+        List<BroadcastDomainType> broadcastTypes = Arrays.asList(BroadcastDomainType.Vlan, BroadcastDomainType.Vxlan);
+        boolean criteriaMet = offering.isPersistent() && broadcastTypes.contains(broadcastDomainType);
         if (!cleanup) {
             return criteriaMet && network.getGuestType() == GuestType.L2;
         } else {

engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java

@@ -59,6 +59,7 @@
 import com.cloud.network.Network.GuestType;
 import com.cloud.network.Network.Service;
 import com.cloud.network.NetworkModel;
+import com.cloud.network.Networks.BroadcastDomainType;
 import com.cloud.network.Networks.TrafficType;
 import com.cloud.network.dao.IPAddressDao;
 import com.cloud.network.dao.IPAddressVO;
@@ -1010,4 +1011,33 @@ public void testImportNicWithIP4Address() throws Exception {
             assertEquals("testtag", nicProfile.getName());
         }
     }
+
+    private NetworkVO mockPersistentNetwork(URI broadcastUri, GuestType guestType) {
+        NetworkVO network = mock(NetworkVO.class);
+        when(network.getBroadcastUri()).thenReturn(broadcastUri);
+        when(network.getGuestType()).thenReturn(guestType);
+        return network;
+    }
+
+    @Test
+    public void testNetworkMeetsPersistenceCriteriaForVlanAndVxlan() {
+        NetworkOfferingVO offering = mock(NetworkOfferingVO.class);
+        when(offering.isPersistent()).thenReturn(true);
+
+        NetworkVO vlanNetwork = mockPersistentNetwork(BroadcastDomainType.Vlan.toUri(100), GuestType.Isolated);
+        NetworkVO vxlanNetwork = mockPersistentNetwork(BroadcastDomainType.Vxlan.toUri(5000), GuestType.Isolated);
+
+        assertTrue(testOrchestrator.networkMeetsPersistenceCriteria(vlanNetwork, offering, true));
+        assertTrue(testOrchestrator.networkMeetsPersistenceCriteria(vxlanNetwork, offering, true));
+    }
+
+    @Test
+    public void testNetworkMeetsPersistenceCriteriaForNonPersistentOffering() {
+        NetworkOfferingVO offering = mock(NetworkOfferingVO.class);
+        when(offering.isPersistent()).thenReturn(false);
+
+        NetworkVO vxlanNetwork = mockPersistentNetwork(BroadcastDomainType.Vxlan.toUri(5000), GuestType.Isolated);
+
+        assertFalse(testOrchestrator.networkMeetsPersistenceCriteria(vxlanNetwork, offering, true));
+    }
 }

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

@@ -343,7 +343,7 @@ private void createVnet(String vnetId, String pif, String brName, String protoco
         }
     }
 
-    private void deleteVnetBr(String brName, boolean deleteBr) {
+    protected void deleteVnetBr(String brName, boolean deleteBr) {
         synchronized (_vnetBridgeMonitor) {
             String cmdout = Script.runSimpleBashScript("ls /sys/class/net/" + brName);
             if (cmdout == null)
@@ -458,6 +458,14 @@ public boolean isExistingBridge(String bridgeName) {
 
     @Override
     public void deleteBr(NicTO nic) {
+        if (Networks.BroadcastDomainType.getSchemeValue(nic.getBroadcastUri()) == Networks.BroadcastDomainType.Vxlan) {
+            // VXLAN bridges are named after the VNI alone, so no physical interface lookup is needed
+            String vxlanId = Networks.BroadcastDomainType.getValue(nic.getBroadcastUri());
+            if (vxlanId != null) {
+                deleteVnetBr(generateVxnetBrName(null, vxlanId), true);
+            }
+            return;
+        }
         String vlanId = Networks.BroadcastDomainType.getValue(nic.getBroadcastUri());
         String trafficLabel = nic.getName();
         String pifName = _pifs.get(trafficLabel);

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriverTest.java

@@ -18,18 +18,21 @@
 
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.util.HashMap;
 
 import org.junit.Assert;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.InjectMocks;
+import org.mockito.MockedConstruction;
 import org.mockito.Mockito;
 import org.mockito.Spy;
 import org.mockito.junit.MockitoJUnitRunner;
 
 import com.cloud.agent.api.to.NicTO;
 import com.cloud.exception.InternalErrorException;
 import com.cloud.network.Networks;
+import com.cloud.utils.script.Script;
 
 @RunWith(MockitoJUnitRunner.class)
 public class BridgeVifDriverTest {
@@ -100,4 +103,39 @@ public void createStorageVnetBridgeIfNeededCreatesVnetBridgeWhenUntaggedVlan() t
         String result = driver.createStorageVnetBridgeIfNeeded(nic, "trafficLabel", BRIDGE_NAME);
         Assert.assertEquals(BRIDGE_NAME, result);
     }
+
+    private String createAndDeleteBridge(String vNetId, String trafficLabel, Networks.BroadcastDomainType broadcastType) throws InternalErrorException {
+        Mockito.doNothing().when(driver).deleteVnetBr(Mockito.anyString(), Mockito.anyBoolean());
+        String createdBridge;
+        try (MockedConstruction<Script> ignored = Mockito.mockConstruction(Script.class)) {
+            createdBridge = driver.createVnetBr(vNetId, trafficLabel, broadcastType.scheme());
+        }
+
+        NicTO nic = new NicTO();
+        nic.setName(trafficLabel);
+        nic.setBroadcastUri(broadcastType.toUri(vNetId));
+        driver.deleteBr(nic);
+        return createdBridge;
+    }
+
+    @Test
+    public void deleteBrRemovesTheBridgeCreatedForVxlan() throws InternalErrorException {
+        driver._pifs = new HashMap<>();
+
+        String createdBridge = createAndDeleteBridge("5000", "unknownTrafficLabel", Networks.BroadcastDomainType.Vxlan);
+
+        Assert.assertEquals("brvx-5000", createdBridge);
+        Mockito.verify(driver).deleteVnetBr(createdBridge, true);
+    }
+
+    @Test
+    public void deleteBrRemovesTheBridgeCreatedForVlan() throws InternalErrorException {
+        driver._pifs = new HashMap<>();
+        driver._pifs.put("guestLabel", "eth1");
+
+        String createdBridge = createAndDeleteBridge("100", "guestLabel", Networks.BroadcastDomainType.Vlan);
+
+        Assert.assertEquals("breth1-100", createdBridge);
+        Mockito.verify(driver).deleteVnetBr(createdBridge, true);
+    }
 }