From 3680ff432b10b3d5d96a64c955761eb880cfbb5d Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Tue, 2 Sep 2025 14:01:48 +0530 Subject: [PATCH 1/2] kvm: add ssvm storage nic null uri check during plug Fixes #11552 Signed-off-by: Abhishek Kumar --- .../java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java index 0602fd6322f3..ed83aa642758 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java @@ -250,7 +250,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) { + if (Networks.BroadcastDomainType.Storage.equals(nic.getBroadcastType()) && nic.getBroadcastUri() != null) { vNetId = Networks.BroadcastDomainType.getValue(nic.getBroadcastUri()); protocol = Networks.BroadcastDomainType.Vlan.scheme(); } From 9e2e8c9d6f6e4eb3be1aaa28a80c0ea9014278f7 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Thu, 4 Sep 2025 12:35:35 +0530 Subject: [PATCH 2/2] refactor Signed-off-by: Abhishek Kumar --- .../kvm/resource/BridgeVifDriver.java | 27 +++++---- .../kvm/resource/BridgeVifDriverTest.java | 58 ++++++++++++++++--- 2 files changed, 67 insertions(+), 18 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java index ed83aa642758..3b66529ccaf7 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java @@ -184,6 +184,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 extraConfig) throws InternalErrorException, LibvirtException { @@ -250,15 +265,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 (Networks.BroadcastDomainType.Storage.equals(nic.getBroadcastType()) && nic.getBroadcastUri() != null) { - 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()) { @@ -291,7 +298,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 || protocol.equals(Networks.BroadcastDomainType.Vxlan.scheme())) { // if not found in bridge map, maybe traffic label refers to pif already? diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriverTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriverTest.java index 48bba2b78ed2..00364948f828 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriverTest.java +++ b/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); + } }