Copilot commented on code in PR #12779:
URL: https://github.com/apache/cloudstack/pull/12779#discussion_r3195274466
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -1144,78 +1132,89 @@ public boolean stopVirtualMachine(long userId, long
vmId) {
status = vmEntity.stop(Long.toString(userId));
} catch (ResourceUnavailableException e) {
logger.debug("Unable to stop due to ", e);
- status = false;
} catch (CloudException e) {
throw new CloudRuntimeException("Unable to contact the agent to
stop the Instance " + vm, e);
}
return status;
}
- private UserVm rebootVirtualMachine(long userId, long vmId, boolean
enterSetup, boolean forced) throws InsufficientCapacityException,
ResourceUnavailableException {
+ 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 (vm.getState() != State.Running) {
+ logger.error("Vm {} is not in Running state, failed to reboot",
vm);
+ return null;
+ }
+ if ( null == vm.getHostId() ) {
+ logger.error("Vm {} , is in state 'Running', but doesn't have a
host id, failed to reboot", vm);
+ return null;
+ }
- 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);
- }
+ collectVmDiskAndNetworkStatistics(vm, State.Running);
- DataCenterVO dc = _dcDao.findById(vm.getDataCenterId());
- try {
- if (dc.getNetworkType() == DataCenter.NetworkType.Advanced) {
- //List all networks of vm
- List<Long> vmNetworks = _vmNetworkMapDao.getNetworks(vmId);
- List<DomainRouterVO> routers = new ArrayList<>();
- //List the stopped routers
- for (long vmNetworkId : vmNetworks) {
- List<DomainRouterVO> 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<VirtualMachineProfile.Param,Object> 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);
+ 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");
}
- return _vmDao.findById(vmId);
- } else {
- logger.error("Vm {} is not in Running state, failed to reboot",
vm);
+ Map<VirtualMachineProfile.Param, Object> 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<Long> vmNetworks = _vmNetworkMapDao.getNetworks(vmId);
+ List<DomainRouterVO> routers = new ArrayList<>();
+ // List the stopped routers
+ for (long vmNetworkId : vmNetworks) {
+ List<DomainRouterVO> router = _routerDao.listStopped(vmNetworkId);
+ routers.addAll(router);
+ }
+
+ // Safe to start the stopped router serially, consistent with
deploy/start behavior
+ for (DomainRouterVO routerToStart : routers) {
Review Comment:
`startRoutersIfNeeded` aggregates routers from each VM network without
de-duplicating. In VPC/Advanced setups the same router can be associated with
multiple networks, which can lead to repeated `startRouter` calls for the same
router ID and potentially trigger concurrent-operation failures. Consider
de-duplicating by router ID (e.g., use a Set of IDs) before starting routers.
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -1144,78 +1132,89 @@ public boolean stopVirtualMachine(long userId, long
vmId) {
status = vmEntity.stop(Long.toString(userId));
} catch (ResourceUnavailableException e) {
logger.debug("Unable to stop due to ", e);
- status = false;
} catch (CloudException e) {
throw new CloudRuntimeException("Unable to contact the agent to
stop the Instance " + vm, e);
}
return status;
}
- private UserVm rebootVirtualMachine(long userId, long vmId, boolean
enterSetup, boolean forced) throws InsufficientCapacityException,
ResourceUnavailableException {
+ 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 (vm.getState() != State.Running) {
+ logger.error("Vm {} is not in Running state, failed to reboot",
vm);
+ return null;
+ }
+ if ( null == vm.getHostId() ) {
+ logger.error("Vm {} , is in state 'Running', but doesn't have a
host id, failed to reboot", vm);
+ return null;
+ }
- 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);
- }
+ collectVmDiskAndNetworkStatistics(vm, State.Running);
- DataCenterVO dc = _dcDao.findById(vm.getDataCenterId());
- try {
- if (dc.getNetworkType() == DataCenter.NetworkType.Advanced) {
- //List all networks of vm
- List<Long> vmNetworks = _vmNetworkMapDao.getNetworks(vmId);
- List<DomainRouterVO> routers = new ArrayList<>();
- //List the stopped routers
- for (long vmNetworkId : vmNetworks) {
- List<DomainRouterVO> 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<VirtualMachineProfile.Param,Object> 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);
+ 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);
Review Comment:
In the router-start error handling, the caught exception is concatenated
into the message and then discarded (no cause is set). This loses stack traces
and produces messages like "due to<exception>" without spacing. Wrap these
exceptions with a CloudRuntimeException that includes the original exception as
the cause (and prefer parameterized messages) so failures are diagnosable.
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -4751,11 +4689,11 @@ protected long configureCustomRootDiskSize(Map<String,
String> customParameters,
}
if (customParameters.containsKey(VmDetailConstants.ROOT_DISK_SIZE)) {
- Long rootDiskSize =
NumbersUtil.parseLong(customParameters.get(VmDetailConstants.ROOT_DISK_SIZE),
-1);
+ long rootDiskSize =
NumbersUtil.parseLong(customParameters.get(VmDetailConstants.ROOT_DISK_SIZE),
-1);
if (rootDiskSize <= 0) {
throw new InvalidParameterValueException("Root disk size
should be a positive number.");
}
- rootDiskSize = rootDiskSizeCustomParam * GiB_TO_BYTES;
+ rootDiskSize = (rootDiskSizeCustomParam == null ? 0 :
rootDiskSizeCustomParam) * GiB_TO_BYTES;
Review Comment:
`configureCustomRootDiskSize` parses `rootDiskSize` from `customParameters`,
validates it, and then immediately overwrites it using
`rootDiskSizeCustomParam`. This makes the `rootDiskSize` parse/validation
redundant and harder to follow. Use a single parsed variable consistently (or
remove the second parse) to avoid dead/duplicated logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
