Fix duplicate public IP allocated to system VMs at zone startup

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

@@ -345,6 +345,9 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage
     SearchBuilder<IPAddressVO> AssignIpAddressSearch;
     SearchBuilder<IPAddressVO> 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;
 
@@ -369,7 +372,7 @@ private List<Long> 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<IPAddressVO> addressVOS) throws CloudRuntimeException {
@@ -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) {
@@ -420,22 +426,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;
         }
-        return finalAddress;
+        possibleAddr.setState(State.Allocating);
+        if (_ipAddressDao.update(possibleAddr.getId(), possibleAddr)) {
+            logger.info("successfully allocated ip address {}", possibleAddr.getAddress());
+            return possibleAddr;
+        }
+        return null;
     }
 
     @Override
@@ -950,16 +955,39 @@ public PublicIp fetchNewPublicIp(final long dcId, final Long podId, final List<L
     public PublicIp fetchNewPublicIp(final long dcId, final Long podId, final List<Long> 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<IPAddressVO> 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<IPAddressVO> 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);
         }
 
+        return buildPublicIp(addr);
+    }
+
+    protected PublicIp buildPublicIp(IPAddressVO addr) {
         return PublicIp.createFromAddrAndVlan(addr, _vlanDao.findById(addr.getVlanId()));
     }
 

server/src/test/java/com/cloud/network/IpAddressManagerImplTest.java

@@ -0,0 +1,134 @@
+// 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 java.util.Collections;
+
+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.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.dc.Vlan.VlanType;
+import com.cloud.network.IpAddress.State;
+import com.cloud.network.addr.PublicIp;
+import com.cloud.network.dao.IPAddressDao;
+import com.cloud.network.dao.IPAddressVO;
+import com.cloud.user.Account;
+
+@RunWith(MockitoJUnitRunner.class)
+public class IpAddressManagerImplTest {
+
+    @Mock
+    IPAddressDao ipAddressDao;
+
+    @Spy
+    @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));
+    }
+
+    @Test
+    public void testFetchNewPublicIpRetriesWhenTheFirstPickIsTakenConcurrently() throws Exception {
+        IPAddressVO taken = Mockito.mock(IPAddressVO.class);     // first pick, lost to a concurrent allocation
+        IPAddressVO allocated = Mockito.mock(IPAddressVO.class); // the free ip the retry succeeds with
+        PublicIp expected = Mockito.mock(PublicIp.class);
+
+        // each selection returns a single candidate (listAvailablePublicIps is called with lockOneRow=true)
+        Mockito.doReturn(Collections.singletonList(taken)).when(ipAddressManager).listAvailablePublicIps(
+                Mockito.anyLong(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(),
+                Mockito.anyBoolean(), Mockito.anyBoolean(), Mockito.anyBoolean(), Mockito.any(), Mockito.any(),
+                Mockito.anyBoolean(), Mockito.any(), Mockito.any(), Mockito.anyBoolean(), Mockito.anyBoolean());
+        // the first allocation loses the race (null), the retry gets another ip
+        Mockito.doReturn(null).doReturn(allocated).when(ipAddressManager).assignAndAllocateIpAddressEntry(
+                Mockito.any(), Mockito.any(), Mockito.any(), Mockito.anyBoolean(), Mockito.anyBoolean(),
+                Mockito.anyBoolean(), Mockito.any(), Mockito.any(), Mockito.anyList());
+        Mockito.doReturn(expected).when(ipAddressManager).buildPublicIp(allocated);
+
+        PublicIp result = ipAddressManager.fetchNewPublicIp(1L, null, null, Mockito.mock(Account.class),
+                VlanType.DirectAttached, null, false, true, true, null, null, true, null, null, false);
+
+        // the losing pick did not fail the call; it retried and allocated a different free ip
+        Assert.assertSame(expected, result);
+        Mockito.verify(ipAddressManager, Mockito.times(2)).assignAndAllocateIpAddressEntry(
+                Mockito.any(), Mockito.any(), Mockito.any(), Mockito.anyBoolean(), Mockito.anyBoolean(),
+                Mockito.anyBoolean(), Mockito.any(), Mockito.any(), Mockito.anyList());
+    }
+}