From f3b418105d5fb5a4291d7d5d7a5f0e3b118cfab3 Mon Sep 17 00:00:00 2001 From: Ramgopal Nagaboina Date: Wed, 7 Oct 2026 16:59:16 -0400 Subject: [PATCH] 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)); + } +}