Copilot commented on code in PR #13788:
URL: https://github.com/apache/cloudstack/pull/13788#discussion_r4515866851
##########
engine/schema/src/test/java/com/cloud/vm/dao/VMInstanceDaoImplTest.java:
##########
@@ -210,6 +210,53 @@ public void
testUpdatePowerStateNoChangeMaxUpdatesInvalidStateVmRunning() {
assertTrue(result);
}
+ /*
+ * Mirrors testUpdatePowerStateNoChangeMaxUpdatesInvalidStateVmStopped but
with a null
+ * hostId (the default from setUp), which is what happens when the row was
loaded through
+ * a partial-select projection that doesn't include hostId.
isPowerStateInSyncWithInstanceState
+ * must not NPE on the null hostId, and must still detect the state is
out-of-sync.
Review Comment:
This comment is ambiguous about which `hostId` is null (the method parameter
vs `vm.getHostId()` vs `vm.getPowerHostId()`). Since the regression is
specifically about the entity field missing from a partial-select, consider
explicitly stating that `VMInstanceVO.getHostId()` is null due to the
projection, while the `updatePowerState(..., hostId, ...)` parameter is still
non-null. This will make the intent of the test clearer and prevent confusion
with `powerHostId`.
##########
engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java:
##########
@@ -460,8 +460,13 @@ private AgentControlAnswer handleControlCommand(final
AgentAttache attache, fina
public void handleCommands(final AgentAttache attache, final long
sequence, final Command[] cmds) {
for (final Pair<Integer, Listener> listener : _cmdMonitors) {
- final boolean processed =
listener.second().processCommands(attache.getId(), sequence, cmds);
- logger.trace("SeqA {}-{}: {} by {}", attache.getId(), sequence,
(processed ? "processed" : "not processed"), listener.getClass());
+ try {
+ final boolean processed =
listener.second().processCommands(attache.getId(), sequence, cmds);
+ logger.trace("SeqA {}-{}: {} by {}", attache.getId(),
sequence, (processed ? "processed" : "not processed"),
listener.second().getClass());
+ } catch (final Exception e) {
+ logger.warn("Listener {} threw an exception processing
commands for agent {} seq {}; continuing to next listener",
+ listener.second().getClass().getName(),
attache.getId(), sequence, e);
+ }
Review Comment:
Catching `Exception` will also swallow `InterruptedException`, which can
clear or ignore interrupt handling in the thread executing `handleCommands`. To
avoid breaking cancellation/shutdown semantics, special-case
`InterruptedException` (restore the interrupt flag via
`Thread.currentThread().interrupt()` and typically return), or narrow the catch
to `RuntimeException` (and optionally `Error` if you explicitly want to
continue on those).
##########
engine/orchestration/src/test/java/com/cloud/agent/manager/AgentManagerImplTest.java:
##########
@@ -172,4 +173,84 @@ public void testGetHostSshPortWithKVMHostCustomPort() {
int hostSshPort = mgr.getHostSshPort(host);
Assert.assertEquals(3922, hostSshPort);
}
+
+ /*
+ * When the first registered listener throws a RuntimeException,
+ * handleCommands must catch it, log a warning, and continue so that
+ * the second listener still runs.
+ */
+ @Test
+ public void testHandleCommandsListenerExceptionDoesNotAbortLoop() throws
Exception {
+ Listener throwingListener = Mockito.mock(Listener.class);
+ Mockito.when(throwingListener.processCommands(Mockito.anyLong(),
Mockito.anyLong(), Mockito.any()))
+ .thenThrow(new RuntimeException("simulated NPE from
power-state sync"));
+
+ Listener goodListener = Mockito.mock(Listener.class);
+ Mockito.when(goodListener.processCommands(Mockito.anyLong(),
Mockito.anyLong(), Mockito.any()))
Review Comment:
These stubbings use `Mockito.any()` for the `Command[]` argument, which is
less specific than necessary and can hide accidental signature/argument
mismatches at compile time. Prefer `Mockito.any(Command[].class)` (or a typed
matcher) for the third argument so the tests fail fast if the method signature
changes or the wrong overload is targeted.
--
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]
