Skip to content

Commit 98ec46c

Browse files
Kukuninclaude
andcommitted
Guard null host in releaseVmCapacity and reuse resolved lastHost
Two defensive fixes from Copilot's review of PR #13363: 1. releaseVmCapacity(VM, ..., Long hostId): _hostDao.findById(hostId) can return null when last_host_id points to a deleted host. The subsequent host.getHypervisorType() check would NPE. The Host overload already null-checks AND has the External check, so delegate to it instead of duplicating the External check here. 2. postStateTransitionEvent: the lastHost was being fetched twice (once for logging at L942, again inside releaseVmCapacity via the Long overload at L987). Reuse the already-resolved Host. The Host overload also handles null, so the explicit null check on getLastHostId() becomes redundant. (Skipped Copilot's third suggestion to guard findById(null) — the GenericDaoBase.findById(ID, boolean, Boolean) implementation at GenericDaoBase.java:1066 explicitly returns null for null id, so the existing code is already safe.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 6f09730 commit 98ec46c

2 files changed

Lines changed: 10 additions & 11 deletions

File tree

server/src/main/java/com/cloud/capacity/CapacityManagerImpl.java

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -172,9 +172,6 @@ public boolean releaseVmCapacity(VirtualMachine vm, final boolean moveFromReserv
172172
return true;
173173
}
174174
HostVO host = _hostDao.findById(hostId);
175-
if (HypervisorType.External.equals(host.getHypervisorType())) {
176-
return true;
177-
}
178175
return releaseVmCapacity(vm, moveFromReserved, moveToReserved, host);
179176
}
180177

@@ -983,9 +980,7 @@ public boolean postStateTransitionEvent(StateMachine2.Transition<State, Event> t
983980
}
984981

985982
if ((newState == State.Starting || newState == State.Migrating || event == Event.AgentReportMigrated) && vm.getHostId() != null) {
986-
if (vm.getLastHostId() != null) {
987-
releaseVmCapacity(vm, true, false, vm.getLastHostId());
988-
}
983+
releaseVmCapacity(vm, true, false, lastHost);
989984
allocateVmCapacity(vm);
990985
}
991986

server/src/test/java/com/cloud/capacity/CapacityManagerImplTest.java

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import static org.junit.Assert.assertTrue;
2222
import static org.mockito.ArgumentMatchers.anyBoolean;
2323
import static org.mockito.ArgumentMatchers.anyList;
24-
import static org.mockito.ArgumentMatchers.anyLong;
2524
import static org.mockito.ArgumentMatchers.eq;
2625
import static org.mockito.Mockito.any;
2726
import static org.mockito.Mockito.doNothing;
@@ -46,6 +45,7 @@
4645

4746
import com.cloud.dc.ClusterDetailsDao;
4847
import com.cloud.host.Host;
48+
import com.cloud.host.HostVO;
4949
import com.cloud.host.dao.HostDao;
5050
import com.cloud.offering.ServiceOffering;
5151
import com.cloud.service.ServiceOfferingVO;
@@ -174,44 +174,48 @@ public void testCheckIfHostHasNoCpuCapabilityButHasCapacity() {
174174
public void testPostStateTransitionReleasesStaleReservationWhenStartingOnDifferentHost() {
175175
final Long oldHostId = 1L;
176176
final Long newHostId = 2L;
177+
HostVO oldHost = mock(HostVO.class);
178+
when(hostDao.findById(oldHostId)).thenReturn(oldHost);
177179

178180
VirtualMachine vm = mock(VirtualMachine.class);
179181
when(vm.getHostId()).thenReturn(newHostId);
180182
when(vm.getLastHostId()).thenReturn(oldHostId);
181183

182184
doNothing().when(capacityManager).allocateVmCapacity(any(VirtualMachine.class));
183185
doReturn(true).when(capacityManager).releaseVmCapacity(
184-
any(VirtualMachine.class), anyBoolean(), anyBoolean(), anyLong());
186+
any(VirtualMachine.class), anyBoolean(), anyBoolean(), any(Host.class));
185187

186188
StateMachine2.Transition<State, Event> transition = new StateMachine2.Transition<>(
187189
State.Stopped, Event.StartRequested, State.Starting, Collections.emptyList());
188190
Pair<Long, Long> opaque = new Pair<>(null, 0L);
189191

190192
capacityManager.postStateTransitionEvent(transition, vm, true, opaque);
191193

192-
verify(capacityManager).releaseVmCapacity(vm, true, false, oldHostId);
194+
verify(capacityManager).releaseVmCapacity(vm, true, false, oldHost);
193195
verify(capacityManager).allocateVmCapacity(vm);
194196
}
195197

196198
@Test
197199
public void testPostStateTransitionReleasesReservationWhenStartingOnSameHost() {
198200
final Long hostId = 1L;
201+
HostVO h = mock(HostVO.class);
202+
when(hostDao.findById(hostId)).thenReturn(h);
199203

200204
VirtualMachine vm = mock(VirtualMachine.class);
201205
when(vm.getHostId()).thenReturn(hostId);
202206
when(vm.getLastHostId()).thenReturn(hostId);
203207

204208
doNothing().when(capacityManager).allocateVmCapacity(any(VirtualMachine.class));
205209
doReturn(true).when(capacityManager).releaseVmCapacity(
206-
any(VirtualMachine.class), anyBoolean(), anyBoolean(), anyLong());
210+
any(VirtualMachine.class), anyBoolean(), anyBoolean(), any(Host.class));
207211

208212
StateMachine2.Transition<State, Event> transition = new StateMachine2.Transition<>(
209213
State.Stopped, Event.StartRequested, State.Starting, Collections.emptyList());
210214
Pair<Long, Long> opaque = new Pair<>(null, 0L);
211215

212216
capacityManager.postStateTransitionEvent(transition, vm, true, opaque);
213217

214-
verify(capacityManager).releaseVmCapacity(vm, true, false, hostId);
218+
verify(capacityManager).releaseVmCapacity(vm, true, false, h);
215219
verify(capacityManager).allocateVmCapacity(vm);
216220
}
217221

0 commit comments

Comments
 (0)