Skip to content

Commit b24cb08

Browse files
author
Daan Hoogland
committed
sonar remarks addressed and some extra tests generated
1 parent 229073d commit b24cb08

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
@@ -1142,68 +1142,76 @@ public boolean stopVirtualMachine(long userId, long vmId) {
11421142
private UserVm rebootVirtualMachine(long vmId, boolean enterSetup, boolean forced) throws InsufficientCapacityException, ResourceUnavailableException {
11431143
UserVmVO vm = _vmDao.findById(vmId);
11441144

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

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

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

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

12091217
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
@@ -85,6 +85,7 @@
8585
import org.apache.cloudstack.backup.dao.BackupScheduleDao;
8686
import org.apache.cloudstack.context.CallContext;
8787
import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
88+
import org.apache.cloudstack.engine.cloud.entity.api.db.dao.VMNetworkMapDao;
8889
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore;
8990
import org.apache.cloudstack.engine.subsystem.api.storage.Scope;
9091
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
@@ -147,6 +148,7 @@
147148
import com.cloud.network.dao.PhysicalNetworkDao;
148149
import com.cloud.network.dao.PhysicalNetworkVO;
149150
import com.cloud.network.guru.NetworkGuru;
151+
import com.cloud.network.router.VpcVirtualNetworkApplianceManager;
150152
import com.cloud.network.rules.FirewallRuleVO;
151153
import com.cloud.network.rules.PortForwardingRule;
152154
import com.cloud.network.rules.dao.PortForwardingRulesDao;
@@ -196,6 +198,7 @@
196198
import com.cloud.utils.exception.CloudRuntimeException;
197199
import com.cloud.utils.exception.ExceptionProxyObject;
198200
import com.cloud.vm.dao.NicDao;
201+
import com.cloud.vm.dao.DomainRouterDao;
199202
import com.cloud.vm.dao.UserVmDao;
200203
import com.cloud.vm.dao.VMInstanceDetailsDao;
201204
import com.cloud.vm.snapshot.VMSnapshotVO;
@@ -446,6 +449,15 @@ public class UserVmManagerImplTest {
446449
@Mock
447450
private BackupScheduleDao backupScheduleDao;
448451

452+
@Mock
453+
private VMNetworkMapDao _vmNetworkMapDao;
454+
455+
@Mock
456+
private DomainRouterDao _routerDao;
457+
458+
@Mock
459+
private VpcVirtualNetworkApplianceManager _virtualNetAppliance;
460+
449461
MockedStatic<UnmanagedVMsManager> unmanagedVMsManagerMockedStatic;
450462

451463
private static final long vmId = 1L;
@@ -4232,4 +4244,165 @@ public void testTransitionExpungingToErrorHandlesNoTransitionException() throws
42324244
method.setAccessible(true);
42334245
method.invoke(userVmManagerImpl, vmId);
42344246
}
4247+
4248+
@Test
4249+
public void testBuildRebootParamsIfNeededFalseReturnsNull() {
4250+
Object params = ReflectionTestUtils.invokeMethod(userVmManagerImpl, "buildRebootParamsIfNeeded", false);
4251+
Assert.assertNull(params);
4252+
}
4253+
4254+
@Test
4255+
public void testBuildRebootParamsIfNeededTrueAddsBootIntoSetup() {
4256+
@SuppressWarnings("unchecked")
4257+
Map<VirtualMachineProfile.Param, Object> params =
4258+
ReflectionTestUtils.invokeMethod(userVmManagerImpl, "buildRebootParamsIfNeeded", true);
4259+
4260+
assertNotNull(params);
4261+
assertEquals(1, params.size());
4262+
assertEquals(Boolean.TRUE, params.get(VirtualMachineProfile.Param.BootIntoSetup));
4263+
}
4264+
4265+
@Test
4266+
public void testStartRoutersIfNeededBasicZoneSkipsRouterOperations() throws Exception {
4267+
UserVmVO vm = mock(UserVmVO.class);
4268+
when(vm.getDataCenterId()).thenReturn(zoneId);
4269+
4270+
DataCenterVO dc = mock(DataCenterVO.class);
4271+
when(_dcDao.findById(zoneId)).thenReturn(dc);
4272+
when(dc.getNetworkType()).thenReturn(DataCenter.NetworkType.Basic);
4273+
4274+
ReflectionTestUtils.invokeMethod(userVmManagerImpl, "startRoutersIfNeeded", vm, vmId);
4275+
4276+
verify(_vmNetworkMapDao, never()).getNetworks(anyLong());
4277+
verify(_virtualNetAppliance, never()).startRouter(anyLong(), anyBoolean());
4278+
}
4279+
4280+
@Test
4281+
public void testStartRoutersIfNeededAdvancedZoneWithoutNetworksStartsNothing() throws Exception {
4282+
UserVmVO vm = mock(UserVmVO.class);
4283+
when(vm.getDataCenterId()).thenReturn(zoneId);
4284+
4285+
DataCenterVO dc = mock(DataCenterVO.class);
4286+
when(_dcDao.findById(zoneId)).thenReturn(dc);
4287+
when(dc.getNetworkType()).thenReturn(DataCenter.NetworkType.Advanced);
4288+
when(_vmNetworkMapDao.getNetworks(vmId)).thenReturn(Collections.emptyList());
4289+
4290+
ReflectionTestUtils.invokeMethod(userVmManagerImpl, "startRoutersIfNeeded", vm, vmId);
4291+
4292+
verify(_vmNetworkMapDao).getNetworks(vmId);
4293+
verify(_routerDao, never()).listStopped(anyLong());
4294+
verify(_virtualNetAppliance, never()).startRouter(anyLong(), anyBoolean());
4295+
}
4296+
4297+
@Test
4298+
public void testStartRoutersIfNeededAdvancedZoneStartsAllStoppedRouters() throws Exception {
4299+
UserVmVO vm = mock(UserVmVO.class);
4300+
when(vm.getDataCenterId()).thenReturn(zoneId);
4301+
4302+
DataCenterVO dc = mock(DataCenterVO.class);
4303+
when(_dcDao.findById(zoneId)).thenReturn(dc);
4304+
when(dc.getNetworkType()).thenReturn(DataCenter.NetworkType.Advanced);
4305+
4306+
when(_vmNetworkMapDao.getNetworks(vmId)).thenReturn(Arrays.asList(100L, 200L));
4307+
4308+
DomainRouterVO router1 = mock(DomainRouterVO.class);
4309+
DomainRouterVO router2 = mock(DomainRouterVO.class);
4310+
DomainRouterVO router3 = mock(DomainRouterVO.class);
4311+
when(router1.getId()).thenReturn(1000L);
4312+
when(router2.getId()).thenReturn(1001L);
4313+
when(router3.getId()).thenReturn(2000L);
4314+
4315+
when(_routerDao.listStopped(100L)).thenReturn(Arrays.asList(router1, router2));
4316+
when(_routerDao.listStopped(200L)).thenReturn(Collections.singletonList(router3));
4317+
4318+
ReflectionTestUtils.invokeMethod(userVmManagerImpl, "startRoutersIfNeeded", vm, vmId);
4319+
4320+
verify(_virtualNetAppliance).startRouter(1000L, true);
4321+
verify(_virtualNetAppliance).startRouter(1001L, true);
4322+
verify(_virtualNetAppliance).startRouter(2000L, true);
4323+
}
4324+
4325+
@Test
4326+
public void testStartRoutersIfNeededPropagatesRouterStartException() throws Exception {
4327+
UserVmVO vm = mock(UserVmVO.class);
4328+
when(vm.getDataCenterId()).thenReturn(zoneId);
4329+
4330+
DataCenterVO dc = mock(DataCenterVO.class);
4331+
when(_dcDao.findById(zoneId)).thenReturn(dc);
4332+
when(dc.getNetworkType()).thenReturn(DataCenter.NetworkType.Advanced);
4333+
when(_vmNetworkMapDao.getNetworks(vmId)).thenReturn(Collections.singletonList(100L));
4334+
4335+
DomainRouterVO router = mock(DomainRouterVO.class);
4336+
when(router.getId()).thenReturn(1000L);
4337+
when(_routerDao.listStopped(100L)).thenReturn(Collections.singletonList(router));
4338+
doThrow(new CloudRuntimeException("router start failed")).when(_virtualNetAppliance).startRouter(1000L, true);
4339+
4340+
assertThrows(CloudRuntimeException.class,
4341+
() -> ReflectionTestUtils.invokeMethod(userVmManagerImpl, "startRoutersIfNeeded", vm, vmId));
4342+
}
4343+
4344+
@Test
4345+
public void testHandleForcedRebootThrowsWhenHostNotFound() {
4346+
UserVmVO vm = mock(UserVmVO.class);
4347+
when(vm.getHostId()).thenReturn(42L);
4348+
4349+
when(hostDao.findById(42L)).thenReturn(null);
4350+
4351+
assertThrows(CloudRuntimeException.class,
4352+
() -> ReflectionTestUtils.invokeMethod(userVmManagerImpl, "handleForcedReboot", vm, false));
4353+
}
4354+
4355+
@Test
4356+
public void testHandleForcedRebootThrowsWhenHostNotUsable() {
4357+
UserVmVO vm = mock(UserVmVO.class);
4358+
when(vm.getHostId()).thenReturn(42L);
4359+
4360+
HostVO host = mock(HostVO.class);
4361+
when(hostDao.findById(42L)).thenReturn(host);
4362+
when(host.getResourceState()).thenReturn(com.cloud.resource.ResourceState.Enabled);
4363+
when(host.getStatus()).thenReturn(com.cloud.host.Status.Down);
4364+
4365+
assertThrows(CloudRuntimeException.class,
4366+
() -> ReflectionTestUtils.invokeMethod(userVmManagerImpl, "handleForcedReboot", vm, false));
4367+
}
4368+
4369+
@Test
4370+
public void testHandleForcedRebootReturnsNullWhenStopFails() {
4371+
UserVmVO vm = mock(UserVmVO.class);
4372+
when(vm.getId()).thenReturn(vmId);
4373+
when(vm.getHostId()).thenReturn(42L);
4374+
4375+
HostVO host = mock(HostVO.class);
4376+
when(hostDao.findById(42L)).thenReturn(host);
4377+
when(host.getResourceState()).thenReturn(com.cloud.resource.ResourceState.Enabled);
4378+
when(host.getStatus()).thenReturn(com.cloud.host.Status.Up);
4379+
4380+
doReturn(null).when(userVmManagerImpl).stopVirtualMachine(vmId, false);
4381+
4382+
Object result = ReflectionTestUtils.invokeMethod(userVmManagerImpl, "handleForcedReboot", vm, false);
4383+
Assert.assertNull(result);
4384+
}
4385+
4386+
@Test
4387+
public void testHandleForcedRebootSuccessWithEnterSetup() throws Exception {
4388+
UserVmVO vm = mock(UserVmVO.class);
4389+
when(vm.getId()).thenReturn(vmId);
4390+
when(vm.getHostId()).thenReturn(42L);
4391+
4392+
HostVO host = mock(HostVO.class);
4393+
when(hostDao.findById(42L)).thenReturn(host);
4394+
when(host.getResourceState()).thenReturn(com.cloud.resource.ResourceState.Enabled);
4395+
when(host.getStatus()).thenReturn(com.cloud.host.Status.Up);
4396+
4397+
UserVmVO stoppedVm = mock(UserVmVO.class);
4398+
doReturn(stoppedVm).when(userVmManagerImpl).stopVirtualMachine(vmId, false);
4399+
4400+
Pair<UserVmVO, Map<VirtualMachineProfile.Param, Object>> startedVm = new Pair<>(userVmVoMock, new HashMap<>());
4401+
doReturn(startedVm).when(userVmManagerImpl).startVirtualMachine(eq(vmId), isNull(), isNull(), eq(42L), anyMap(), isNull(), eq(false));
4402+
4403+
UserVm result = ReflectionTestUtils.invokeMethod(userVmManagerImpl, "handleForcedReboot", vm, true);
4404+
4405+
assertEquals(userVmVoMock, result);
4406+
verify(userVmManagerImpl).stopVirtualMachine(vmId, false);
4407+
}
42354408
}

‎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)