Copilot commented on code in PR #12779:
URL: https://github.com/apache/cloudstack/pull/12779#discussion_r3186757312
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -2833,8 +2797,8 @@ protected void runInContext() {
try {
List<UserVmVO> vms = _vmDao.findDestroyedVms(new
Date(System.currentTimeMillis() - ((long)_expungeDelay << 10)));
if (logger.isInfoEnabled()) {
- if (vms.size() == 0) {
- logger.trace("Found " + vms.size() + "
Instances to expunge.");
+ if (vms.isEmpty()) {
+ logger.trace("Found no Instances to expunge.");
} else {
logger.info("Found " + vms.size() + "
Instances to expunge.");
Review Comment:
The empty-case log uses `logger.trace(...)` but is guarded by
`logger.isInfoEnabled()`. This makes the guard misleading and may suppress the
message even when info logging is enabled. Consider either logging at
info/debug here, or guarding the trace log with `logger.isTraceEnabled()`
(and/or removing the `isInfoEnabled()` guard).
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -1144,78 +1132,85 @@ 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 || vm.getHostId() == null) {
+ logger.error("Vm {} is not in Running state, failed to reboot",
vm);
+ return null;
+ }
Review Comment:
This branch also triggers when `vm.getHostId() == null`, but the log message
says the VM is not in Running state. Consider adjusting the condition/log to
report the actual reason (e.g., running-without-host vs not-running) to avoid
misleading operational logs during reboot failures.
##########
utils/src/main/java/com/cloud/utils/StringUtils.java:
##########
@@ -438,4 +438,9 @@ public static String
getFirstValueFromCommaSeparatedString(String inputString) {
return null;
}
+
+ public static boolean isBlank(String str) {
+ return org.apache.commons.lang3.StringUtils.isBlank(str);
+ }
+
Review Comment:
`com.cloud.utils.StringUtils` already extends
`org.apache.commons.lang3.StringUtils`, so `isBlank` is already available.
Adding a new `isBlank` method here just hides the inherited static method and
is redundant; consider removing it to avoid confusion and potential
static-analysis warnings about hiding inherited statics.
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -5978,15 +5905,21 @@ protected String
getCurrentVmPasswordOrDefineNewPassword(String newPassword, Use
return password;
}
- private Map<VirtualMachineProfile.Param, Object>
createParameterInParameterMap(Map<VirtualMachineProfile.Param, Object> params,
Map<VirtualMachineProfile.Param, Object> parameterMap,
VirtualMachineProfile.Param parameter,
+ /**
+ * Create or overwrite a parameter in the list
+ * @param params the list of parameters
+ * @param parameter the parameter to creat/overwrite
Review Comment:
The Javadoc has a typo (“creat/overwrite” → “create/overwrite”) and is
missing `@param parameterMap` even though the method takes it. Please fix the
typo and add the missing parameter documentation so the comment matches the
method signature.
--
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]
