From f4d9e03a19b6dd0d28d04aa86cf81a6b22296c9e Mon Sep 17 00:00:00 2001 From: Ramgopal Nagaboina Date: Tue, 15 Sep 2026 13:03:44 -0400 Subject: [PATCH] consoleproxy: do not restart a console proxy that is being destroyed destroyProxy stops the console proxy and then expunges it. The console proxy scanner takes proxies in Starting, Stopped, Migrating or Stopping state from its stopped pool and starts them, with no coordination with a destroy in progress. When a scan runs while a destroy has the proxy in Stopping or Stopped, the scanner starts it again and the destroy fails with "Unable to expunge the vm because it is not in the correct state", or the scanner recycles it with a second destroy of its own. Hold the existing console proxy allocation lock while the scanner assigns and starts a proxy, and while destroyProxy stops and expunges one. If destroyProxy cannot get the lock in time it logs a warning and destroys the proxy as before. --- .../consoleproxy/ConsoleProxyManagerImpl.java | 26 ++++++ .../ConsoleProxyManagerImplTest.java | 85 ++++++++++++++++++- 2 files changed, 110 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java b/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java index f1e422ec36cb..de02ca2b7606 100644 --- a/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java +++ b/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java @@ -824,6 +824,18 @@ private boolean checkCapacity(ConsoleProxyLoadInfo proxyCountInfo, ConsoleProxyL } private void allocCapacity(long dataCenterId) { + if (!allocProxyLock.lock(ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_SYNC_IN_SECONDS)) { + logger.info("Unable to acquire synchronization lock for console proxy vm allocation, wait for next scan"); + return; + } + try { + doAllocCapacity(dataCenterId); + } finally { + allocProxyLock.unlock(); + } + } + + private void doAllocCapacity(long dataCenterId) { DataCenterVO zone = dataCenterDao.findById(dataCenterId); if (logger.isDebugEnabled()) { logger.debug("Allocating console proxy standby capacity for zone [{}].", zone); @@ -1093,6 +1105,20 @@ public boolean rebootProxy(long proxyVmId) { @Override public boolean destroyProxy(long vmId) { + boolean locked = allocProxyLock.lock(ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_SYNC_IN_SECONDS); + if (!locked) { + logger.warn("Unable to acquire synchronization lock for console proxy vm allocation, destroying console proxy [{}] without it", vmId); + } + try { + return doDestroyProxy(vmId); + } finally { + if (locked) { + allocProxyLock.unlock(); + } + } + } + + private boolean doDestroyProxy(long vmId) { ConsoleProxyVO proxy = consoleProxyDao.findById(vmId); try { virtualMachineManager.expunge(proxy.getUuid()); diff --git a/server/src/test/java/com/cloud/consoleproxy/ConsoleProxyManagerImplTest.java b/server/src/test/java/com/cloud/consoleproxy/ConsoleProxyManagerImplTest.java index 1feced5f464d..8923a9373287 100644 --- a/server/src/test/java/com/cloud/consoleproxy/ConsoleProxyManagerImplTest.java +++ b/server/src/test/java/com/cloud/consoleproxy/ConsoleProxyManagerImplTest.java @@ -18,24 +18,38 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import java.util.List; + import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.InOrder; import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; +import org.springframework.test.util.ReflectionTestUtils; +import com.cloud.dc.dao.DataCenterDao; +import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor; import com.cloud.offering.ServiceOffering; import com.cloud.storage.VMTemplateVO; import com.cloud.user.Account; import com.cloud.user.AccountManager; import com.cloud.user.User; +import com.cloud.utils.db.GlobalLock; import com.cloud.vm.ConsoleProxyVO; +import com.cloud.vm.VirtualMachine.State; +import com.cloud.vm.VirtualMachineManager; import com.cloud.vm.dao.ConsoleProxyDao; @RunWith(MockitoJUnitRunner.class) @@ -58,9 +72,19 @@ public class ConsoleProxyManagerImplTest { @Mock private User systemUser; + @Mock + private VirtualMachineManager virtualMachineManager; + @Mock + private HostDao hostDao; + @Mock + private DataCenterDao dataCenterDao; + @Mock + private GlobalLock allocProxyLock; + @Before public void setUp() { - when(accountManager.getSystemUser()).thenReturn(systemUser); + lenient().when(accountManager.getSystemUser()).thenReturn(systemUser); + ReflectionTestUtils.setField(consoleProxyManager, "allocProxyLock", allocProxyLock); } @Test @@ -104,4 +128,63 @@ public void testUpdateConsoleProxy() { assertEquals(template.getGuestOSId(), result.getGuestOSId()); assertEquals(template.isDynamicallyScalable(), result.isDynamicallyScalable()); } + + private ConsoleProxyVO mockProxyToDestroy() { + ConsoleProxyVO proxy = Mockito.mock(ConsoleProxyVO.class); + when(proxy.getUuid()).thenReturn("proxy-uuid"); + when(consoleProxyDao.findById(5L)).thenReturn(proxy); + return proxy; + } + + @Test + public void destroyProxyHoldsAllocationLockWhileExpunging() throws Exception { + mockProxyToDestroy(); + when(allocProxyLock.lock(anyInt())).thenReturn(true); + + assertTrue(consoleProxyManager.destroyProxy(5L)); + + InOrder inOrder = Mockito.inOrder(allocProxyLock, virtualMachineManager); + inOrder.verify(allocProxyLock).lock(anyInt()); + inOrder.verify(virtualMachineManager).expunge("proxy-uuid"); + inOrder.verify(allocProxyLock).unlock(); + } + + @Test + public void destroyProxyStillDestroysWhenAllocationLockIsBusy() throws Exception { + mockProxyToDestroy(); + when(allocProxyLock.lock(anyInt())).thenReturn(false); + + assertTrue(consoleProxyManager.destroyProxy(5L)); + + verify(virtualMachineManager).expunge("proxy-uuid"); + verify(allocProxyLock, never()).unlock(); + } + + @Test + public void expandPoolSkipsScanWhenAllocationLockIsBusy() { + when(allocProxyLock.lock(anyInt())).thenReturn(false); + + consoleProxyManager.expandPool(1L, null); + + Mockito.verifyNoInteractions(consoleProxyDao, dataCenterDao); + verify(allocProxyLock, never()).unlock(); + } + + @Test + public void expandPoolStartsStoppedProxyWhileHoldingAllocationLock() { + ConsoleProxyVO proxy = Mockito.mock(ConsoleProxyVO.class); + when(proxy.getId()).thenReturn(5L); + when(proxy.getState()).thenReturn(State.Running); + when(consoleProxyDao.getProxyListInStates(1L, State.Starting, State.Stopped, State.Migrating, State.Stopping)).thenReturn(List.of(proxy)); + when(consoleProxyDao.findById(5L)).thenReturn(proxy); + when(allocProxyLock.lock(anyInt())).thenReturn(true); + + consoleProxyManager.expandPool(1L, null); + + InOrder inOrder = Mockito.inOrder(allocProxyLock, consoleProxyDao); + inOrder.verify(allocProxyLock).lock(anyInt()); + inOrder.verify(consoleProxyDao).getProxyListInStates(1L, State.Starting, State.Stopped, State.Migrating, State.Stopping); + inOrder.verify(consoleProxyDao).findById(5L); + inOrder.verify(allocProxyLock).unlock(); + } }