Copilot commented on code in PR #12879:
URL: https://github.com/apache/cloudstack/pull/12879#discussion_r2980152809
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -1194,21 +1194,21 @@ private UserVm rebootVirtualMachine(long userId, long
vmId, boolean enterSetup,
List<Long> vmNetworks = _vmNetworkMapDao.getNetworks(vmId);
List<DomainRouterVO> routers = new
ArrayList<DomainRouterVO>();
//List the stopped routers
- for(long vmNetworkId : vmNetworks) {
+ 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) {
+ 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){
+ } catch (Exception ex) {
throw new CloudRuntimeException("Router start failed due to" +
ex);
Review Comment:
The exception message concatenation is missing a space ("due to" + ex) and
the thrown CloudRuntimeException drops the original cause/stack trace. Consider
including a separating space and passing the caught exception as the cause so
callers/logs preserve details.
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -9221,8 +9219,14 @@ private void handleManagedStorage(UserVmVO vm, VolumeVO
root) {
Long hostId = vm.getHostId() != null ? vm.getHostId() :
vm.getLastHostId();
if (hostId != null) {
- VolumeInfo volumeInfo = volFactory.getVolume(root.getId());
+ // default findById() won't search entries with removed field
not null
Host host = _hostDao.findById(hostId);
+ if (host == null) {
+ logger.warn("Host {} not found", hostId);
Review Comment:
This new log line only prints the hostId; when debugging restore/re-image
issues it would be helpful to include the VM and root volume identifiers (e.g.,
vm UUID/id and root volume UUID/id) in the warning so operators can correlate
the skip with the affected resources.
```suggestion
logger.warn("Host {} not found for vm id: {}, uuid: {},
root volume id: {}, uuid: {}", hostId, vm.getId(), vm.getUuid(), root.getId(),
root.getUuid());
```
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -9221,8 +9219,14 @@ private void handleManagedStorage(UserVmVO vm, VolumeVO
root) {
Long hostId = vm.getHostId() != null ? vm.getHostId() :
vm.getLastHostId();
if (hostId != null) {
- VolumeInfo volumeInfo = volFactory.getVolume(root.getId());
+ // default findById() won't search entries with removed field
not null
Host host = _hostDao.findById(hostId);
+ if (host == null) {
+ logger.warn("Host {} not found", hostId);
+ return;
+ }
Review Comment:
There are unit tests for restoreVirtualMachine() in
server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java, but none appear
to cover the managed-storage restore path where vm.getLastHostId() is set and
_hostDao.findById(hostId) returns null (deleted host). Adding a test for this
scenario would help prevent regressions (e.g., ensure restore proceeds without
leaving an extra ROOT volume in Allocated state).
--
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]
