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..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 @@ -276,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 @@ -1063,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); } }); } @@ -1121,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) { @@ -1141,6 +1160,22 @@ private NicVO checkForRaceAndAllocateNic(final NicProfile requested, final Netwo return vo; } + /** + * 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. 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 allocateNic(requested, network, isDefaultNic, FREE_DEVICE_ID, vm).first(); + } + @DB @Override public Pair allocateNic(final NicProfile requested, final Network network, final Boolean isDefaultNic, int deviceId, final VirtualMachineProfile vm) @@ -4510,11 +4545,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..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; @@ -41,6 +43,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 +79,7 @@ import com.cloud.utils.db.EntityManager; import com.cloud.utils.db.Transaction; import com.cloud.utils.db.TransactionCallback; +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 +92,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 +140,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 +1016,85 @@ public void testImportNicWithIP4Address() throws Exception { assertEquals("testtag", nicProfile.getName()); } } + + @Test + public void testAllocateNicWithFreeDeviceIdLeavesDeviceIdToThePersist() throws Exception { + VirtualMachineProfile vmProfile = mock(VirtualMachineProfile.class); + 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); + + 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)) { + transactionMocked.when(() -> Transaction.execute(any(TransactionCallback.class))) + .thenAnswer(invocation -> { + 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; + } + } + + @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(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()); + } }