From f3b418105d5fb5a4291d7d5d7a5f0e3b118cfab3 Mon Sep 17 00:00:00 2001 From: Ramgopal Nagaboina Date: Wed, 7 Oct 2026 16:59:16 -0400 Subject: [PATCH 1/3] server: hold a row lock across system VM public IP allocation assignIpAddressWithLock guarded the Free to Allocating transition with an op_lock application lock plus a plain, non-locking read, and released that lock before the enclosing transaction committed. Two system VM scanner threads (console proxy and secondary storage) starting at zone bring-up could both read the same pool row as Free and both allocate it, leaving two nics with the same public IP and a VM console that never connects. Re-read the candidate row with a FOR UPDATE row lock held for the whole allocation transaction and re-check the state under it. A second thread now blocks until the first commits and then sees the row is no longer Free, so it backs off instead of allocating the same address. This is the same row lock the following markPublicIpAsAllocated step already relies on. --- .../cloud/network/IpAddressManagerImpl.java | 29 +++-- .../network/IpAddressManagerImplTest.java | 101 ++++++++++++++++++ 2 files changed, 115 insertions(+), 15 deletions(-) create mode 100644 server/src/test/java/com/cloud/network/IpAddressManagerImplTest.java diff --git a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java index 508d15fe3fc4..4784f99b7d81 100644 --- a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java +++ b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java @@ -420,22 +420,21 @@ private IPAddressVO assignAndAllocateIpAddressEntry(final Account owner, final V } private IPAddressVO assignIpAddressWithLock(IPAddressVO possibleAddr) { - IPAddressVO finalAddress = null; - IPAddressVO userIp = _ipAddressDao.acquireInLockTable(possibleAddr.getId()); - if (userIp != null) { - logger.debug("locked row for ip address {} (id: {})", possibleAddr.getAddress(), possibleAddr.getUuid()); - if (userIp.getState() == State.Free) { - possibleAddr.setState(State.Allocating); - if (_ipAddressDao.update(possibleAddr.getId(), possibleAddr)) { - logger.info("successfully allocated ip address {}", possibleAddr.getAddress()); - finalAddress = possibleAddr; - } - } else { - logger.debug("locked ip address {} is not free {}", possibleAddr.getAddress(), userIp.getState()); - } - _ipAddressDao.releaseFromLockTable(possibleAddr.getId()); + IPAddressVO userIp = _ipAddressDao.lockRow(possibleAddr.getId(), true); + if (userIp == null) { + return null; + } + logger.debug("locked row for ip address {} (id: {})", possibleAddr.getAddress(), possibleAddr.getUuid()); + if (userIp.getState() != State.Free) { + logger.debug("locked ip address {} is not free {}", possibleAddr.getAddress(), userIp.getState()); + return null; + } + possibleAddr.setState(State.Allocating); + if (_ipAddressDao.update(possibleAddr.getId(), possibleAddr)) { + logger.info("successfully allocated ip address {}", possibleAddr.getAddress()); + return possibleAddr; } - return finalAddress; + return null; } @Override diff --git a/server/src/test/java/com/cloud/network/IpAddressManagerImplTest.java b/server/src/test/java/com/cloud/network/IpAddressManagerImplTest.java new file mode 100644 index 000000000000..b43aa73ed498 --- /dev/null +++ b/server/src/test/java/com/cloud/network/IpAddressManagerImplTest.java @@ -0,0 +1,101 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package com.cloud.network; + +import java.lang.reflect.Method; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.network.IpAddress.State; +import com.cloud.network.dao.IPAddressDao; +import com.cloud.network.dao.IPAddressVO; + +@RunWith(MockitoJUnitRunner.class) +public class IpAddressManagerImplTest { + + @Mock + IPAddressDao ipAddressDao; + + @InjectMocks + IpAddressManagerImpl ipAddressManager = new IpAddressManagerImpl(); + + private Method assignIpAddressWithLock; + + @Before + public void setUp() throws Exception { + assignIpAddressWithLock = IpAddressManagerImpl.class.getDeclaredMethod("assignIpAddressWithLock", IPAddressVO.class); + assignIpAddressWithLock.setAccessible(true); + } + + private IPAddressVO invoke(IPAddressVO candidate) throws Exception { + return (IPAddressVO) assignIpAddressWithLock.invoke(ipAddressManager, candidate); + } + + @Test + public void testAssignAllocatesWhenRowLockedReadIsFree() throws Exception { + IPAddressVO candidate = Mockito.mock(IPAddressVO.class); + Mockito.when(candidate.getId()).thenReturn(2L); + IPAddressVO lockedRow = Mockito.mock(IPAddressVO.class); + Mockito.when(lockedRow.getState()).thenReturn(State.Free); + // the fix must re-read the row under a FOR UPDATE lock, not a plain read + Mockito.when(ipAddressDao.lockRow(2L, true)).thenReturn(lockedRow); + Mockito.when(ipAddressDao.update(Mockito.eq(2L), Mockito.eq(candidate))).thenReturn(true); + + IPAddressVO result = invoke(candidate); + + Assert.assertSame(candidate, result); + Mockito.verify(ipAddressDao).lockRow(2L, true); + Mockito.verify(candidate).setState(State.Allocating); + Mockito.verify(ipAddressDao).update(2L, candidate); + } + + @Test + public void testAssignReturnsNullWhenRowLockedReadIsNotFree() throws Exception { + IPAddressVO candidate = Mockito.mock(IPAddressVO.class); + Mockito.when(candidate.getId()).thenReturn(2L); + IPAddressVO lockedRow = Mockito.mock(IPAddressVO.class); + // the winning thread already flipped it; the loser must see the committed state and back off + Mockito.when(lockedRow.getState()).thenReturn(State.Allocating); + Mockito.when(ipAddressDao.lockRow(2L, true)).thenReturn(lockedRow); + + IPAddressVO result = invoke(candidate); + + Assert.assertNull(result); + Mockito.verify(ipAddressDao).lockRow(2L, true); + Mockito.verify(candidate, Mockito.never()).setState(State.Allocating); + Mockito.verify(ipAddressDao, Mockito.never()).update(Mockito.anyLong(), Mockito.any(IPAddressVO.class)); + } + + @Test + public void testAssignReturnsNullWhenRowIsGone() throws Exception { + IPAddressVO candidate = Mockito.mock(IPAddressVO.class); + Mockito.when(candidate.getId()).thenReturn(2L); + Mockito.when(ipAddressDao.lockRow(2L, true)).thenReturn(null); + + IPAddressVO result = invoke(candidate); + + Assert.assertNull(result); + Mockito.verify(ipAddressDao, Mockito.never()).update(Mockito.anyLong(), Mockito.any(IPAddressVO.class)); + } +} From 85aba61c242f7c8f9ebdfe4c4fb38f2b7421d8a4 Mon Sep 17 00:00:00 2001 From: Ramgopal Nagaboina Date: Thu, 8 Oct 2026 15:27:39 -0400 Subject: [PATCH 2/3] server: retry another free IP when a system VM loses the allocation race assignAndAllocateIpAddressEntry failed by throwing when its single candidate was locked and found no longer Free (taken by a concurrent allocation), so the losing system VM reported no free IP even when other addresses were free. Return null in that case and have fetchNewPublicIp re-select a different free IP and retry, bounded to a few attempts. listAvailablePublicIps filters on the Free state, so the retry skips the address the winner already moved to Allocating and picks another; InsufficientAddressCapacityException is thrown only once the attempts are exhausted. --- .../cloud/network/IpAddressManagerImpl.java | 37 ++++++++++++++++--- 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java index 4784f99b7d81..539502b6badf 100644 --- a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java +++ b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java @@ -345,6 +345,9 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage SearchBuilder AssignIpAddressSearch; SearchBuilder AssignIpAddressFromPodVlanSearch; private static final Object allocatedLock = new Object(); + // How many times fetchNewPublicIp re-selects a free IP when its candidate is taken by a concurrent + // allocation before giving up. A handful is ample: only a few system VMs ever allocate at once. + protected static final int MAX_PUBLIC_IP_ALLOCATION_ATTEMPTS = 5; static Boolean rulesContinueOnErrFlag = true; @@ -402,8 +405,11 @@ private IPAddressVO assignAndAllocateIpAddressEntry(final Account owner, final V } if (finalAddress == null) { - logger.error("Failed to fetch any free public IP address"); - throw new CloudRuntimeException("Failed to fetch any free public IP address"); + // Every candidate in this batch was taken by a concurrent allocation before we could lock it. + // Return null so the caller can re-select a different free IP and retry, rather than failing + // when free addresses still exist. + logger.debug("No free public IP address could be locked among the candidates; the caller may retry"); + return null; } if (allocate) { @@ -949,11 +955,30 @@ public PublicIp fetchNewPublicIp(final long dcId, final Long podId, final List vlanDbIds, final Account owner, final VlanType vlanUse, final Long guestNetworkId, final boolean sourceNat, final boolean assign, final boolean allocate, final String requestedIp, final String requestedGateway, final boolean isSystem, final Long vpcId, final Boolean displayIp, final boolean forSystemVms) throws InsufficientAddressCapacityException { - List addrs = listAvailablePublicIps(dcId, podId, vlanDbIds, owner, vlanUse, guestNetworkId, sourceNat, assign, allocate, requestedIp, requestedGateway, isSystem, vpcId, displayIp, forSystemVms, true); - IPAddressVO addr = addrs.get(0); - if (assign) { + // Two allocations (for example the console proxy and secondary storage system VMs started at the same + // time) can be handed the same free address because listAvailablePublicIps selects a candidate in a + // separate transaction from the one that marks it Allocating. assignIpAddressWithLock now locks the row + // and re-checks it is still Free, so the loser gets back null; re-select a different free IP and retry + // rather than failing with no-free-IP while free addresses still exist. + IPAddressVO addr = null; + for (int attempt = 1; attempt <= MAX_PUBLIC_IP_ALLOCATION_ATTEMPTS; attempt++) { + List addrs = listAvailablePublicIps(dcId, podId, vlanDbIds, owner, vlanUse, guestNetworkId, sourceNat, assign, allocate, requestedIp, requestedGateway, isSystem, vpcId, displayIp, forSystemVms, true); + if (!assign) { + addr = addrs.get(0); + break; + } addr = assignAndAllocateIpAddressEntry(owner, vlanUse, guestNetworkId, sourceNat, allocate, - isSystem,vpcId, displayIp, addrs); + isSystem, vpcId, displayIp, addrs); + if (addr != null) { + break; + } + logger.debug("Public IP candidate was allocated concurrently; retrying with another free IP (attempt {} of {})", + attempt, MAX_PUBLIC_IP_ALLOCATION_ATTEMPTS); + } + if (addr == null) { + throw new InsufficientAddressCapacityException( + "Unable to allocate a free public IP after " + MAX_PUBLIC_IP_ALLOCATION_ATTEMPTS + " attempts due to concurrent allocations", + DataCenter.class, dcId); } if (vlanUse == VlanType.VirtualNetwork) { _firewallMgr.addSystemFirewallRules(addr, owner); From 43384a56a69758ec34cf4182d739223805a25780 Mon Sep 17 00:00:00 2001 From: Ramgopal Nagaboina Date: Thu, 8 Oct 2026 17:15:48 -0400 Subject: [PATCH 3/3] server: test the public IP allocation retry path Add a unit test proving fetchNewPublicIp re-selects a different free IP when the first candidate is taken by a concurrent allocation, instead of failing with no-free-IP. Extract the final PublicIp build into buildPublicIp and make assignAndAllocateIpAddressEntry package-visible so the retry loop can be exercised without a live database. --- .../cloud/network/IpAddressManagerImpl.java | 6 +++- .../network/IpAddressManagerImplTest.java | 33 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java index 539502b6badf..2f714717521e 100644 --- a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java +++ b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java @@ -372,7 +372,7 @@ private List getIpv6SupportingVlanRangeIds(long dcId) throws InsufficientA } @DB - private IPAddressVO assignAndAllocateIpAddressEntry(final Account owner, final VlanType vlanUse, final Long guestNetworkId, + protected IPAddressVO assignAndAllocateIpAddressEntry(final Account owner, final VlanType vlanUse, final Long guestNetworkId, final boolean sourceNat, final boolean allocate, final boolean isSystem, final Long vpcId, final Boolean displayIp, final List addressVOS) throws CloudRuntimeException { @@ -984,6 +984,10 @@ public PublicIp fetchNewPublicIp(final long dcId, final Long podId, final List