Skip to content

Commit 1f081f3

Browse files
author
Pearl Dsilva
committed
Fix and de-risk UserVmManagerImpl external-DHCP startup scan
1 parent e8df87e commit 1f081f3

6 files changed

Lines changed: 130 additions & 19 deletions

File tree

‎engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDao.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,4 +190,6 @@ List<VMInstanceVO> searchRemovedByRemoveDate(final Date startDate, final Date en
190190
int getVmCountByOfferingId(Long serviceOfferingId);
191191

192192
int getVmCountByOfferingNotInDomain(Long serviceOfferingId, List<Long> domainIds);
193+
194+
List<VMInstanceVO> listByIds(List<Long> ids);
193195
}

‎engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1261,4 +1261,17 @@ public int getVmCountByOfferingNotInDomain(Long serviceOfferingId, List<Long> do
12611261
List<Integer> count = customSearch(sc, null);
12621262
return count.get(0);
12631263
}
1264+
1265+
@Override
1266+
public List<VMInstanceVO> listByIds(List<Long> ids) {
1267+
if (ids == null || ids.isEmpty()) {
1268+
return new ArrayList<>();
1269+
}
1270+
SearchBuilder<VMInstanceVO> sb = createSearchBuilder();
1271+
sb.and("id", sb.entity().getId(), Op.IN);
1272+
sb.done();
1273+
SearchCriteria<VMInstanceVO> sc = sb.create();
1274+
sc.setParameters("id", ids.toArray());
1275+
return listBy(sc);
1276+
}
12641277
}

‎server/src/main/java/com/cloud/network/NetworkModelImpl.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -965,7 +965,11 @@ public boolean isSharedNetworkWithoutServices (long networkId) {
965965

966966
Network network = _networksDao.findById(networkId);
967967

968-
if (network != null && network.getGuestType() != GuestType.Shared) {
968+
if (network == null) {
969+
return false;
970+
}
971+
972+
if (network.getGuestType() != GuestType.Shared) {
969973
return false;
970974
}
971975

‎server/src/main/java/com/cloud/vm/UserVmManagerImpl.java‎

Lines changed: 39 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@
5454
import javax.xml.parsers.DocumentBuilder;
5555
import javax.xml.parsers.ParserConfigurationException;
5656

57+
import com.cloud.utils.Profiler;
5758
import org.apache.cloudstack.acl.ControlledEntity;
5859
import org.apache.cloudstack.acl.ControlledEntity.ACLType;
5960
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
@@ -2452,33 +2453,53 @@ public String getName() {
24522453
public boolean start() {
24532454
_executor.scheduleWithFixedDelay(new ExpungeTask(), _expungeInterval, _expungeInterval, TimeUnit.SECONDS);
24542455
_vmIpFetchExecutor.scheduleWithFixedDelay(new VmIpFetchTask(), VmIpFetchWaitInterval.value(), VmIpFetchWaitInterval.value(), TimeUnit.SECONDS);
2455-
loadVmDetailsInMapForExternalDhcpIp();
2456+
_vmIpFetchExecutor.submit(this::loadVmDetailsInMapForExternalDhcpIp);
24562457
return true;
24572458
}
24582459

2459-
private void loadVmDetailsInMapForExternalDhcpIp() {
2460+
protected void loadVmDetailsInMapForExternalDhcpIp() {
2461+
try {
2462+
Profiler profiler = new Profiler();
2463+
profiler.start();
2464+
2465+
List<NetworkVO> networks = _networkDao.listByGuestType(Network.GuestType.Shared);
2466+
Map<Long, Boolean> offeringWithoutServices = new HashMap<>();
2467+
int networksScanned = 0;
2468+
int nicsAdded = 0;
2469+
2470+
for (NetworkVO network : networks) {
2471+
boolean withoutServices = offeringWithoutServices.computeIfAbsent(network.getNetworkOfferingId(),
2472+
offeringId -> _networkModel.listNetworkOfferingServices(offeringId).isEmpty());
2473+
if (!withoutServices) {
2474+
continue;
2475+
}
2476+
networksScanned++;
24602477

2461-
List<NetworkVO> networks = _networkDao.listByGuestType(Network.GuestType.Shared);
2462-
networks.addAll(_networkDao.listByGuestType(Network.GuestType.L2));
2478+
List<NicVO> nullIpNics = _nicDao.listByNetworkId(network.getId()).stream()
2479+
.filter(nic -> nic.getIPv4Address() == null)
2480+
.collect(Collectors.toList());
2481+
if (nullIpNics.isEmpty()) {
2482+
continue;
2483+
}
24632484

2464-
for (NetworkVO network: networks) {
2465-
if (GuestType.L2.equals(network.getGuestType()) || _networkModel.isSharedNetworkWithoutServices(network.getId())) {
2466-
List<NicVO> nics = _nicDao.listByNetworkId(network.getId());
2485+
List<Long> vmIds = nullIpNics.stream().map(NicVO::getInstanceId).distinct().collect(Collectors.toList());
2486+
Map<Long, VMInstanceVO> runningVmsById = _vmInstanceDao.listByIds(vmIds).stream()
2487+
.filter(vm -> vm != null && vm.getState() == State.Running)
2488+
.collect(Collectors.toMap(VMInstanceVO::getId, vm -> vm));
24672489

2468-
for (NicVO nic : nics) {
2469-
if (nic.getIPv4Address() == null) {
2470-
long nicId = nic.getId();
2471-
long vmId = nic.getInstanceId();
2472-
VMInstanceVO vmInstance = _vmInstanceDao.findById(vmId);
2473-
2474-
// only load running vms. For stopped vms get loaded on starting
2475-
if (vmInstance != null && vmInstance.getState() == State.Running) {
2476-
VmAndCountDetails vmAndCount = new VmAndCountDetails(vmId, VmIpFetchTrialMax.value());
2477-
vmIdCountMap.put(nicId, vmAndCount);
2478-
}
2490+
for (NicVO nic : nullIpNics) {
2491+
if (runningVmsById.containsKey(nic.getInstanceId())) {
2492+
vmIdCountMap.put(nic.getId(), new VmAndCountDetails(nic.getInstanceId(), VmIpFetchTrialMax.value()));
2493+
nicsAdded++;
24792494
}
24802495
}
24812496
}
2497+
2498+
profiler.stop();
2499+
logger.info("External-DHCP VM-IP map seeded: {} shared-without-service networks, {} nics added, took {} ms",
2500+
networksScanned, nicsAdded, profiler.getDurationInMillis());
2501+
} catch (Exception e) {
2502+
logger.error("Failed to seed external-DHCP VM-IP retrieval map", e);
24822503
}
24832504
}
24842505

‎server/src/test/java/com/cloud/network/NetworkModelImplTest.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import com.cloud.exception.InvalidParameterValueException;
2222
import com.cloud.network.addr.PublicIp;
2323
import com.cloud.network.dao.IPAddressVO;
24+
import com.cloud.network.dao.NetworkDao;
2425
import com.cloud.network.dao.NetworkServiceMapDao;
2526
import com.cloud.network.dao.NetworkServiceMapVO;
2627
import com.cloud.network.dao.NetworkVO;
@@ -232,4 +233,13 @@ public void testGetProviderToIpList() {
232233
Map<Network.Provider, ArrayList<PublicIpAddress>> result = networkModel.getProviderToIpList(network, ipToServices);
233234
Assert.assertNotNull(result);
234235
}
236+
237+
@Test
238+
public void testIsSharedNetworkWithoutServicesReturnsFalseWhenNetworkMissing() {
239+
NetworkDao networksDao = Mockito.mock(NetworkDao.class);
240+
ReflectionTestUtils.setField(networkModel, "_networksDao", networksDao);
241+
Mockito.when(networksDao.findById(123L)).thenReturn(null);
242+
243+
Assert.assertFalse(networkModel.isSharedNetworkWithoutServices(123L));
244+
}
235245
}

‎server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,11 @@
3333
import static org.mockito.Mockito.lenient;
3434
import static org.mockito.Mockito.mock;
3535
import static org.mockito.Mockito.never;
36+
import static org.mockito.Mockito.times;
3637
import static org.mockito.Mockito.when;
3738

3839
import java.util.ArrayList;
40+
import java.util.Arrays;
3941
import java.util.Collections;
4042
import java.util.HashMap;
4143
import java.util.HashSet;
@@ -45,6 +47,7 @@
4547
import java.util.List;
4648
import java.util.Map;
4749

50+
import com.cloud.vm.dao.VMInstanceDao;
4851
import org.apache.cloudstack.acl.ControlledEntity;
4952
import org.apache.cloudstack.acl.SecurityChecker;
5053
import org.apache.cloudstack.api.ApiConstants;
@@ -188,6 +191,9 @@ public class UserVmManagerImplTest {
188191
@Mock
189192
protected NicDao nicDao;
190193

194+
@Mock
195+
protected VMInstanceDao vmInstanceDao;
196+
191197
@Mock
192198
private NetworkDao _networkDao;
193199

@@ -3207,4 +3213,59 @@ public void verifyVmLimits_constrainedOffering_throwsException() {
32073213
userVmManagerImpl.verifyVmLimits(userVmVoMock, customParameters));
32083214
Assert.assertTrue(ex.getMessage().startsWith("The CPU speed of this offering"));
32093215
}
3216+
3217+
@Test
3218+
public void testLoadVmDetailsInMapForExternalDhcpIpSeedsOnlyRunningNullIpNics() {
3219+
NetworkVO network = mock(NetworkVO.class);
3220+
when(network.getId()).thenReturn(100L);
3221+
when(network.getNetworkOfferingId()).thenReturn(7L);
3222+
when(_networkDao.listByGuestType(Network.GuestType.Shared)).thenReturn(Arrays.asList(network));
3223+
when(networkModel.listNetworkOfferingServices(7L)).thenReturn(new ArrayList<>());
3224+
3225+
NicVO runningNullIp = mock(NicVO.class);
3226+
when(runningNullIp.getId()).thenReturn(11L);
3227+
when(runningNullIp.getInstanceId()).thenReturn(1000L);
3228+
when(runningNullIp.getIPv4Address()).thenReturn(null);
3229+
3230+
NicVO withIp = mock(NicVO.class);
3231+
when(withIp.getIPv4Address()).thenReturn("10.1.1.5");
3232+
3233+
NicVO stoppedNullIp = mock(NicVO.class);
3234+
when(stoppedNullIp.getInstanceId()).thenReturn(2000L);
3235+
when(stoppedNullIp.getIPv4Address()).thenReturn(null);
3236+
3237+
when(nicDao.listByNetworkId(100L)).thenReturn(Arrays.asList(runningNullIp, withIp, stoppedNullIp));
3238+
3239+
VMInstanceVO running = mock(VMInstanceVO.class);
3240+
when(running.getId()).thenReturn(1000L);
3241+
when(running.getState()).thenReturn(VirtualMachine.State.Running);
3242+
VMInstanceVO stopped = mock(VMInstanceVO.class);
3243+
when(stopped.getState()).thenReturn(VirtualMachine.State.Stopped);
3244+
when(vmInstanceDao.listByIds(Mockito.anyList())).thenReturn(Arrays.asList(running, stopped));
3245+
3246+
userVmManagerImpl.loadVmDetailsInMapForExternalDhcpIp();
3247+
3248+
@SuppressWarnings("unchecked")
3249+
Map<Long, ?> vmIdCountMap = (Map<Long, ?>) ReflectionTestUtils.getField(userVmManagerImpl, "vmIdCountMap");
3250+
assertNotNull(vmIdCountMap);
3251+
assertEquals(1, vmIdCountMap.size());
3252+
assertTrue(vmIdCountMap.containsKey(11L));
3253+
assertFalse(vmIdCountMap.containsKey(12L));
3254+
Mockito.verify(vmInstanceDao, times(1)).listByIds(Mockito.anyList());
3255+
Mockito.verify(vmInstanceDao, Mockito.never()).findById(Mockito.anyLong());
3256+
}
3257+
3258+
@Test
3259+
public void testStartSeedsExternalDhcpMapAsynchronously() {
3260+
java.util.concurrent.ScheduledExecutorService expungeExecutor = mock(java.util.concurrent.ScheduledExecutorService.class);
3261+
java.util.concurrent.ScheduledExecutorService ipFetchExecutor = mock(java.util.concurrent.ScheduledExecutorService.class);
3262+
ReflectionTestUtils.setField(userVmManagerImpl, "_executor", expungeExecutor);
3263+
ReflectionTestUtils.setField(userVmManagerImpl, "_vmIpFetchExecutor", ipFetchExecutor);
3264+
3265+
boolean result = userVmManagerImpl.start();
3266+
3267+
assertTrue(result);
3268+
Mockito.verify(ipFetchExecutor, times(1)).submit(any(Runnable.class));
3269+
Mockito.verify(userVmManagerImpl, Mockito.never()).loadVmDetailsInMapForExternalDhcpIp();
3270+
}
32103271
}

0 commit comments

Comments
 (0)