From 7acfc1a2b601dca459a1649228453ff45e7a526c Mon Sep 17 00:00:00 2001 From: Aaron Chung Date: Mon, 4 May 2026 22:28:17 -0700 Subject: [PATCH 1/2] Batch network lookups per NIC in NetworkOrchestrator to eliminate N+1 queries during migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three methods called _networksDao.findById(nic.getNetworkId()) per NIC in a loop: setHypervisorHostname, prepareNicForMigration, prepareAllNicsForMigration. Now batch-loads all networks for a VM's NICs in a single WHERE id IN (...) query. - Add NetworkDao.listByIds(List) for batch ID lookup - Add getNetworkMapForNics helper returning Map - Replace per-NIC findById with map lookup at 3 sites - Add null guard for deleted networks (pre-existing NPE risk on all 3 sites) - Site 2 (prepare) left unchanged — findById only on error path, not N+1 --- .../orchestration/NetworkOrchestrator.java | 34 +++++++++++++++++-- .../com/cloud/network/dao/NetworkDao.java | 2 ++ .../com/cloud/network/dao/NetworkDaoImpl.java | 13 +++++++ .../com/cloud/vpc/dao/MockNetworkDaoImpl.java | 5 +++ 4 files changed, 51 insertions(+), 3 deletions(-) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java index 6fc673cc2c9b..2a5220654689 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java @@ -2085,13 +2085,31 @@ public void finalizeUpdateInSequence(Network network, boolean success) { } } + private Map batchLoadNetworksForNics(List nics) { + List networkIds = new ArrayList<>(nics.size()); + for (NicVO nic : nics) { + networkIds.add(nic.getNetworkId()); + } + Map result = new HashMap<>(networkIds.size()); + if (!networkIds.isEmpty()) { + for (NetworkVO network : _networksDao.listByIds(networkIds)) { + result.put(network.getId(), network); + } + } + return result; + } + @Override public void setHypervisorHostname(VirtualMachineProfile vm, DeployDestination dest, boolean migrationSuccessful) throws ResourceUnavailableException { String hypervisorHostName = VirtualMachineManager.getHypervisorHostname(dest.getHost().getName()); if (StringUtils.isNotEmpty(hypervisorHostName)) { final List nics = _nicDao.listByVmId(vm.getId()); + final Map networksById = batchLoadNetworksForNics(nics); for (final NicVO nic : nics) { - final NetworkVO network = _networksDao.findById(nic.getNetworkId()); + final NetworkVO network = networksById.get(nic.getNetworkId()); + if (network == null) { + continue; + } final Integer networkRate = _networkModel.getNetworkRate(network.getId(), vm.getId()); final NicProfile profile = new NicProfile(nic, network, nic.getBroadcastUri(), nic.getIsolationUri(), networkRate, _networkModel.isSecurityGroupSupportedInNetwork(network), _networkModel.getNetworkTag(vm.getHypervisorType(), network)); @@ -2266,8 +2284,13 @@ public void prepareNicForMigration(final VirtualMachineProfile vm, final DeployD } final List nics = _nicDao.listByVmId(vm.getId()); final ReservationContext context = new ReservationContextImpl(UUID.randomUUID().toString(), null, null); + final Map networksById = batchLoadNetworksForNics(nics); for (final NicVO nic : nics) { - final NetworkVO network = _networksDao.findById(nic.getNetworkId()); + final NetworkVO network = networksById.get(nic.getNetworkId()); + if (network == null) { + logger.warn("Network {} not found for nic {} during migration prep, skipping", nic.getNetworkId(), nic); + continue; + } final Integer networkRate = _networkModel.getNetworkRate(network.getId(), vm.getId()); final NetworkGuru guru = AdapterBase.getAdapterByName(networkGurus, network.getGuruName()); @@ -2311,9 +2334,14 @@ public void prepareNicForMigration(final VirtualMachineProfile vm, final DeployD public void prepareAllNicsForMigration(final VirtualMachineProfile vm, final DeployDestination dest) { final List nics = _nicDao.listByVmId(vm.getId()); final ReservationContext context = new ReservationContextImpl(UUID.randomUUID().toString(), null, null); + final Map networksById = batchLoadNetworksForNics(nics); Long guestNetworkId = null; for (final NicVO nic : nics) { - final NetworkVO network = _networksDao.findById(nic.getNetworkId()); + final NetworkVO network = networksById.get(nic.getNetworkId()); + if (network == null) { + logger.warn("Network {} not found for nic {} during migration prep, skipping", nic.getNetworkId(), nic); + continue; + } if (network.getTrafficType().equals(TrafficType.Guest) && network.getGuestType().equals(GuestType.Isolated)) { guestNetworkId = network.getId(); } diff --git a/engine/schema/src/main/java/com/cloud/network/dao/NetworkDao.java b/engine/schema/src/main/java/com/cloud/network/dao/NetworkDao.java index fdca6e43f00f..4ef1489a25c9 100644 --- a/engine/schema/src/main/java/com/cloud/network/dao/NetworkDao.java +++ b/engine/schema/src/main/java/com/cloud/network/dao/NetworkDao.java @@ -32,6 +32,8 @@ public interface NetworkDao extends GenericDao, StateDao listByOwner(long ownerId); + List listByIds(List ids); + List listByGuestType(GuestType type); List listBy(long accountId, long offeringId, long dataCenterId); diff --git a/engine/schema/src/main/java/com/cloud/network/dao/NetworkDaoImpl.java b/engine/schema/src/main/java/com/cloud/network/dao/NetworkDaoImpl.java index 5887498e094c..1999a58fc845 100644 --- a/engine/schema/src/main/java/com/cloud/network/dao/NetworkDaoImpl.java +++ b/engine/schema/src/main/java/com/cloud/network/dao/NetworkDaoImpl.java @@ -563,6 +563,19 @@ public List listByOwner(final long ownerId) { return listBy(sc); } + @Override + public List listByIds(List ids) { + if (ids == null || ids.isEmpty()) { + return new ArrayList<>(); + } + SearchBuilder sb = createSearchBuilder(); + sb.and("id", sb.entity().getId(), Op.IN); + sb.done(); + SearchCriteria sc = sb.create(); + sc.setParameters("id", ids.toArray()); + return listBy(sc); + } + @Override public void addDomainToNetwork(final long networkId, final long domainId, final Boolean subdomainAccess) { addDomainToNetworknetwork(networkId, domainId, subdomainAccess); diff --git a/server/src/test/java/com/cloud/vpc/dao/MockNetworkDaoImpl.java b/server/src/test/java/com/cloud/vpc/dao/MockNetworkDaoImpl.java index 8a0bec56df7b..4e77d2c89bbe 100644 --- a/server/src/test/java/com/cloud/vpc/dao/MockNetworkDaoImpl.java +++ b/server/src/test/java/com/cloud/vpc/dao/MockNetworkDaoImpl.java @@ -280,4 +280,9 @@ public List listByNetworkDomainsAndAccountIds(Set uniqueNtwkD public List listByNetworkDomainsAndDomainIds(Set uniqueNtwkDomains, Set domainIds) { return List.of(); } + + @Override + public List listByIds(List ids) { + return null; + } } From adadc326e81eeba9f5eba8cc4e9bc369aad9ef01 Mon Sep 17 00:00:00 2001 From: vishesh92 Date: Mon, 12 Oct 2026 13:38:54 +0530 Subject: [PATCH 2/2] Address comments --- .../orchestration/NetworkOrchestrator.java | 11 +++------ .../cloud/network/dao/NetworkDaoImplTest.java | 24 +++++++++++++++++++ .../com/cloud/vpc/dao/MockNetworkDaoImpl.java | 2 +- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java index 2a5220654689..dc13cc0c24bd 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java @@ -2086,15 +2086,10 @@ public void finalizeUpdateInSequence(Network network, boolean success) { } private Map batchLoadNetworksForNics(List nics) { - List networkIds = new ArrayList<>(nics.size()); - for (NicVO nic : nics) { - networkIds.add(nic.getNetworkId()); - } + List networkIds = nics.stream().map(NicVO::getNetworkId).distinct().collect(Collectors.toList()); Map result = new HashMap<>(networkIds.size()); - if (!networkIds.isEmpty()) { - for (NetworkVO network : _networksDao.listByIds(networkIds)) { - result.put(network.getId(), network); - } + for (NetworkVO network : _networksDao.listByIds(networkIds)) { + result.put(network.getId(), network); } return result; } diff --git a/engine/schema/src/test/java/com/cloud/network/dao/NetworkDaoImplTest.java b/engine/schema/src/test/java/com/cloud/network/dao/NetworkDaoImplTest.java index ab5f43521052..b92066dbd947 100644 --- a/engine/schema/src/test/java/com/cloud/network/dao/NetworkDaoImplTest.java +++ b/engine/schema/src/test/java/com/cloud/network/dao/NetworkDaoImplTest.java @@ -26,11 +26,13 @@ import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; +import java.util.Collections; import java.util.List; @RunWith(MockitoJUnitRunner.class) @@ -68,4 +70,26 @@ public void listByPhysicalNetworkTrafficTypeTestSetParametersValidation() throws txn.close(); } } + + @Test + public void listByIdsTestNullOrEmptyIdsReturnsEmptyList() { + NetworkDaoImpl networkDaoImplSpy = Mockito.spy(NetworkDaoImpl.class); + + Assert.assertTrue(networkDaoImplSpy.listByIds(null).isEmpty()); + Assert.assertTrue(networkDaoImplSpy.listByIds(Collections.emptyList()).isEmpty()); + Mockito.verify(networkDaoImplSpy, Mockito.never()).listBy(Mockito.any(SearchCriteria.class)); + } + + @Test + public void listByIdsTestBuildsInClause() { + NetworkDaoImpl networkDaoImplSpy = Mockito.spy(NetworkDaoImpl.class); + Mockito.doReturn(listNetworkVoMock).when(networkDaoImplSpy).listBy(Mockito.any(SearchCriteria.class)); + + List result = networkDaoImplSpy.listByIds(List.of(1L, 2L, 3L)); + + Assert.assertEquals(listNetworkVoMock, result); + ArgumentCaptor scCaptor = ArgumentCaptor.forClass(SearchCriteria.class); + Mockito.verify(networkDaoImplSpy).listBy(scCaptor.capture()); + Assert.assertEquals("networks.id IN (?,?,?)", scCaptor.getValue().getWhereClause().trim()); + } } diff --git a/server/src/test/java/com/cloud/vpc/dao/MockNetworkDaoImpl.java b/server/src/test/java/com/cloud/vpc/dao/MockNetworkDaoImpl.java index 4e77d2c89bbe..b1d7e82839e1 100644 --- a/server/src/test/java/com/cloud/vpc/dao/MockNetworkDaoImpl.java +++ b/server/src/test/java/com/cloud/vpc/dao/MockNetworkDaoImpl.java @@ -283,6 +283,6 @@ public List listByNetworkDomainsAndDomainIds(Set uniqueNtwkDo @Override public List listByIds(List ids) { - return null; + return List.of(); } }