From d1067a6febaadc8dbf9359036533ed2b6ef77850 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Fri, 17 Apr 2026 11:14:12 +0200 Subject: [PATCH] sonar remarks addressed and some extra tests generated --- .../java/com/cloud/vm/UserVmManagerImpl.java | 116 ++++++------ .../com/cloud/vm/UserVmManagerImplTest.java | 173 ++++++++++++++++++ .../java/com/cloud/utils/StringUtils.java | 5 + 3 files changed, 240 insertions(+), 54 deletions(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 2d32d30e40b..1780fc47a92 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -1141,68 +1141,76 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir private UserVm rebootVirtualMachine(long vmId, boolean enterSetup, boolean forced) throws InsufficientCapacityException, ResourceUnavailableException { UserVmVO vm = _vmDao.findById(vmId); - if (logger.isTraceEnabled()) { - logger.trace("reboot {} with enterSetup set to {}", vm, Boolean.toString(enterSetup)); - } - if (vm == null || vm.getState() == State.Destroyed || vm.getState() == State.Expunging || vm.getRemoved() != null) { logger.warn("Vm {} with id={} doesn't exist or is not in correct state", vm, vmId); return null; } - if (vm.getState() == State.Running && vm.getHostId() != null) { - collectVmDiskAndNetworkStatistics(vm, State.Running); - - if (forced) { - Host vmOnHost = _hostDao.findById(vm.getHostId()); - if (vmOnHost == null || vmOnHost.getResourceState() != ResourceState.Enabled || vmOnHost.getStatus() != Status.Up ) { - throw new CloudRuntimeException("Unable to force reboot the VM as the host: " + vm.getHostId() + " is not in the right state"); - } - return forceRebootVirtualMachine(vm, vm.getHostId(), enterSetup); - } - - DataCenterVO dc = _dcDao.findById(vm.getDataCenterId()); - try { - if (dc.getNetworkType() == DataCenter.NetworkType.Advanced) { - //List all networks of vm - List vmNetworks = _vmNetworkMapDao.getNetworks(vmId); - List routers = new ArrayList<>(); - //List the stopped routers - for (long vmNetworkId : vmNetworks) { - List router = _routerDao.listStopped(vmNetworkId); - routers.addAll(router); - } - //A vm may not have many nics attached and even fewer routers might be stopped (only in exceptional cases) - //Safe to start the stopped router serially, this is consistent with the way how multiple networks are added to vm during deploy - //and routers are started serially ,may revisit to make this process parallel - for (DomainRouterVO routerToStart : routers) { - logger.warn("Trying to start router {} as part of vm: {} reboot", routerToStart, vm); - _virtualNetAppliance.startRouter(routerToStart.getId(),true); - } - } - } catch (ConcurrentOperationException e) { - throw new CloudRuntimeException("Concurrent operations on starting router. " + e); - } catch (Exception ex) { - throw new CloudRuntimeException("Router start failed due to" + ex); - } finally { - if (logger.isInfoEnabled()) { - logger.info("Rebooting vm {}{}.", vm, enterSetup ? " entering hardware setup menu" : " as is"); - } - Map params = null; - if (enterSetup) { - params = new HashMap<>(); - params.put(VirtualMachineProfile.Param.BootIntoSetup, Boolean.TRUE); - if (logger.isTraceEnabled()) { - logger.trace(String.format("Adding %s to paramlist", VirtualMachineProfile.Param.BootIntoSetup)); - } - } - _itMgr.reboot(vm.getUuid(), params); - } - return _vmDao.findById(vmId); - } else { + if (vm.getState() != State.Running || vm.getHostId() == null) { logger.error("Vm {} is not in Running state, failed to reboot", vm); return null; } + + collectVmDiskAndNetworkStatistics(vm, State.Running); + + if (forced) { + return handleForcedReboot(vm, enterSetup); + } + + try { + startRoutersIfNeeded(vm, vmId); + } catch (ConcurrentOperationException e) { + throw new CloudRuntimeException("Concurrent operations on starting router. " + e); + } catch (Exception ex) { + throw new CloudRuntimeException("Router start failed due to" + ex); + } finally { + if (logger.isInfoEnabled()) { + logger.info("Rebooting vm {}{}.", vm, enterSetup ? " entering hardware setup menu" : " as is"); + } + Map params = buildRebootParamsIfNeeded(enterSetup); + _itMgr.reboot(vm.getUuid(), params); + } + + return _vmDao.findById(vmId); + } + + private UserVm handleForcedReboot(UserVmVO vm, boolean enterSetup) { + Host vmOnHost = _hostDao.findById(vm.getHostId()); + if (vmOnHost == null || vmOnHost.getResourceState() != ResourceState.Enabled || vmOnHost.getStatus() != Status.Up) { + throw new CloudRuntimeException("Unable to force reboot the VM as the host: " + vm.getHostId() + " is not in the right state"); + } + return forceRebootVirtualMachine(vm, vm.getHostId(), enterSetup); + } + + private void startRoutersIfNeeded(UserVmVO vm, long vmId) throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { + DataCenterVO dc = _dcDao.findById(vm.getDataCenterId()); + if (dc.getNetworkType() != DataCenter.NetworkType.Advanced) { + return; + } + + // List all networks of vm + List vmNetworks = _vmNetworkMapDao.getNetworks(vmId); + List routers = new ArrayList<>(); + // List the stopped routers + for (long vmNetworkId : vmNetworks) { + List router = _routerDao.listStopped(vmNetworkId); + routers.addAll(router); + } + + // Safe to start the stopped router serially, consistent with deploy/start behavior + for (DomainRouterVO routerToStart : routers) { + logger.warn("Trying to start router {} as part of vm: {} reboot", routerToStart, vm); + _virtualNetAppliance.startRouter(routerToStart.getId(), true); + } + } + + private Map buildRebootParamsIfNeeded(boolean enterSetup) { + if (!enterSetup) { + return null; + } + Map params = new HashMap<>(); + params.put(VirtualMachineProfile.Param.BootIntoSetup, Boolean.TRUE); + return params; } private UserVm forceRebootVirtualMachine(UserVmVO vm, long hostId, boolean enterSetup) { diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index 073245e2a11..b3080e31b5e 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -91,6 +91,7 @@ import org.apache.cloudstack.backup.dao.BackupDao; import org.apache.cloudstack.backup.dao.BackupScheduleDao; import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService; +import org.apache.cloudstack.engine.cloud.entity.api.db.dao.VMNetworkMapDao; import org.apache.cloudstack.resourcelimit.Reserver; import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore; import org.apache.cloudstack.engine.subsystem.api.storage.Scope; @@ -152,6 +153,7 @@ import com.cloud.network.dao.PhysicalNetworkDao; import com.cloud.network.dao.PhysicalNetworkVO; import com.cloud.network.element.UserDataServiceProvider; import com.cloud.network.guru.NetworkGuru; +import com.cloud.network.router.VpcVirtualNetworkApplianceManager; import com.cloud.network.rules.FirewallRuleVO; import com.cloud.network.rules.PortForwardingRule; import com.cloud.network.rules.dao.PortForwardingRulesDao; @@ -205,6 +207,7 @@ import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.exception.ExceptionProxyObject; import com.cloud.utils.fsm.NoTransitionException; import com.cloud.vm.dao.NicDao; +import com.cloud.vm.dao.DomainRouterDao; import com.cloud.vm.dao.UserVmDao; import com.cloud.vm.dao.VMInstanceDetailsDao; import com.cloud.vm.snapshot.VMSnapshotVO; @@ -461,6 +464,15 @@ public class UserVmManagerImplTest { @Mock private BackupScheduleDao backupScheduleDao; + @Mock + private VMNetworkMapDao _vmNetworkMapDao; + + @Mock + private DomainRouterDao _routerDao; + + @Mock + private VpcVirtualNetworkApplianceManager _virtualNetAppliance; + MockedStatic unmanagedVMsManagerMockedStatic; @Mock @@ -4363,6 +4375,167 @@ public class UserVmManagerImplTest { method.invoke(userVmManagerImpl, vmId); } + @Test + public void testBuildRebootParamsIfNeededFalseReturnsNull() { + Object params = ReflectionTestUtils.invokeMethod(userVmManagerImpl, "buildRebootParamsIfNeeded", false); + Assert.assertNull(params); + } + + @Test + public void testBuildRebootParamsIfNeededTrueAddsBootIntoSetup() { + @SuppressWarnings("unchecked") + Map params = + ReflectionTestUtils.invokeMethod(userVmManagerImpl, "buildRebootParamsIfNeeded", true); + + assertNotNull(params); + assertEquals(1, params.size()); + assertEquals(Boolean.TRUE, params.get(VirtualMachineProfile.Param.BootIntoSetup)); + } + + @Test + public void testStartRoutersIfNeededBasicZoneSkipsRouterOperations() throws Exception { + UserVmVO vm = mock(UserVmVO.class); + when(vm.getDataCenterId()).thenReturn(zoneId); + + DataCenterVO dc = mock(DataCenterVO.class); + when(_dcDao.findById(zoneId)).thenReturn(dc); + when(dc.getNetworkType()).thenReturn(DataCenter.NetworkType.Basic); + + ReflectionTestUtils.invokeMethod(userVmManagerImpl, "startRoutersIfNeeded", vm, vmId); + + verify(_vmNetworkMapDao, never()).getNetworks(anyLong()); + verify(_virtualNetAppliance, never()).startRouter(anyLong(), anyBoolean()); + } + + @Test + public void testStartRoutersIfNeededAdvancedZoneWithoutNetworksStartsNothing() throws Exception { + UserVmVO vm = mock(UserVmVO.class); + when(vm.getDataCenterId()).thenReturn(zoneId); + + DataCenterVO dc = mock(DataCenterVO.class); + when(_dcDao.findById(zoneId)).thenReturn(dc); + when(dc.getNetworkType()).thenReturn(DataCenter.NetworkType.Advanced); + when(_vmNetworkMapDao.getNetworks(vmId)).thenReturn(Collections.emptyList()); + + ReflectionTestUtils.invokeMethod(userVmManagerImpl, "startRoutersIfNeeded", vm, vmId); + + verify(_vmNetworkMapDao).getNetworks(vmId); + verify(_routerDao, never()).listStopped(anyLong()); + verify(_virtualNetAppliance, never()).startRouter(anyLong(), anyBoolean()); + } + + @Test + public void testStartRoutersIfNeededAdvancedZoneStartsAllStoppedRouters() throws Exception { + UserVmVO vm = mock(UserVmVO.class); + when(vm.getDataCenterId()).thenReturn(zoneId); + + DataCenterVO dc = mock(DataCenterVO.class); + when(_dcDao.findById(zoneId)).thenReturn(dc); + when(dc.getNetworkType()).thenReturn(DataCenter.NetworkType.Advanced); + + when(_vmNetworkMapDao.getNetworks(vmId)).thenReturn(Arrays.asList(100L, 200L)); + + DomainRouterVO router1 = mock(DomainRouterVO.class); + DomainRouterVO router2 = mock(DomainRouterVO.class); + DomainRouterVO router3 = mock(DomainRouterVO.class); + when(router1.getId()).thenReturn(1000L); + when(router2.getId()).thenReturn(1001L); + when(router3.getId()).thenReturn(2000L); + + when(_routerDao.listStopped(100L)).thenReturn(Arrays.asList(router1, router2)); + when(_routerDao.listStopped(200L)).thenReturn(Collections.singletonList(router3)); + + ReflectionTestUtils.invokeMethod(userVmManagerImpl, "startRoutersIfNeeded", vm, vmId); + + verify(_virtualNetAppliance).startRouter(1000L, true); + verify(_virtualNetAppliance).startRouter(1001L, true); + verify(_virtualNetAppliance).startRouter(2000L, true); + } + + @Test + public void testStartRoutersIfNeededPropagatesRouterStartException() throws Exception { + UserVmVO vm = mock(UserVmVO.class); + when(vm.getDataCenterId()).thenReturn(zoneId); + + DataCenterVO dc = mock(DataCenterVO.class); + when(_dcDao.findById(zoneId)).thenReturn(dc); + when(dc.getNetworkType()).thenReturn(DataCenter.NetworkType.Advanced); + when(_vmNetworkMapDao.getNetworks(vmId)).thenReturn(Collections.singletonList(100L)); + + DomainRouterVO router = mock(DomainRouterVO.class); + when(router.getId()).thenReturn(1000L); + when(_routerDao.listStopped(100L)).thenReturn(Collections.singletonList(router)); + doThrow(new CloudRuntimeException("router start failed")).when(_virtualNetAppliance).startRouter(1000L, true); + + assertThrows(CloudRuntimeException.class, + () -> ReflectionTestUtils.invokeMethod(userVmManagerImpl, "startRoutersIfNeeded", vm, vmId)); + } + + @Test + public void testHandleForcedRebootThrowsWhenHostNotFound() { + UserVmVO vm = mock(UserVmVO.class); + when(vm.getHostId()).thenReturn(42L); + + when(hostDao.findById(42L)).thenReturn(null); + + assertThrows(CloudRuntimeException.class, + () -> ReflectionTestUtils.invokeMethod(userVmManagerImpl, "handleForcedReboot", vm, false)); + } + + @Test + public void testHandleForcedRebootThrowsWhenHostNotUsable() { + UserVmVO vm = mock(UserVmVO.class); + when(vm.getHostId()).thenReturn(42L); + + HostVO host = mock(HostVO.class); + when(hostDao.findById(42L)).thenReturn(host); + when(host.getResourceState()).thenReturn(com.cloud.resource.ResourceState.Enabled); + when(host.getStatus()).thenReturn(com.cloud.host.Status.Down); + + assertThrows(CloudRuntimeException.class, + () -> ReflectionTestUtils.invokeMethod(userVmManagerImpl, "handleForcedReboot", vm, false)); + } + + @Test + public void testHandleForcedRebootReturnsNullWhenStopFails() { + UserVmVO vm = mock(UserVmVO.class); + when(vm.getId()).thenReturn(vmId); + when(vm.getHostId()).thenReturn(42L); + + HostVO host = mock(HostVO.class); + when(hostDao.findById(42L)).thenReturn(host); + when(host.getResourceState()).thenReturn(com.cloud.resource.ResourceState.Enabled); + when(host.getStatus()).thenReturn(com.cloud.host.Status.Up); + + doReturn(null).when(userVmManagerImpl).stopVirtualMachine(vmId, false); + + Object result = ReflectionTestUtils.invokeMethod(userVmManagerImpl, "handleForcedReboot", vm, false); + Assert.assertNull(result); + } + + @Test + public void testHandleForcedRebootSuccessWithEnterSetup() throws Exception { + UserVmVO vm = mock(UserVmVO.class); + when(vm.getId()).thenReturn(vmId); + when(vm.getHostId()).thenReturn(42L); + + HostVO host = mock(HostVO.class); + when(hostDao.findById(42L)).thenReturn(host); + when(host.getResourceState()).thenReturn(com.cloud.resource.ResourceState.Enabled); + when(host.getStatus()).thenReturn(com.cloud.host.Status.Up); + + UserVmVO stoppedVm = mock(UserVmVO.class); + doReturn(stoppedVm).when(userVmManagerImpl).stopVirtualMachine(vmId, false); + + Pair> startedVm = new Pair<>(userVmVoMock, new HashMap<>()); + doReturn(startedVm).when(userVmManagerImpl).startVirtualMachine(eq(vmId), isNull(), isNull(), eq(42L), anyMap(), isNull(), eq(false)); + + UserVm result = ReflectionTestUtils.invokeMethod(userVmManagerImpl, "handleForcedReboot", vm, true); + + assertEquals(userVmVoMock, result); + verify(userVmManagerImpl).stopVirtualMachine(vmId, false); + } + private ServiceOfferingVO getMockedServiceOffering(boolean custom, boolean customSpeed) { ServiceOfferingVO serviceOffering = mock(ServiceOfferingVO.class); when(serviceOffering.getUuid()).thenReturn("offering-uuid"); diff --git a/utils/src/main/java/com/cloud/utils/StringUtils.java b/utils/src/main/java/com/cloud/utils/StringUtils.java index 73b8d04bf00..dfc488c2c7d 100644 --- a/utils/src/main/java/com/cloud/utils/StringUtils.java +++ b/utils/src/main/java/com/cloud/utils/StringUtils.java @@ -438,4 +438,9 @@ public class StringUtils extends org.apache.commons.lang3.StringUtils { return null; } + + public static boolean isBlank(String str) { + return org.apache.commons.lang3.StringUtils.isBlank(str); + } + }