Skip to content

Commit d1067a6

Browse files
author
Daan Hoogland
committed
sonar remarks addressed and some extra tests generated
1 parent 11a25af commit d1067a6

3 files changed

Lines changed: 238 additions & 52 deletions

File tree

‎server/src/main/java/com/cloud/vm/UserVmManagerImpl.java‎

Lines changed: 60 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -1141,68 +1141,76 @@ public boolean stopVirtualMachine(long userId, long vmId) {
11411141
private UserVm rebootVirtualMachine(long vmId, boolean enterSetup, boolean forced) throws InsufficientCapacityException, ResourceUnavailableException {
11421142
UserVmVO vm = _vmDao.findById(vmId);
11431143

1144-
if (logger.isTraceEnabled()) {
1145-
logger.trace("reboot {} with enterSetup set to {}", vm, Boolean.toString(enterSetup));
1146-
}
1147-
11481144
if (vm == null || vm.getState() == State.Destroyed || vm.getState() == State.Expunging || vm.getRemoved() != null) {
11491145
logger.warn("Vm {} with id={} doesn't exist or is not in correct state", vm, vmId);
11501146
return null;
11511147
}
11521148

1153-
if (vm.getState() == State.Running && vm.getHostId() != null) {
1154-
collectVmDiskAndNetworkStatistics(vm, State.Running);
1149+
if (vm.getState() != State.Running || vm.getHostId() == null) {
1150+
logger.error("Vm {} is not in Running state, failed to reboot", vm);
1151+
return null;
1152+
}
11551153

1156-
if (forced) {
1157-
Host vmOnHost = _hostDao.findById(vm.getHostId());
1158-
if (vmOnHost == null || vmOnHost.getResourceState() != ResourceState.Enabled || vmOnHost.getStatus() != Status.Up ) {
1159-
throw new CloudRuntimeException("Unable to force reboot the VM as the host: " + vm.getHostId() + " is not in the right state");
1160-
}
1161-
return forceRebootVirtualMachine(vm, vm.getHostId(), enterSetup);
1162-
}
1154+
collectVmDiskAndNetworkStatistics(vm, State.Running);
11631155

1164-
DataCenterVO dc = _dcDao.findById(vm.getDataCenterId());
1165-
try {
1166-
if (dc.getNetworkType() == DataCenter.NetworkType.Advanced) {
1167-
//List all networks of vm
1168-
List<Long> vmNetworks = _vmNetworkMapDao.getNetworks(vmId);
1169-
List<DomainRouterVO> routers = new ArrayList<>();
1170-
//List the stopped routers
1171-
for (long vmNetworkId : vmNetworks) {
1172-
List<DomainRouterVO> router = _routerDao.listStopped(vmNetworkId);
1173-
routers.addAll(router);
1174-
}
1175-
//A vm may not have many nics attached and even fewer routers might be stopped (only in exceptional cases)
1176-
//Safe to start the stopped router serially, this is consistent with the way how multiple networks are added to vm during deploy
1177-
//and routers are started serially ,may revisit to make this process parallel
1178-
for (DomainRouterVO routerToStart : routers) {
1179-
logger.warn("Trying to start router {} as part of vm: {} reboot", routerToStart, vm);
1180-
_virtualNetAppliance.startRouter(routerToStart.getId(),true);
1181-
}
1182-
}
1183-
} catch (ConcurrentOperationException e) {
1184-
throw new CloudRuntimeException("Concurrent operations on starting router. " + e);
1185-
} catch (Exception ex) {
1186-
throw new CloudRuntimeException("Router start failed due to" + ex);
1187-
} finally {
1188-
if (logger.isInfoEnabled()) {
1189-
logger.info("Rebooting vm {}{}.", vm, enterSetup ? " entering hardware setup menu" : " as is");
1190-
}
1191-
Map<VirtualMachineProfile.Param,Object> params = null;
1192-
if (enterSetup) {
1193-
params = new HashMap<>();
1194-
params.put(VirtualMachineProfile.Param.BootIntoSetup, Boolean.TRUE);
1195-
if (logger.isTraceEnabled()) {
1196-
logger.trace(String.format("Adding %s to paramlist", VirtualMachineProfile.Param.BootIntoSetup));
1197-
}
1198-
}
1199-
_itMgr.reboot(vm.getUuid(), params);
1156+
if (forced) {
1157+
return handleForcedReboot(vm, enterSetup);
1158+
}
1159+
1160+
try {
1161+
startRoutersIfNeeded(vm, vmId);
1162+
} catch (ConcurrentOperationException e) {
1163+
throw new CloudRuntimeException("Concurrent operations on starting router. " + e);
1164+
} catch (Exception ex) {
1165+
throw new CloudRuntimeException("Router start failed due to" + ex);
1166+
} finally {
1167+
if (logger.isInfoEnabled()) {
1168+
logger.info("Rebooting vm {}{}.", vm, enterSetup ? " entering hardware setup menu" : " as is");
12001169
}
1201-
return _vmDao.findById(vmId);
1202-
} else {
1203-
logger.error("Vm {} is not in Running state, failed to reboot", vm);
1170+
Map<VirtualMachineProfile.Param, Object> params = buildRebootParamsIfNeeded(enterSetup);
1171+
_itMgr.reboot(vm.getUuid(), params);
1172+
}
1173+
1174+
return _vmDao.findById(vmId);
1175+
}
1176+
1177+
private UserVm handleForcedReboot(UserVmVO vm, boolean enterSetup) {
1178+
Host vmOnHost = _hostDao.findById(vm.getHostId());
1179+
if (vmOnHost == null || vmOnHost.getResourceState() != ResourceState.Enabled || vmOnHost.getStatus() != Status.Up) {
1180+
throw new CloudRuntimeException("Unable to force reboot the VM as the host: " + vm.getHostId() + " is not in the right state");
1181+
}
1182+
return forceRebootVirtualMachine(vm, vm.getHostId(), enterSetup);
1183+
}
1184+
1185+
private void startRoutersIfNeeded(UserVmVO vm, long vmId) throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException {
1186+
DataCenterVO dc = _dcDao.findById(vm.getDataCenterId());
1187+
if (dc.getNetworkType() != DataCenter.NetworkType.Advanced) {
1188+
return;
1189+
}
1190+
1191+
// List all networks of vm
1192+
List<Long> vmNetworks = _vmNetworkMapDao.getNetworks(vmId);
1193+
List<DomainRouterVO> routers = new ArrayList<>();
1194+
// List the stopped routers
1195+
for (long vmNetworkId : vmNetworks) {
1196+
List<DomainRouterVO> router = _routerDao.listStopped(vmNetworkId);
1197+
routers.addAll(router);
1198+
}
1199+
1200+
// Safe to start the stopped router serially, consistent with deploy/start behavior
1201+
for (DomainRouterVO routerToStart : routers) {
1202+
logger.warn("Trying to start router {} as part of vm: {} reboot", routerToStart, vm);
1203+
_virtualNetAppliance.startRouter(routerToStart.getId(), true);
1204+
}
1205+
}
1206+
1207+
private Map<VirtualMachineProfile.Param, Object> buildRebootParamsIfNeeded(boolean enterSetup) {
1208+
if (!enterSetup) {
12041209
return null;
12051210
}
1211+
Map<VirtualMachineProfile.Param, Object> params = new HashMap<>();
1212+
params.put(VirtualMachineProfile.Param.BootIntoSetup, Boolean.TRUE);
1213+
return params;
12061214
}
12071215

12081216
private UserVm forceRebootVirtualMachine(UserVmVO vm, long hostId, boolean enterSetup) {

‎server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java‎

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@
9191
import org.apache.cloudstack.backup.dao.BackupScheduleDao;
9292
import org.apache.cloudstack.context.CallContext;
9393
import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
94+
import org.apache.cloudstack.engine.cloud.entity.api.db.dao.VMNetworkMapDao;
9495
import org.apache.cloudstack.resourcelimit.Reserver;
9596
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore;
9697
import org.apache.cloudstack.engine.subsystem.api.storage.Scope;
@@ -152,6 +153,7 @@
152153
import com.cloud.network.dao.PhysicalNetworkVO;
153154
import com.cloud.network.element.UserDataServiceProvider;
154155
import com.cloud.network.guru.NetworkGuru;
156+
import com.cloud.network.router.VpcVirtualNetworkApplianceManager;
155157
import com.cloud.network.rules.FirewallRuleVO;
156158
import com.cloud.network.rules.PortForwardingRule;
157159
import com.cloud.network.rules.dao.PortForwardingRulesDao;
@@ -205,6 +207,7 @@
205207
import com.cloud.utils.exception.ExceptionProxyObject;
206208
import com.cloud.utils.fsm.NoTransitionException;
207209
import com.cloud.vm.dao.NicDao;
210+
import com.cloud.vm.dao.DomainRouterDao;
208211
import com.cloud.vm.dao.UserVmDao;
209212
import com.cloud.vm.dao.VMInstanceDetailsDao;
210213
import com.cloud.vm.snapshot.VMSnapshotVO;
@@ -461,6 +464,15 @@ public class UserVmManagerImplTest {
461464
@Mock
462465
private BackupScheduleDao backupScheduleDao;
463466

467+
@Mock
468+
private VMNetworkMapDao _vmNetworkMapDao;
469+
470+
@Mock
471+
private DomainRouterDao _routerDao;
472+
473+
@Mock
474+
private VpcVirtualNetworkApplianceManager _virtualNetAppliance;
475+
464476
MockedStatic<UnmanagedVMsManager> unmanagedVMsManagerMockedStatic;
465477

466478
@Mock
@@ -4363,6 +4375,167 @@ public void testTransitionExpungingToErrorHandlesNoTransitionException() throws
43634375
method.invoke(userVmManagerImpl, vmId);
43644376
}
43654377

4378+
@Test
4379+
public void testBuildRebootParamsIfNeededFalseReturnsNull() {
4380+
Object params = ReflectionTestUtils.invokeMethod(userVmManagerImpl, "buildRebootParamsIfNeeded", false);
4381+
Assert.assertNull(params);
4382+
}
4383+
4384+
@Test
4385+
public void testBuildRebootParamsIfNeededTrueAddsBootIntoSetup() {
4386+
@SuppressWarnings("unchecked")
4387+
Map<VirtualMachineProfile.Param, Object> params =
4388+
ReflectionTestUtils.invokeMethod(userVmManagerImpl, "buildRebootParamsIfNeeded", true);
4389+
4390+
assertNotNull(params);
4391+
assertEquals(1, params.size());
4392+
assertEquals(Boolean.TRUE, params.get(VirtualMachineProfile.Param.BootIntoSetup));
4393+
}
4394+
4395+
@Test
4396+
public void testStartRoutersIfNeededBasicZoneSkipsRouterOperations() throws Exception {
4397+
UserVmVO vm = mock(UserVmVO.class);
4398+
when(vm.getDataCenterId()).thenReturn(zoneId);
4399+
4400+
DataCenterVO dc = mock(DataCenterVO.class);
4401+
when(_dcDao.findById(zoneId)).thenReturn(dc);
4402+
when(dc.getNetworkType()).thenReturn(DataCenter.NetworkType.Basic);
4403+
4404+
ReflectionTestUtils.invokeMethod(userVmManagerImpl, "startRoutersIfNeeded", vm, vmId);
4405+
4406+
verify(_vmNetworkMapDao, never()).getNetworks(anyLong());
4407+
verify(_virtualNetAppliance, never()).startRouter(anyLong(), anyBoolean());
4408+
}
4409+
4410+
@Test
4411+
public void testStartRoutersIfNeededAdvancedZoneWithoutNetworksStartsNothing() throws Exception {
4412+
UserVmVO vm = mock(UserVmVO.class);
4413+
when(vm.getDataCenterId()).thenReturn(zoneId);
4414+
4415+
DataCenterVO dc = mock(DataCenterVO.class);
4416+
when(_dcDao.findById(zoneId)).thenReturn(dc);
4417+
when(dc.getNetworkType()).thenReturn(DataCenter.NetworkType.Advanced);
4418+
when(_vmNetworkMapDao.getNetworks(vmId)).thenReturn(Collections.emptyList());
4419+
4420+
ReflectionTestUtils.invokeMethod(userVmManagerImpl, "startRoutersIfNeeded", vm, vmId);
4421+
4422+
verify(_vmNetworkMapDao).getNetworks(vmId);
4423+
verify(_routerDao, never()).listStopped(anyLong());
4424+
verify(_virtualNetAppliance, never()).startRouter(anyLong(), anyBoolean());
4425+
}
4426+
4427+
@Test
4428+
public void testStartRoutersIfNeededAdvancedZoneStartsAllStoppedRouters() throws Exception {
4429+
UserVmVO vm = mock(UserVmVO.class);
4430+
when(vm.getDataCenterId()).thenReturn(zoneId);
4431+
4432+
DataCenterVO dc = mock(DataCenterVO.class);
4433+
when(_dcDao.findById(zoneId)).thenReturn(dc);
4434+
when(dc.getNetworkType()).thenReturn(DataCenter.NetworkType.Advanced);
4435+
4436+
when(_vmNetworkMapDao.getNetworks(vmId)).thenReturn(Arrays.asList(100L, 200L));
4437+
4438+
DomainRouterVO router1 = mock(DomainRouterVO.class);
4439+
DomainRouterVO router2 = mock(DomainRouterVO.class);
4440+
DomainRouterVO router3 = mock(DomainRouterVO.class);
4441+
when(router1.getId()).thenReturn(1000L);
4442+
when(router2.getId()).thenReturn(1001L);
4443+
when(router3.getId()).thenReturn(2000L);
4444+
4445+
when(_routerDao.listStopped(100L)).thenReturn(Arrays.asList(router1, router2));
4446+
when(_routerDao.listStopped(200L)).thenReturn(Collections.singletonList(router3));
4447+
4448+
ReflectionTestUtils.invokeMethod(userVmManagerImpl, "startRoutersIfNeeded", vm, vmId);
4449+
4450+
verify(_virtualNetAppliance).startRouter(1000L, true);
4451+
verify(_virtualNetAppliance).startRouter(1001L, true);
4452+
verify(_virtualNetAppliance).startRouter(2000L, true);
4453+
}
4454+
4455+
@Test
4456+
public void testStartRoutersIfNeededPropagatesRouterStartException() throws Exception {
4457+
UserVmVO vm = mock(UserVmVO.class);
4458+
when(vm.getDataCenterId()).thenReturn(zoneId);
4459+
4460+
DataCenterVO dc = mock(DataCenterVO.class);
4461+
when(_dcDao.findById(zoneId)).thenReturn(dc);
4462+
when(dc.getNetworkType()).thenReturn(DataCenter.NetworkType.Advanced);
4463+
when(_vmNetworkMapDao.getNetworks(vmId)).thenReturn(Collections.singletonList(100L));
4464+
4465+
DomainRouterVO router = mock(DomainRouterVO.class);
4466+
when(router.getId()).thenReturn(1000L);
4467+
when(_routerDao.listStopped(100L)).thenReturn(Collections.singletonList(router));
4468+
doThrow(new CloudRuntimeException("router start failed")).when(_virtualNetAppliance).startRouter(1000L, true);
4469+
4470+
assertThrows(CloudRuntimeException.class,
4471+
() -> ReflectionTestUtils.invokeMethod(userVmManagerImpl, "startRoutersIfNeeded", vm, vmId));
4472+
}
4473+
4474+
@Test
4475+
public void testHandleForcedRebootThrowsWhenHostNotFound() {
4476+
UserVmVO vm = mock(UserVmVO.class);
4477+
when(vm.getHostId()).thenReturn(42L);
4478+
4479+
when(hostDao.findById(42L)).thenReturn(null);
4480+
4481+
assertThrows(CloudRuntimeException.class,
4482+
() -> ReflectionTestUtils.invokeMethod(userVmManagerImpl, "handleForcedReboot", vm, false));
4483+
}
4484+
4485+
@Test
4486+
public void testHandleForcedRebootThrowsWhenHostNotUsable() {
4487+
UserVmVO vm = mock(UserVmVO.class);
4488+
when(vm.getHostId()).thenReturn(42L);
4489+
4490+
HostVO host = mock(HostVO.class);
4491+
when(hostDao.findById(42L)).thenReturn(host);
4492+
when(host.getResourceState()).thenReturn(com.cloud.resource.ResourceState.Enabled);
4493+
when(host.getStatus()).thenReturn(com.cloud.host.Status.Down);
4494+
4495+
assertThrows(CloudRuntimeException.class,
4496+
() -> ReflectionTestUtils.invokeMethod(userVmManagerImpl, "handleForcedReboot", vm, false));
4497+
}
4498+
4499+
@Test
4500+
public void testHandleForcedRebootReturnsNullWhenStopFails() {
4501+
UserVmVO vm = mock(UserVmVO.class);
4502+
when(vm.getId()).thenReturn(vmId);
4503+
when(vm.getHostId()).thenReturn(42L);
4504+
4505+
HostVO host = mock(HostVO.class);
4506+
when(hostDao.findById(42L)).thenReturn(host);
4507+
when(host.getResourceState()).thenReturn(com.cloud.resource.ResourceState.Enabled);
4508+
when(host.getStatus()).thenReturn(com.cloud.host.Status.Up);
4509+
4510+
doReturn(null).when(userVmManagerImpl).stopVirtualMachine(vmId, false);
4511+
4512+
Object result = ReflectionTestUtils.invokeMethod(userVmManagerImpl, "handleForcedReboot", vm, false);
4513+
Assert.assertNull(result);
4514+
}
4515+
4516+
@Test
4517+
public void testHandleForcedRebootSuccessWithEnterSetup() throws Exception {
4518+
UserVmVO vm = mock(UserVmVO.class);
4519+
when(vm.getId()).thenReturn(vmId);
4520+
when(vm.getHostId()).thenReturn(42L);
4521+
4522+
HostVO host = mock(HostVO.class);
4523+
when(hostDao.findById(42L)).thenReturn(host);
4524+
when(host.getResourceState()).thenReturn(com.cloud.resource.ResourceState.Enabled);
4525+
when(host.getStatus()).thenReturn(com.cloud.host.Status.Up);
4526+
4527+
UserVmVO stoppedVm = mock(UserVmVO.class);
4528+
doReturn(stoppedVm).when(userVmManagerImpl).stopVirtualMachine(vmId, false);
4529+
4530+
Pair<UserVmVO, Map<VirtualMachineProfile.Param, Object>> startedVm = new Pair<>(userVmVoMock, new HashMap<>());
4531+
doReturn(startedVm).when(userVmManagerImpl).startVirtualMachine(eq(vmId), isNull(), isNull(), eq(42L), anyMap(), isNull(), eq(false));
4532+
4533+
UserVm result = ReflectionTestUtils.invokeMethod(userVmManagerImpl, "handleForcedReboot", vm, true);
4534+
4535+
assertEquals(userVmVoMock, result);
4536+
verify(userVmManagerImpl).stopVirtualMachine(vmId, false);
4537+
}
4538+
43664539
private ServiceOfferingVO getMockedServiceOffering(boolean custom, boolean customSpeed) {
43674540
ServiceOfferingVO serviceOffering = mock(ServiceOfferingVO.class);
43684541
when(serviceOffering.getUuid()).thenReturn("offering-uuid");

‎utils/src/main/java/com/cloud/utils/StringUtils.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -438,4 +438,9 @@ public static String getFirstValueFromCommaSeparatedString(String inputString) {
438438

439439
return null;
440440
}
441+
442+
public static boolean isBlank(String str) {
443+
return org.apache.commons.lang3.StringUtils.isBlank(str);
444+
}
445+
441446
}

0 commit comments

Comments
 (0)