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(); + } }