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 <[email protected]> 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(); + } }
