kvm: add ssvm storage nic null uri check during plug (apache#11557)

* kvm: add ssvm storage nic null uri check during plug

Fixes apache#11552

Signed-off-by: Abhishek Kumar <[email protected]>

* refactor

Signed-off-by: Abhishek Kumar <[email protected]>

---------

Signed-off-by: Abhishek Kumar <[email protected]>

Author: shwstppr

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

@@ -188,6 +188,21 @@ protected boolean isValidProtocolAndVnetId(final String vNetId, final String pro
         return vNetId != null && protocol != null && !vNetId.equalsIgnoreCase("untagged");
     }
 
+    protected String createStorageVnetBridgeIfNeeded(NicTO nic, String trafficLabel,
+                 String storageBrName) throws InternalErrorException {
+        if (!Networks.BroadcastDomainType.Storage.equals(nic.getBroadcastType()) || nic.getBroadcastUri() == null) {
+            return storageBrName;
+        }
+        String vNetId = Networks.BroadcastDomainType.getValue(nic.getBroadcastUri());
+        String protocol = Networks.BroadcastDomainType.Vlan.scheme();
+        if (!isValidProtocolAndVnetId(vNetId, protocol))  {
+            return storageBrName;
+        }
+        logger.debug(String.format("creating a vNet dev and bridge for %s traffic per traffic label %s",
+                Networks.TrafficType.Storage.name(), trafficLabel));
+        return createVnetBr(vNetId, storageBrName, protocol);
+    }
+
     @Override
     public LibvirtVMDef.InterfaceDef plug(NicTO nic, String guestOsType, String nicAdapter, Map<String, String> extraConfig) throws InternalErrorException, LibvirtException {
 
@@ -256,15 +271,7 @@ public LibvirtVMDef.InterfaceDef plug(NicTO nic, String guestOsType, String nicA
             intf.defBridgeNet(_bridges.get("private"), null, nic.getMac(), getGuestNicModel(guestOsType, nicAdapter));
         } else if (nic.getType() == Networks.TrafficType.Storage) {
             String storageBrName = nic.getName() == null ? _bridges.get("private") : nic.getName();
-            if (nic.getBroadcastType() == Networks.BroadcastDomainType.Storage) {
-                vNetId = Networks.BroadcastDomainType.getValue(nic.getBroadcastUri());
-                protocol = Networks.BroadcastDomainType.Vlan.scheme();
-            }
-            if (isValidProtocolAndVnetId(vNetId, protocol))  {
-                logger.debug(String.format("creating a vNet dev and bridge for %s traffic per traffic label %s",
-                        Networks.TrafficType.Storage.name(), trafficLabel));
-                storageBrName = createVnetBr(vNetId, storageBrName, protocol);
-            }
+            storageBrName = createStorageVnetBridgeIfNeeded(nic, trafficLabel, storageBrName);
             intf.defBridgeNet(storageBrName, null, nic.getMac(), getGuestNicModel(guestOsType, nicAdapter));
         }
         if (nic.getPxeDisable()) {
@@ -297,7 +304,7 @@ private String generateVxnetBrName(String pifName, String vnetId) {
         return "brvx-" + vnetId;
     }
 
-    private String createVnetBr(String vNetId, String pifKey, String protocol) throws InternalErrorException {
+    protected String createVnetBr(String vNetId, String pifKey, String protocol) throws InternalErrorException {
         String nic = _pifs.get(pifKey);
         if (nic == null || isVxlanOrNetris(protocol)) {
             // if not found in bridge map, maybe traffic label refers to pif already?

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

@@ -16,24 +16,29 @@
 // under the License.
 package com.cloud.hypervisor.kvm.resource;
 
+import java.net.URI;
+import java.net.URISyntaxException;
+
 import org.junit.Assert;
-import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+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 org.junit.runner.RunWith;
-import org.mockito.junit.MockitoJUnitRunner;
 
 @RunWith(MockitoJUnitRunner.class)
 public class BridgeVifDriverTest {
 
-    private BridgeVifDriver driver;
+    private static final String BRIDGE_NAME = "cloudbr1";
 
-    @Before
-    public void setUp() throws Exception {
-        driver = new BridgeVifDriver();
-    }
+    @Spy
+    @InjectMocks
+    private BridgeVifDriver driver = new BridgeVifDriver();
 
     @Test
     public void isBroadcastTypeVlanOrVxlan() {
@@ -58,4 +63,41 @@ public void isValidProtocolAndVnetId() {
         Assert.assertTrue(driver.isValidProtocolAndVnetId("123", "vlan"));
         Assert.assertTrue(driver.isValidProtocolAndVnetId("456", "vxlan"));
     }
+
+    @Test
+    public void createStorageVnetBridgeIfNeededReturnsStorageBrNameWhenBroadcastTypeIsNotStorageButValidValues() throws InternalErrorException {
+        NicTO nic = new NicTO();
+        nic.setBroadcastType(Networks.BroadcastDomainType.Storage);
+        int vlan = 123;
+        String newBridge = "br-" + vlan;
+        nic.setBroadcastUri(Networks.BroadcastDomainType.Storage.toUri(vlan));
+        Mockito.doReturn(newBridge).when(driver).createVnetBr(Mockito.anyString(), Mockito.anyString(), Mockito.anyString());
+        String result = driver.createStorageVnetBridgeIfNeeded(nic, "trafficLabel", BRIDGE_NAME);
+        Assert.assertEquals(newBridge, result);
+    }
+
+    @Test
+    public void createStorageVnetBridgeIfNeededReturnsStorageBrNameWhenBroadcastTypeIsNotStorage() throws InternalErrorException {
+        NicTO nic = new NicTO();
+        nic.setBroadcastType(Networks.BroadcastDomainType.Vlan);
+        String result = driver.createStorageVnetBridgeIfNeeded(nic, "trafficLabel", BRIDGE_NAME);
+        Assert.assertEquals(BRIDGE_NAME, result);
+    }
+
+    @Test
+    public void createStorageVnetBridgeIfNeededReturnsStorageBrNameWhenBroadcastUriIsNull() throws InternalErrorException {
+        NicTO nic = new NicTO();
+        nic.setBroadcastType(Networks.BroadcastDomainType.Storage);
+        String result = driver.createStorageVnetBridgeIfNeeded(nic,  "trafficLabel", BRIDGE_NAME);
+        Assert.assertEquals(BRIDGE_NAME, result);
+    }
+
+    @Test
+    public void createStorageVnetBridgeIfNeededCreatesVnetBridgeWhenUntaggedVlan() throws InternalErrorException, URISyntaxException {
+        NicTO nic = new NicTO();
+        nic.setBroadcastType(Networks.BroadcastDomainType.Storage);
+        nic.setBroadcastUri(new URI(Networks.BroadcastDomainType.Storage.scheme() + "://untagged"));
+        String result = driver.createStorageVnetBridgeIfNeeded(nic, "trafficLabel", BRIDGE_NAME);
+        Assert.assertEquals(BRIDGE_NAME, result);
+    }
 }