Copilot commented on code in PR #12939:
URL: https://github.com/apache/cloudstack/pull/12939#discussion_r3020349618
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -2713,22 +2714,29 @@ private void updateVmStateForFailedVmCreation(Long
vmId, Long hostId) {
if (vm != null) {
if (vm.getState().equals(State.Stopped)) {
HostVO host = _hostDao.findById(hostId);
- logger.debug("Destroying vm {} as it failed to create on Host:
{} with id {}", vm, host, hostId);
+ logger.debug("Destroying vm [{}] as it was unable to be
deployed on Host: {} with id {}", vm, host, hostId);
try {
_itMgr.stateTransitTo(vm,
VirtualMachine.Event.OperationFailedToError, null);
} catch (NoTransitionException e1) {
- logger.warn(e1.getMessage());
+ logger.error("Error when transitioning state of [{}] due
to [{}].", vm, e1.getMessage());
}
// destroy associated volumes for vm in error state
// get all volumes in non destroyed state
+ logger.debug("Destroying associated volumes of [{}] as it was
unable to be deployed.", vm);
List<VolumeVO> volumesForThisVm =
_volsDao.findUsableVolumesForInstance(vm.getId());
for (VolumeVO volume : volumesForThisVm) {
if (volume.getState() != Volume.State.Destroy) {
+ logger.trace("Destroying volume [{}] as its VM was
unable to be deployed.", volume);
volumeMgr.destroyVolume(volume);
}
}
- String msg = String.format("Failed to deploy Vm %s, on Host %s
with Id: %d", vm, host, hostId);
- _alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_USERVM,
vm.getDataCenterId(), vm.getPodIdToDeployIn(), msg, msg);
+ String subject = String.format("Failed to deploy Instance [ID:
%s]", vm.getId());
+ String body = String.format("Failed to deploy [%s]%s. To
troubleshoot, please check the logs with [logid:%s].",
+ vm,
+ hostId != null ? String.format(" on host [%s]",
hostId) : "",
+ ThreadContext.get("logcontextid"));
Review Comment:
`ThreadContext.get("logcontextid")` can return null/empty if the logging
context isn't initialized for the current thread, which will produce alert
emails containing `[logid:null]`. Consider ensuring a LogContext exists (or
guarding with `StringUtils.isNotBlank`) and omitting/adjusting the logid text
when unavailable.
```suggestion
String logContextId = ThreadContext.get("logcontextid");
String logIdText = StringUtils.isNotBlank(logContextId)
? String.format(" To troubleshoot, please check the
logs with [logid:%s].", logContextId)
: "";
String body = String.format("Failed to deploy [%s]%s.%s",
vm,
hostId != null ? String.format(" on host [%s]",
hostId) : "",
logIdText);
```
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -2713,22 +2714,29 @@ private void updateVmStateForFailedVmCreation(Long
vmId, Long hostId) {
if (vm != null) {
if (vm.getState().equals(State.Stopped)) {
HostVO host = _hostDao.findById(hostId);
- logger.debug("Destroying vm {} as it failed to create on Host:
{} with id {}", vm, host, hostId);
+ logger.debug("Destroying vm [{}] as it was unable to be
deployed on Host: {} with id {}", vm, host, hostId);
try {
_itMgr.stateTransitTo(vm,
VirtualMachine.Event.OperationFailedToError, null);
} catch (NoTransitionException e1) {
- logger.warn(e1.getMessage());
+ logger.error("Error when transitioning state of [{}] due
to [{}].", vm, e1.getMessage());
Review Comment:
The NoTransitionException is logged with only `e1.getMessage()` and without
the exception itself, which drops the stack trace and makes this harder to
debug. Log the exception object (and consider keeping severity consistent with
similar transitions in this class) so the root cause is preserved in logs.
```suggestion
logger.error("Error when transitioning state of [{}].",
vm, e1);
```
--
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]
