Allocate NIC device id atomically to stop duplicate ethN on a VM

createNicForVm() reads the next free device id and persists the nic in two
separate steps:

    int deviceId = _nicDao.getFreeDeviceId(vm.getId());   // reads existing nics
    ...
    allocateNic(..., deviceId, ...);                       // persists much later

getFreeDeviceId() has no lock, and the nic row it is choosing an id for is not
written until the end of allocateNic(). Two nics added to the same VM at once
therefore both read the same free id and land on the same ethN.

This happens when several tiers of a VPC are brought up in parallel: each tier
attaches the *shared* redundant VR, so N concurrent createNicForVm() calls run
against one router. The result is two guest nics on one ethN (one network
blackholed) and, because the two routers of a redundant VPC end up with
different device -> network maps, divergent keepalived configs -> both routers
go PRIMARY and the VPC is dead. (issue #11710)

Fix: compute the free device id and persist the nic under the vm_instance row
lock, so device id assignment is atomic per VM. This is the same shape as the
existing persistNicAfterRaceCheck() IPv4 race guard and needs no change to job
dispatch. Serializing per router is what the non-concurrent path already does
implicitly (one tier at a time).

Fixes: #11710

Signed-off-by: Brad House <bhouse@nexthop.ai>

Author: bhouse-nexthop

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

@@ -237,6 +237,7 @@
 import com.cloud.utils.db.Transaction;
 import com.cloud.utils.db.TransactionCallback;
 import com.cloud.utils.db.TransactionCallbackNoReturn;
+import com.cloud.utils.db.TransactionCallbackWithException;
 import com.cloud.utils.db.TransactionCallbackWithExceptionNoReturn;
 import com.cloud.utils.db.TransactionStatus;
 import com.cloud.utils.exception.CloudRuntimeException;
@@ -1141,6 +1142,29 @@ private NicVO checkForRaceAndAllocateNic(final NicProfile requested, final Netwo
         return vo;
     }
 
+    /**
+     * Allocate a nic for {@code vm} on {@code network}, choosing its device id atomically.
+     *
+     * {@link NicDao#getFreeDeviceId(long)} picks the first unused device id by reading the vm's
+     * existing nics, but the new nic row is not persisted until the end of {@link #allocateNic}. Two
+     * nics being added to the same vm concurrently (e.g. several tiers of a VPC brought up in parallel,
+     * each attaching the shared redundant VR) would otherwise both read the same free id and land on
+     * the same {@code ethN} — corrupting the VR config and, for a redundant VPC, driving both routers
+     * PRIMARY (issue #11710). Holding the {@code vm_instance} row lock across the read-and-persist makes
+     * the device id assignment atomic per vm.
+     */
+    protected NicProfile allocateNicWithFreeDeviceId(final NicProfile requested, final Network network, final boolean isDefaultNic, final VirtualMachineProfile vm)
+            throws InsufficientCapacityException, ConcurrentOperationException {
+        return Transaction.execute(new TransactionCallbackWithException<NicProfile, InsufficientCapacityException>() {
+            @Override
+            public NicProfile doInTransaction(final TransactionStatus status) throws InsufficientCapacityException {
+                _vmDao.lockRow(vm.getId(), true);
+                final int deviceId = _nicDao.getFreeDeviceId(vm.getId());
+                return allocateNic(requested, network, isDefaultNic, deviceId, vm).first();
+            }
+        });
+    }
+
     @DB
     @Override
     public Pair<NicProfile, Integer> allocateNic(final NicProfile requested, final Network network, final Boolean isDefaultNic, int deviceId, final VirtualMachineProfile vm)
@@ -4510,11 +4534,9 @@ public NicProfile createNicForVm(final Network network, final NicProfile request
 
         //1) allocate nic (if needed) Always allocate if it is a user vm
         if (nic == null || vmProfile.getType() == VirtualMachine.Type.User) {
-            final int deviceId = _nicDao.getFreeDeviceId(vm.getId());
-
-            boolean isDefaultNic = getNicProfileDefaultNic(requested);
+            final boolean isDefaultNic = getNicProfileDefaultNic(requested);
 
-            nic = allocateNic(requested, network, isDefaultNic, deviceId, vmProfile).first();
+            nic = allocateNicWithFreeDeviceId(requested, network, isDefaultNic, vmProfile);
 
             if (nic == null) {
                 throw new CloudRuntimeException("Failed to allocate nic for Instance " + vm + " in network " + network);

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

@@ -41,6 +41,7 @@
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 import org.mockito.ArgumentMatchers;
+import org.mockito.InOrder;
 import org.mockito.MockedStatic;
 import org.mockito.Mockito;
 
@@ -76,6 +77,8 @@
 import com.cloud.utils.db.EntityManager;
 import com.cloud.utils.db.Transaction;
 import com.cloud.utils.db.TransactionCallback;
+import com.cloud.utils.db.TransactionCallbackWithException;
+import com.cloud.utils.db.TransactionStatus;
 import com.cloud.utils.exception.CloudRuntimeException;
 import com.cloud.utils.net.Ip;
 import com.cloud.vm.DomainRouterVO;
@@ -88,6 +91,7 @@
 import com.cloud.vm.VirtualMachineProfile;
 import com.cloud.vm.dao.DomainRouterDao;
 import com.cloud.vm.dao.NicDao;
+import com.cloud.vm.dao.VMInstanceDao;
 import com.cloud.vm.dao.NicExtraDhcpOptionDao;
 import com.cloud.vm.dao.NicIpAliasDao;
 import com.cloud.vm.dao.NicSecondaryIpDao;
@@ -135,6 +139,7 @@ public void setUp() {
         testOrchestrator.routerJoinDao = mock(DomainRouterJoinDao.class);
         testOrchestrator._ipAddrMgr = mock(IpAddressManager.class);
         testOrchestrator._entityMgr = mock(EntityManager.class);
+        testOrchestrator._vmDao = mock(VMInstanceDao.class);
         DhcpServiceProvider provider = mock(DhcpServiceProvider.class);
 
         Map<Network.Capability, String> capabilities = new HashMap<Network.Capability, String>();
@@ -1010,4 +1015,43 @@ public void testImportNicWithIP4Address() throws Exception {
             assertEquals("testtag", nicProfile.getName());
         }
     }
+
+    @Test
+    public void testAllocateNicWithFreeDeviceIdLocksVmRowBeforeReadingFreeDeviceId() throws Exception {
+        final long vmId = 100L;
+        final int freeDeviceId = 5;
+
+        VirtualMachine vm = mock(VirtualMachine.class);
+        when(vm.getId()).thenReturn(vmId);
+        VirtualMachineProfile vmProfile = mock(VirtualMachineProfile.class);
+        when(vmProfile.getId()).thenReturn(vmId);
+        when(vmProfile.getVirtualMachine()).thenReturn(vm);
+
+        Network network = mock(Network.class);
+        NicProfile requested = mock(NicProfile.class);
+        NicProfile allocated = mock(NicProfile.class);
+
+        when(testOrchestrator._nicDao.getFreeDeviceId(vmId)).thenReturn(freeDeviceId);
+        Mockito.doReturn(new Pair<>(allocated, freeDeviceId)).when(testOrchestrator)
+                .allocateNic(requested, network, false, freeDeviceId, vmProfile);
+
+        try (MockedStatic<Transaction> transactionMocked = Mockito.mockStatic(Transaction.class)) {
+            // run the callback body so the lock/read/allocate ordering is exercised
+            transactionMocked.when(() -> Transaction.execute(any(TransactionCallbackWithException.class)))
+                    .thenAnswer(invocation -> {
+                        TransactionCallbackWithException<NicProfile, InsufficientCapacityException> cb = invocation.getArgument(0);
+                        return cb.doInTransaction(mock(TransactionStatus.class));
+                    });
+
+            NicProfile result = testOrchestrator.allocateNicWithFreeDeviceId(requested, network, false, vmProfile);
+
+            assertEquals(allocated, result);
+            // the vm_instance row must be locked before the free device id is read,
+            // otherwise two concurrent adds pick the same id (issue #11710)
+            InOrder inOrder = Mockito.inOrder(testOrchestrator._vmDao, testOrchestrator._nicDao);
+            inOrder.verify(testOrchestrator._vmDao).lockRow(vmId, true);
+            inOrder.verify(testOrchestrator._nicDao).getFreeDeviceId(vmId);
+            verify(testOrchestrator, times(1)).allocateNic(requested, network, false, freeDeviceId, vmProfile);
+        }
+    }
 }