Copilot commented on code in PR #13363:
URL: https://github.com/apache/cloudstack/pull/13363#discussion_r3412877874
##########
server/src/main/java/com/cloud/capacity/CapacityManagerImpl.java:
##########
@@ -167,19 +167,19 @@ public boolean start() {
@DB
@Override
- public boolean releaseVmCapacity(VirtualMachine vm, final boolean
moveFromReserved, final boolean moveToReservered, final Long hostId) {
+ public boolean releaseVmCapacity(VirtualMachine vm, final boolean
moveFromReserved, final boolean moveToReserved, final Long hostId) {
if (hostId == null) {
return true;
}
HostVO host = _hostDao.findById(hostId);
if (HypervisorType.External.equals(host.getHypervisorType())) {
return true;
}
- return releaseVmCapacity(vm, moveFromReserved, moveToReservered, host);
+ return releaseVmCapacity(vm, moveFromReserved, moveToReserved, host);
Review Comment:
`releaseVmCapacity(vm, ..., Long hostId)` dereferences
`host.getHypervisorType()` without checking if `_hostDao.findById(hostId)`
returned null. With the new `postStateTransitionEvent` behavior, this can be
hit when `last_host_id` points to a deleted/unknown host and will throw a
NullPointerException instead of being a no-op release.
##########
server/src/main/java/com/cloud/capacity/CapacityManagerImpl.java:
##########
@@ -1000,12 +983,10 @@ public boolean
postStateTransitionEvent(StateMachine2.Transition<State, Event> t
}
if ((newState == State.Starting || newState == State.Migrating || event
== Event.AgentReportMigrated) && vm.getHostId() != null) {
- boolean fromLastHost = false;
- if (vm.getHostId().equals(vm.getLastHostId())) {
- logger.debug("VM starting again on the last host it was stopped on");
- fromLastHost = true;
+ if (vm.getLastHostId() != null) {
+ releaseVmCapacity(vm, true, false, vm.getLastHostId());
}
- allocateVmCapacity(vm, fromLastHost);
+ allocateVmCapacity(vm);
}
Review Comment:
The new reservation drain in `postStateTransitionEvent` fetches the last
host twice (once for logging as `lastHost`, and again via
`releaseVmCapacity(..., vm.getLastHostId())`). Reuse the already-resolved
`lastHost` and call the `releaseVmCapacity(..., Host)` overload to avoid an
extra DAO hit and keep the null-host handling consistent.
##########
server/src/main/java/com/cloud/capacity/CapacityManagerImpl.java:
##########
@@ -959,8 +942,8 @@ public boolean
postStateTransitionEvent(StateMachine2.Transition<State, Event> t
Host lastHost = _hostDao.findById(vm.getLastHostId());
Host oldHost = _hostDao.findById(oldHostId);
Host newHost = _hostDao.findById(vm.getHostId());
Review Comment:
`postStateTransitionEvent` calls `_hostDao.findById(...)` with values that
can be null (e.g., `vm.getLastHostId()` / `vm.getHostId()` / `oldHostId`), but
`GenericDaoBase.findById` does not handle null IDs and can throw an NPE. Guard
the DAO lookups so null IDs result in null `Host` references (safe for logging
and the existing null-tolerant release overloads).
--
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]
