Copilot commented on code in PR #13669:
URL: https://github.com/apache/cloudstack/pull/13669#discussion_r3630558304
##########
server/src/main/java/com/cloud/resource/ResourceManagerImpl.java:
##########
@@ -1726,6 +1726,9 @@ protected boolean
setHostIntoErrorInPrepareForMaintenance(HostVO host, List<VMIn
logger.debug("Host {} entering in PrepareForMaintenanceWithErrors
state", host);
configureVncAccessForKVMHostFailedMigrations(host, errorVms);
resourceStateTransitTo(host, ResourceState.Event.UnableToMigrate,
_nodeId);
+
ActionEventUtils.onCompletedActionEvent(CallContext.current().getCallingUserId(),
CallContext.current().getCallingAccountId(),
+ EventVO.LEVEL_ERROR,
EventTypes.EVENT_MAINTENANCE_PREPARE_ERROR,
+ String.format("failed to prepare host %s for maintenance due
to migration or VM state errors", host), host.getId(), null, 0);
Review Comment:
This introduces a potential NPE if
`setHostIntoErrorInPrepareForMaintenance(...)` executes on a thread without a
registered `CallContext` (common for internal/scheduled workflows). Consider
capturing `CallContext current = CallContext.current();` once and falling back
to system user/account IDs (or registering a system call context) when
`current` is null.
##########
server/src/main/java/com/cloud/resource/ResourceManagerImpl.java:
##########
@@ -1726,6 +1726,9 @@ protected boolean
setHostIntoErrorInPrepareForMaintenance(HostVO host, List<VMIn
logger.debug("Host {} entering in PrepareForMaintenanceWithErrors
state", host);
configureVncAccessForKVMHostFailedMigrations(host, errorVms);
resourceStateTransitTo(host, ResourceState.Event.UnableToMigrate,
_nodeId);
+
ActionEventUtils.onCompletedActionEvent(CallContext.current().getCallingUserId(),
CallContext.current().getCallingAccountId(),
+ EventVO.LEVEL_ERROR,
EventTypes.EVENT_MAINTENANCE_PREPARE_ERROR,
+ String.format("failed to prepare host %s for maintenance due
to migration or VM state errors", host), host.getId(), null, 0);
Review Comment:
Using `%s` with `host` relies on `HostVO.toString()` which can be noisy
and/or unstable over time, making event descriptions harder to search/alert on
consistently. Prefer a stable identifier in the message (e.g., host name and/or
ID) so ops tooling and tests remain robust.
##########
server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java:
##########
@@ -442,16 +445,26 @@ private void verifyErrorInMaintenanceCalls() throws
NoTransitionException {
}
private void verifyErrorInPrepareForMaintenanceCalls() throws
NoTransitionException {
+
when(host.getResourceState()).thenReturn(ResourceState.PrepareForMaintenance);
boolean enterMaintenanceMode =
resourceManager.checkAndMaintain(hostId);
verify(resourceManager).attemptMaintain(host);
verify(resourceManager).setHostIntoErrorInPrepareForMaintenance(eq(host),
any());
verify(resourceManager, never()).setHostIntoMaintenance(any());
verify(resourceManager, never()).setHostIntoErrorInMaintenance(any(),
any());
verify(resourceManager,
never()).setHostIntoPrepareForMaintenanceAfterErrorsFixed(any());
verify(resourceManager).resourceStateTransitTo(eq(host),
eq(UnableToMigrate), anyLong());
+ String expectedDescription = String.format("failed to prepare host %s
for maintenance due to migration or VM state errors", host);
+ actionEventUtilsMocked.verify(() ->
ActionEventUtils.onCompletedActionEvent(
+ anyLong(), anyLong(), eq(EventVO.LEVEL_ERROR),
eq(EventTypes.EVENT_MAINTENANCE_PREPARE_ERROR),
+ eq(expectedDescription), eq(hostId), isNull(), eq(0L)));
Review Comment:
This assertion is brittle because it depends on the mock/VO `toString()`
output. To reduce test fragility, match the description with a partial/semantic
matcher (e.g., `contains(\"failed to prepare host\")` and a stable host
identifier) rather than an exact full string.
##########
api/src/main/java/com/cloud/event/EventTypes.java:
##########
@@ -458,6 +458,7 @@ public class EventTypes {
public static final String EVENT_MAINTENANCE_CANCEL = "MAINT.CANCEL";
public static final String EVENT_MAINTENANCE_CANCEL_PRIMARY_STORAGE =
"MAINT.CANCEL.PS";
public static final String EVENT_MAINTENANCE_PREPARE = "MAINT.PREPARE";
+ public static final String EVENT_MAINTENANCE_PREPARE_ERROR =
"MAINT.PREPARE.ERROR";
Review Comment:
The PR description still contains the unfilled template (e.g., placeholder
text and no type/severity checkbox selected). Since the template notes
automation depends on these fields for labeling/documentation, please complete
the description metadata so the change is categorized correctly.
--
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]
