Copilot commented on code in PR #14284:
URL: https://github.com/apache/cloudstack/pull/14284#discussion_r4347080521
##########
server/src/main/java/com/cloud/network/as/AutoScaleManagerImpl.java:
##########
@@ -2112,7 +2112,7 @@ public void doScaleUp(long groupId, Integer numVm) {
String.format("Failed to assign LB rule for VM
%s in AutoScale VM group %s", vm, asGroup), groupId,
ApiCommandResourceType.AutoScaleVmGroup.toString(), 0);
break;
}
- } catch (ServerApiException e) {
+ } catch (CloudRuntimeException e) {
Review Comment:
Changing the catch from `ServerApiException` to only `CloudRuntimeException`
can introduce a regression if the start path still throws `ServerApiException`
in some cases (those exceptions would no longer be handled here, potentially
skipping cleanup/state-reset behavior in this block). Consider using a
multi-catch (e.g., `catch (ServerApiException | CloudRuntimeException e)`) or
otherwise preserving handling for both exception types.
##########
server/src/test/java/com/cloud/network/as/AutoScaleManagerImplTest.java:
##########
@@ -1526,6 +1526,43 @@ public void testDoScaleUp() throws
ResourceUnavailableException, InsufficientCap
}
}
+ /**
+ * Regression test for #14185: VirtualMachineManagerImpl.start() wraps a
failed start into an unchecked
+ * CloudRuntimeException rather than the checked exceptions startNewVM
converts to ServerApiException.
+ * Before the fix, doScaleUp's catch(ServerApiException) missed it, the VM
was never destroyed, and its
+ * autoscale_vmgroup_vm_map row leaked forever (the VM stays in
State.Stopped, invisible to both
+ * getErroredInstanceCount() and countAvailableVmsByGroup(), so the group
scales up again next interval).
+ */
+ @Test
+ public void testDoScaleUpDestroysVmWhenStartThrowsCloudRuntimeException()
throws ResourceUnavailableException, InsufficientCapacityException,
ResourceAllocationException {
+ try (MockedStatic<ActionEventUtils> ignored =
Mockito.mockStatic(ActionEventUtils.class)) {
+
when(autoScaleVmGroupDao.findById(vmGroupId)).thenReturn(asVmGroupMock);
+ when(asVmGroupMock.getId()).thenReturn(vmGroupId);
+ when(asVmGroupMock.getAccountId()).thenReturn(accountId);
+ when(asVmGroupMock.getMaxMembers()).thenReturn(maxMembers);
+
when(autoScaleVmGroupVmMapDao.countAvailableVmsByGroup(vmGroupId)).thenReturn(maxMembers
- 1);
+
when(autoScaleVmGroupVmMapDao.getErroredInstanceCount(vmGroupId)).thenReturn(0);
+
when(asVmGroupMock.getState()).thenReturn(AutoScaleVmGroup.State.ENABLED);
+
+ when(autoScaleVmGroupDao.updateState(vmGroupId,
AutoScaleVmGroup.State.ENABLED,
AutoScaleVmGroup.State.SCALING)).thenReturn(true);
+ when(autoScaleVmGroupDao.updateState(vmGroupId,
AutoScaleVmGroup.State.SCALING,
AutoScaleVmGroup.State.ENABLED)).thenReturn(true);
+
Mockito.doReturn(userVmMock).when(autoScaleManagerImplSpy).createNewVM(asVmGroupMock);
+ when(userVmMock.getId()).thenReturn(virtualMachineId);
+
+ Mockito.doThrow(new CloudRuntimeException(String.format("Unable to
start a VM [%s] due to [Resource unavailable].", virtualMachineId)))
+ .when(userVmMgr).startVirtualMachine(virtualMachineId,
null, new HashMap<>(), null);
Review Comment:
This stub is sensitive to the exact arguments passed at runtime (notably the
`Map` instance and `null` values). To make the regression test resilient to
benign implementation changes (e.g., passing a different empty map
implementation or non-empty map), prefer Mockito argument matchers for the
nullable and map arguments (e.g., `isNull()` / `anyMap()` with
`eq(virtualMachineId)`), while keeping the exception behavior the same.
--
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]
