Skip to content

Serialize NIC add/remove on the target VM's work job queue #14139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bhouse-nexthop wants to merge 2 commits into
base: 4.22
Choose a base branch
from
Open

Serialize NIC add/remove on the target VM's work job queue #14139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c65369e
Allocate NIC device id atomically to stop duplicate ethN on a VM
bhouse-nexthop Sep 10, 2026
a6762e9
Take the nic device id lock only for the write of the nic
bhouse-nexthop Oct 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 50 additions & 17 deletions ...tration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
});
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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<NicProfile, Integer> allocateNic(final NicProfile requested, final Network network, final Boolean isDefaultNic, int deviceId, final VirtualMachineProfile vm)
Expand Down Expand Up @@ -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);
Expand Down
87 changes: 87 additions & 0 deletions ...ion/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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<Network.Capability, String> capabilities = new HashMap<Network.Capability, String>();
Expand Down Expand Up @@ -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<Transaction> 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<Transaction> transactionMocked = Mockito.mockStatic(Transaction.class)) {
transactionMocked.when(() -> Transaction.execute(any(TransactionCallback.class)))
.thenAnswer(invocation -> {
TransactionCallback<NicVO> 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());
}
}
Loading