From c65369ed320c08d4d999f072d0579922d36aa020 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 10 Sep 2026 22:34:08 +0000 Subject: [PATCH 1/2] 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 --- .../orchestration/NetworkOrchestrator.java | 30 +++++++++++-- .../NetworkOrchestratorTest.java | 44 +++++++++++++++++++ 2 files changed, 70 insertions(+), 4 deletions(-) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java index c6968cddd2f5..16c0c63c27f8 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java +++ b/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() { + @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 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); diff --git a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java index e3989737112d..bbe59aa01043 100644 --- a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java +++ b/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 capabilities = new HashMap(); @@ -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 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 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); + } + } } From a6762e94b8abca898d8dc6acc8e2e05a2d8d0c52 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 1 Oct 2026 20:30:46 +0000 Subject: [PATCH 2/2] Take the nic device id lock only for the write of the nic The previous commit held the vm_instance row lock in a transaction that spanned the whole of allocateNic(). That made the device id atomic, but it also pulled persistNicAfterRaceCheck() into the outer transaction, so a new nic was no longer committed when it was written but only once the rest of the allocation had finished. Until then its guest ip is invisible to other vms in the network, both to the guru picking a free ip and to the race check that is meant to catch two nics taking the same one. The vm row lock does nothing for that, as it only serialises nics of one vm. Choose the device id where the nic is written instead. persistNic() runs the ip race check, takes the vm row lock, reads the free device id and persists the nic in one short transaction, as persistNicAfterRaceCheck() did before, so the nic is committed as soon as it exists. allocateNicWithFreeDeviceId() asks for that with FREE_DEVICE_ID and no longer opens a transaction of its own. Signed-off-by: Brad House --- .../orchestration/NetworkOrchestrator.java | 69 ++++++++------ .../NetworkOrchestratorTest.java | 95 ++++++++++++++----- 2 files changed, 109 insertions(+), 55 deletions(-) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java index 16c0c63c27f8..1c22213b34fa 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java @@ -237,7 +237,6 @@ 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; @@ -277,6 +276,12 @@ */ public class NetworkOrchestrator extends ManagerBase implements NetworkOrchestrationService, Listener, Configurable { + /** + * Device id that asks {@link #allocateNic} to put the nic on the first device id the vm has free, + * chosen atomically with the write of the nic. See {@link #allocateNicWithFreeDeviceId}. + */ + protected static final int FREE_DEVICE_ID = -1; + @Inject EntityManager _entityMgr; @Inject @@ -1064,18 +1069,36 @@ public void saveExtraDhcpOptions(final String networkUuid, final Long nicId, fin } } - private NicVO persistNicAfterRaceCheck(final NicVO nic, final Long networkId, final NicProfile profile, int deviceId) { + /** + * Persist {@code nic} in a transaction of its own, so it is committed, and visible to other + * allocations, as soon as it is written. + * + * With {@link NicProfile#getIpv4AllocationRaceCheck()} set, the guru picked the ip from the + * committed nics of the network, so the nic is only written if no other nic has taken that ip in + * the meantime; null is returned otherwise and the caller allocates again. + * + * With {@code deviceId} set to {@link #FREE_DEVICE_ID}, the first unused device id of the vm is + * chosen here, under the {@code vm_instance} row lock and in the same transaction as the write, + * so that two nics added to one vm at the same time cannot both take it (issue #11710). The lock + * is held only for the read and the write: holding it across the whole allocation would leave + * the nic uncommitted, and its ip invisible to the race check of other vms in the network, for + * as long as the rest of the allocation takes. + */ + protected NicVO persistNic(final NicVO nic, final long networkId, final NicProfile profile, final int deviceId, final long vmId) { return Transaction.execute(new TransactionCallback<>() { @Override public NicVO doInTransaction(TransactionStatus status) { - NicVO vo = _nicDao.findNonPlaceHolderByIp4AddressAndNetworkId(profile.getIPv4Address(), networkId); - if (vo == null) { - applyProfileToNic(nic, profile, deviceId); - vo = _nicDao.persist(nic); - return vo; - } else { + if (profile.getIpv4AllocationRaceCheck() && + _nicDao.findNonPlaceHolderByIp4AddressAndNetworkId(profile.getIPv4Address(), networkId) != null) { return null; } + int nicDeviceId = deviceId; + if (deviceId == FREE_DEVICE_ID) { + _vmDao.lockRow(vmId, true); + nicDeviceId = _nicDao.getFreeDeviceId(vmId); + } + applyProfileToNic(nic, profile, nicDeviceId); + return _nicDao.persist(nic); } }); } @@ -1122,12 +1145,7 @@ private NicVO checkForRaceAndAllocateNic(final NicProfile requested, final Netwo configureNicProfileBasedOnRequestedIp(requested, profile, network); } - if (profile.getIpv4AllocationRaceCheck()) { - vo = persistNicAfterRaceCheck(vo, network.getId(), profile, deviceId); - } else { - applyProfileToNic(vo, profile, deviceId); - vo = _nicDao.persist(vo); - } + vo = persistNic(vo, network.getId(), profile, deviceId, vm.getId()); if (vo == null) { if (requested.getRequestedIPv4() != null) { @@ -1143,26 +1161,19 @@ private NicVO checkForRaceAndAllocateNic(final NicProfile requested, final Netwo } /** - * Allocate a nic for {@code vm} on {@code network}, choosing its device id atomically. + * Allocate a nic for {@code vm} on {@code network} on the first device id the vm has free. * * {@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. + * existing nics. Read before the allocation, as it used to be, 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 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). Passing + * {@link #FREE_DEVICE_ID} has {@link #persistNic} choose the id under the {@code vm_instance} row + * lock, in the transaction that writes the nic. */ protected NicProfile allocateNicWithFreeDeviceId(final NicProfile requested, final Network network, final boolean isDefaultNic, final VirtualMachineProfile vm) throws InsufficientCapacityException, ConcurrentOperationException { - return Transaction.execute(new TransactionCallbackWithException() { - @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(); - } - }); + return allocateNic(requested, network, isDefaultNic, FREE_DEVICE_ID, vm).first(); } @DB diff --git a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java index bbe59aa01043..5bbe3c50a7c7 100644 --- a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java +++ b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java @@ -18,6 +18,8 @@ import static org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService.NetworkLockTimeout; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -77,7 +79,6 @@ 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; @@ -1017,41 +1018,83 @@ public void testImportNicWithIP4Address() throws Exception { } @Test - public void testAllocateNicWithFreeDeviceIdLocksVmRowBeforeReadingFreeDeviceId() throws Exception { - final long vmId = 100L; - final int freeDeviceId = 5; - - VirtualMachine vm = mock(VirtualMachine.class); - when(vm.getId()).thenReturn(vmId); + public void testAllocateNicWithFreeDeviceIdLeavesDeviceIdToThePersist() throws Exception { 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); + Mockito.doReturn(new Pair<>(allocated, NetworkOrchestrator.FREE_DEVICE_ID)).when(testOrchestrator) + .allocateNic(requested, network, false, NetworkOrchestrator.FREE_DEVICE_ID, vmProfile); - when(testOrchestrator._nicDao.getFreeDeviceId(vmId)).thenReturn(freeDeviceId); - Mockito.doReturn(new Pair<>(allocated, freeDeviceId)).when(testOrchestrator) - .allocateNic(requested, network, false, freeDeviceId, vmProfile); + try (MockedStatic transactionMocked = Mockito.mockStatic(Transaction.class)) { + assertEquals(allocated, testOrchestrator.allocateNicWithFreeDeviceId(requested, network, false, vmProfile)); + + // No transaction may be held across the allocation: the nic has to be committed as soon as + // it is written, or its ip stays invisible to the race check of other vms in the network. + transactionMocked.verifyNoInteractions(); + verify(testOrchestrator._nicDao, never()).getFreeDeviceId(anyLong()); + } + } + private NicVO persistNicInTransaction(NicVO nic, NicProfile profile, int deviceId, long vmId) { + // a mocked Integer getter returns 0, which would read as a device id the profile asks for + when(profile.getDeviceId()).thenReturn(null); try (MockedStatic transactionMocked = Mockito.mockStatic(Transaction.class)) { - // run the callback body so the lock/read/allocate ordering is exercised - transactionMocked.when(() -> Transaction.execute(any(TransactionCallbackWithException.class))) + transactionMocked.when(() -> Transaction.execute(any(TransactionCallback.class))) .thenAnswer(invocation -> { - TransactionCallbackWithException cb = invocation.getArgument(0); - return cb.doInTransaction(mock(TransactionStatus.class)); + TransactionCallback callback = invocation.getArgument(0); + return callback.doInTransaction(mock(TransactionStatus.class)); }); + NicVO persisted = testOrchestrator.persistNic(nic, 10L, profile, deviceId, vmId); + transactionMocked.verify(() -> Transaction.execute(any(TransactionCallback.class)), times(1)); + return persisted; + } + } - NicProfile result = testOrchestrator.allocateNicWithFreeDeviceId(requested, network, false, vmProfile); + @Test + public void testPersistNicChoosesFreeDeviceIdUnderVmRowLock() { + final long vmId = 100L; + final int freeDeviceId = 5; + NicVO nic = mock(NicVO.class); + NicProfile profile = mock(NicProfile.class); + when(testOrchestrator._nicDao.getFreeDeviceId(vmId)).thenReturn(freeDeviceId); + when(testOrchestrator._nicDao.persist(nic)).thenReturn(nic); - 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); - } + assertEquals(nic, persistNicInTransaction(nic, profile, NetworkOrchestrator.FREE_DEVICE_ID, vmId)); + + // the vm_instance row must be locked before the free device id is read, and the nic written in + // the same transaction, otherwise two concurrent adds pick the same id (issue #11710) + InOrder inOrder = Mockito.inOrder(testOrchestrator._vmDao, testOrchestrator._nicDao, nic); + inOrder.verify(testOrchestrator._vmDao).lockRow(vmId, true); + inOrder.verify(testOrchestrator._nicDao).getFreeDeviceId(vmId); + inOrder.verify(nic).setDeviceId(freeDeviceId); + inOrder.verify(testOrchestrator._nicDao).persist(nic); + } + + @Test + public void testPersistNicKeepsGivenDeviceIdWithoutLocking() { + NicVO nic = mock(NicVO.class); + NicProfile profile = mock(NicProfile.class); + when(testOrchestrator._nicDao.persist(nic)).thenReturn(nic); + + assertEquals(nic, persistNicInTransaction(nic, profile, 2, 100L)); + + verify(nic).setDeviceId(2); + verify(testOrchestrator._vmDao, never()).lockRow(anyLong(), anyBoolean()); + verify(testOrchestrator._nicDao, never()).getFreeDeviceId(anyLong()); + } + + @Test + public void testPersistNicReturnsNullWhenRaceCheckFindsIpTaken() { + NicVO nic = mock(NicVO.class); + NicProfile profile = mock(NicProfile.class); + when(profile.getIpv4AllocationRaceCheck()).thenReturn(true); + when(profile.getIPv4Address()).thenReturn("10.1.1.5"); + when(testOrchestrator._nicDao.findNonPlaceHolderByIp4AddressAndNetworkId("10.1.1.5", 10L)).thenReturn(mock(NicVO.class)); + + assertNull(persistNicInTransaction(nic, profile, NetworkOrchestrator.FREE_DEVICE_ID, 100L)); + + verify(testOrchestrator._nicDao, never()).persist(any(NicVO.class)); + verify(testOrchestrator._vmDao, never()).lockRow(anyLong(), anyBoolean()); } }