You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by pe...@apache.org on 2026/07/16 17:35:23 UTC
(cloudstack) 01/01: Fix and de-risk UserVmManagerImpl external-DHCP startup scan
This is an automated email from the ASF dual-hosted git repository.
Pearl1594 pushed a commit to branch externaldhcp-startup-uservm
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit 404efecdf7ae0c71e483746a61743ac8815ec0f6
Author: Pearl Dsilva <pe...@gmail.com>
AuthorDate: Thu Jul 16 13:34:58 2026 -0400
Fix and de-risk UserVmManagerImpl external-DHCP startup scan
---
.../main/java/com/cloud/vm/dao/VMInstanceDao.java | 2 +
.../java/com/cloud/vm/dao/VMInstanceDaoImpl.java | 13 +++++
.../java/com/cloud/network/NetworkModelImpl.java | 6 ++-
.../main/java/com/cloud/vm/UserVmManagerImpl.java | 57 +++++++++++++-------
.../com/cloud/network/NetworkModelImplTest.java | 10 ++++
.../java/com/cloud/vm/UserVmManagerImplTest.java | 62 ++++++++++++++++++++++
6 files changed, 131 insertions(+), 19 deletions(-)
diff --git a/engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDao.java b/engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDao.java
index 56e16ddd871..ff7e352a482 100755
--- a/engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDao.java
+++ b/engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDao.java
@@ -190,4 +190,6 @@ public interface VMInstanceDao extends GenericDao<VMInstanceVO, Long>, StateDao<
int getVmCountByOfferingId(Long serviceOfferingId);
int getVmCountByOfferingNotInDomain(Long serviceOfferingId, List<Long> domainIds);
+
+ List<VMInstanceVO> listByIds(List<Long> ids);
}
diff --git a/engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java b/engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java
index 518bc3cf497..9c129a24229 100755
--- a/engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java
+++ b/engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java
@@ -1261,4 +1261,17 @@ public class VMInstanceDaoImpl extends GenericDaoBase<VMInstanceVO, Long> implem
List<Integer> count = customSearch(sc, null);
return count.get(0);
}
+
+ @Override
+ public List<VMInstanceVO> listByIds(List<Long> ids) {
+ if (ids == null || ids.isEmpty()) {
+ return new ArrayList<>();
+ }
+ SearchBuilder<VMInstanceVO> sb = createSearchBuilder();
+ sb.and("id", sb.entity().getId(), Op.IN);
+ sb.done();
+ SearchCriteria<VMInstanceVO> sc = sb.create();
+ sc.setParameters("id", ids.toArray());
+ return listBy(sc);
+ }
}
diff --git a/server/src/main/java/com/cloud/network/NetworkModelImpl.java b/server/src/main/java/com/cloud/network/NetworkModelImpl.java
index 86791b87851..ad404528b75 100644
--- a/server/src/main/java/com/cloud/network/NetworkModelImpl.java
+++ b/server/src/main/java/com/cloud/network/NetworkModelImpl.java
@@ -965,7 +965,11 @@ public class NetworkModelImpl extends ManagerBase implements NetworkModel, Confi
Network network = _networksDao.findById(networkId);
- if (network != null && network.getGuestType() != GuestType.Shared) {
+ if (network == null) {
+ return false;
+ }
+
+ if (network.getGuestType() != GuestType.Shared) {
return false;
}
diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
index 7e58cd01050..3bff22c8c0e 100644
--- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
+++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
@@ -54,6 +54,7 @@ import javax.naming.ConfigurationException;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.ParserConfigurationException;
+import com.cloud.utils.Profiler;
import org.apache.cloudstack.acl.ControlledEntity;
import org.apache.cloudstack.acl.ControlledEntity.ACLType;
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
@@ -2452,33 +2453,53 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
public boolean start() {
_executor.scheduleWithFixedDelay(new ExpungeTask(), _expungeInterval, _expungeInterval, TimeUnit.SECONDS);
_vmIpFetchExecutor.scheduleWithFixedDelay(new VmIpFetchTask(), VmIpFetchWaitInterval.value(), VmIpFetchWaitInterval.value(), TimeUnit.SECONDS);
- loadVmDetailsInMapForExternalDhcpIp();
+ _vmIpFetchExecutor.submit(this::loadVmDetailsInMapForExternalDhcpIp);
return true;
}
- private void loadVmDetailsInMapForExternalDhcpIp() {
+ protected void loadVmDetailsInMapForExternalDhcpIp() {
+ try {
+ Profiler profiler = new Profiler();
+ profiler.start();
+
+ List<NetworkVO> networks = _networkDao.listByGuestType(Network.GuestType.Shared);
+ Map<Long, Boolean> offeringWithoutServices = new HashMap<>();
+ int networksScanned = 0;
+ int nicsAdded = 0;
+
+ for (NetworkVO network : networks) {
+ boolean withoutServices = offeringWithoutServices.computeIfAbsent(network.getNetworkOfferingId(),
+ offeringId -> _networkModel.listNetworkOfferingServices(offeringId).isEmpty());
+ if (!withoutServices) {
+ continue;
+ }
+ networksScanned++;
- List<NetworkVO> networks = _networkDao.listByGuestType(Network.GuestType.Shared);
- networks.addAll(_networkDao.listByGuestType(Network.GuestType.L2));
+ List<NicVO> nullIpNics = _nicDao.listByNetworkId(network.getId()).stream()
+ .filter(nic -> nic.getIPv4Address() == null)
+ .collect(Collectors.toList());
+ if (nullIpNics.isEmpty()) {
+ continue;
+ }
- for (NetworkVO network: networks) {
- if (GuestType.L2.equals(network.getGuestType()) || _networkModel.isSharedNetworkWithoutServices(network.getId())) {
- List<NicVO> nics = _nicDao.listByNetworkId(network.getId());
+ List<Long> vmIds = nullIpNics.stream().map(NicVO::getInstanceId).distinct().collect(Collectors.toList());
+ Map<Long, VMInstanceVO> runningVmsById = _vmInstanceDao.listByIds(vmIds).stream()
+ .filter(vm -> vm != null && vm.getState() == State.Running)
+ .collect(Collectors.toMap(VMInstanceVO::getId, vm -> vm));
- for (NicVO nic : nics) {
- if (nic.getIPv4Address() == null) {
- long nicId = nic.getId();
- long vmId = nic.getInstanceId();
- VMInstanceVO vmInstance = _vmInstanceDao.findById(vmId);
-
- // only load running vms. For stopped vms get loaded on starting
- if (vmInstance != null && vmInstance.getState() == State.Running) {
- VmAndCountDetails vmAndCount = new VmAndCountDetails(vmId, VmIpFetchTrialMax.value());
- vmIdCountMap.put(nicId, vmAndCount);
- }
+ for (NicVO nic : nullIpNics) {
+ if (runningVmsById.containsKey(nic.getInstanceId())) {
+ vmIdCountMap.put(nic.getId(), new VmAndCountDetails(nic.getInstanceId(), VmIpFetchTrialMax.value()));
+ nicsAdded++;
}
}
}
+
+ profiler.stop();
+ logger.info("External-DHCP VM-IP map seeded: {} shared-without-service networks, {} nics added, took {} ms",
+ networksScanned, nicsAdded, profiler.getDurationInMillis());
+ } catch (Exception e) {
+ logger.error("Failed to seed external-DHCP VM-IP retrieval map", e);
}
}
diff --git a/server/src/test/java/com/cloud/network/NetworkModelImplTest.java b/server/src/test/java/com/cloud/network/NetworkModelImplTest.java
index 889031f2b50..d0054abf5b8 100644
--- a/server/src/test/java/com/cloud/network/NetworkModelImplTest.java
+++ b/server/src/test/java/com/cloud/network/NetworkModelImplTest.java
@@ -21,6 +21,7 @@ import com.cloud.dc.VlanVO;
import com.cloud.exception.InvalidParameterValueException;
import com.cloud.network.addr.PublicIp;
import com.cloud.network.dao.IPAddressVO;
+import com.cloud.network.dao.NetworkDao;
import com.cloud.network.dao.NetworkServiceMapDao;
import com.cloud.network.dao.NetworkServiceMapVO;
import com.cloud.network.dao.NetworkVO;
@@ -232,4 +233,13 @@ public class NetworkModelImplTest {
Map<Network.Provider, ArrayList<PublicIpAddress>> result = networkModel.getProviderToIpList(network, ipToServices);
Assert.assertNotNull(result);
}
+
+ @Test
+ public void testIsSharedNetworkWithoutServicesReturnsFalseWhenNetworkMissing() {
+ NetworkDao networksDao = Mockito.mock(NetworkDao.class);
+ ReflectionTestUtils.setField(networkModel, "_networksDao", networksDao);
+ Mockito.when(networksDao.findById(123L)).thenReturn(null);
+
+ Assert.assertFalse(networkModel.isSharedNetworkWithoutServices(123L));
+ }
}
diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java
index b21cd53d743..c48b4447157 100644
--- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java
+++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java
@@ -33,9 +33,11 @@ import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.lenient;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
import static org.mockito.Mockito.when;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
@@ -45,6 +47,7 @@ import java.util.LinkedList;
import java.util.List;
import java.util.Map;
+import com.cloud.vm.dao.VMInstanceDao;
import org.apache.cloudstack.acl.ControlledEntity;
import org.apache.cloudstack.acl.SecurityChecker;
import org.apache.cloudstack.api.ApiConstants;
@@ -188,6 +191,9 @@ public class UserVmManagerImplTest {
@Mock
protected NicDao nicDao;
+ @Mock
+ protected VMInstanceDao vmInstanceDao;
+
@Mock
private NetworkDao _networkDao;
@@ -3207,4 +3213,60 @@ public class UserVmManagerImplTest {
userVmManagerImpl.verifyVmLimits(userVmVoMock, customParameters));
Assert.assertTrue(ex.getMessage().startsWith("The CPU speed of this offering"));
}
+
+ @Test
+ public void testLoadVmDetailsInMapForExternalDhcpIpSeedsOnlyRunningNullIpNics() {
+ NetworkVO network = mock(NetworkVO.class);
+ when(network.getId()).thenReturn(100L);
+ when(network.getNetworkOfferingId()).thenReturn(7L);
+ when(_networkDao.listByGuestType(Network.GuestType.Shared)).thenReturn(Arrays.asList(network));
+ when(networkModel.listNetworkOfferingServices(7L)).thenReturn(new ArrayList<>());
+
+ NicVO runningNullIp = mock(NicVO.class);
+ when(runningNullIp.getId()).thenReturn(11L);
+ when(runningNullIp.getInstanceId()).thenReturn(1000L);
+ when(runningNullIp.getIPv4Address()).thenReturn(null);
+
+ NicVO withIp = mock(NicVO.class);
+ when(withIp.getIPv4Address()).thenReturn("10.1.1.5");
+
+ NicVO stoppedNullIp = mock(NicVO.class);
+ when(stoppedNullIp.getInstanceId()).thenReturn(2000L);
+ when(stoppedNullIp.getIPv4Address()).thenReturn(null);
+
+ when(nicDao.listByNetworkId(100L)).thenReturn(Arrays.asList(runningNullIp, withIp, stoppedNullIp));
+
+ VMInstanceVO running = mock(VMInstanceVO.class);
+ when(running.getId()).thenReturn(1000L);
+ when(running.getState()).thenReturn(VirtualMachine.State.Running);
+ VMInstanceVO stopped = mock(VMInstanceVO.class);
+ when(stopped.getId()).thenReturn(2000L);
+ when(stopped.getState()).thenReturn(VirtualMachine.State.Stopped);
+ when(vmInstanceDao.listByIds(Mockito.anyList())).thenReturn(Arrays.asList(running, stopped));
+
+ userVmManagerImpl.loadVmDetailsInMapForExternalDhcpIp();
+
+ @SuppressWarnings("unchecked")
+ Map<Long, ?> vmIdCountMap = (Map<Long, ?>) ReflectionTestUtils.getField(userVmManagerImpl, "vmIdCountMap");
+ assertNotNull(vmIdCountMap);
+ assertEquals(1, vmIdCountMap.size());
+ assertTrue(vmIdCountMap.containsKey(11L));
+ assertFalse(vmIdCountMap.containsKey(12L));
+ Mockito.verify(vmInstanceDao, times(1)).listByIds(Mockito.anyList());
+ Mockito.verify(vmInstanceDao, Mockito.never()).findById(Mockito.anyLong());
+ }
+
+ @Test
+ public void testStartSeedsExternalDhcpMapAsynchronously() {
+ java.util.concurrent.ScheduledExecutorService expungeExecutor = mock(java.util.concurrent.ScheduledExecutorService.class);
+ java.util.concurrent.ScheduledExecutorService ipFetchExecutor = mock(java.util.concurrent.ScheduledExecutorService.class);
+ ReflectionTestUtils.setField(userVmManagerImpl, "_executor", expungeExecutor);
+ ReflectionTestUtils.setField(userVmManagerImpl, "_vmIpFetchExecutor", ipFetchExecutor);
+
+ boolean result = userVmManagerImpl.start();
+
+ assertTrue(result);
+ Mockito.verify(ipFetchExecutor, times(1)).submit(any(Runnable.class));
+ Mockito.verify(userVmManagerImpl, Mockito.never()).loadVmDetailsInMapForExternalDhcpIp();
+ }
}