reject a trunk nic with a VLAN 1 nic on one VM, and only treat Guest VLAN 1 nics as needing a legacy host, including in host planning

Author: Pearl1594

core/src/main/java/com/cloud/agent/api/VlanTrunkMigrationHelper.java

@@ -17,6 +17,7 @@
 package com.cloud.agent.api;
 
 import java.net.URI;
+import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 
@@ -73,11 +74,11 @@ public static boolean vmNeedsPostMigrationVlanTrunkMembership(long vmId, long ds
         return nicDao.listByVmId(vmId).stream().anyMatch(nic -> nicNeedsManualVlanMembership(nic, destVlanFilteringEnabled));
     }
 
-    // A multi-VLAN trunk nic needs a VLAN-filtering host; a VLAN 1 nic needs a legacy host, as the shared bridge's PVID is VLAN 1. Null when the host fits.
+    // A multi-VLAN trunk nic needs a VLAN-filtering host; a Guest VLAN 1 nic needs a legacy host, as the shared bridge's PVID is VLAN 1. Null when the host fits.
     public static String getVlanFilteringUnsuitableReason(long vmId, long hostId, NicDao nicDao, NetworkDao networkDao, HostDetailsDao hostDetailsDao) {
         List<NicVO> nics = nicDao.listByVmId(vmId);
-        boolean hasTrunkNic = nics.stream().anyMatch(NicVO::getMultiNetwork);
-        boolean hasNativeVlanNic = nics.stream().anyMatch(nic -> !nic.getMultiNetwork() && isNativeVlan(getEffectiveBroadcastUri(nic, networkDao)));
+        boolean hasTrunkNic = hasTrunkNic(nics);
+        boolean hasNativeVlanNic = hasNativeVlanGuestNic(nics, networkDao);
         if (!hasTrunkNic && !hasNativeVlanNic) {
             return null;
         }
@@ -93,7 +94,20 @@ public static String getVlanFilteringUnsuitableReason(long vmId, long hostId, Ni
 
     // For a network about to be plugged into a running Instance on this host.
     public static String getNativeVlanUnsuitableReason(Network network, long hostId, HostDetailsDao hostDetailsDao) {
-        return isNativeVlan(network.getBroadcastUri()) && isVlanFilteringEnabled(hostId, hostDetailsDao) ? NATIVE_VLAN_ON_FILTERING_HOST_REASON : null;
+        return isNativeVlanGuestNetwork(network) && isVlanFilteringEnabled(hostId, hostDetailsDao) ? NATIVE_VLAN_ON_FILTERING_HOST_REASON : null;
+    }
+
+    public static boolean hasTrunkNic(Collection<NicVO> nics) {
+        return nics.stream().anyMatch(NicVO::getMultiNetwork);
+    }
+
+    // Only Guest nics ride the shared bridge; Public, Management and Storage nics use their own bridges.
+    public static boolean hasNativeVlanGuestNic(Collection<NicVO> nics, NetworkDao networkDao) {
+        return nics.stream().anyMatch(nic -> isNativeVlanGuestNic(nic, networkDao));
+    }
+
+    public static boolean isNativeVlanGuestNetwork(Network network) {
+        return network.getTrafficType() == Networks.TrafficType.Guest && isNativeVlan(network.getBroadcastUri());
     }
 
     private static boolean isVlanFilteringEnabled(long hostId, HostDetailsDao hostDetailsDao) {
@@ -102,12 +116,19 @@ private static boolean isVlanFilteringEnabled(long hostId, HostDetailsDao hostDe
     }
 
     // a stopped Instance's nics carry no broadcast URI (released on stop), so fall back to the network's
-    private static URI getEffectiveBroadcastUri(NicVO nic, NetworkDao networkDao) {
-        if (nic.getBroadcastUri() != null) {
-            return nic.getBroadcastUri();
+    private static boolean isNativeVlanGuestNic(NicVO nic, NetworkDao networkDao) {
+        if (nic.getMultiNetwork()) {
+            return false;
+        }
+        URI nicBroadcastUri = nic.getBroadcastUri();
+        if (nicBroadcastUri != null && !isNativeVlan(nicBroadcastUri)) {
+            return false;
         }
         NetworkVO network = networkDao.findById(nic.getNetworkId());
-        return network == null ? null : network.getBroadcastUri();
+        if (network == null || network.getTrafficType() != Networks.TrafficType.Guest) {
+            return false;
+        }
+        return isNativeVlan(nicBroadcastUri != null ? nicBroadcastUri : network.getBroadcastUri());
     }
 
     private static boolean isNativeVlan(URI broadcastUri) {

core/src/test/java/com/cloud/agent/api/VlanTrunkMigrationHelperTest.java

@@ -22,6 +22,7 @@
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
@@ -283,10 +284,20 @@ public void sendPostMigrationVlanTrunkMembershipIfNeededWarnsOnSendException() t
     // -- getVlanFilteringUnsuitableReason --
 
     private NicVO nicOnVlan(int vlan, boolean multiNetwork) {
+        return nicOnVlan(vlan, multiNetwork, Networks.TrafficType.Guest);
+    }
+
+    private NicVO nicOnVlan(int vlan, boolean multiNetwork, Networks.TrafficType trafficType) {
         URI broadcastUri = Networks.BroadcastDomainType.Vlan.toUri(vlan);
         NicVO nic = mock(NicVO.class);
         when(nic.getBroadcastUri()).thenReturn(broadcastUri);
         when(nic.getMultiNetwork()).thenReturn(multiNetwork);
+        if (!multiNetwork) {
+            lenient().when(nic.getNetworkId()).thenReturn(5L);
+            NetworkVO network = mock(NetworkVO.class);
+            lenient().when(network.getTrafficType()).thenReturn(trafficType);
+            lenient().when(networkDao.findById(5L)).thenReturn(network);
+        }
         return nic;
     }
 
@@ -340,6 +351,7 @@ public void getVlanFilteringUnsuitableReasonUsesNetworkVlanForStoppedNicWithoutB
         NetworkVO network = mock(NetworkVO.class);
         URI vlanOne = Networks.BroadcastDomainType.Vlan.toUri(1);
         when(network.getBroadcastUri()).thenReturn(vlanOne);
+        when(network.getTrafficType()).thenReturn(Networks.TrafficType.Guest);
         when(networkDao.findById(5L)).thenReturn(network);
         givenHostVlanFiltering(true);
 
@@ -351,10 +363,62 @@ public void getNativeVlanUnsuitableReasonRejectsVlanOneNetworkOnFilteringHostOnl
         Network network = mock(Network.class);
         URI vlanOne = Networks.BroadcastDomainType.Vlan.toUri(1);
         when(network.getBroadcastUri()).thenReturn(vlanOne);
+        when(network.getTrafficType()).thenReturn(Networks.TrafficType.Guest);
         givenHostVlanFiltering(true);
         assertTrue(VlanTrunkMigrationHelper.getNativeVlanUnsuitableReason(network, DEST_HOST_ID, hostDetailsDao).contains("VLAN 1"));
 
         givenHostVlanFiltering(false);
         assertNull(VlanTrunkMigrationHelper.getNativeVlanUnsuitableReason(network, DEST_HOST_ID, hostDetailsDao));
     }
+
+    @Test
+    public void getVlanFilteringUnsuitableReasonIgnoresVlanOnePublicNic() {
+        List<NicVO> nics = Collections.singletonList(nicOnVlan(1, false, Networks.TrafficType.Public));
+        when(nicDao.listByVmId(VM_ID)).thenReturn(nics);
+
+        assertNull(VlanTrunkMigrationHelper.getVlanFilteringUnsuitableReason(VM_ID, DEST_HOST_ID, nicDao, networkDao, hostDetailsDao));
+    }
+
+    @Test
+    public void getNativeVlanUnsuitableReasonIgnoresVlanOneNonGuestNetwork() {
+        Network network = mock(Network.class);
+        when(network.getTrafficType()).thenReturn(Networks.TrafficType.Public);
+
+        assertNull(VlanTrunkMigrationHelper.getNativeVlanUnsuitableReason(network, DEST_HOST_ID, hostDetailsDao));
+    }
+
+    @Test
+    public void hasNativeVlanGuestNicIsTrueOnlyForGuestVlanOne() {
+        assertTrue(VlanTrunkMigrationHelper.hasNativeVlanGuestNic(Collections.singletonList(nicOnVlan(1, false)), networkDao));
+        assertFalse(VlanTrunkMigrationHelper.hasNativeVlanGuestNic(Collections.singletonList(nicOnVlan(100, false)), networkDao));
+    }
+
+    @Test
+    public void isNativeVlanGuestNetworkIsTrueOnlyForGuestVlanOne() {
+        Network guestVlanOne = mock(Network.class);
+        when(guestVlanOne.getTrafficType()).thenReturn(Networks.TrafficType.Guest);
+        when(guestVlanOne.getBroadcastUri()).thenReturn(Networks.BroadcastDomainType.Vlan.toUri(1));
+        Network guestOtherVlan = mock(Network.class);
+        when(guestOtherVlan.getTrafficType()).thenReturn(Networks.TrafficType.Guest);
+        when(guestOtherVlan.getBroadcastUri()).thenReturn(Networks.BroadcastDomainType.Vlan.toUri(100));
+
+        assertTrue(VlanTrunkMigrationHelper.isNativeVlanGuestNetwork(guestVlanOne));
+        assertFalse(VlanTrunkMigrationHelper.isNativeVlanGuestNetwork(guestOtherVlan));
+    }
+
+    @Test
+    public void hasTrunkNicDetectsMultiNetworkNic() {
+        assertTrue(VlanTrunkMigrationHelper.hasTrunkNic(Collections.singletonList(nicOnVlan(100, true))));
+        assertFalse(VlanTrunkMigrationHelper.hasTrunkNic(Collections.singletonList(nicOnVlan(100, false))));
+    }
+
+    @Test
+    public void getVlanFilteringUnsuitableReasonDoesNotLookUpNetworksOrHostForNicsOnOtherVlans() {
+        List<NicVO> nics = Collections.singletonList(nicOnVlan(100, false));
+        when(nicDao.listByVmId(VM_ID)).thenReturn(nics);
+
+        assertNull(VlanTrunkMigrationHelper.getVlanFilteringUnsuitableReason(VM_ID, DEST_HOST_ID, nicDao, networkDao, hostDetailsDao));
+        verify(networkDao, never()).findById(any(Long.class));
+        verify(hostDetailsDao, never()).findDetail(any(Long.class), any(String.class));
+    }
 }

server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java

@@ -73,6 +73,7 @@
 import com.cloud.agent.api.Command;
 import com.cloud.agent.api.StartupCommand;
 import com.cloud.agent.api.StartupRoutingCommand;
+import com.cloud.agent.api.VlanTrunkMigrationHelper;
 import com.cloud.agent.manager.allocator.HostAllocator;
 import com.cloud.capacity.CapacityManager;
 import com.cloud.capacity.dao.CapacityDao;
@@ -151,6 +152,7 @@
 import com.cloud.vm.VirtualMachine.Event;
 import com.cloud.vm.VirtualMachine.State;
 import com.cloud.vm.VirtualMachineProfile;
+import com.cloud.network.dao.NetworkDao;
 import com.cloud.vm.NicVO;
 import com.cloud.vm.dao.NicDao;
 import com.cloud.vm.dao.UserVmDao;
@@ -192,6 +194,8 @@ public class DeploymentPlanningManagerImpl extends ManagerBase implements Deploy
     private VMTemplateDao templateDao;
     @Inject
     protected NicDao _nicDao;
+    @Inject
+    protected NetworkDao _networkDao;
 
     private static final long ADMIN_ACCOUNT_ROLE_ID = 1l;
     private static final long INITIAL_RESERVATION_RELEASE_CHECKER_DELAY = 30L * 1000L; // thirty seconds expressed in milliseconds
@@ -484,22 +488,27 @@ private void avoidDifferentArchResources(VirtualMachineProfile vmProfile, DataCe
      */
     @Override
     public void avoidHostsNotReadyForMultiNetworkNics(VirtualMachineProfile vmProfile, DataCenter dc, ExcludeList avoids) {
-        boolean hasMultiNetworkNic = _nicDao.listByVmId(vmProfile.getId()).stream().anyMatch(NicVO::getMultiNetwork);
-        if (!hasMultiNetworkNic) {
+        List<NicVO> nics = _nicDao.listByVmId(vmProfile.getId());
+        boolean needsFilteringHost = VlanTrunkMigrationHelper.hasTrunkNic(nics);
+        boolean needsLegacyHost = !needsFilteringHost && VlanTrunkMigrationHelper.hasNativeVlanGuestNic(nics, _networkDao);
+        if (!needsFilteringHost && !needsLegacyHost) {
+            return;
+        }
+        Set<Long> filteringHostIds = _hostDetailsDao.findHostIdsWithVlanFilteringEnabled();
+        // without VLAN-filtering hosts a VLAN 1 nic is placeable anywhere
+        if (needsLegacyHost && filteringHostIds.isEmpty()) {
             return;
         }
         List<Long> candidateHostIds = _hostDao.listEnabledIdsByDataCenterId(dc.getId());
         if (CollectionUtils.isEmpty(candidateHostIds)) {
             return;
         }
-        Set<Long> readyHostIds = _hostDetailsDao.findHostIdsWithVlanFilteringEnabled();
-        List<Long> notReadyHostIds = candidateHostIds.stream()
-                .filter(hostId -> !readyHostIds.contains(hostId))
+        List<Long> unsuitableHostIds = candidateHostIds.stream()
+                .filter(hostId -> filteringHostIds.contains(hostId) ? needsLegacyHost : needsFilteringHost)
                 .collect(Collectors.toList());
-        if (!notReadyHostIds.isEmpty()) {
-            logger.debug("Excluding {} host(s) without VLAN filtering enabled, since VM [{}] has a multi-VLAN trunk nic",
-                    notReadyHostIds.size(), vmProfile);
-            avoids.addHostList(notReadyHostIds);
+        if (!unsuitableHostIds.isEmpty()) {
+            logger.debug("Excluding {} host(s) unsuitable for the multi-VLAN trunk or VLAN 1 nics of VM [{}]", unsuitableHostIds.size(), vmProfile);
+            avoids.addHostList(unsuitableHostIds);
         }
     }
 

server/src/main/java/com/cloud/network/NetworkServiceImpl.java

@@ -39,6 +39,7 @@
 import java.util.Set;
 import java.util.TreeSet;
 import java.util.UUID;
+import java.util.stream.Collectors;
 
 import javax.inject.Inject;
 import javax.naming.ConfigurationException;
@@ -1082,6 +1083,7 @@ private List<NetworkVO> associateNetworksInternal(NicVO nic, VirtualMachine vm,
 
             validateTrunkSet(primaryNetwork, resultingSet);
             resultingSet.forEach(this::validateNotNativeVlan);
+            validateNoNativeVlanOnOtherNics(vm, nic);
             validateNoOverlappingCidrs(resultingSet);
             // defer implement to the Instance's next start when stopped, like its primary network; a running Instance needs it now
             boolean vmRunning = vm.getState() == VirtualMachine.State.Running;
@@ -1685,6 +1687,15 @@ private void validateNoDuplicateVlan(List<NetworkVO> resultingSet, Set<Long> jus
         }
     }
 
+    // A trunk nic needs a VLAN-filtering host and a Guest VLAN 1 nic a legacy one, so the Instance could start nowhere.
+    private void validateNoNativeVlanOnOtherNics(VirtualMachine vm, NicVO nic) {
+        List<NicVO> otherNics = _nicDao.listByVmId(vm.getId()).stream().filter(other -> other.getId() != nic.getId()).collect(Collectors.toList());
+        if (VlanTrunkMigrationHelper.hasNativeVlanGuestNic(otherNics, _networksDao)) {
+            throw new InvalidParameterValueException(String.format(
+                    "Instance %s has a nic on a VLAN 1 network, which cannot be combined with a trunk nic", vm.getUuid()));
+        }
+    }
+
     // VLAN 1 is the host uplink's PVID (egress untagged), so it is not isolated from untagged networks
     private void validateNotNativeVlan(NetworkVO network) {
         URI broadcastUri = network.getBroadcastUri();

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

@@ -1576,6 +1576,7 @@ public UserVm addNicToVirtualMachine(AddNicToVMCmd cmd) throws InvalidParameterV
         }
 
         failIfNativeVlanOnVlanFilteringHost(vmInstance, network);
+        failIfNativeVlanOnVmWithTrunkNic(vmInstance, network);
 
         setNicAsDefaultIfNeeded(vmInstance, profile);
 
@@ -6811,6 +6812,7 @@ public UserVm createVirtualMachine(DeployVMCmd cmd) throws InsufficientCapacityE
             throw new InvalidParameterValueException("Unable to use template " + templateId);
         }
         verifyTemplate(cmd, template, serviceOfferingId);
+        validateNoNativeVlanNicWithTrunkNic(cmd);
 
         Long diskOfferingId = cmd.getDiskOfferingId();
         DiskOffering diskOffering = null;
@@ -7863,6 +7865,28 @@ private void failIfNativeVlanOnVlanFilteringHost(UserVmVO vm, Network network) {
         }
     }
 
+    // A trunk nic needs a VLAN-filtering host and a Guest VLAN 1 nic a legacy one, so the Instance could start nowhere.
+    private void failIfNativeVlanOnVmWithTrunkNic(UserVmVO vm, Network network) {
+        if (VlanTrunkMigrationHelper.isNativeVlanGuestNetwork(network) && VlanTrunkMigrationHelper.hasTrunkNic(_nicDao.listByVmId(vm.getId()))) {
+            throw new InvalidParameterValueException(String.format("Cannot add network %s to Instance %s: it is on VLAN 1, the native VLAN of the host uplink, "
+                    + "and the Instance has a trunk nic", network.getUuid(), vm.getUuid()));
+        }
+    }
+
+    private void validateNoNativeVlanNicWithTrunkNic(BaseDeployVMCmd cmd) {
+        List<BaseDeployVMCmd.NicNetworkGrouping> groupings = cmd.getNicNetworksList();
+        if (groupings.stream().allMatch(grouping -> grouping.getAssociatedNetworkIds().isEmpty())) {
+            return;
+        }
+        for (BaseDeployVMCmd.NicNetworkGrouping grouping : groupings) {
+            Network primaryNetwork = _networkDao.findById(grouping.getPrimaryNetworkId());
+            if (primaryNetwork != null && VlanTrunkMigrationHelper.isNativeVlanGuestNetwork(primaryNetwork)) {
+                throw new InvalidParameterValueException(String.format("Network %s is on VLAN 1, the native VLAN of the host uplink, "
+                        + "and cannot be on an Instance that has a trunk nic", primaryNetwork.getUuid()));
+            }
+        }
+    }
+
     private String getVlanFilteringUnsuitableReason(VMInstanceVO vm, Host host) {
         return VlanTrunkMigrationHelper.getVlanFilteringUnsuitableReason(vm.getId(), host.getId(), _nicDao, _networkDao, _hostDetailsDao);
     }
@@ -10290,6 +10314,7 @@ public UserVm allocateVMFromBackup(CreateVMFromBackupCmd cmd) throws Insufficien
             }
         }
         verifyTemplate(cmd, template, serviceOffering.getId());
+        validateNoNativeVlanNicWithTrunkNic(cmd);
 
         Long size = cmd.getSize();